Use rounded close time in Consensus (RIPD-1528):

Switches the default behavior of Consensus to use roundCloseTime instead of
effCloseTime. effCloseTime is still used when accepting the consensus ledger to
ensure the consensus close time comes after the parent ledger close time. This
change eliminates an edge case in which peers could reach agreement on the close
time, but end up generating ledgers with different close times.

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);

src/ripple/app/main/Amendments.cpp

@@ -59,7 +59,8 @@ supportedAmendments ()
         { "CC5ABAE4F3EC92E94A59B1908C2BE82D2228B6485C00AFF8F22DF930D89C194E SortedDirectories" },
         { "B4D44CC3111ADD964E846FC57760C8B50FFCD5A82C86A72756F6B058DDDF96AD fix1201" },
         { "6C92211186613F9647A89DFFBAB8F94C99D4C7E956D495270789128569177DA1 fix1512" },
-        { "B9E739B8296B4A1BB29BE990B17D66E21B62A300A909F25AC55C22D6C72E1F9D fix1523" }
+        { "B9E739B8296B4A1BB29BE990B17D66E21B62A300A909F25AC55C22D6C72E1F9D fix1523" },
+        { "1D3463A5891F9E589C5AE839FFAC4A917CE96197098A1EF22304E1BC5B98A454 fix1528" }
     };
 }
 

src/ripple/consensus/Consensus.h

@@ -494,6 +494,10 @@ class Consensus
     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<Adaptor>::updateOurPositions()
     auto const ourCutoff = now_ - parms.proposeINTERVAL;
 
     // Verify freshness of peer positions and compute close times
-    std::map<NetClock::time_point, int> effCloseTimes;
+    std::map<NetClock::time_point, int> closeTimeVotes;
     {
         auto it = currPeerPositions_.begin();
         while (it != currPeerPositions_.end())
@@ -1222,10 +1226,7 @@ Consensus<Adaptor>::updateOurPositions()
             else
             {
                 // proposal is still fresh
-                ++effCloseTimes[effCloseTime(
-                    peerProp.closeTime(),
-                    closeResolution_,
-                    previousLedger_.closeTime())];
+                ++closeTimeVotes[asCloseTime(peerProp.closeTime())];
                 ++it;
             }
         }
@@ -1273,10 +1274,7 @@ Consensus<Adaptor>::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<Adaptor>::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<Adaptor>::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<Adaptor>::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<Adaptor>::updateDisputes(NodeID_t const& node, TxSet_t const& other)
     }
 }
 
+template <class Adaptor>
+NetClock::time_point
+Consensus<Adaptor>::asCloseTime(NetClock::time_point raw) const
+{
+    if (adaptor_.parms().useRoundedCloseTime)
+        return roundCloseTime(raw, closeResolution_);
+    else
+        return effCloseTime(raw, closeResolution_, previousLedger_.closeTime());
+}
+
 }  // namespace ripple
 
 #endif

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

src/ripple/protocol/Feature.h

@@ -71,7 +71,8 @@ class FeatureCollections
         "SortedDirectories",
         "fix1201",
         "fix1512",
-        "fix1523"
+        "fix1523",
+        "fix1528"
     };
 
     std::vector<uint256> 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
 

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

src/test/consensus/Consensus_test.cpp

@@ -521,6 +521,152 @@ class Consensus_test : public beast::unit_test::suite
         }
     }
 
+    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<UNL> unls;
+            unls.push_back({0,1,2,3,4,5});
+
+            std::vector<int> 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<milliseconds>(
+                            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<std::uint32_t>(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 @@ class Consensus_test : public beast::unit_test::suite
         testSlowPeers();
         testCloseTimeDisagree();
         testWrongLCL();
+        testConsensusCloseTimeRounding();
         testFork();
 
         simClockSkew();

src/test/csf/Ledger.h

@@ -102,12 +102,6 @@ class Ledger
         return closeTime_;
     }
 
-    auto
-    actualCloseTime() const
-    {
-        return actualCloseTime_;
-    }
-
     auto
     parentCloseTime() const
     {
@@ -140,9 +134,8 @@ class Ledger
         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 @@ class Ledger
 
     //! Parent ledger close time
     NetClock::time_point parentCloseTime_;
-
-    //! Close time unadjusted by closeTimeResolution
-    NetClock::time_point actualCloseTime_;
 };
 
 inline std::ostream&

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;