Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly use ledger hash to break ties #2257

Closed
wants to merge 4 commits into from
Closed

Properly use ledger hash to break ties #2257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a429c23
Properly use ledger hash to break ties
bachase Nov 2, 2017
e11279f
[FOLD] Bump logging level
bachase Nov 2, 2017
5496ac4
[FOLD] Add additional logging
bachase Nov 7, 2017
4eff1ec
[FOLD] Copy consensus results into accept job
bachase Nov 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions src/ripple/app/consensus/RCLConsensus.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ RCLConsensus::Adaptor::onAccept(
app_.getJobQueue().addJob(
jtACCEPT,
"acceptLedger",
[&, cj = std::move(consensusJson) ](auto&) mutable {
[=, cj = std::move(consensusJson) ](auto&) mutable {
// Note that no lock is held or acquired during this job.
// This is because generic Consensus guarantees that once a ledger
// is accepted, the consensus results and capture by reference state
Expand Down Expand Up @@ -876,6 +876,16 @@ RCLConsensus::Adaptor::validate(RCLCxLedger const& ledger, bool proposing)
app_.overlay().send(val);
}

void
RCLConsensus::Adaptor::onModeChange(
ConsensusMode before,
ConsensusMode after)
{
JLOG(j_.info()) << "Consensus mode change before=" << to_string(before)
<< ", after=" << to_string(after);
mode_ = after;
}

Json::Value
RCLConsensus::getJson(bool full) const
{
Expand Down Expand Up @@ -950,14 +960,18 @@ RCLConsensus::Adaptor::preStartRound(RCLCxLedger const & prevLgr)
prevLgr.seq() >= app_.getMaxDisallowedLedger() &&
!app_.getOPs().isAmendmentBlocked();

const bool synced = app_.getOPs().getOperatingMode() == NetworkOPs::omFULL;

if (validating_)
{
JLOG(j_.info()) << "Entering consensus process, validating";
JLOG(j_.info()) << "Entering consensus process, validating, synced="
<< (synced ? "yes" : "no");
}
else
{
// Otherwise we just want to monitor the validation process.
JLOG(j_.info()) << "Entering consensus process, watching";
JLOG(j_.info()) << "Entering consensus process, watching, synced="
<< (synced ? "yes" : "no");
}

// Notify inbound ledgers that we are starting a new round
Expand All @@ -967,8 +981,7 @@ RCLConsensus::Adaptor::preStartRound(RCLCxLedger const & prevLgr)
parms_.useRoundedCloseTime = prevLgr.ledger_->rules().enabled(fix1528);

// propose only if we're in sync with the network (and validating)
return validating_ &&
(app_.getOPs().getOperatingMode() == NetworkOPs::omFULL);
return validating_ && synced;
}

void
Expand Down
12 changes: 8 additions & 4 deletions src/ripple/app/consensus/RCLConsensus.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,11 +239,15 @@ class RCLConsensus
RCLCxLedger const& ledger,
ConsensusMode mode);

/** Notified of change in consensus mode

@param before The prior consensus mode
@param after The new consensus mode
*/
void
onModeChange(ConsensusMode before, ConsensusMode after)
{
mode_ = after;
}
onModeChange(
ConsensusMode before,
ConsensusMode after);

/** Close the open ledger and return initial consensus position.

Expand Down
72 changes: 30 additions & 42 deletions src/ripple/app/misc/NetworkOPs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1275,29 +1275,8 @@ bool NetworkOPsImp::checkLastClosedLedger (

struct ValidationCount
{
int trustedValidations, nodesUsing;

ValidationCount() : trustedValidations(0), nodesUsing(0)
{
}

auto
asTie() const
{
return std::tie(trustedValidations, nodesUsing);
}

bool
operator>(const ValidationCount& v) const
{
return asTie() > v.asTie();
}

bool
operator==(const ValidationCount& v) const
{
return asTie() == v.asTie();
}
std::uint32_t trustedValidations = 0;
std::uint32_t nodesUsing = 0;
};

hash_map<uint256, ValidationCount> ledgers;
Expand Down Expand Up @@ -1327,38 +1306,47 @@ bool NetworkOPsImp::checkLastClosedLedger (
++ledgers[peerLedger].nodesUsing;
}

auto bestVC = ledgers[closedLedger];

// 3) Is there a network ledger we'd like to switch to? If so, do we have
// it?
bool switchLedgers = false;
ValidationCount bestCounts = ledgers[closedLedger];

for (auto const& it: ledgers)
{
JLOG(m_journal.debug()) << "L: " << it.first
<< " t=" << it.second.trustedValidations
<< ", n=" << it.second.nodesUsing;
uint256 const & currLedger = it.first;
ValidationCount const & currCounts = it.second;

// Temporary logging to make sure tiebreaking isn't broken
if (it.second.trustedValidations > 0)
JLOG(m_journal.trace())
<< " TieBreakTV: " << it.first;
else
JLOG(m_journal.debug()) << "L: " << currLedger
<< " t=" << currCounts.trustedValidations
<< ", n=" << currCounts.nodesUsing;

bool const preferCurr = [&]()
{
if (it.second.nodesUsing > 0)
// Prefer ledger with more trustedValidations
if (currCounts.trustedValidations > bestCounts.trustedValidations)
return true;
if (currCounts.trustedValidations < bestCounts.trustedValidations)
return false;
// If neither are trusted, prefer more nodesUsing
if (currCounts.trustedValidations == 0)
{
JLOG(m_journal.trace())
<< " TieBreakNU: " << it.first;
if (currCounts.nodesUsing > bestCounts.nodesUsing)
return true;
if (currCounts.nodesUsing < bestCounts.nodesUsing)
return false;
}
}
// If tied trustedValidations (non-zero) or tied nodesUsing,
// prefer higher ledger hash
return currLedger > closedLedger;

}();

// Switch to a ledger with more support
// or the one with higher hash if they have the same support
if (it.second > bestVC ||
(it.second == bestVC && it.first > closedLedger))
// Switch to current ledger if it is preferred over best so far
if (preferCurr)
{
bestVC = it.second;
closedLedger = it.first;
bestCounts = currCounts;
closedLedger = currLedger;
switchLedgers = true;
}
}
Copy link
Contributor

@nbougalis nbougalis Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the code from line 1313 through here can be replaced with this:

auto result = std::max_element(ledgers.begin(), ledgers.end(),
    [&m_journal](auto const& best, auto const& curr)
    {
        JLOG(m_journal.debug()) <<
            "L: " << curr.first <<
            ": t=" << curr.second.trustedValidations <<
            ", n=" << curr.second.nodesUsing;

        // Prefer ledger with more trustedValidations
        if (curr.second.trustedValidations > best.second.trustedValidations)
            return true;
        if (curr.second.trustedValidations < best.second.trustedValidations)
            return false;
        // If neither are trusted, prefer more nodesUsing
        if (curr.second.trustedValidations == 0)
        {
            if (curr.second.nodesUsing > best.second.nodesUsing)
                return true;
            if (curr.second.nodesUsing < best.second.nodesUsing)
                return false;
        }
        // If tied trustedValidations (non-zero) or tied nodesUsing,
        // prefer higher ledger hash
        return curr.first > best.first;
    });

assert (result != ledgers.end());

Note, that we are guaranteed that ledgers is not empty and will contain an entry for the initial value of closedLedger because of line 1294:

auto& ourVC = ledgers[closedLedger];

Determining whether we need to switch ledgers now becomes trivial: simply compare result->first to closedLedger. The bestCounts is no longer needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the suggestion, although I think it won't print the debug output for the first item in ledgers. This area is likely to see a refactor soon so I will take this idea up then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you’re right, it (understandably) won’t. That could be a problem when examining logs.

Expand Down
6 changes: 3 additions & 3 deletions src/ripple/overlay/impl/PeerImp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,7 +1305,7 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMStatusChange> const& m)
{
if (!closedLedgerHash_.isZero ())
{
JLOG(p_journal_.trace()) << "Status: Out of sync";
JLOG(p_journal_.debug()) << "Status: Out of sync";
closedLedgerHash_.zero ();
}

Expand All @@ -1318,11 +1318,11 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMStatusChange> const& m)
// a peer has changed ledgers
memcpy (closedLedgerHash_.begin (), m->ledgerhash ().data (), 256 / 8);
addLedger (closedLedgerHash_);
JLOG(p_journal_.trace()) << "LCL is " << closedLedgerHash_;
JLOG(p_journal_.debug()) << "LCL is " << closedLedgerHash_;
}
else
{
JLOG(p_journal_.trace()) << "Status: No ledger";
JLOG(p_journal_.debug()) << "Status: No ledger";
closedLedgerHash_.zero ();
}

Expand Down