Merge pull request #1768 from MonsieurNicolas/SCPFixHighConfirmed

Scp fix for setting `h.n` in PREPARE message

Reviewed-by: MonsieurNicolas

src/herder/HerderImpl.cpp

@@ -174,9 +174,20 @@ HerderImpl::valueExternalized(uint64 slotIndex, StellarValue const& value)
     // called both here and at the end (this one is in case of an exception)
     trackingHeartBeat();
 
+    bool validated = getSCP().isSlotFullyValidated(slotIndex);
+
     if (Logging::logDebug("Herder"))
-        CLOG(DEBUG, "Herder") << "HerderSCPDriver::valueExternalized"
-                              << " txSet: " << hexAbbrev(value.txSetHash);
+        CLOG(DEBUG, "Herder") << fmt::format(
+            "HerderSCPDriver::valueExternalized index: {} txSet: {}", slotIndex,
+            hexAbbrev(value.txSetHash));
+
+    if (getSCP().isValidator() && !validated)
+    {
+        CLOG(WARNING, "Herder")
+            << fmt::format("Ledger {} ({}) closed and could NOT be fully "
+                           "validated by validator",
+                           slotIndex, hexAbbrev(value.txSetHash));
+    }
 
     TxSetFramePtr externalizedSet = mPendingEnvelopes.getTxSet(value.txSetHash);
 

src/herder/HerderSCPDriver.cpp

@@ -198,6 +198,11 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex,
         {
             // if we're not tracking, there is not much more we can do to
             // validate
+            if (Logging::logTrace("Herder"))
+            {
+                CLOG(TRACE, "Herder")
+                    << "MaybeValidValue (not tracking) for slot " << slotIndex;
+            }
             return SCPDriver::kMaybeValidValue;
         }
 
@@ -206,6 +211,12 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex,
         {
             // we already moved on from this slot
             // still send it through for emitting the final messages
+            if (Logging::logTrace("Herder"))
+            {
+                CLOG(TRACE, "Herder")
+                    << "MaybeValidValue (already moved on) for slot "
+                    << slotIndex << ", at " << nextConsensusLedgerIndex();
+            }
             return SCPDriver::kMaybeValidValue;
         }
         if (nextConsensusLedgerIndex() < slotIndex)
@@ -215,8 +226,8 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex,
             CLOG(ERROR, "Herder")
                 << "HerderSCPDriver::validateValue"
                 << " i: " << slotIndex
-                << " processing a future message while tracking";
-
+                << " processing a future message while tracking; got: "
+                << nextConsensusLedgerIndex();
             return SCPDriver::kInvalidValue;
         }
         lastCloseTime = mTrackingSCP->mConsensusValue.closeTime;
@@ -225,19 +236,37 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex,
     // Check closeTime (not too old)
     if (b.closeTime <= lastCloseTime)
     {
+        if (Logging::logTrace("Herder"))
+        {
+            CLOG(TRACE, "Herder")
+                << "Close time too old for slot " << slotIndex << ", got "
+                << b.closeTime << " vs " << lastCloseTime;
+        }
         return SCPDriver::kInvalidValue;
     }
 
     // Check closeTime (not too far in future)
     uint64_t timeNow = mApp.timeNow();
     if (b.closeTime > timeNow + Herder::MAX_TIME_SLIP_SECONDS.count())
     {
+        if (Logging::logTrace("Herder"))
+        {
+            CLOG(TRACE, "Herder")
+                << "Close time too recent for slot " << slotIndex << ", got "
+                << b.closeTime << " vs " << timeNow;
+        }
         return SCPDriver::kInvalidValue;
     }
 
     if (!compat)
     {
         // this is as far as we can go if we don't have the state
+        if (Logging::logTrace("Herder"))
+        {
+            CLOG(TRACE, "Herder")
+                << "Can't validate locally, value may be valid for slot "
+                << slotIndex;
+        }
         return SCPDriver::kMaybeValidValue;
     }
 

src/scp/BallotProtocol.cpp

@@ -369,10 +369,11 @@ BallotProtocol::bumpState(Value const& value, uint32 n)
 
     newb.counter = n;
 
-    if (mHighBallot)
+    if (mValueOverride)
     {
-        // can only bump the counter if we committed to something already
-        newb.value = mHighBallot->value;
+        // we use the value that we saw confirmed prepared
+        // or that we at least voted to commit to
+        newb.value = *mValueOverride;
     }
     else
     {
@@ -483,6 +484,12 @@ BallotProtocol::bumpToBallot(SCPBallot const& ballot, bool check)
 
     mCurrentBallot = std::make_unique<SCPBallot>(ballot);
 
+    // invariant: h.value = b.value
+    if (mHighBallot && !areBallotsCompatible(*mCurrentBallot, *mHighBallot))
+    {
+        mHighBallot.reset();
+    }
+
     if (gotBumped)
     {
         mHeardFromQuorum = false;
@@ -556,7 +563,6 @@ BallotProtocol::createStatement(SCPStatementType const& type)
     {
         auto& c = statement.pledges.confirm();
         c.quorumSetHash = getLocalNode()->getQuorumSetHash();
-        dbgAssert(areBallotsLessAndCompatible(*mCommit, *mHighBallot));
         c.ballot = *mCurrentBallot;
         c.nPrepared = mPrepared->counter;
         c.nCommit = mCommit->counter;
@@ -565,7 +571,6 @@ BallotProtocol::createStatement(SCPStatementType const& type)
     break;
     case SCPStatementType::SCP_ST_EXTERNALIZE:
     {
-        dbgAssert(areBallotsLessAndCompatible(*mCommit, *mHighBallot));
         auto& e = statement.pledges.externalize();
         e.commit = *mCommit;
         e.nH = mHighBallot->counter;
@@ -643,6 +648,11 @@ BallotProtocol::checkInvariants()
     {
         dbgAssert(areBallotsLessAndIncompatible(*mPreparedPrime, *mPrepared));
     }
+    if (mHighBallot)
+    {
+        dbgAssert(mCurrentBallot);
+        dbgAssert(areBallotsLessAndCompatible(*mHighBallot, *mCurrentBallot));
+    }
     if (mCommit)
     {
         dbgAssert(mCurrentBallot);
@@ -770,13 +780,16 @@ BallotProtocol::getPrepareCandidates(SCPStatement const& hint)
     return candidates;
 }
 
-void
-BallotProtocol::updateCurrentIfNeeded()
+bool
+BallotProtocol::updateCurrentIfNeeded(SCPBallot const& h)
 {
-    if (!mCurrentBallot || compareBallots(*mCurrentBallot, *mHighBallot) < 0)
+    bool didWork = false;
+    if (!mCurrentBallot || compareBallots(*mCurrentBallot, h) < 0)
     {
-        bumpToBallot(*mHighBallot, true);
+        bumpToBallot(h, true);
+        didWork = true;
     }
+    return didWork;
 }
 
 bool
@@ -961,6 +974,11 @@ BallotProtocol::attemptPreparedConfirmed(SCPStatement const& hint)
                 {
                     break;
                 }
+                // c and h must be compatible
+                if (!areBallotsLessAndCompatible(*cur, newH))
+                {
+                    continue;
+                }
                 bool ratified = federatedRatify(
                     std::bind(&BallotProtocol::hasPreparedBallot, ballot, _1));
                 if (ratified)
@@ -1023,25 +1041,37 @@ BallotProtocol::setPreparedConfirmed(SCPBallot const& newC,
 
     bool didWork = false;
 
-    if (!mHighBallot || compareBallots(newH, *mHighBallot) > 0)
-    {
-        didWork = true;
-        mHighBallot = std::make_unique<SCPBallot>(newH);
-    }
+    // remember newH's value
+    mValueOverride = std::make_unique<Value>(newH.value);
 
-    if (newC.counter != 0)
+    // we don't set c/h if we're not on a compatible ballot
+    if (!mCurrentBallot || areBallotsCompatible(*mCurrentBallot, newH))
     {
-        dbgAssert(!mCommit);
-        mCommit = std::make_unique<SCPBallot>(newC);
-        didWork = true;
+        if (!mHighBallot || compareBallots(newH, *mHighBallot) > 0)
+        {
+            didWork = true;
+            mHighBallot = std::make_unique<SCPBallot>(newH);
+        }
+
+        if (newC.counter != 0)
+        {
+            dbgAssert(!mCommit);
+            mCommit = std::make_unique<SCPBallot>(newC);
+            didWork = true;
+        }
+
+        if (didWork)
+        {
+            mSlot.getSCPDriver().confirmedBallotPrepared(mSlot.getSlotIndex(),
+                                                         newH);
+        }
     }
 
+    // always perform step (8) with the computed value of h
+    didWork = updateCurrentIfNeeded(newH) || didWork;
+
     if (didWork)
     {
-        updateCurrentIfNeeded();
-
-        mSlot.getSCPDriver().confirmedBallotPrepared(mSlot.getSlotIndex(),
-                                                     newH);
         emitCurrentStateStatement();
     }
 
@@ -1272,6 +1302,9 @@ BallotProtocol::setAcceptCommit(SCPBallot const& c, SCPBallot const& h)
 
     bool didWork = false;
 
+    // remember h's value
+    mValueOverride = std::make_unique<Value>(h.value);
+
     if (!mHighBallot || !mCommit || compareBallots(*mHighBallot, h) != 0 ||
         compareBallots(*mCommit, c) != 0)
     {
@@ -1295,7 +1328,7 @@ BallotProtocol::setAcceptCommit(SCPBallot const& c, SCPBallot const& h)
 
     if (didWork)
     {
-        updateCurrentIfNeeded();
+        updateCurrentIfNeeded(*mHighBallot);
 
         mSlot.getSCPDriver().acceptedCommit(mSlot.getSlotIndex(), h);
         emitCurrentStateStatement();
@@ -1473,7 +1506,7 @@ BallotProtocol::setConfirmCommit(SCPBallot const& c, SCPBallot const& h)
 
     mCommit = std::make_unique<SCPBallot>(c);
     mHighBallot = std::make_unique<SCPBallot>(h);
-    updateCurrentIfNeeded();
+    updateCurrentIfNeeded(*mHighBallot);
 
     mPhase = SCP_PHASE_EXTERNALIZE;
 

src/scp/BallotProtocol.h

@@ -48,6 +48,7 @@ class BallotProtocol
     std::unique_ptr<SCPBallot> mCommit;             // c
     std::map<NodeID, SCPEnvelope> mLatestEnvelopes; // M
     SCPPhase mPhase;                                // Phi
+    std::unique_ptr<Value> mValueOverride;          // z
 
     int mCurrentMessageLevel; // number of messages triggered in one run
 
@@ -164,7 +165,7 @@ class BallotProtocol
     std::set<SCPBallot> getPrepareCandidates(SCPStatement const& hint);
 
     // helper to perform step (8) from the paper
-    void updateCurrentIfNeeded();
+    bool updateCurrentIfNeeded(SCPBallot const& h);
 
     // An interval is [low,high] represented as a pair
     using Interval = std::pair<uint32, uint32>;

src/scp/SCP.cpp

@@ -163,6 +163,20 @@ SCP::isValidator()
     return mLocalNode->isValidator();
 }
 
+bool
+SCP::isSlotFullyValidated(uint64 slotIndex)
+{
+    auto slot = getSlot(slotIndex, false);
+    if (slot)
+    {
+        return slot->isFullyValidated();
+    }
+    else
+    {
+        return false;
+    }
+}
+
 size_t
 SCP::getKnownSlotsCount() const
 {

src/scp/SCP.h

@@ -90,6 +90,9 @@ class SCP
     // Returns whether the local node is a validator.
     bool isValidator();
 
+    // returns the validation state of the given slot
+    bool isSlotFullyValidated(uint64 slotIndex);
+
     // Helpers for monitoring and reporting the internal memory-usage of the SCP
     // protocol to system metric reporters.
     size_t getKnownSlotsCount() const;