Declare consensus if peers enter into a stable state

- Stable state means that neither we, nor any of our peers has changed
  a vote on a disputed transaction in a while. This is undesirable if an
  80% consensus has not otherwise been reached. It will cause
  a validation to be sent, which will help get other (trusting)
  validators back on track using preferred ledger logic.

Author: ximinez

src/test/consensus/Consensus_test.cpp

@@ -82,47 +82,96 @@ class Consensus_test : public beast::unit_test::suite
         // Use default parameterss
         ConsensusParms const p{};
 
+        ///////////////
+        // Disputes still in doubt
+        //
         // Not enough time has elapsed
         BEAST_EXPECT(
             ConsensusState::No ==
-            checkConsensus(10, 2, 2, 0, 3s, 2s, p, true, journal_));
+            checkConsensus(10, 2, 2, 0, 3s, 2s, false, p, true, journal_));
 
         // If not enough peers have propsed, ensure
         // more time for proposals
         BEAST_EXPECT(
             ConsensusState::No ==
-            checkConsensus(10, 2, 2, 0, 3s, 4s, p, true, journal_));
+            checkConsensus(10, 2, 2, 0, 3s, 4s, false, p, true, journal_));
 
         // Enough time has elapsed and we all agree
         BEAST_EXPECT(
             ConsensusState::Yes ==
-            checkConsensus(10, 2, 2, 0, 3s, 10s, p, true, journal_));
+            checkConsensus(10, 2, 2, 0, 3s, 10s, false, p, true, journal_));
 
         // Enough time has elapsed and we don't yet agree
         BEAST_EXPECT(
             ConsensusState::No ==
-            checkConsensus(10, 2, 1, 0, 3s, 10s, p, true, journal_));
+            checkConsensus(10, 2, 1, 0, 3s, 10s, false, p, true, journal_));
 
         // Our peers have moved on
         // Enough time has elapsed and we all agree
         BEAST_EXPECT(
             ConsensusState::MovedOn ==
-            checkConsensus(10, 2, 1, 8, 3s, 10s, p, true, journal_));
+            checkConsensus(10, 2, 1, 8, 3s, 10s, false, p, true, journal_));
 
         // If no peers, don't agree until time has passed.
         BEAST_EXPECT(
             ConsensusState::No ==
-            checkConsensus(0, 0, 0, 0, 3s, 10s, p, true, journal_));
+            checkConsensus(0, 0, 0, 0, 3s, 10s, false, p, true, journal_));
 
         // Agree if no peers and enough time has passed.
         BEAST_EXPECT(
             ConsensusState::Yes ==
-            checkConsensus(0, 0, 0, 0, 3s, 16s, p, true, journal_));
+            checkConsensus(0, 0, 0, 0, 3s, 16s, false, p, true, journal_));
 
         // Expire if too much time has passed without agreement
         BEAST_EXPECT(
             ConsensusState::Expired ==
-            checkConsensus(10, 8, 1, 0, 1s, 19s, p, true, journal_));
+            checkConsensus(10, 8, 1, 0, 1s, 19s, false, p, true, journal_));
+        ///////////////
+        // Stable state
+        //
+        // Not enough time has elapsed
+        BEAST_EXPECT(
+            ConsensusState::No ==
+            checkConsensus(10, 2, 2, 0, 3s, 2s, true, p, true, journal_));
+
+        // If not enough peers have propsed, ensure
+        // more time for proposals
+        BEAST_EXPECT(
+            ConsensusState::No ==
+            checkConsensus(10, 2, 2, 0, 3s, 4s, true, p, true, journal_));
+
+        // Enough time has elapsed and we all agree
+        BEAST_EXPECT(
+            ConsensusState::Yes ==
+            checkConsensus(10, 2, 2, 0, 3s, 10s, true, p, true, journal_));
+
+        // Enough time has elapsed and we don't yet agree, but there's nothing
+        // left to dispute
+        BEAST_EXPECT(
+            ConsensusState::Yes ==
+            checkConsensus(10, 2, 1, 0, 3s, 10s, true, p, true, journal_));
+
+        // Our peers have moved on
+        // Enough time has elapsed and we all agree, nothing left to dispute
+        BEAST_EXPECT(
+            ConsensusState::Yes ==
+            checkConsensus(10, 2, 1, 8, 3s, 10s, true, p, true, journal_));
+
+        // If no peers, don't agree until time has passed.
+        BEAST_EXPECT(
+            ConsensusState::No ==
+            checkConsensus(0, 0, 0, 0, 3s, 10s, true, p, true, journal_));
+
+        // Agree if no peers and enough time has passed.
+        BEAST_EXPECT(
+            ConsensusState::Yes ==
+            checkConsensus(0, 0, 0, 0, 3s, 16s, true, p, true, journal_));
+
+        // We are done if there's nothing left to dispute, no matter how much
+        // time has passed
+        BEAST_EXPECT(
+            ConsensusState::Yes ==
+            checkConsensus(10, 8, 1, 0, 1s, 19s, true, p, true, journal_));
     }
 
     void
@@ -1057,6 +1106,41 @@ class Consensus_test : public beast::unit_test::suite
         BEAST_EXPECT(sim.synchronized());
     }
 
+    void
+    testDisputes()
+    {
+        using namespace csf;
+        using namespace std::chrono;
+
+        std::uint32_t numPeers = 35;
+
+        ConsensusParms const parms{};
+        Sim sim;
+
+        PeerGroup peers = sim.createGroup(numPeers);
+
+        SimDuration delay = round<milliseconds>(0.2 * parms.ledgerGRANULARITY);
+        peers.trustAndConnect(peers, delay);
+
+        // Initial round to set prior state
+        sim.run(1);
+        for (Peer* peer : peers)
+        {
+            // Every transaction will be seen by every node but two.
+            // To accomplish that, every peer will add the ids of every peer
+            // except itself, and the one following.
+            auto const myId = peer->id;
+            auto const nextId = (peer->id + PeerID(1)) % PeerID(numPeers);
+            for (Peer* to : sim.trustGraph.trustedPeers(peer))
+            {
+                if (to->id == myId || to->id == nextId)
+                    continue;
+                peer->openTxs.insert(Tx{static_cast<std::uint32_t>(to->id)});
+            }
+        }
+        sim.run(1);
+    }
+
     void
     run() override
     {
@@ -1073,6 +1157,7 @@ class Consensus_test : public beast::unit_test::suite
         testHubNetwork();
         testPreferredByBranch();
         testPauseForLaggards();
+        testDisputes();
     }
 };
 

src/xrpld/app/consensus/RCLValidations.cpp

@@ -189,8 +189,9 @@ handleNewValidation(
     auto& validations = app.getValidations();
 
     // masterKey is seated only if validator is trusted or listed
-    auto const outcome =
-        validations.add(calcNodeID(masterKey.value_or(signingKey)), val);
+    auto const nodeKey = masterKey.value_or(signingKey);
+    assert(nodeKey != app.getValidationPublicKey());
+    auto const outcome = validations.add(calcNodeID(nodeKey), val);
 
     if (outcome == ValStatus::current)
     {

src/xrpld/consensus/Consensus.cpp

@@ -88,7 +88,8 @@ checkConsensusReached(
     std::size_t total,
     bool count_self,
     std::size_t minConsensusPct,
-    bool reachedMax)
+    bool reachedMax,
+    bool stableState)
 {
     // If we are alone for too long, we have consensus.
     // Delaying consensus like this avoids a circumstance where a peer
@@ -114,7 +115,7 @@ checkConsensusReached(
 
     std::size_t currentPercentage = (agreeing * 100) / total;
 
-    return currentPercentage >= minConsensusPct;
+    return currentPercentage >= minConsensusPct || stableState;
 }
 
 ConsensusState
@@ -125,6 +126,7 @@ checkConsensus(
     std::size_t currentFinished,
     std::chrono::milliseconds previousAgreeTime,
     std::chrono::milliseconds currentAgreeTime,
+    bool stableState,
     ConsensusParms const& parms,
     bool proposing,
     beast::Journal j)
@@ -162,9 +164,11 @@ checkConsensus(
             currentProposers,
             proposing,
             parms.minCONSENSUS_PCT,
-            currentAgreeTime > parms.ledgerMAX_CONSENSUS))
+            currentAgreeTime > parms.ledgerMAX_CONSENSUS,
+            stableState))
     {
-        JLOG(j.debug()) << "normal consensus";
+        JLOG(j.debug()) << "normal consensus with " << (stableState ? "" : "un")
+                        << "stable state";
         return ConsensusState::Yes;
     }
 
@@ -175,7 +179,8 @@ checkConsensus(
             currentProposers,
             false,
             parms.minCONSENSUS_PCT,
-            currentAgreeTime > parms.ledgerMAX_CONSENSUS))
+            currentAgreeTime > parms.ledgerMAX_CONSENSUS,
+            false))
     {
         JLOG(j.warn()) << "We see no consensus, but 80% of nodes have moved on";
         return ConsensusState::MovedOn;

src/xrpld/consensus/Consensus.h

@@ -79,6 +79,10 @@ shouldCloseLedger(
                              last ledger
     @param currentAgreeTime how long, in milliseconds, we've been trying to
                             agree
+    @param stableState the network appears to be in a stable state, where
+           neither we nor our peers have changed their vote on any disputes in a
+           while. This is undesirable, and will cause us to end consensus
+           without 80% agreement.
     @param parms            Consensus constant parameters
     @param proposing        whether we should count ourselves
     @param j                journal for logging
@@ -91,6 +95,7 @@ checkConsensus(
     std::size_t currentFinished,
     std::chrono::milliseconds previousAgreeTime,
     std::chrono::milliseconds currentAgreeTime,
+    bool stableState,
     ConsensusParms const& parms,
     bool proposing,
     beast::Journal j);
@@ -559,6 +564,9 @@ class Consensus
 
     NetClock::duration closeResolution_ = ledgerDefaultTimeResolution;
 
+    ConsensusParms::AvalancheState closeTimeAvalancheState_ =
+        ConsensusParms::init;
+
     // Time it took for the last consensus round to converge
     std::chrono::milliseconds prevRoundTime_;
 
@@ -676,6 +684,7 @@ Consensus<Adaptor>::startRoundInternal(
     previousLedger_ = prevLedger;
     result_.reset();
     convergePercent_ = 0;
+    closeTimeAvalancheState_ = ConsensusParms::init;
     haveCloseTimeConsensus_ = false;
     openTime_.reset(clock_.now());
     currPeerPositions_.clear();
@@ -832,6 +841,8 @@ Consensus<Adaptor>::timerEntry(NetClock::time_point const& now)
     }
     else if (phase_ == ConsensusPhase::establish)
     {
+        if (result_)
+            ++result_->peerUnchangedCounter;
         phaseEstablish();
     }
 }
@@ -1443,16 +1454,10 @@ Consensus<Adaptor>::updateOurPositions()
     }
     else
     {
-        int neededWeight;
-
-        if (convergePercent_ < parms.avMID_CONSENSUS_TIME)
-            neededWeight = parms.avINIT_CONSENSUS_PCT;
-        else if (convergePercent_ < parms.avLATE_CONSENSUS_TIME)
-            neededWeight = parms.avMID_CONSENSUS_PCT;
-        else if (convergePercent_ < parms.avSTUCK_CONSENSUS_TIME)
-            neededWeight = parms.avLATE_CONSENSUS_PCT;
-        else
-            neededWeight = parms.avSTUCK_CONSENSUS_PCT;
+        auto const [neededWeight, newState] = getNeededWeight(
+            parms, closeTimeAvalancheState_, convergePercent_, {});
+        if (newState)
+            closeTimeAvalancheState_ = *newState;
 
         int participants = currPeerPositions_.size();
         if (mode_.get() == ConsensusMode::proposing)
@@ -1566,7 +1571,8 @@ Consensus<Adaptor>::haveConsensus()
         }
         else
         {
-            JLOG(j_.debug()) << nodeId << " has " << peerProp.position();
+            JLOG(j_.debug()) << "Proposal disagreement: Peer " << nodeId
+                             << " has " << peerProp.position();
             ++disagree;
         }
     }
@@ -1576,6 +1582,18 @@ Consensus<Adaptor>::haveConsensus()
     JLOG(j_.debug()) << "Checking for TX consensus: agree=" << agree
                      << ", disagree=" << disagree;
 
+    ConsensusParms const& parms = adaptor_.parms();
+    // stable state is NOT desirable if we don't have 80% agreement
+    bool stableState = true;
+    for (auto const& [txid, dt] : result_->disputes)
+    {
+        if (!dt.stableState(parms, result_->peerUnchangedCounter))
+        {
+            stableState = false;
+            break;
+        }
+    }
+
     // Determine if we actually have consensus or not
     result_->state = checkConsensus(
         prevProposers_,
@@ -1584,7 +1602,8 @@ Consensus<Adaptor>::haveConsensus()
         currentFinished,
         prevRoundTime_,
         result_->roundTime.read(),
-        adaptor_.parms(),
+        stableState,
+        parms,
         mode_.get() == ConsensusMode::proposing,
         j_);
 
@@ -1676,8 +1695,9 @@ Consensus<Adaptor>::createDisputes(TxSet_t const& o)
         {
             Proposal_t const& peerProp = peerPos.proposal();
             auto const cit = acquired_.find(peerProp.position());
-            if (cit != acquired_.end())
-                dtx.setVote(nodeId, cit->second.exists(txID));
+            if (cit != acquired_.end() &&
+                dtx.setVote(nodeId, cit->second.exists(txID)))
+                result_->peerUnchangedCounter = 0;
         }
         adaptor_.share(dtx.tx());
 
@@ -1701,7 +1721,8 @@ Consensus<Adaptor>::updateDisputes(NodeID_t const& node, TxSet_t const& other)
     for (auto& it : result_->disputes)
     {
         auto& d = it.second;
-        d.setVote(node, other.exists(d.tx().id()));
+        if (d.setVote(node, other.exists(d.tx().id())))
+            result_->peerUnchangedCounter = 0;
     }
 }