diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index 94d4ace251..1c74d4134c 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -118,6 +118,9 @@ RCLConsensus::Adaptor::acquireLedger(LedgerHash const& ledger) // Notify inbound transactions of the new ledger sequence number inboundTransactions_.newRound(buildLCL->info().seq); + // Use the ledger timing rules of the acquired ledger + parms_.useRoundedCloseTime = buildLCL->rules().enabled(fix1528); + return RCLCxLedger(buildLCL); } @@ -957,9 +960,12 @@ RCLConsensus::Adaptor::preStartRound(RCLCxLedger const & prevLgr) JLOG(j_.info()) << "Entering consensus process, watching"; } - // Notify inbOund ledgers that we are starting a new round + // Notify inbound ledgers that we are starting a new round inboundTransactions_.newRound(prevLgr.seq()); + // Use parent ledger's rules to determine whether to use rounded close time + 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); diff --git a/src/ripple/app/main/Amendments.cpp b/src/ripple/app/main/Amendments.cpp index 5407bad02d..c8d4ed9443 100644 --- a/src/ripple/app/main/Amendments.cpp +++ b/src/ripple/app/main/Amendments.cpp @@ -59,7 +59,8 @@ supportedAmendments () { "CC5ABAE4F3EC92E94A59B1908C2BE82D2228B6485C00AFF8F22DF930D89C194E SortedDirectories" }, { "B4D44CC3111ADD964E846FC57760C8B50FFCD5A82C86A72756F6B058DDDF96AD fix1201" }, { "6C92211186613F9647A89DFFBAB8F94C99D4C7E956D495270789128569177DA1 fix1512" }, - { "B9E739B8296B4A1BB29BE990B17D66E21B62A300A909F25AC55C22D6C72E1F9D fix1523" } + { "B9E739B8296B4A1BB29BE990B17D66E21B62A300A909F25AC55C22D6C72E1F9D fix1523" }, + { "1D3463A5891F9E589C5AE839FFAC4A917CE96197098A1EF22304E1BC5B98A454 fix1528" } }; } diff --git a/src/ripple/consensus/Consensus.h b/src/ripple/consensus/Consensus.h index 2484a4b16b..572430f13d 100644 --- a/src/ripple/consensus/Consensus.h +++ b/src/ripple/consensus/Consensus.h @@ -494,6 +494,10 @@ private: void leaveConsensus(); + // The rounded or effective close time estimate from a proposer + NetClock::time_point + asCloseTime(NetClock::time_point raw) const; + private: Adaptor& adaptor_; @@ -1204,7 +1208,7 @@ Consensus::updateOurPositions() auto const ourCutoff = now_ - parms.proposeINTERVAL; // Verify freshness of peer positions and compute close times - std::map effCloseTimes; + std::map closeTimeVotes; { auto it = currPeerPositions_.begin(); while (it != currPeerPositions_.end()) @@ -1222,10 +1226,7 @@ Consensus::updateOurPositions() else { // proposal is still fresh - ++effCloseTimes[effCloseTime( - peerProp.closeTime(), - closeResolution_, - previousLedger_.closeTime())]; + ++closeTimeVotes[asCloseTime(peerProp.closeTime())]; ++it; } } @@ -1273,10 +1274,7 @@ Consensus::updateOurPositions() { // no other times haveCloseTimeConsensus_ = true; - consensusCloseTime = effCloseTime( - result_->position.closeTime(), - closeResolution_, - previousLedger_.closeTime()); + consensusCloseTime = asCloseTime(result_->position.closeTime()); } else { @@ -1294,10 +1292,7 @@ Consensus::updateOurPositions() int participants = currPeerPositions_.size(); if (mode_.get() == ConsensusMode::proposing) { - ++effCloseTimes[effCloseTime( - result_->position.closeTime(), - closeResolution_, - previousLedger_.closeTime())]; + ++closeTimeVotes[asCloseTime(result_->position.closeTime())]; ++participants; } @@ -1312,7 +1307,7 @@ Consensus::updateOurPositions() << " nw:" << neededWeight << " thrV:" << threshVote << " thrC:" << threshConsensus; - for (auto const& it : effCloseTimes) + for (auto const& it : closeTimeVotes) { JLOG(j_.debug()) << "CCTime: seq " << previousLedger_.seq() + 1 << ": " @@ -1342,11 +1337,7 @@ Consensus::updateOurPositions() } if (!ourNewSet && - ((consensusCloseTime != - effCloseTime( - result_->position.closeTime(), - closeResolution_, - previousLedger_.closeTime())) || + ((consensusCloseTime != asCloseTime(result_->position.closeTime())) || result_->position.isStale(ourCutoff))) { // close time changed or our position is stale @@ -1538,6 +1529,16 @@ Consensus::updateDisputes(NodeID_t const& node, TxSet_t const& other) } } +template +NetClock::time_point +Consensus::asCloseTime(NetClock::time_point raw) const +{ + if (adaptor_.parms().useRoundedCloseTime) + return roundCloseTime(raw, closeResolution_); + else + return effCloseTime(raw, closeResolution_, previousLedger_.closeTime()); +} + } // namespace ripple #endif diff --git a/src/ripple/consensus/ConsensusParms.h b/src/ripple/consensus/ConsensusParms.h index 02ed12a61f..569c6547ee 100644 --- a/src/ripple/consensus/ConsensusParms.h +++ b/src/ripple/consensus/ConsensusParms.h @@ -129,6 +129,17 @@ struct ConsensusParms //! Percentage of nodes required to reach agreement on ledger close time std::size_t avCT_CONSENSUS_PCT = 75; + + //-------------------------------------------------------------------------- + + /** Whether to use roundCloseTime or effCloseTime for reaching close time + consensus. + This was added to migrate from effCloseTime to roundCloseTime on the + live network. The desired behavior (as given by the default value) is + to use roundCloseTime during consensus voting and then use effCloseTime + when accepting the consensus ledger. + */ + bool useRoundedCloseTime = true; }; } // ripple diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 9f99a87ff0..aadde6e3b9 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -71,7 +71,8 @@ class FeatureCollections "SortedDirectories", "fix1201", "fix1512", - "fix1523" + "fix1523", + "fix1528" }; std::vector features; @@ -168,6 +169,7 @@ extern uint256 const featureSortedDirectories; extern uint256 const fix1201; extern uint256 const fix1512; extern uint256 const fix1523; +extern uint256 const fix1528; } // ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index f7f10b1322..97a280f121 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -117,5 +117,6 @@ uint256 const featureSortedDirectories = *getRegisteredFeature("SortedDirectorie uint256 const fix1201 = *getRegisteredFeature("fix1201"); uint256 const fix1512 = *getRegisteredFeature("fix1512"); uint256 const fix1523 = *getRegisteredFeature("fix1523"); +uint256 const fix1528 = *getRegisteredFeature("fix1528"); } // ripple diff --git a/src/test/consensus/Consensus_test.cpp b/src/test/consensus/Consensus_test.cpp index 1221958ad8..669bbf9fc2 100644 --- a/src/test/consensus/Consensus_test.cpp +++ b/src/test/consensus/Consensus_test.cpp @@ -521,6 +521,152 @@ public: } } + void + testConsensusCloseTimeRounding() + { + using namespace csf; + using namespace std::chrono; + + // This is a specialized test engineered to yield ledgers with different + // close times even though the peers believe they had close time + // consensus on the ledger. + + for (bool useRoundedCloseTime : {false, true}) + { + ConsensusParms parms; + parms.useRoundedCloseTime = useRoundedCloseTime; + std::vector unls; + unls.push_back({0,1,2,3,4,5}); + + std::vector membership(unls[0].size(), 0); + + TrustGraph tg{unls, membership}; + + // This requires a group of 4 fast and 2 slow peers to create a + // situation in which a subset of peers requires seeing additional + // proposals to declare consensus. + + Sim sim( + parms, + tg, + topology(tg, [&](PeerID i, PeerID j) { + auto delayFactor = (i <= 1 || j <= 1) ? 1.1 : 0.2; + return round( + delayFactor * parms.ledgerGRANULARITY); + })); + + // Run to the ledger *prior* to decreasing the resolution + sim.run(increaseLedgerTimeResolutionEvery - 2); + + // In order to create the discrepency, we want a case where if + // X = effCloseTime(closeTime, resolution, parentCloseTime) + // X != effCloseTime(X, resolution, parentCloseTime) + // + // That is, the effective close time is not a fixed point. This can + // happen if X = parentCloseTime + 1, but a subsequent rounding goes + // to the next highest multiple of resolution. + + // So we want to find an offset (now + offset) % 30s = 15 + // (now + offset) % 20s = 15 + // This way, the next ledger will close and round up Due to the + // network delay settings, the round of consensus will take 5s, so + // the next ledger's close time will + + NetClock::duration when = sim.peers[0].now().time_since_epoch(); + + // Check we are before the 30s to 20s transition + NetClock::duration resolution = + sim.peers[0].lastClosedLedger.closeTimeResolution(); + BEAST_EXPECT(resolution == NetClock::duration{30s}); + + while ( + ((when % NetClock::duration{30s}) != NetClock::duration{15s}) || + ((when % NetClock::duration{20s}) != NetClock::duration{15s})) + when += 1s; + // Advance the clock without consensus running (IS THIS WHAT + // PREVENTS IT IN PRACTICE?) + sim.net.step_for( + NetClock::time_point{when} - sim.peers[0].now()); + + // Run one more ledger with 30s resolution + sim.run(1); + + // close time should be ahead of clock time since we engineered + // the close time to round up + for (Peer const & peer : sim.peers) + { + BEAST_EXPECT( + peer.lastClosedLedger.closeTime() > peer.now()); + BEAST_EXPECT(peer.lastClosedLedger.closeAgree()); + BEAST_EXPECT( + peer.lastClosedLedger.id() == + sim.peers[0].lastClosedLedger.id()); + } + + + // All peers submit their own ID as a transaction + for (Peer & peer : sim.peers) + peer.submit(Tx{static_cast(peer.id)}); + + // Run 1 more round, this time it will have a decreased + // resolution of 20 seconds. + + // The network delays are engineered so that the slow peers + // initially have the wrong tx hash, but they see a majority + // of agreement from their peers and declare consensus + // + // The trick is that everyone starts with a raw close time of + // 84681s + // Which has + // effCloseTime(86481s, 20s, 86490s) = 86491s + // However, when the slow peers update their position, they change + // the close time to 86451s. The fast peers declare consensus with + // the 86481s as their position still. + // + // When accepted the ledger + // - fast peers use eff(86481s) -> 86491s as the close time + // - slow peers use eff(eff(86481s)) -> eff(86491s) -> 86500s! + + sim.run(1); + + for (Peer const& peer : sim.peers) + { + BEAST_EXPECT( + peer.lastClosedLedger.id() == + sim.peers[0].lastClosedLedger.id()); + } + + if (!parms.useRoundedCloseTime) + { + auto const & slowLCL = sim.peers[0].lastClosedLedger; + auto const & fastLCL = sim.peers[2].lastClosedLedger; + + + // Agree on parent close and close resolution + BEAST_EXPECT( + slowLCL.parentCloseTime() == + fastLCL.parentCloseTime()); + BEAST_EXPECT( + slowLCL.closeTimeResolution() == + fastLCL.closeTimeResolution()); + + auto parentClose = slowLCL.parentCloseTime(); + auto closeResolution = + slowLCL.closeTimeResolution(); + + auto slowClose = sim.peers[0].lastClosedLedger.closeTime(); + auto fastClose = sim.peers[2].lastClosedLedger.closeTime(); + + // Close times disagree ... + BEAST_EXPECT(slowClose != fastClose); + // Effective close times agree! The slow peer already rounded! + BEAST_EXPECT( + effCloseTime(slowClose, closeResolution, parentClose) == + effCloseTime(fastClose, closeResolution, parentClose)); + } + } + } + void testFork() { @@ -691,6 +837,7 @@ public: testSlowPeers(); testCloseTimeDisagree(); testWrongLCL(); + testConsensusCloseTimeRounding(); testFork(); simClockSkew(); diff --git a/src/test/csf/Ledger.h b/src/test/csf/Ledger.h index addc064273..03d3fef001 100644 --- a/src/test/csf/Ledger.h +++ b/src/test/csf/Ledger.h @@ -102,12 +102,6 @@ public: return closeTime_; } - auto - actualCloseTime() const - { - return actualCloseTime_; - } - auto parentCloseTime() const { @@ -140,9 +134,8 @@ public: res.id_.txs.insert(txs.begin(), txs.end()); res.id_.seq = seq() + 1; res.closeTimeResolution_ = closeTimeResolution; - res.actualCloseTime_ = consensusCloseTime; res.closeTime_ = effCloseTime( - consensusCloseTime, closeTimeResolution, parentCloseTime_); + consensusCloseTime, closeTimeResolution, closeTime()); res.closeTimeAgree_ = closeTimeAgree; res.parentCloseTime_ = closeTime(); res.parentID_ = id(); @@ -167,9 +160,6 @@ private: //! Parent ledger close time NetClock::time_point parentCloseTime_; - - //! Close time unadjusted by closeTimeResolution - NetClock::time_point actualCloseTime_; }; inline std::ostream& diff --git a/src/test/csf/Peer.h b/src/test/csf/Peer.h index 41e5aae9dd..f877f38aa7 100644 --- a/src/test/csf/Peer.h +++ b/src/test/csf/Peer.h @@ -311,7 +311,7 @@ struct Peer auto newLedger = prevLedger.close( result.set.txs_, closeResolution, - rawCloseTimes.self, + result.position.closeTime(), result.position.closeTime() != NetClock::time_point{}); ledgers[newLedger.id()] = newLedger; prevProposers_ = result.proposers;