From efcd2a0ab71f1e30ca74873ad008253fe720eb5b Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Sat, 27 Jun 2026 10:53:34 +0700 Subject: [PATCH] fix(export): close replay witness review gaps Thread the replay parent through ApplyOptions so export replay can rebuild the historical validator view without relying on LedgerMaster visibility for freshly replayed parents. Harden ttEXPORT_SIGNATURES canonicality before witness indexing, validate witness signer accounts against their pubkeys, reject ambiguous absent NetworkID on low-ID source chains, and lock the consensus phase accessor. Add focused regression coverage and clarify that destination SignerList/quorum compatibility is an operator/client contract. --- src/test/app/ExportResultBuilder_test.cpp | 29 ++++++++++++++ src/test/app/Export_test.cpp | 40 +++++++++++++++++-- src/xrpld/app/consensus/ExportIntent.md | 4 +- src/xrpld/app/consensus/RCLConsensus.cpp | 1 + src/xrpld/app/ledger/detail/BuildLedger.cpp | 9 +++-- src/xrpld/app/tx/applySteps.h | 8 ++++ src/xrpld/app/tx/detail/ApplyContext.cpp | 4 +- src/xrpld/app/tx/detail/ApplyContext.h | 13 +++++- src/xrpld/app/tx/detail/Export.cpp | 14 ++++++- src/xrpld/app/tx/detail/ExportLedgerOps.h | 8 ++-- .../app/tx/detail/ExportResultBuilder.cpp | 8 +++- src/xrpld/app/tx/detail/applySteps.cpp | 3 +- 12 files changed, 123 insertions(+), 18 deletions(-) diff --git a/src/test/app/ExportResultBuilder_test.cpp b/src/test/app/ExportResultBuilder_test.cpp index c88944b82..9cace80fb 100644 --- a/src/test/app/ExportResultBuilder_test.cpp +++ b/src/test/app/ExportResultBuilder_test.cpp @@ -332,6 +332,34 @@ public: } } + void + testRejectsNonCanonicalWitnessSigner() + { + testcase("signature witness validates signer account"); + + auto const src = randomKeyPair(KeyType::secp256k1); + auto const dst = randomKeyPair(KeyType::secp256k1); + auto const signer = randomKeyPair(KeyType::secp256k1); + auto const wrongAccount = randomKeyPair(KeyType::secp256k1); + auto const innerTx = makeExportedPayment( + calcAccountID(src.first), calcAccountID(dst.first)); + auto const exportTxHash = makeHash("bad-witness-signer"); + + ExportResultBuilder::SignatureSnapshot signatures; + signatures.emplace( + signer.first, + ExportResultBuilder::signExportedTxn( + innerTx, signer.first, signer.second)); + + auto witness = ExportResultBuilder::buildSignatureWitness( + exportTxHash, signatures, 654); + auto signers = witness.getFieldArray(sfSigners); + signers[0].setAccountID(sfAccount, calcAccountID(wrongAccount.first)); + witness.setFieldArray(sfSigners, signers); + + BEAST_EXPECT(!ExportResultBuilder::signaturesFromWitness(witness)); + } + void run() override { @@ -341,6 +369,7 @@ public: testCapsSignerArray(); testAssemblesWitnessReferenceMetadata(); testSignatureWitnessRoundTrip(); + testRejectsNonCanonicalWitnessSigner(); } }; diff --git a/src/test/app/Export_test.cpp b/src/test/app/Export_test.cpp index c9b873ace..065b53c35 100644 --- a/src/test/app/Export_test.cpp +++ b/src/test/app/Export_test.cpp @@ -655,6 +655,33 @@ struct Export_test : public beast::unit_test::suite BEAST_EXPECT(dirIsEmpty(*env.current(), emittedDirKey)); } + void + testExportRejectsAmbiguousLowNetworkID() + { + testcase("Export rejects absent NetworkID on low source networks"); + + using namespace jtx; + + Account const alice{"alice"}; + Account const carol{"carol"}; + auto innerObj = buildExportedPayment(alice.id(), carol.id(), 2, 6); + auto const innerTx = makeSTTx(innerObj); + auto const j = beast::Journal{beast::Journal::getNullSink()}; + + // Low-ID networks do not encode sfNetworkID on ordinary transactions, + // so an absent destination ID is ambiguous with a self-target. + BEAST_EXPECT( + ExportLedgerOps::validateNetworkID(innerTx, 0, j) == temMALFORMED); + BEAST_EXPECT( + ExportLedgerOps::validateNetworkID(innerTx, 1024, j) == + temMALFORMED); + + // A high-ID source chain can still export to a legacy low-ID + // destination whose transactions omit sfNetworkID. + BEAST_EXPECT( + ExportLedgerOps::validateNetworkID(innerTx, 1025, j) == tesSUCCESS); + } + void testXportEmissionLimit(FeatureBitset features) { @@ -868,7 +895,8 @@ struct Export_test : public beast::unit_test::suite expectedSigs.emplace(valPK, originalSig); auto const exportSignatureWitnesses = makeExportSignatureWitnesses(txHash, expectedSigs, applySeq); - ApplyOptions const applyOptions{&exportSignatureWitnesses}; + ApplyOptions const applyOptions{ + &exportSignatureWitnesses, false, nullptr}; auto const expectedSignedTxHash = ExportResultBuilder::assemble( innerTx, expectedSigs, applySeq, txHash) @@ -938,7 +966,8 @@ struct Export_test : public beast::unit_test::suite auto const exportSignatureWitnesses = makeExportSignatureWitnesses(txHash, signatures, applySeq); - ApplyOptions const liveOptions{&exportSignatureWitnesses}; + ApplyOptions const liveOptions{ + &exportSignatureWitnesses, false, nullptr}; { auto next = std::make_shared( *parent, env.app().timeKeeper().closeTime()); @@ -952,7 +981,8 @@ struct Export_test : public beast::unit_test::suite // Historical replay reconstructs an already-validated ledger. The // persisted witness supplies the historical signing-key membership; // current ManifestCache may no longer know a rotated signing key. - ApplyOptions const replayOptions{&exportSignatureWitnesses, true}; + ApplyOptions const replayOptions{ + &exportSignatureWitnesses, true, nullptr}; auto const expectedSignedTxHash = ExportResultBuilder::assemble(innerTx, signatures, applySeq, txHash) .signedTxHash; @@ -1104,7 +1134,8 @@ struct Export_test : public beast::unit_test::suite exportSignatureWitnesses = makeExportSignatureWitnesses(txHash, signatures, applySeq); } - ApplyOptions const applyOptions{&exportSignatureWitnesses}; + ApplyOptions const applyOptions{ + &exportSignatureWitnesses, false, nullptr}; auto next = std::make_shared( *parent, env.app().timeKeeper().closeTime()); @@ -1645,6 +1676,7 @@ struct Export_test : public beast::unit_test::suite testXportPayment(allWithExport); testXportRejectsLocalNetworkID(allWithExport); testXportRejectsUnconfiguredNetworkID(allWithExport); + testExportRejectsAmbiguousLowNetworkID(); testXportEmissionLimit(allWithExport); // ttEXPORT transactor tests diff --git a/src/xrpld/app/consensus/ExportIntent.md b/src/xrpld/app/consensus/ExportIntent.md index af01e5164..6f1724988 100644 --- a/src/xrpld/app/consensus/ExportIntent.md +++ b/src/xrpld/app/consensus/ExportIntent.md @@ -101,7 +101,9 @@ must follow the same deterministic contract as `ExportResultBuilder`: sort signers canonically by AccountID, use an empty `SigningPubKey`, and cap the target-chain `Signers` array at `STTx::maxMultiSigners()` before computing or submitting the blob. The witness may contain extra source-side signatures that -are valid replay input but are not part of the target-chain blob. +are valid replay input but are not part of the target-chain blob. Source-chain +export does not prove the destination account's SignerList or quorum policy; +that compatibility is an operator/client contract for the chosen target chain. This is not an XPOP-style self-contained proof. XPOP embeds its UNL and manifest bundle because it is imported as external proof material. Export witnesses are diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 8037d6ca6..485d0b671 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -1010,6 +1010,7 @@ RCLConsensus::Adaptor::onModeChange(ConsensusMode before, ConsensusMode after) ConsensusPhase RCLConsensus::phase() const { + std::lock_guard _{mutex_}; return consensus_->phase(); } diff --git a/src/xrpld/app/ledger/detail/BuildLedger.cpp b/src/xrpld/app/ledger/detail/BuildLedger.cpp index 26311d4ed..647cdba05 100644 --- a/src/xrpld/app/ledger/detail/BuildLedger.cpp +++ b/src/xrpld/app/ledger/detail/BuildLedger.cpp @@ -68,7 +68,9 @@ collectExportSignatureWitness( if (tx.getFieldU32(sfLedgerSequence) != ledgerSeq || tx.getAccountID(sfAccount) != AccountID{} || tx.getFieldU32(sfSequence) != 0 || - tx.getFieldAmount(sfFee) != beast::zero) + tx.getFieldAmount(sfFee) != beast::zero || + !tx.getSigningPubKey().empty() || !tx.getSignature().empty() || + tx.isFieldPresent(sfPreviousTxnID)) { JLOG(j.warn()) << "Export: ignoring non-canonical signature witness" << " witnessHash=" << tx.getTransactionID() @@ -203,7 +205,7 @@ applyTransactions( view.seq(), j); } - ApplyOptions const applyOptions{&exportSignatureWitnesses}; + ApplyOptions const applyOptions{&exportSignatureWitnesses, false, nullptr}; //@@start rng-entropy-first-application // CRITICAL: Apply consensus entropy pseudo-tx FIRST before any other @@ -393,7 +395,8 @@ buildLedger( accum.seq(), j); } - ApplyOptions const applyOptions{&exportSignatureWitnesses, true}; + ApplyOptions const applyOptions{ + &exportSignatureWitnesses, true, replayData.parent()}; for (auto& tx : replayData.orderedTxns()) applyTransaction( diff --git a/src/xrpld/app/tx/applySteps.h b/src/xrpld/app/tx/applySteps.h index e9f97c23c..d24dd9d1b 100644 --- a/src/xrpld/app/tx/applySteps.h +++ b/src/xrpld/app/tx/applySteps.h @@ -24,9 +24,12 @@ #include #include +#include + namespace ripple { class Application; +class Ledger; class STTx; class TxQ; @@ -56,6 +59,11 @@ struct ApplyOptions // historical manifest map; current ManifestCache state may have rotated // since the ledger closed. bool historicalLedgerReplay = false; + + // LedgerReplay can build consecutive ledgers before the rebuilt parent is + // visible through LedgerMaster. Export apply needs the exact replay parent + // to rebuild the historical validator view. + std::shared_ptr replayParentLedger; }; /** Return true if the transaction can claim a fee (tec), diff --git a/src/xrpld/app/tx/detail/ApplyContext.cpp b/src/xrpld/app/tx/detail/ApplyContext.cpp index 5de9cb456..1ce5a0c79 100644 --- a/src/xrpld/app/tx/detail/ApplyContext.cpp +++ b/src/xrpld/app/tx/detail/ApplyContext.cpp @@ -38,7 +38,8 @@ ApplyContext::ApplyContext( ApplyFlags flags, beast::Journal journal_, ExportResultBuilder::SignatureWitnesses const* exportSignatureWitnesses, - bool historicalLedgerReplay) + bool historicalLedgerReplay, + std::shared_ptr replayParentLedger) : app(app_) , tx(tx_) , preclaimResult(preclaimResult_) @@ -48,6 +49,7 @@ ApplyContext::ApplyContext( , flags_(flags) , exportSignatureWitnesses_(exportSignatureWitnesses) , historicalLedgerReplay_(historicalLedgerReplay) + , replayParentLedger_(std::move(replayParentLedger)) { view_.emplace(&base_, flags_); } diff --git a/src/xrpld/app/tx/detail/ApplyContext.h b/src/xrpld/app/tx/detail/ApplyContext.h index 7ff33e463..8336119ef 100644 --- a/src/xrpld/app/tx/detail/ApplyContext.h +++ b/src/xrpld/app/tx/detail/ApplyContext.h @@ -27,11 +27,14 @@ #include #include #include +#include #include #include namespace ripple { +class Ledger; + /** State information when applying a tx. */ class ApplyContext { @@ -46,7 +49,8 @@ public: beast::Journal = beast::Journal{beast::Journal::getNullSink()}, ExportResultBuilder::SignatureWitnesses const* exportSignatureWitnesses = nullptr, - bool historicalLedgerReplay = false); + bool historicalLedgerReplay = false, + std::shared_ptr replayParentLedger = nullptr); Application& app; STTx const& tx; @@ -152,6 +156,12 @@ public: return historicalLedgerReplay_; } + std::shared_ptr + replayParentLedger() const + { + return replayParentLedger_; + } + ApplyFlags const& flags() { @@ -174,6 +184,7 @@ private: std::optional view_; ExportResultBuilder::SignatureWitnesses const* exportSignatureWitnesses_; bool historicalLedgerReplay_; + std::shared_ptr replayParentLedger_; }; } // namespace ripple diff --git a/src/xrpld/app/tx/detail/Export.cpp b/src/xrpld/app/tx/detail/Export.cpp index 41f5390c9..39135eba8 100644 --- a/src/xrpld/app/tx/detail/Export.cpp +++ b/src/xrpld/app/tx/detail/Export.cpp @@ -125,8 +125,18 @@ Export::doApply() auto& consensusExtensions = ctx_.app.getConsensusExtensions(); bool const standalone = ctx_.app.config().standalone(); auto const& valKeys = ctx_.app.getValidatorKeys(); - auto const parentLedger = - ctx_.app.getLedgerMaster().getLedgerByHash(view().info().parentHash); + auto parentLedger = ctx_.replayParentLedger(); + if (parentLedger && parentLedger->info().hash != view().info().parentHash) + { + JLOG(j_.warn()) << "Export: ignoring mismatched replay parent" + << " txHash=" << txId << " ledgerSeq=" << currentSeq + << " parentHash=" << view().info().parentHash + << " replayParentHash=" << parentLedger->info().hash; + parentLedger.reset(); + } + if (!parentLedger) + parentLedger = ctx_.app.getLedgerMaster().getLedgerByHash( + view().info().parentHash); if (!standalone && !parentLedger) { JLOG(j_.warn()) << "Export: retrying without parent ledger" diff --git a/src/xrpld/app/tx/detail/ExportLedgerOps.h b/src/xrpld/app/tx/detail/ExportLedgerOps.h index b6eff353f..c11cdd346 100644 --- a/src/xrpld/app/tx/detail/ExportLedgerOps.h +++ b/src/xrpld/app/tx/detail/ExportLedgerOps.h @@ -103,8 +103,8 @@ checkExportTxnLimit(ReadView const& view, beast::Journal j) /// - Networks > 1024: sfNetworkID is REQUIRED and must match /// /// So: if exported tx has sfNetworkID matching local → self-target. -/// if local NETWORK_ID is 0 (unconfigured) and tx has no -/// sfNetworkID → can't distinguish self from cross-chain, reject. +/// if local NETWORK_ID <= 1024 and tx has no sfNetworkID → can't +/// distinguish self from another low-ID chain, reject. inline TER validateNetworkID( STTx const& stx, @@ -120,10 +120,10 @@ validateNetworkID( return temMALFORMED; } - if (localNetworkID == 0 && !stx.isFieldPresent(sfNetworkID)) + if (localNetworkID <= 1024 && !stx.isFieldPresent(sfNetworkID)) { JLOG(j.warn()) << "ExportLedgerOps: rejected export with " - "unconfigured NETWORK_ID"; + "ambiguous low NETWORK_ID"; return temMALFORMED; } diff --git a/src/xrpld/app/tx/detail/ExportResultBuilder.cpp b/src/xrpld/app/tx/detail/ExportResultBuilder.cpp index 6591ee668..5e5d79e87 100644 --- a/src/xrpld/app/tx/detail/ExportResultBuilder.cpp +++ b/src/xrpld/app/tx/detail/ExportResultBuilder.cpp @@ -40,7 +40,8 @@ buildSigners(SignatureSnapshot const& signatures, bool capForTargetChain) // XRPL validates the Signers array size before checking signer weights. // Export quorum is decided earlier from the agreed sidecar snapshot; this - // cap only materializes a target-chain-valid canonical prefix. + // cap only materializes a protocol-size-valid canonical prefix. The + // destination account's SignerList/quorum remains an operator contract. if (capForTargetChain) { auto const maxSigners = STTx::maxMultiSigners(); @@ -114,6 +115,7 @@ signaturesFromWitness(STTx const& witness) for (auto const& signer : witness.getFieldArray(sfSigners)) { if (signer.getFName() != sfSigner || + !signer.isFieldPresent(sfAccount) || !signer.isFieldPresent(sfSigningPubKey) || !signer.isFieldPresent(sfTxnSignature)) return std::nullopt; @@ -122,6 +124,10 @@ signaturesFromWitness(STTx const& witness) if (!publicKeyType(makeSlice(pkBlob))) return std::nullopt; + if (signer.getAccountID(sfAccount) != + calcAccountID(PublicKey(makeSlice(pkBlob)))) + return std::nullopt; + auto const sigBlob = signer.getFieldVL(sfTxnSignature); if (sigBlob.empty() || sigBlob.size() > maxTxnSignatureBytes) return std::nullopt; diff --git a/src/xrpld/app/tx/detail/applySteps.cpp b/src/xrpld/app/tx/detail/applySteps.cpp index d1def68d6..2e091b93f 100644 --- a/src/xrpld/app/tx/detail/applySteps.cpp +++ b/src/xrpld/app/tx/detail/applySteps.cpp @@ -411,7 +411,8 @@ doApply( preclaimResult.flags, preclaimResult.j, options.exportSignatureWitnesses, - options.historicalLedgerReplay); + options.historicalLedgerReplay, + options.replayParentLedger); return invoke_apply(ctx); } catch (std::exception const& e)