Merge pull request #3977 from dmkozh/txset_fix

Only interpret `TxSetFrame` with correct ledger state Reviewed-by: marta-lokhova
stellar · Nov 9, 2023 · acca92e · acca92e
2 parents 4d97f07 + fe7bbcf
commit acca92e
Show file tree

Hide file tree

Showing 38 changed files with 1,454 additions and 1,034 deletions.
diff --git a/Builds/VisualStudio/stellar-core.vcxproj b/Builds/VisualStudio/stellar-core.vcxproj
@@ -688,6 +688,7 @@ exit /b 0
     <ClCompile Include="..\..\src\transactions\test\SetOptionsTests.cpp" />
     <ClCompile Include="..\..\src\transactions\test\SetTrustLineFlagsTests.cpp" />
     <ClCompile Include="..\..\src\transactions\test\SignatureUtilsTest.cpp" />
+    <ClCompile Include="..\..\src\transactions\test\SorobanTxTestUtils.cpp" />
     <ClCompile Include="..\..\src\transactions\test\SponsorshipTestUtils.cpp" />
     <ClCompile Include="..\..\src\transactions\test\TxEnvelopeTests.cpp" />
     <ClCompile Include="..\..\src\transactions\test\TxResultsTests.cpp" />
@@ -1061,6 +1062,7 @@ exit /b 0
     <ClInclude Include="..\..\src\transactions\SignatureChecker.h" />
     <ClInclude Include="..\..\src\transactions\SignatureUtils.h" />
     <ClInclude Include="..\..\src\transactions\SponsorshipUtils.h" />
+    <ClInclude Include="..\..\src\transactions\test\SorobanTxTestUtils.h" />
     <ClInclude Include="..\..\src\transactions\test\SponsorshipTestUtils.h" />
     <ClInclude Include="..\..\src\transactions\TransactionBridge.h" />
     <ClInclude Include="..\..\src\transactions\TransactionFrame.h" />

diff --git a/Builds/VisualStudio/stellar-core.vcxproj.filters b/Builds/VisualStudio/stellar-core.vcxproj.filters
@@ -1312,6 +1312,9 @@
     <ClCompile Include="..\..\src\catchup\ReplayDebugMetaWork.cpp">
       <Filter>catchup</Filter>
     </ClCompile>
+    <ClCompile Include="..\..\src\transactions\test\SorobanTxTestUtils.cpp">
+      <Filter>transactions\tests</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="..\..\lib\util\cpptoml.h">
@@ -2281,6 +2284,9 @@
     <ClInclude Include="..\..\src\catchup\ReplayDebugMetaWork.h">
       <Filter>catchup</Filter>
     </ClInclude>
+    <ClInclude Include="..\..\src\transactions\test\SorobanTxTestUtils.h">
+      <Filter>transactions\tests</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <None Include="..\..\AUTHORS" />

diff --git a/src/catchup/ApplyBufferedLedgersWork.cpp b/src/catchup/ApplyBufferedLedgersWork.cpp
@@ -47,13 +47,13 @@ ApplyBufferedLedgersWork::onRun()
     }
     auto const& lcd = maybeLcd.value();
 
-    CLOG_INFO(History,
-              "Scheduling buffered ledger-close: [seq={}, prev={}, txs={}, "
-              "ops={}, sv: {}]",
-              lcd.getLedgerSeq(),
-              hexAbbrev(lcd.getTxSet()->previousLedgerHash()),
-              lcd.getTxSet()->sizeTxTotal(), lcd.getTxSet()->sizeOpTotal(),
-              stellarValueToString(mApp.getConfig(), lcd.getValue()));
+    CLOG_INFO(
+        History,
+        "Scheduling buffered ledger-close: [seq={}, prev={}, txs={}, "
+        "ops={}, sv: {}]",
+        lcd.getLedgerSeq(), hexAbbrev(lcd.getTxSet()->previousLedgerHash()),
+        lcd.getTxSet()->sizeTxTotal(), lcd.getTxSet()->sizeOpTotalForLogging(),
+        stellarValueToString(mApp.getConfig(), lcd.getValue()));
 
     auto applyLedger = std::make_shared<ApplyLedgerWork>(mApp, lcd);
 

diff --git a/src/catchup/ApplyCheckpointWork.cpp b/src/catchup/ApplyCheckpointWork.cpp
@@ -126,12 +126,12 @@ ApplyCheckpointWork::getCurrentTxSet()
             CLOG_DEBUG(History, "Loaded txset for ledger {}", seq);
             if (mTxHistoryEntry.ext.v() == 0)
             {
-                return TxSetFrame::makeFromWire(mApp, mTxHistoryEntry.txSet);
+                return TxSetFrame::makeFromWire(mTxHistoryEntry.txSet);
             }
             else
             {
                 return TxSetFrame::makeFromWire(
-                    mApp, mTxHistoryEntry.ext.generalizedTxSet());
+                    mTxHistoryEntry.ext.generalizedTxSet());
             }
         }
     } while (mTxIn && mTxIn.readOne(mTxHistoryEntry));

diff --git a/src/catchup/ReplayDebugMetaWork.cpp b/src/catchup/ReplayDebugMetaWork.cpp
@@ -111,11 +111,11 @@ class ApplyLedgersFromMetaWork : public Work
         TxSetFrameConstPtr txSet;
         if (lcm.v() == 0)
         {
-            txSet = TxSetFrame::makeFromWire(mApp, lcm.v0().txSet);
+            txSet = TxSetFrame::makeFromWire(lcm.v0().txSet);
         }
         else
         {
-            txSet = TxSetFrame::makeFromWire(mApp, lcm.v1().txSet);
+            txSet = TxSetFrame::makeFromWire(lcm.v1().txSet);
         }
 
         LedgerCloseData ledgerCloseData(ledgerSeqToApply, txSet,
@@ -164,7 +164,7 @@ ReplayDebugMetaWork::applyLastLedger()
     if (lcl + 1 == debugTxSet.ledgerSeq)
     {
         mApp.getLedgerManager().closeLedger(
-            LedgerCloseData::toLedgerCloseData(debugTxSet, mApp));
+            LedgerCloseData::toLedgerCloseData(debugTxSet));
     }
     else
     {

diff --git a/src/herder/Herder.h b/src/herder/Herder.h
@@ -139,6 +139,9 @@ class Herder
     virtual EnvelopeStatus recvSCPEnvelope(SCPEnvelope const& envelope,
                                            const SCPQuorumSet& qset,
                                            TxSetFrameConstPtr txset) = 0;
+    virtual EnvelopeStatus recvSCPEnvelope(SCPEnvelope const& envelope,
+                                           const SCPQuorumSet& qset,
+                                           StellarMessage const& txset) = 0;
 
     virtual void
     externalizeValue(TxSetFrameConstPtr txSet, uint32_t ledgerSeq,

diff --git a/src/herder/HerderImpl.cpp b/src/herder/HerderImpl.cpp
@@ -231,13 +231,11 @@ HerderImpl::newSlotExternalized(bool synchronous, StellarValue const& value)
     // start timing next externalize from this point
     mLastExternalize = mApp.getClock().now();
 
-    // perform cleanups
-    auto externalizedSet = mPendingEnvelopes.getTxSet(value.txSetHash);
-    if (externalizedSet)
-    {
-        updateTransactionQueue(externalizedSet);
-    }
+    // In order to update the transaction queue we need to get the
+    // applied transactions.
+    updateTransactionQueue(mPendingEnvelopes.getTxSet(value.txSetHash));
 
+    // perform cleanups
     // Evict slots that are outside of our ledger validity bracket
     auto minSlotToRemember = getMinLedgerSeqToRemember();
     if (minSlotToRemember > LedgerManager::GENESIS_LEDGER_SEQ)
@@ -820,6 +818,17 @@ HerderImpl::recvSCPEnvelope(SCPEnvelope const& envelope,
     return recvSCPEnvelope(envelope);
 }
 
+Herder::EnvelopeStatus
+HerderImpl::recvSCPEnvelope(SCPEnvelope const& envelope,
+                            const SCPQuorumSet& qset,
+                            StellarMessage const& txset)
+{
+    auto txSetFrame = txset.type() == TX_SET
+                          ? TxSetFrame::makeFromWire(txset.txSet())
+                          : TxSetFrame::makeFromWire(txset.generalizedTxSet());
+    return recvSCPEnvelope(envelope, qset, txSetFrame);
+}
+
 void
 HerderImpl::externalizeValue(TxSetFrameConstPtr txSet, uint32_t ledgerSeq,
                              uint64_t closeTime,
@@ -1326,9 +1335,10 @@ HerderImpl::triggerNextLedger(uint32_t ledgerSeqToTrigger,
     TxSetFrame::TxPhases invalidTxPhases;
     invalidTxPhases.resize(txPhases.size());
 
-    auto proposedSet = TxSetFrame::makeFromTransactions(
-        txPhases, mApp, lowerBoundCloseTimeOffset, upperBoundCloseTimeOffset,
-        invalidTxPhases);
+    auto [proposedSet, applicableProposedSet] =
+        TxSetFrame::makeFromTransactions(
+            txPhases, mApp, lowerBoundCloseTimeOffset,
+            upperBoundCloseTimeOffset, invalidTxPhases);
 
     if (protocolVersionStartsFrom(lcl.header.ledgerVersion,
                                   SOROBAN_PROTOCOL_VERSION))
@@ -1870,15 +1880,7 @@ HerderImpl::persistSCPState(uint64 slot)
     for (auto it : txSets)
     {
         StoredTransactionSet tempTxSet;
-        if (it.second->isGeneralizedTxSet())
-        {
-            tempTxSet.v(1);
-            it.second->toXDR(tempTxSet.generalizedTxSet());
-        }
-        else
-        {
-            it.second->toXDR(tempTxSet.txSet());
-        }
+        it.second->storeXDR(tempTxSet);
         txSetsToPersist.emplace(
             it.first, decoder::encode_b64(xdr::xdr_to_opaque(tempTxSet)));
     }
@@ -1910,9 +1912,7 @@ HerderImpl::restoreSCPState()
 
             StoredTransactionSet storedSet;
             xdr::xdr_from_opaque(buffer, storedSet);
-            TxSetFrameConstPtr cur =
-                TxSetFrame::makeFromStoredTxSet(storedSet, mApp);
-
+            TxSetFrameConstPtr cur = TxSetFrame::makeFromStoredTxSet(storedSet);
             Hash h = cur->getContentsHash();
             mPendingEnvelopes.addTxSet(h, 0, cur);
         }
@@ -2192,17 +2192,25 @@ HerderImpl::trackingHeartBeat()
 }
 
 void
-HerderImpl::updateTransactionQueue(TxSetFrameConstPtr txSet)
+HerderImpl::updateTransactionQueue(TxSetFrameConstPtr externalizedTxSet)
 {
     ZoneScoped;
+    if (externalizedTxSet == nullptr)
+    {
+        CLOG_DEBUG(Herder,
+                   "No tx set to update tx queue - expected during bootstrap");
+        return;
+    }
+    auto txsPerPhase =
+        externalizedTxSet->createTransactionFrames(mApp.getNetworkID());
+
     // Generate a transaction set from a random hash and drop invalid
     auto lhhe = mLedgerManager.getLastClosedLedgerHeader();
     lhhe.hash = HashUtils::random();
 
     auto updateQueue = [&](auto& queue, auto const& applied) {
         queue.removeApplied(applied);
         queue.shift();
-
         queue.maybeVersionUpgraded();
 
         auto txSet = queue.getTransactions(lhhe.header);
@@ -2215,17 +2223,22 @@ HerderImpl::updateTransactionQueue(TxSetFrameConstPtr txSet)
 
         queue.rebroadcast();
     };
+    if (txsPerPhase.size() > static_cast<size_t>(TxSetFrame::Phase::CLASSIC))
+    {
+        updateQueue(
+            mTransactionQueue,
+            txsPerPhase[static_cast<size_t>(TxSetFrame::Phase::CLASSIC)]);
+    }
 
-    updateQueue(mTransactionQueue,
-                txSet->getTxsForPhase(TxSetFrame::Phase::CLASSIC));
     // Even if we're in protocol 20, still check for number of phases, in case
     // we're dealing with the upgrade ledger that contains old-style transaction
     // set
-    if (txSet->numPhases() > static_cast<size_t>(TxSetFrame::Phase::SOROBAN) &&
-        mSorobanTransactionQueue)
+    if (mSorobanTransactionQueue != nullptr &&
+        txsPerPhase.size() > static_cast<size_t>(TxSetFrame::Phase::SOROBAN))
     {
-        updateQueue(*mSorobanTransactionQueue,
-                    txSet->getTxsForPhase(TxSetFrame::Phase::SOROBAN));
+        updateQueue(
+            *mSorobanTransactionQueue,
+            txsPerPhase[static_cast<size_t>(TxSetFrame::Phase::SOROBAN)]);
     }
 }
 

diff --git a/src/herder/HerderImpl.h b/src/herder/HerderImpl.h
@@ -103,6 +103,9 @@ class HerderImpl : public Herder
     EnvelopeStatus recvSCPEnvelope(SCPEnvelope const& envelope,
                                    const SCPQuorumSet& qset,
                                    TxSetFrameConstPtr txset) override;
+    EnvelopeStatus recvSCPEnvelope(SCPEnvelope const& envelope,
+                                   const SCPQuorumSet& qset,
+                                   StellarMessage const& txset) override;
 
     void externalizeValue(TxSetFrameConstPtr txSet, uint32_t ledgerSeq,
                           uint64_t closeTime,

diff --git a/src/herder/HerderSCPDriver.cpp b/src/herder/HerderSCPDriver.cpp
@@ -317,9 +317,29 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex, StellarValue const& b,
         CLOG_ERROR(Herder, "validateValue i:{} unknown txSet {}", slotIndex,
                    hexAbbrev(txSetHash));
 
+        return SCPDriver::kInvalidValue;
+    }
+    ApplicableTxSetFrameConstPtr applicableTxSet;
+
+    // The invariant here is that we only validate tx sets nominated
+    // to be applied to the current ledger state. However, in case
+    // if we receive a bad SCP value for the current state, we still
+    // might end up with malformed tx set that doesn't refer to the
+    // LCL.
+    if (txSet->previousLedgerHash() ==
+        mApp.getLedgerManager().getLastClosedLedgerHeader().hash)
+    {
+        applicableTxSet = txSet->prepareForApply(mApp);
+    }
+
+    if (applicableTxSet == nullptr)
+    {
+        CLOG_ERROR(Herder,
+                   "validateValue i:{} can't prepare txSet {} for apply",
+                   slotIndex, hexAbbrev(txSetHash));
         res = SCPDriver::kInvalidValue;
     }
-    else if (!checkAndCacheTxSetValid(txSet, closeTimeOffset))
+    else if (!checkAndCacheTxSetValid(*applicableTxSet, closeTimeOffset))
     {
         CLOG_DEBUG(Herder,
                    "HerderSCPDriver::validateValue i: {} invalid txSet {}",
@@ -561,37 +581,30 @@ HerderSCPDriver::stopTimer(uint64 slotIndex, int timerID)
 // returns true if l < r
 // lh, rh are the hashes of l,h
 static bool
-compareTxSets(TxSetFrameConstPtr l, TxSetFrameConstPtr r, Hash const& lh,
-              Hash const& rh, LedgerHeader const& header, Hash const& s)
+compareTxSets(ApplicableTxSetFrame const& l, ApplicableTxSetFrame const& r,
+              Hash const& lh, Hash const& rh, size_t lEncodedSize,
+              size_t rEncodedSize, LedgerHeader const& header, Hash const& s)
 {
-    if (l == nullptr)
-    {
-        return r != nullptr;
-    }
-    if (r == nullptr)
-    {
-        return false;
-    }
-    auto lSize = l->size(header);
-    auto rSize = r->size(header);
+    auto lSize = l.size(header);
+    auto rSize = r.size(header);
     if (lSize != rSize)
     {
         return lSize < rSize;
     }
     if (protocolVersionStartsFrom(header.ledgerVersion,
                                   SOROBAN_PROTOCOL_VERSION))
     {
-        auto lBids = l->getTotalInclusionFees();
-        auto rBids = r->getTotalInclusionFees();
+        auto lBids = l.getTotalInclusionFees();
+        auto rBids = r.getTotalInclusionFees();
         if (lBids != rBids)
         {
             return lBids < rBids;
         }
     }
     if (protocolVersionStartsFrom(header.ledgerVersion, ProtocolVersion::V_11))
     {
-        auto lFee = l->getTotalFees(header);
-        auto rFee = r->getTotalFees(header);
+        auto lFee = l.getTotalFees(header);
+        auto rFee = r.getTotalFees(header);
         if (lFee != rFee)
         {
             return lFee < rFee;
@@ -600,8 +613,6 @@ compareTxSets(TxSetFrameConstPtr l, TxSetFrameConstPtr r, Hash const& lh,
     if (protocolVersionStartsFrom(header.ledgerVersion,
                                   SOROBAN_PROTOCOL_VERSION))
     {
-        auto lEncodedSize = l->encodedSize();
-        auto rEncodedSize = r->encodedSize();
         if (lEncodedSize != rEncodedSize)
         {
             // Look for the smallest encoded size.
@@ -709,20 +720,32 @@ HerderSCPDriver::combineCandidates(uint64_t slotIndex,
     {
         auto highest = candidateValues.cend();
         TxSetFrameConstPtr highestTxSet;
+        ApplicableTxSetFrameConstPtr highestApplicableTxSet;
         for (auto it = candidateValues.cbegin(); it != candidateValues.cend();
              ++it)
         {
             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)))
+            auto cTxSet = mPendingEnvelopes.getTxSet(sv.txSetHash);
+            releaseAssert(cTxSet);
+            // Only valid applicable tx sets should be combined.
+            auto cApplicableTxSet = cTxSet->prepareForApply(mApp);
+            releaseAssert(cApplicableTxSet);
+            if (cTxSet->previousLedgerHash() == lcl.hash)
             {
-                highest = it;
-                highestTxSet = cTxSet;
+
+                if (!highestTxSet ||
+                    compareTxSets(*highestApplicableTxSet, *cApplicableTxSet,
+                                  highest->txSetHash, sv.txSetHash,
+                                  highestTxSet->encodedSize(),
+                                  cTxSet->encodedSize(), lcl.header,
+                                  candidatesHash))
+                {
+                    highest = it;
+                    highestTxSet = cTxSet;
+                    highestApplicableTxSet = std::move(cApplicableTxSet);
+                }
             }
-        };
+        }
         if (highest == candidateValues.cend())
         {
             throw std::runtime_error(
@@ -1210,17 +1233,18 @@ HerderSCPDriver::wrapStellarValue(StellarValue const& sv)
 }
 
 bool
-HerderSCPDriver::checkAndCacheTxSetValid(TxSetFrameConstPtr txSet,
+HerderSCPDriver::checkAndCacheTxSetValid(ApplicableTxSetFrame const& txSet,
                                          uint64_t closeTimeOffset) const
 {
     auto key = TxSetValidityKey{
         mApp.getLedgerManager().getLastClosedLedgerHeader().hash,
-        txSet->getContentsHash(), closeTimeOffset, closeTimeOffset};
+        txSet.getContentsHash(), closeTimeOffset, closeTimeOffset};
 
     bool* pRes = mTxSetValidCache.maybeGet(key);
     if (pRes == nullptr)
     {
-        bool res = txSet->checkValid(mApp, closeTimeOffset, closeTimeOffset);
+        bool res = txSet.checkValid(mApp, closeTimeOffset, closeTimeOffset);
+
         mTxSetValidCache.put(key, res);
         return res;
     }