From a429c235babe44eacfb7072283e80d3e513fed63 Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Wed, 1 Nov 2017 20:42:40 -0400 Subject: [PATCH 1/4] Properly use ledger hash to break ties --- src/ripple/app/misc/NetworkOPs.cpp | 72 +++++++++++++----------------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 1743034b118..f85d3379c4c 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -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 ledgers; @@ -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; } } From e11279f9d9d158f89d75332fb822cd8612ac04c0 Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Thu, 2 Nov 2017 15:41:08 -0400 Subject: [PATCH 2/4] [FOLD] Bump logging level --- src/ripple/overlay/impl/PeerImp.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index f8d634c1319..442f6e063a1 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/src/ripple/overlay/impl/PeerImp.cpp @@ -1305,7 +1305,7 @@ PeerImp::onMessage (std::shared_ptr const& m) { if (!closedLedgerHash_.isZero ()) { - JLOG(p_journal_.trace()) << "Status: Out of sync"; + JLOG(p_journal_.debug()) << "Status: Out of sync"; closedLedgerHash_.zero (); } @@ -1318,11 +1318,11 @@ PeerImp::onMessage (std::shared_ptr 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 (); } From 5496ac4d34c2681761732af8f656c14f3992c6e9 Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Tue, 7 Nov 2017 09:13:38 -0500 Subject: [PATCH 3/4] [FOLD] Add additional logging --- src/ripple/app/consensus/RCLConsensus.cpp | 21 +++++++++++++++++---- src/ripple/app/consensus/RCLConsensus.h | 12 ++++++++---- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index 1c74d4134cf..19d117c12a6 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -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 { @@ -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 @@ -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 diff --git a/src/ripple/app/consensus/RCLConsensus.h b/src/ripple/app/consensus/RCLConsensus.h index 5522b8d4e87..93bf2a2aafa 100644 --- a/src/ripple/app/consensus/RCLConsensus.h +++ b/src/ripple/app/consensus/RCLConsensus.h @@ -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. From 4eff1ec2bfc47e76ff44d179381f024b2feebd7e Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Wed, 8 Nov 2017 11:38:52 -0500 Subject: [PATCH 4/4] [FOLD] Copy consensus results into accept job --- src/ripple/app/consensus/RCLConsensus.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index 19d117c12a6..2dc47223142 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -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