From c6b8e97fba8aebcfd5fb18348f01c2032b8e20b9 Mon Sep 17 00:00:00 2001 From: marta-lokhova Date: Mon, 3 Mar 2025 15:45:10 -0800 Subject: [PATCH 1/2] Legacy tx sets: avoid using read-only LCL meant for main thread only --- src/herder/HerderSCPDriver.cpp | 4 ++-- src/herder/TxSetFrame.cpp | 8 ++++---- src/herder/TxSetFrame.h | 3 ++- src/herder/test/TestTxSetUtils.cpp | 13 +++++++++--- src/herder/test/TxSetTests.cpp | 32 ++++++++++++++++++++++++------ src/ledger/LedgerManagerImpl.cpp | 2 +- 6 files changed, 45 insertions(+), 17 deletions(-) diff --git a/src/herder/HerderSCPDriver.cpp b/src/herder/HerderSCPDriver.cpp index 89e3fddd14..fcb9d931b5 100644 --- a/src/herder/HerderSCPDriver.cpp +++ b/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; diff --git a/src/herder/TxSetFrame.cpp b/src/herder/TxSetFrame.cpp index 86f24d1030..3fe1473cf5 100644 --- a/src/herder/TxSetFrame.cpp +++ b/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(mXDRTxSet); auto maybePhase = TxSetPhaseFrame::makeFromWireLegacy( - app.getLedgerManager().getLastClosedLedgerHeader().header, - app.getNetworkID(), xdrTxSet.txs); + lclHeader, app.getNetworkID(), xdrTxSet.txs); if (!maybePhase) { return nullptr; diff --git a/src/herder/TxSetFrame.h b/src/herder/TxSetFrame.h index d5fe7d433c..8c3319ec2c 100644 --- a/src/herder/TxSetFrame.h +++ b/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; diff --git a/src/herder/test/TestTxSetUtils.cpp b/src/herder/test/TestTxSetUtils.cpp index 17c8ae3448..3674ead0a5 100644 --- a/src/herder/test/TestTxSetUtils.cpp +++ b/src/herder/test/TestTxSetUtils.cpp @@ -113,7 +113,8 @@ makeNonValidatedTxSet(std::vector 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 @@ -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 diff --git a/src/herder/test/TxSetTests.cpp b/src/herder/test/TxSetTests.cpp index e8d8d8c5a9..4011aa2e26 100644 --- a/src/herder/test/TxSetTests.cpp +++ b/src/herder/test/TxSetTests.cpp @@ -41,13 +41,21 @@ 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") { @@ -55,7 +63,11 @@ TEST_CASE("generalized tx set XDR validation", "[txset]") 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); diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp index 6e01e72abc..8dcc8a553c 100644 --- a/src/ledger/LedgerManagerImpl.cpp +++ b/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) { From 80068b72314f19f0d7c754b57209f4c16c87ec02 Mon Sep 17 00:00:00 2001 From: marta-lokhova Date: Tue, 4 Mar 2025 10:56:27 -0800 Subject: [PATCH 2/2] Bump cargo-deny --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 8fef42a5fc..b9a9211b5b 100644 --- a/.github/workflows/build.yml +++ b/.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