Upgrade protocol for CAP-0034 / protocol issue 622

This commit implements the protocol change documented in CAP-0034:

https://github.com/stellar/stellar-protocol/blob/master/core/cap-0034.md

(It does not bump the protocol version, because the version
has already been bumped since the last stellar-core
release.)

That proposal came from the discussion of stellar-protocol
issue stellar#622:

stellar/stellar-protocol#622

The CAP enumerates all the semantic changes in this commit.
Here are a couple of implementation notes too specific to
the code to be mentioned in the CAP:

- The interfaces that had an "upperBoundCloseTimeOffset"
added by PR 2608 have now also been extended with
"lowerBoundCloseTimeOffset"s, which analogously affect
the conditions under which isTooEarly() returns true,
allowing the new CAP-34 code to validate minTime as well
as maxTime (the latter uses "upperBoundCloseTimeOffset")
against the next close time (rather than the last ledger
close time).

- testSCPDriver() has been not only extended to test new
behaviors, but also refactored to allow more test cases to
be added by adding more test case parameters to a vector.

src/herder/HerderImpl.cpp

@@ -848,17 +848,45 @@ HerderImpl::triggerNextLedger(uint32_t ledgerSeqToTrigger)
     auto const& lcl = mLedgerManager.getLastClosedLedgerHeader();
     auto proposedSet = mTransactionQueue.toTxSet(lcl);
 
-    auto upperBoundCloseTimeOffset =
-        getUpperBoundCloseTimeOffset(mApp, lcl.header.scpValue.closeTime);
+    // We pick as next close time the current time unless it's before the last
+    // close time. We don't know how much time it will take to reach consensus
+    // so this is the most appropriate value to use as closeTime.
+    uint64_t nextCloseTime =
+        VirtualClock::to_time_t(mApp.getClock().system_now());
+    if (nextCloseTime <= lcl.header.scpValue.closeTime)
+    {
+        nextCloseTime = lcl.header.scpValue.closeTime + 1;
+    }
 
-    auto removed = proposedSet->trimInvalid(mApp, upperBoundCloseTimeOffset);
+    // Protocols including the "closetime change" (CAP-0034) externalize
+    // the exact closeTime contained in the StellarValue with the best
+    // transaction set, so we know the exact closeTime against which to
+    // validate here -- 'nextCloseTime'.  (The _offset_, therefore, is
+    // the difference between 'nextCloseTime' and the last ledger close time.)
+    TimePoint upperBoundCloseTimeOffset, lowerBoundCloseTimeOffset;
+    if (getHerderSCPDriver().curProtocolPreservesTxSetCloseTimeAffinity())
+    {
+        upperBoundCloseTimeOffset =
+            nextCloseTime - lcl.header.scpValue.closeTime;
+        lowerBoundCloseTimeOffset = upperBoundCloseTimeOffset;
+    }
+    else
+    {
+        upperBoundCloseTimeOffset =
+            getUpperBoundCloseTimeOffset(mApp, lcl.header.scpValue.closeTime);
+        lowerBoundCloseTimeOffset = 0;
+    }
+
+    auto removed = proposedSet->trimInvalid(mApp, lowerBoundCloseTimeOffset,
+                                            upperBoundCloseTimeOffset);
     mTransactionQueue.ban(removed);
 
     proposedSet->surgePricingFilter(mApp);
 
     // we not only check that the value is valid for consensus (offset=0) but
     // also that we performed the proper cleanup above
-    if (!proposedSet->checkValid(mApp, upperBoundCloseTimeOffset))
+    if (!proposedSet->checkValid(mApp, lowerBoundCloseTimeOffset,
+                                 upperBoundCloseTimeOffset))
     {
         throw std::runtime_error("wanting to emit an invalid txSet");
     }
@@ -881,16 +909,6 @@ HerderImpl::triggerNextLedger(uint32_t ledgerSeqToTrigger)
         return;
     }
 
-    // We pick as next close time the current time unless it's before the last
-    // close time. We don't know how much time it will take to reach consensus
-    // so this is the most appropriate value to use as closeTime.
-    uint64_t nextCloseTime =
-        VirtualClock::to_time_t(mApp.getClock().system_now());
-    if (nextCloseTime <= lcl.header.scpValue.closeTime)
-    {
-        nextCloseTime = lcl.header.scpValue.closeTime + 1;
-    }
-
     StellarValue newProposedValue(txSetHash, nextCloseTime, emptyUpgradeSteps,
                                   STELLAR_VALUE_BASIC);
 
@@ -1530,7 +1548,7 @@ HerderImpl::updateTransactionQueue(
     auto txSet = mTransactionQueue.toTxSet(lhhe);
 
     auto removed = txSet->trimInvalid(
-        mApp,
+        mApp, 0,
         getUpperBoundCloseTimeOffset(mApp, lhhe.header.scpValue.closeTime));
     mTransactionQueue.ban(removed);
 

src/herder/HerderSCPDriver.cpp

@@ -18,13 +18,19 @@
 #include "util/Logging.h"
 #include "xdr/Stellar-SCP.h"
 #include "xdr/Stellar-ledger-entries.h"
+#include "xdr/Stellar-ledger.h"
 #include <Tracy.hpp>
+#include <algorithm>
 #include <fmt/format.h>
 #include <medida/metrics_registry.h>
+#include <numeric>
+#include <stdexcept>
 #include <xdrpp/marshal.h>
 
 namespace stellar
 {
+uint32_t const HerderSCPDriver::FIRST_PROTOCOL_WITH_TXSET_CLOSETIME_AFFINITY =
+    14;
 
 HerderSCPDriver::SCPMetrics::SCPMetrics(Application& app)
     : mEnvelopeSign(
@@ -205,17 +211,9 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex, StellarValue const& b,
     ZoneScoped;
     if (b.ext.v() == STELLAR_VALUE_SIGNED)
     {
-        if (nomination)
+        ZoneNamedN(sigZone, "signature check", true);
+        if (!mHerder.verifyStellarValueSignature(b))
         {
-            ZoneNamedN(sigZone, "signature check", true);
-            if (!mHerder.verifyStellarValueSignature(b))
-            {
-                return SCPDriver::kInvalidValue;
-            }
-        }
-        else
-        {
-            // don't use signed values in ballot protocol
             return SCPDriver::kInvalidValue;
         }
     }
@@ -319,15 +317,16 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex, StellarValue const& b,
         return SCPDriver::kInvalidValue;
     }
 
-    if (!nomination && b.ext.v() != STELLAR_VALUE_BASIC)
+    bool const expectSignedValue =
+        nomination || (compositeValueType() == STELLAR_VALUE_SIGNED);
+    if (!expectSignedValue && (b.ext.v() != STELLAR_VALUE_BASIC))
     {
-        // ballot protocol only supports BASIC
         CLOG(TRACE, "Herder")
             << "HerderSCPDriver::validateValue"
             << " i: " << slotIndex << " invalid value type - expected BASIC";
         return SCPDriver::kInvalidValue;
     }
-    if (nomination && b.ext.v() != STELLAR_VALUE_SIGNED)
+    if (expectSignedValue && (b.ext.v() != STELLAR_VALUE_SIGNED))
     {
         CLOG(TRACE, "Herder")
             << "HerderSCPDriver::validateValue"
@@ -340,14 +339,18 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex, StellarValue const& b,
 
     SCPDriver::ValidationLevel res;
 
+    auto closeTimeOffset = curProtocolPreservesTxSetCloseTimeAffinity()
+                               ? (b.closeTime - lastCloseTime)
+                               : 0;
+
     if (!txSet)
     {
         CLOG(ERROR, "Herder") << "validateValue i:" << slotIndex
                               << " unknown txSet " << hexAbbrev(txSetHash);
 
         res = SCPDriver::kInvalidValue;
     }
-    else if (!txSet->checkValid(mApp, 0))
+    else if (!txSet->checkValid(mApp, closeTimeOffset, closeTimeOffset))
     {
         if (Logging::logDebug("Herder"))
             CLOG(DEBUG, "Herder")
@@ -570,8 +573,8 @@ HerderSCPDriver::setupTimer(uint64_t slotIndex, int timerID,
 // returns true if l < r
 // lh, rh are the hashes of l,h
 static bool
-compareTxSets(TxSetFramePtr l, TxSetFramePtr r, Hash const& lh, Hash const& rh,
-              LedgerHeader const& header, Hash const& s)
+compareTxSets(TxSetFrameConstPtr l, TxSetFrameConstPtr r, Hash const& lh,
+              Hash const& rh, LedgerHeader const& header, Hash const& s)
 {
     if (l == nullptr)
     {
@@ -629,6 +632,8 @@ HerderSCPDriver::combineCandidates(uint64_t slotIndex,
 
     std::vector<StellarValue> candidateValues;
 
+    TimePoint maxCloseTime = 0;
+
     for (auto const& c : candidates)
     {
         candidateValues.emplace_back();
@@ -637,10 +642,9 @@ HerderSCPDriver::combineCandidates(uint64_t slotIndex,
         xdr::xdr_from_opaque(c->getValue(), sv);
         candidatesHash ^= sha256(c->getValue());
 
-        // max closeTime
-        if (comp.closeTime < sv.closeTime)
+        if (maxCloseTime < sv.closeTime)
         {
-            comp.closeTime = sv.closeTime;
+            maxCloseTime = sv.closeTime;
         }
         for (auto const& upgrade : sv.upgrades)
         {
@@ -687,59 +691,42 @@ HerderSCPDriver::combineCandidates(uint64_t slotIndex,
     }
 
     // take the txSet with the biggest size, highest xored hash that we have
-    TxSetFramePtr bestTxSet;
     {
-        Hash highest;
-        TxSetFramePtr highestTxSet;
-        for (auto const& sv : candidateValues)
+        auto highest = candidateValues.cend();
+        TxSetFrameConstPtr highestTxSet;
+        for (auto it = candidateValues.cbegin(); it != candidateValues.cend();
+             ++it)
         {
-            TxSetFramePtr cTxSet = mPendingEnvelopes.getTxSet(sv.txSetHash);
-
-            if (cTxSet && cTxSet->previousLedgerHash() == lcl.hash)
+            auto const& sv = *it;
+            auto const cTxSet = mPendingEnvelopes.getTxSet(sv.txSetHash);
+            if (cTxSet && cTxSet->previousLedgerHash() == lcl.hash &&
+                (!highestTxSet ||
+                 compareTxSets(highestTxSet, cTxSet, highest->txSetHash,
+                               sv.txSetHash, lcl.header, candidatesHash)))
             {
-                if (compareTxSets(highestTxSet, cTxSet, highest, sv.txSetHash,
-                                  lcl.header, candidatesHash))
-                {
-                    highestTxSet = cTxSet;
-                    highest = sv.txSetHash;
-                }
+                highest = it;
+                highestTxSet = cTxSet;
             }
+        };
+        if (highest == candidateValues.cend())
+        {
+            throw std::runtime_error(
+                "No highest candidate transaction set found");
         }
-        // make a copy as we're about to modify it and we don't want to mess
-        // with the txSet cache
-        bestTxSet = std::make_shared<TxSetFrame>(*highestTxSet);
+        comp = *highest;
     }
-
+    if (!curProtocolPreservesTxSetCloseTimeAffinity())
+    {
+        comp.closeTime = maxCloseTime;
+        comp.ext.v(STELLAR_VALUE_BASIC);
+    }
+    comp.upgrades.clear();
     for (auto const& upgrade : upgrades)
     {
         Value v(xdr::xdr_to_opaque(upgrade.second));
         comp.upgrades.emplace_back(v.begin(), v.end());
     }
 
-    // just to be sure
-    auto removed = bestTxSet->trimInvalid(mApp, 0);
-    comp.txSetHash = bestTxSet->getContentsHash();
-
-    if (removed.size() != 0)
-    {
-        CLOG(WARNING, "Herder") << "Candidate set had " << removed.size()
-                                << " invalid transactions";
-
-        // learn the updated tx set
-        bestTxSet = mPendingEnvelopes.putTxSet(bestTxSet->getContentsHash(),
-                                               slotIndex, bestTxSet);
-
-        // post to avoid triggering SCP handling code recursively
-        mApp.postOnMainThread(
-            [this, bestTxSet]() {
-                mPendingEnvelopes.recvTxSet(bestTxSet->getContentsHash(),
-                                            bestTxSet);
-            },
-            "HerderSCPDriver combineCandidates");
-    }
-
-    // Ballot Protocol uses BASIC values
-    comp.ext.v(STELLAR_VALUE_BASIC);
     auto res = wrapStellarValue(comp);
     return res;
 }
@@ -841,6 +828,13 @@ HerderSCPDriver::valueExternalized(uint64_t slotIndex, Value const& value)
     }
 }
 
+bool
+HerderSCPDriver::curProtocolPreservesTxSetCloseTimeAffinity() const
+{
+    return mLedgerManager.getLastClosedLedgerHeader().header.ledgerVersion >=
+           FIRST_PROTOCOL_WITH_TXSET_CLOSETIME_AFFINITY;
+}
+
 void
 HerderSCPDriver::logQuorumInformation(uint64_t index)
 {
@@ -1070,6 +1064,13 @@ HerderSCPDriver::purgeSlots(uint64_t maxSlotIndex)
     getSCP().purgeSlots(maxSlotIndex);
 }
 
+StellarValueType
+HerderSCPDriver::compositeValueType() const
+{
+    return curProtocolPreservesTxSetCloseTimeAffinity() ? STELLAR_VALUE_SIGNED
+                                                        : STELLAR_VALUE_BASIC;
+}
+
 void
 HerderSCPDriver::clearSCPExecutionEvents()
 {

src/herder/HerderSCPDriver.h

@@ -156,6 +156,14 @@ class HerderSCPDriver : public SCPDriver
     // clean up older slots
     void purgeSlots(uint64_t maxSlotIndex);
 
+    // Does the nomination protocol output a BASIC or a SIGNED
+    // StellarValue?
+    virtual StellarValueType compositeValueType() const;
+
+    // Does the current protocol version contain the CAP-0034 closeTime
+    // semantics change?
+    bool curProtocolPreservesTxSetCloseTimeAffinity() const;
+
   private:
     Application& mApp;
     HerderImpl& mHerder;
@@ -164,6 +172,8 @@ class HerderSCPDriver : public SCPDriver
     PendingEnvelopes& mPendingEnvelopes;
     SCP mSCP;
 
+    static uint32_t const FIRST_PROTOCOL_WITH_TXSET_CLOSETIME_AFFINITY;
+
     struct SCPMetrics
     {
         medida::Meter& mEnvelopeSign;

src/herder/TransactionQueue.cpp

@@ -206,7 +206,7 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
                          .getLastClosedLedgerHeader()
                          .header.scpValue.closeTime;
     LedgerTxn ltx(mApp.getLedgerTxnRoot());
-    if (!tx->checkValid(ltx, seqNum,
+    if (!tx->checkValid(ltx, seqNum, 0,
                         getUpperBoundCloseTimeOffset(mApp, closeTime)))
     {
         return TransactionQueue::AddResult::ADD_STATUS_ERROR;

src/herder/TxSetFrame.cpp

@@ -284,7 +284,8 @@ TxSetFrame::surgePricingFilter(Application& app)
 bool
 TxSetFrame::checkOrTrim(Application& app,
                         std::vector<TransactionFrameBasePtr>& trimmed,
-                        bool justCheck, uint64_t upperBoundCloseTimeOffset)
+                        bool justCheck, uint64_t lowerBoundCloseTimeOffset,
+                        uint64_t upperBoundCloseTimeOffset)
 {
     ZoneScoped;
     LedgerTxn ltx(app.getLedgerTxnRoot());
@@ -298,7 +299,8 @@ TxSetFrame::checkOrTrim(Application& app,
         while (iter != kv.second.end())
         {
             auto tx = *iter;
-            if (!tx->checkValid(ltx, lastSeq, upperBoundCloseTimeOffset))
+            if (!tx->checkValid(ltx, lastSeq, lowerBoundCloseTimeOffset,
+                                upperBoundCloseTimeOffset))
             {
                 if (justCheck)
                 {
@@ -367,20 +369,23 @@ TxSetFrame::checkOrTrim(Application& app,
 }
 
 std::vector<TransactionFrameBasePtr>
-TxSetFrame::trimInvalid(Application& app, uint64_t upperBoundCloseTimeOffset)
+TxSetFrame::trimInvalid(Application& app, uint64_t lowerBoundCloseTimeOffset,
+                        uint64_t upperBoundCloseTimeOffset)
 {
     ZoneScoped;
     std::vector<TransactionFrameBasePtr> trimmed;
     sortForHash();
-    checkOrTrim(app, trimmed, false, upperBoundCloseTimeOffset);
+    checkOrTrim(app, trimmed, false, lowerBoundCloseTimeOffset,
+                upperBoundCloseTimeOffset);
     return trimmed;
 }
 
 // need to make sure every account that is submitting a tx has enough to pay
 // the fees of all the tx it has submitted in this set
 // check seq num
 bool
-TxSetFrame::checkValid(Application& app, uint64_t upperBoundCloseTimeOffset)
+TxSetFrame::checkValid(Application& app, uint64_t lowerBoundCloseTimeOffset,
+                       uint64_t upperBoundCloseTimeOffset)
 {
     ZoneScoped;
     auto& lcl = app.getLedgerManager().getLastClosedLedgerHeader();
@@ -420,7 +425,8 @@ TxSetFrame::checkValid(Application& app, uint64_t upperBoundCloseTimeOffset)
     }
 
     std::vector<TransactionFrameBasePtr> trimmed;
-    bool valid = checkOrTrim(app, trimmed, true, upperBoundCloseTimeOffset);
+    bool valid = checkOrTrim(app, trimmed, true, lowerBoundCloseTimeOffset,
+                             upperBoundCloseTimeOffset);
     mValid = make_optional<std::pair<Hash, bool>>(lcl.hash, valid);
     return valid;
 }