Legacy tx sets: avoid using read-only LCL meant for main thread only (#4659)

Looks like parallel ledger close is tripping `threadIsMain` assert in
`getLastClosedLedgerHeader` in `prepareForApply`. This only impacts new
networks in protocol 0 (which use legacy tx sets). Fix is easy: callers
of prepareForApply typically hold a thread-safe LCL already, so use that
instead. We didn't catch this in tests because tests use a custom
upgrade logic, such that we upgrade to generalized transactions sets on
genesis.

Author: marta-lokhova

.github/workflows/build.yml

@@ -39,7 +39,7 @@ jobs:
     continue-on-error: ${{ matrix.checks == 'advisories' }}
     steps:
     - uses: actions/checkout@v4
-    - uses: EmbarkStudios/cargo-deny-action@b01e7a8cfb1f496c52d77361e84c1840d8246393
+    - uses: EmbarkStudios/cargo-deny-action@8d73959fce1cdc8989f23fdf03bec6ae6a6576ef
       with:
         command: check ${{ matrix.checks }}
         # leave arguments empty so we don't test --all-features

src/herder/HerderSCPDriver.cpp

@@ -709,7 +709,7 @@ HerderSCPDriver::combineCandidates(uint64_t slotIndex,
             auto cTxSet = mPendingEnvelopes.getTxSet(sv.txSetHash);
             releaseAssert(cTxSet);
             // Only valid applicable tx sets should be combined.
-            auto cApplicableTxSet = cTxSet->prepareForApply(mApp);
+            auto cApplicableTxSet = cTxSet->prepareForApply(mApp, lcl.header);
             releaseAssert(cApplicableTxSet);
             if (cTxSet->previousLedgerHash() == lcl.hash)
             {
@@ -1246,7 +1246,7 @@ HerderSCPDriver::checkAndCacheTxSetValid(TxSetXDRFrame const& txSet,
         ApplicableTxSetFrameConstPtr applicableTxSet;
         if (txSet.previousLedgerHash() == lcl.hash)
         {
-            applicableTxSet = txSet.prepareForApply(mApp);
+            applicableTxSet = txSet.prepareForApply(mApp, lcl.header);
         }
 
         bool res = true;

src/herder/TxSetFrame.cpp

@@ -783,7 +783,7 @@ makeTxSetFromTransactions(PerPhaseTransactionList const& txPhases,
 #endif
 
     ApplicableTxSetFrameConstPtr outputApplicableTxSet =
-        outputTxSet->prepareForApply(app);
+        outputTxSet->prepareForApply(app, lclHeader.header);
 
     if (!outputApplicableTxSet)
     {
@@ -934,7 +934,8 @@ TxSetXDRFrame::toStellarMessage() const
 #endif
 
 ApplicableTxSetFrameConstPtr
-TxSetXDRFrame::prepareForApply(Application& app) const
+TxSetXDRFrame::prepareForApply(Application& app,
+                               LedgerHeader const& lclHeader) const
 {
 #ifdef BUILD_TESTS
     if (mApplicableTxSetOverride)
@@ -972,8 +973,7 @@ TxSetXDRFrame::prepareForApply(Application& app) const
     {
         auto const& xdrTxSet = std::get<TransactionSet>(mXDRTxSet);
         auto maybePhase = TxSetPhaseFrame::makeFromWireLegacy(
-            app.getLedgerManager().getLastClosedLedgerHeader().header,
-            app.getNetworkID(), xdrTxSet.txs);
+            lclHeader, app.getNetworkID(), xdrTxSet.txs);
         if (!maybePhase)
         {
             return nullptr;

src/herder/TxSetFrame.h

@@ -143,7 +143,8 @@ class TxSetXDRFrame : public NonMovableOrCopyable
     // This may *only* be called when LCL hash matches the `previousLedgerHash`
     // of this `TxSetFrame` - tx sets with a wrong ledger hash shouldn't even
     // be attempted to be interpreted.
-    ApplicableTxSetFrameConstPtr prepareForApply(Application& app) const;
+    ApplicableTxSetFrameConstPtr
+    prepareForApply(Application& app, LedgerHeader const& lclHeader) const;
 
     bool isGeneralizedTxSet() const;
 

src/herder/test/TestTxSetUtils.cpp

@@ -113,7 +113,8 @@ makeNonValidatedTxSet(std::vector<TransactionFrameBasePtr> const& txs,
 {
     auto xdrTxSet = makeTxSetXDR(txs, previousLedgerHash);
     auto txSet = TxSetXDRFrame::makeFromWire(xdrTxSet);
-    auto applicableTxSet = txSet->prepareForApply(app);
+    auto applicableTxSet = txSet->prepareForApply(
+        app, app.getLedgerManager().getLastClosedLedgerHeader().header);
     return std::make_pair(txSet, std::move(applicableTxSet));
 }
 } // namespace
@@ -135,7 +136,10 @@ makeNonValidatedGeneralizedTxSet(
     auto xdrTxSet = makeGeneralizedTxSetXDR(txsPerBaseFee, previousLedgerHash,
                                             *useParallelSorobanPhase);
     auto txSet = TxSetXDRFrame::makeFromWire(xdrTxSet);
-    return std::make_pair(txSet, txSet->prepareForApply(app));
+    return std::make_pair(
+        txSet,
+        txSet->prepareForApply(
+            app, app.getLedgerManager().getLastClosedLedgerHeader().header));
 }
 
 std::pair<TxSetXDRFrameConstPtr, ApplicableTxSetFrameConstPtr>
@@ -214,7 +218,10 @@ makeNonValidatedGeneralizedTxSet(PhaseComponents const& classicTxsPerBaseFee,
     }
     normalizeParallelPhaseXDR(phase);
     auto txSet = TxSetXDRFrame::makeFromWire(xdrTxSet);
-    return std::make_pair(txSet, txSet->prepareForApply(app));
+    return std::make_pair(
+        txSet,
+        txSet->prepareForApply(
+            app, app.getLedgerManager().getLastClosedLedgerHeader().header));
 }
 #endif
 

src/herder/test/TxSetTests.cpp

@@ -41,21 +41,33 @@ TEST_CASE("generalized tx set XDR validation", "[txset]")
     SECTION("no phases")
     {
         auto txSet = TxSetXDRFrame::makeFromWire(xdrTxSet);
-        REQUIRE(txSet->prepareForApply(*app) == nullptr);
+        REQUIRE(
+            txSet->prepareForApply(
+                *app,
+                app->getLedgerManager().getLastClosedLedgerHeader().header) ==
+            nullptr);
     }
     SECTION("one phase")
     {
         auto txSet = TxSetXDRFrame::makeFromWire(xdrTxSet);
         xdrTxSet.v1TxSet().phases.emplace_back();
-        REQUIRE(txSet->prepareForApply(*app) == nullptr);
+        REQUIRE(
+            txSet->prepareForApply(
+                *app,
+                app->getLedgerManager().getLastClosedLedgerHeader().header) ==
+            nullptr);
     }
     SECTION("too many phases")
     {
         xdrTxSet.v1TxSet().phases.emplace_back();
         xdrTxSet.v1TxSet().phases.emplace_back();
         xdrTxSet.v1TxSet().phases.emplace_back();
         auto txSet = TxSetXDRFrame::makeFromWire(xdrTxSet);
-        REQUIRE(txSet->prepareForApply(*app) == nullptr);
+        REQUIRE(
+            txSet->prepareForApply(
+                *app,
+                app->getLedgerManager().getLastClosedLedgerHeader().header) ==
+            nullptr);
     }
 
     SECTION("two phase scenarios")
@@ -442,11 +454,17 @@ TEST_CASE("generalized tx set XDR validation", "[txset]")
                 bool valid = classicIsValid && sorobanIsValid;
                 if (valid)
                 {
-                    REQUIRE(txSet->prepareForApply(*app) != nullptr);
+                    REQUIRE(txSet->prepareForApply(
+                                *app, app->getLedgerManager()
+                                          .getLastClosedLedgerHeader()
+                                          .header) != nullptr);
                 }
                 else
                 {
-                    REQUIRE(txSet->prepareForApply(*app) == nullptr);
+                    REQUIRE(txSet->prepareForApply(
+                                *app, app->getLedgerManager()
+                                          .getLastClosedLedgerHeader()
+                                          .header) == nullptr);
                 }
             }
         }
@@ -502,7 +520,9 @@ testGeneralizedTxSetXDRConversion(ProtocolVersion protocolVersion)
     auto checkXdrRoundtrip = [&](GeneralizedTransactionSet const& txSetXdr) {
         auto txSetFrame = TxSetXDRFrame::makeFromWire(txSetXdr);
         ApplicableTxSetFrameConstPtr applicableFrame =
-            txSetFrame->prepareForApply(*app);
+            txSetFrame->prepareForApply(
+                *app,
+                app->getLedgerManager().getLastClosedLedgerHeader().header);
         REQUIRE(applicableFrame->checkValid(*app, 0, 0));
         GeneralizedTransactionSet newXdr;
         applicableFrame->toWireTxSetFrame()->toXDR(newXdr);

src/ledger/LedgerManagerImpl.cpp

@@ -877,7 +877,7 @@ LedgerManagerImpl::applyLedger(LedgerCloseData const& ledgerData,
     header.current().scpValue = sv;
 
     maybeResetLedgerCloseMetaDebugStream(header.current().ledgerSeq);
-    auto applicableTxSet = txSet->prepareForApply(mApp);
+    auto applicableTxSet = txSet->prepareForApply(mApp, prevHeader);
 
     if (applicableTxSet == nullptr)
     {