From 2b87f6b764f65e05972162e4071b2d77e30b81c0 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Thu, 5 Jun 2025 17:42:20 +0700 Subject: [PATCH 01/18] experiment: debug missing previoustxnid --- src/ripple/ledger/detail/ApplyStateTable.h | 13 +- src/ripple/ledger/impl/ApplyStateTable.cpp | 48 ++++++- src/ripple/ledger/impl/ApplyViewImpl.cpp | 3 +- src/test/app/SetTrust_test.cpp | 145 ++++++++++++++++++++- 4 files changed, 196 insertions(+), 13 deletions(-) diff --git a/src/ripple/ledger/detail/ApplyStateTable.h b/src/ripple/ledger/detail/ApplyStateTable.h index f88063b3c2..40bed71108 100644 --- a/src/ripple/ledger/detail/ApplyStateTable.h +++ b/src/ripple/ledger/detail/ApplyStateTable.h @@ -53,6 +53,16 @@ class ApplyStateTable items_t items_; XRPAmount dropsDestroyed_{0}; + // Track original PreviousTxnID/LgrSeq values to restore after provisional + // metadata + struct ThreadingState + { + uint256 prevTxnID; + uint32_t prevTxnLgrSeq; + bool hasPrevTxnID; + }; + mutable std::map originalThreadingState_; + public: ApplyStateTable() = default; ApplyStateTable(ApplyStateTable&&) = default; @@ -73,6 +83,7 @@ class ApplyStateTable std::optional const& deliver, std::vector const& hookExecution, std::vector const& hookEmission, + bool threadOwners, beast::Journal j); void @@ -138,7 +149,7 @@ class ApplyStateTable } private: - static void + void threadItem(TxMeta& meta, std::shared_ptr const& to); std::shared_ptr diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index 85ce584821..09c3b19f2e 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -118,6 +118,7 @@ ApplyStateTable::generateTxMeta( std::optional const& deliver, std::vector const& hookExecution, std::vector const& hookEmission, + bool doThreading, beast::Journal j) { TxMeta meta(tx.getTransactionID(), to.seq()); @@ -162,7 +163,10 @@ ApplyStateTable::generateTxMeta( if (type == &sfDeletedNode) { assert(origNode && curNode); - threadOwners(to, meta, origNode, newMod, j); + if (doThreading) + { + threadOwners(to, meta, origNode, newMod, j); + } STObject prevs(sfPreviousFields); for (auto const& obj : *origNode) @@ -194,8 +198,9 @@ ApplyStateTable::generateTxMeta( { assert(curNode && origNode); - if (curNode->isThreadedType()) // thread transaction to node - // item modified + if (curNode->isThreadedType() && + doThreading) // thread transaction to node + // item modified threadItem(meta, curNode); STObject prevs(sfPreviousFields); @@ -226,9 +231,11 @@ ApplyStateTable::generateTxMeta( else if (type == &sfCreatedNode) // if created, thread to owner(s) { assert(curNode && !origNode); - threadOwners(to, meta, curNode, newMod, j); + if (doThreading) + threadOwners(to, meta, curNode, newMod, j); - if (curNode->isThreadedType()) // always thread to self + if (curNode->isThreadedType() && + doThreading) // always thread to self threadItem(meta, curNode); STObject news(sfNewFields); @@ -274,8 +281,8 @@ ApplyStateTable::apply( if (!to.open()) { // generate meta - auto [meta, newMod] = - generateTxMeta(to, tx, deliver, hookExecution, hookEmission, j); + auto [meta, newMod] = generateTxMeta( + to, tx, deliver, hookExecution, hookEmission, true, j); // add any new modified nodes to the modification set for (auto& mod : newMod) @@ -552,6 +559,33 @@ ApplyStateTable::destroyXRP(XRPAmount const& fee) void ApplyStateTable::threadItem(TxMeta& meta, std::shared_ptr const& sle) { + // Save the original threading state if we haven't already + auto const key = sle->key(); + if (originalThreadingState_.find(key) == originalThreadingState_.end()) + { + ThreadingState state; + state.hasPrevTxnID = sle->isFieldPresent(sfPreviousTxnID); + if (state.hasPrevTxnID) + { + state.prevTxnID = sle->getFieldH256(sfPreviousTxnID); + state.prevTxnLgrSeq = sle->getFieldU32(sfPreviousTxnLgrSeq); + } + originalThreadingState_[key] = state; + } + + // Restore the original state before threading + auto const& origState = originalThreadingState_[key]; + if (origState.hasPrevTxnID) + { + sle->setFieldH256(sfPreviousTxnID, origState.prevTxnID); + sle->setFieldU32(sfPreviousTxnLgrSeq, origState.prevTxnLgrSeq); + } + else + { + sle->makeFieldAbsent(sfPreviousTxnID); + sle->makeFieldAbsent(sfPreviousTxnLgrSeq); + } + key_type prevTxID; LedgerIndex prevLgrID; diff --git a/src/ripple/ledger/impl/ApplyViewImpl.cpp b/src/ripple/ledger/impl/ApplyViewImpl.cpp index 0fa56141c1..16a1c42935 100644 --- a/src/ripple/ledger/impl/ApplyViewImpl.cpp +++ b/src/ripple/ledger/impl/ApplyViewImpl.cpp @@ -41,8 +41,7 @@ ApplyViewImpl::generateProvisionalMeta( beast::Journal j) { auto [meta, _] = items_.generateTxMeta( - to, tx, deliver_, hookExecution_, hookEmission_, j); - + to, tx, deliver_, hookExecution_, hookEmission_, true, j); return meta; } diff --git a/src/test/app/SetTrust_test.cpp b/src/test/app/SetTrust_test.cpp index fce9c4295c..db02e83ed8 100644 --- a/src/test/app/SetTrust_test.cpp +++ b/src/test/app/SetTrust_test.cpp @@ -256,6 +256,145 @@ class SetTrust_test : public beast::unit_test::suite check_quality(!createQuality); } + void + testPreviousTxnID(FeatureBitset features) + { + testcase("Check PreviousTxnID in trustline metadata"); + + using namespace test::jtx; + Env env{*this, features}; + + auto const alice = Account{"alice"}; + auto const bob = Account{"bob"}; + auto const USD = bob["USD"]; + + env.fund(XRP(10000), alice, bob); + env.close(); + + // Create a trustline + env(trust(alice, USD(1000))); + env.close(); + + // Get the transaction metadata + auto const meta1 = env.meta(); + BEAST_EXPECT(meta1); + + // Check if ModifiedNode has PreviousTxnID at root level + auto const& affectedNodes = meta1->getFieldArray(sfAffectedNodes); + bool foundPreviousTxnID = false; + + for (auto const& node : affectedNodes) + { + if (node.isFieldPresent(sfModifiedNode)) + { + auto const& modNode = const_cast(node) + .getField(sfModifiedNode) + .downcast(); + + if (modNode.getFieldU16(sfLedgerEntryType) == ltRIPPLE_STATE) + { + // PreviousTxnID should be at the root of ModifiedNode + if (modNode.isFieldPresent(sfPreviousTxnID)) + { + foundPreviousTxnID = true; + BEAST_EXPECT( + modNode.isFieldPresent(sfPreviousTxnLgrSeq)); + } + } + } + else if (node.isFieldPresent(sfCreatedNode)) + { + auto const& createdNode = const_cast(node) + .getField(sfCreatedNode) + .downcast(); + + if (createdNode.getFieldU16(sfLedgerEntryType) == + ltRIPPLE_STATE) + { + // For created nodes, PreviousTxnID might not exist yet + // but we should still check + if (createdNode.isFieldPresent(sfPreviousTxnID)) + { + foundPreviousTxnID = true; + } + } + } + } + + // Now modify the trustline with a payment + env(pay(bob, alice, USD(100))); + env.close(); + + // Create another transaction that modifies the trustline + env(trust(alice, USD(2000))); + env.close(); + + // Get the second transaction metadata + auto const meta2 = env.meta(); + BEAST_EXPECT(meta2); + + // Check ModifiedNode for PreviousTxnID + auto const& affectedNodes2 = meta2->getFieldArray(sfAffectedNodes); + bool foundPreviousTxnIDInModified = false; + + for (auto const& node : affectedNodes2) + { + std::cout << "something" << std::endl; + + if (node.getFName() == sfModifiedNode) + { + auto const& modNode = node; + + auto json = modNode.getJson({}); + std::cout << json << std::endl; + + if (modNode.getFieldU16(sfLedgerEntryType) == ltRIPPLE_STATE) + { + // This SHOULD have PreviousTxnID at root level + if (modNode.isFieldPresent(sfPreviousTxnID)) + { + foundPreviousTxnIDInModified = true; + auto prevTxnID = modNode.getFieldH256(sfPreviousTxnID); + auto prevLgrSeq = + modNode.getFieldU32(sfPreviousTxnLgrSeq); + + BEAST_EXPECT( + modNode.isFieldPresent(sfPreviousTxnLgrSeq)); + BEAST_EXPECT(prevTxnID != beast::zero); + BEAST_EXPECT(prevLgrSeq > 0); + + // Log for debugging + std::cout << "Found PreviousTxnID: " << prevTxnID + << " at ledger: " << prevLgrSeq << std::endl; + } + else + { + std::cout + << "ERROR: ModifiedNode missing PreviousTxnID!" + << std::endl; + + // Check if it's incorrectly in PreviousFields + if (modNode.isFieldPresent(sfPreviousFields)) + { + auto const& prevFields = + const_cast(modNode) + .getField(sfPreviousFields) + .downcast(); + if (prevFields.isFieldPresent(sfPreviousTxnID)) + { + std::cout << "WARNING: PreviousTxnID found in " + "PreviousFields instead of root!" + << std::endl; + } + } + } + } + } + } + + BEAST_EXPECT(foundPreviousTxnIDInModified); + } + void testDisallowIncoming(FeatureBitset features) { @@ -368,10 +507,10 @@ class SetTrust_test : public beast::unit_test::suite { using namespace test::jtx; auto const sa = supported_amendments(); - testWithFeats(sa - disallowIncoming); - testWithFeats(sa); + // Just run the PreviousTxnID test for debugging + testPreviousTxnID(sa); } }; BEAST_DEFINE_TESTSUITE(SetTrust, app, ripple); } // namespace test -} // namespace ripple +} // namespace ripple \ No newline at end of file From e260a60347fee57fb67310f6600992ec1d92362e Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Fri, 6 Jun 2025 13:36:01 +0700 Subject: [PATCH 02/18] feat: add fixPreviousTxnID amendment --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/ledger/detail/ApplyStateTable.h | 10 +- src/ripple/ledger/impl/ApplyStateTable.cpp | 107 +++++----- src/ripple/ledger/impl/ApplyViewImpl.cpp | 2 +- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/impl/Feature.cpp | 1 + src/test/app/PreviousTxn_test.cpp | 217 +++++++++++++++++++++ src/test/app/SetTrust_test.cpp | 145 +------------- src/test/app/TxQ_test.cpp | 2 +- 9 files changed, 295 insertions(+), 193 deletions(-) create mode 100644 src/test/app/PreviousTxn_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 92593d538d..25d9258a5b 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -751,6 +751,7 @@ if (tests) src/test/app/Path_test.cpp src/test/app/PayChan_test.cpp src/test/app/PayStrand_test.cpp + src/test/app/PreviousTxn_test.cpp src/test/app/PseudoTx_test.cpp src/test/app/RCLCensorshipDetector_test.cpp src/test/app/RCLValidations_test.cpp diff --git a/src/ripple/ledger/detail/ApplyStateTable.h b/src/ripple/ledger/detail/ApplyStateTable.h index 40bed71108..c64f03469f 100644 --- a/src/ripple/ledger/detail/ApplyStateTable.h +++ b/src/ripple/ledger/detail/ApplyStateTable.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -83,7 +84,6 @@ class ApplyStateTable std::optional const& deliver, std::vector const& hookExecution, std::vector const& hookEmission, - bool threadOwners, beast::Journal j); void @@ -150,7 +150,10 @@ class ApplyStateTable private: void - threadItem(TxMeta& meta, std::shared_ptr const& to); + threadItem( + TxMeta& meta, + std::shared_ptr const& to, + Rules const& rules); std::shared_ptr getForMod( @@ -165,7 +168,8 @@ class ApplyStateTable TxMeta& meta, AccountID const& to, Mods& mods, - beast::Journal j); + beast::Journal j, + Rules const& rules); void threadOwners( diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index 09c3b19f2e..8b1e7aa80f 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -118,7 +118,6 @@ ApplyStateTable::generateTxMeta( std::optional const& deliver, std::vector const& hookExecution, std::vector const& hookEmission, - bool doThreading, beast::Journal j) { TxMeta meta(tx.getTransactionID(), to.seq()); @@ -163,10 +162,7 @@ ApplyStateTable::generateTxMeta( if (type == &sfDeletedNode) { assert(origNode && curNode); - if (doThreading) - { - threadOwners(to, meta, origNode, newMod, j); - } + threadOwners(to, meta, origNode, newMod, j); STObject prevs(sfPreviousFields); for (auto const& obj : *origNode) @@ -198,10 +194,9 @@ ApplyStateTable::generateTxMeta( { assert(curNode && origNode); - if (curNode->isThreadedType() && - doThreading) // thread transaction to node - // item modified - threadItem(meta, curNode); + if (curNode->isThreadedType()) // thread transaction to node + // item modified + threadItem(meta, curNode, to.rules()); STObject prevs(sfPreviousFields); for (auto const& obj : *origNode) @@ -231,12 +226,10 @@ ApplyStateTable::generateTxMeta( else if (type == &sfCreatedNode) // if created, thread to owner(s) { assert(curNode && !origNode); - if (doThreading) - threadOwners(to, meta, curNode, newMod, j); + threadOwners(to, meta, curNode, newMod, j); - if (curNode->isThreadedType() && - doThreading) // always thread to self - threadItem(meta, curNode); + if (curNode->isThreadedType()) // always thread to self + threadItem(meta, curNode, to.rules()); STObject news(sfNewFields); for (auto const& obj : *curNode) @@ -281,8 +274,8 @@ ApplyStateTable::apply( if (!to.open()) { // generate meta - auto [meta, newMod] = generateTxMeta( - to, tx, deliver, hookExecution, hookEmission, true, j); + auto [meta, newMod] = + generateTxMeta(to, tx, deliver, hookExecution, hookEmission, j); // add any new modified nodes to the modification set for (auto& mod : newMod) @@ -557,33 +550,44 @@ ApplyStateTable::destroyXRP(XRPAmount const& fee) // Insert this transaction to the SLE's threading list void -ApplyStateTable::threadItem(TxMeta& meta, std::shared_ptr const& sle) +ApplyStateTable::threadItem( + TxMeta& meta, + std::shared_ptr const& sle, + const Rules& rules) { - // Save the original threading state if we haven't already - auto const key = sle->key(); - if (originalThreadingState_.find(key) == originalThreadingState_.end()) + if (rules.enabled(fixPreviousTxnID)) { - ThreadingState state; - state.hasPrevTxnID = sle->isFieldPresent(sfPreviousTxnID); - if (state.hasPrevTxnID) + auto const key = sle->key(); + auto iter = originalThreadingState_.find(key); + + if (iter == originalThreadingState_.end()) { - state.prevTxnID = sle->getFieldH256(sfPreviousTxnID); - state.prevTxnLgrSeq = sle->getFieldU32(sfPreviousTxnLgrSeq); + // First time (provisional metadata) - save the original state + ThreadingState state; + state.hasPrevTxnID = sle->isFieldPresent(sfPreviousTxnID); + if (state.hasPrevTxnID) + { + state.prevTxnID = sle->getFieldH256(sfPreviousTxnID); + state.prevTxnLgrSeq = sle->getFieldU32(sfPreviousTxnLgrSeq); + } + originalThreadingState_[key] = state; + } + else + { + // Subsequent call (final metadata) - restore to pristine state + // before threading + auto const& origState = iter->second; + if (origState.hasPrevTxnID) + { + sle->setFieldH256(sfPreviousTxnID, origState.prevTxnID); + sle->setFieldU32(sfPreviousTxnLgrSeq, origState.prevTxnLgrSeq); + } + else + { + sle->makeFieldAbsent(sfPreviousTxnID); + sle->makeFieldAbsent(sfPreviousTxnLgrSeq); + } } - originalThreadingState_[key] = state; - } - - // Restore the original state before threading - auto const& origState = originalThreadingState_[key]; - if (origState.hasPrevTxnID) - { - sle->setFieldH256(sfPreviousTxnID, origState.prevTxnID); - sle->setFieldU32(sfPreviousTxnLgrSeq, origState.prevTxnLgrSeq); - } - else - { - sle->makeFieldAbsent(sfPreviousTxnID); - sle->makeFieldAbsent(sfPreviousTxnLgrSeq); } key_type prevTxID; @@ -663,7 +667,8 @@ ApplyStateTable::threadTx( TxMeta& meta, AccountID const& to, Mods& mods, - beast::Journal j) + beast::Journal j, + Rules const& rules) { auto const sle = getForMod(base, keylet::account(to).key, mods, j); if (!sle) @@ -674,7 +679,7 @@ ApplyStateTable::threadTx( JLOG(j.warn()) << "Threading to non-existent account: " << toBase58(to); return; } - threadItem(meta, sle); + threadItem(meta, sle, rules); } void @@ -693,14 +698,26 @@ ApplyStateTable::threadOwners( break; } case ltRIPPLE_STATE: { - threadTx(base, meta, (*sle)[sfLowLimit].getIssuer(), mods, j); - threadTx(base, meta, (*sle)[sfHighLimit].getIssuer(), mods, j); + threadTx( + base, + meta, + (*sle)[sfLowLimit].getIssuer(), + mods, + j, + base.rules()); + threadTx( + base, + meta, + (*sle)[sfHighLimit].getIssuer(), + mods, + j, + base.rules()); break; } default: { // If sfAccount is present, thread to that account if (auto const optSleAcct{(*sle)[~sfAccount]}) - threadTx(base, meta, *optSleAcct, mods, j); + threadTx(base, meta, *optSleAcct, mods, j, base.rules()); // Don't thread a check's sfDestination unless the amendment is // enabled @@ -710,7 +727,7 @@ ApplyStateTable::threadOwners( // If sfDestination is present, thread to that account if (auto const optSleDest{(*sle)[~sfDestination]}) - threadTx(base, meta, *optSleDest, mods, j); + threadTx(base, meta, *optSleDest, mods, j, base.rules()); } } } diff --git a/src/ripple/ledger/impl/ApplyViewImpl.cpp b/src/ripple/ledger/impl/ApplyViewImpl.cpp index 16a1c42935..e21c361c5d 100644 --- a/src/ripple/ledger/impl/ApplyViewImpl.cpp +++ b/src/ripple/ledger/impl/ApplyViewImpl.cpp @@ -41,7 +41,7 @@ ApplyViewImpl::generateProvisionalMeta( beast::Journal j) { auto [meta, _] = items_.generateTxMeta( - to, tx, deliver_, hookExecution_, hookEmission_, true, j); + to, tx, deliver_, hookExecution_, hookEmission_, j); return meta; } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 3764b3b78c..ea8da43490 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -74,7 +74,7 @@ namespace detail { // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 81; +static constexpr std::size_t numFeatures = 82; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -369,6 +369,7 @@ extern uint256 const fixXahauV3; extern uint256 const fix20250131; extern uint256 const featureHookCanEmit; extern uint256 const fixRewardClaimFlags; +extern uint256 const fixPreviousTxnID; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 68767da687..f23f958fbb 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -475,6 +475,7 @@ REGISTER_FIX (fixXahauV3, Supported::yes, VoteBehavior::De REGISTER_FIX (fix20250131, Supported::yes, VoteBehavior::DefaultYes); REGISTER_FEATURE(HookCanEmit, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixRewardClaimFlags, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fixPreviousTxnID, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/test/app/PreviousTxn_test.cpp b/src/test/app/PreviousTxn_test.cpp new file mode 100644 index 0000000000..3c82561f25 --- /dev/null +++ b/src/test/app/PreviousTxn_test.cpp @@ -0,0 +1,217 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2025 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include + +namespace ripple { +namespace test { + +class PreviousTxnID_test : public beast::unit_test::suite +{ +public: + void + testPreviousTxnID(FeatureBitset features) + { + testcase("Check PreviousTxnID in trustline metadata"); + + using namespace test::jtx; + Env env{ + *this, envconfig(), features, nullptr, beast::severities::kInfo}; + auto j = env.app().logs().journal("PreviousTxnID_test"); + + auto const alice = Account{"alice"}; + auto const bob = Account{"bob"}; + auto const USD = bob["USD"]; + + env.fund(XRP(10000), alice, bob); + env.close(); + + // Create a trustline + env(trust(alice, USD(1000))); + env.close(); + + // Get the transaction metadata + auto const meta1 = env.meta(); + BEAST_EXPECT(meta1); + + // Check if ModifiedNode has PreviousTxnID at root level + auto const& affectedNodes = meta1->getFieldArray(sfAffectedNodes); + bool foundPreviousTxnID = false; + bool foundModifiedRippleState = false; + bool foundCreatedRippleState = false; + + for (auto const& node : affectedNodes) + { + if (node.getFieldU16(sfLedgerEntryType) == ltRIPPLE_STATE) + { + if (node.getFName() == sfModifiedNode) + { + BEAST_EXPECT(false); + } + else if (node.getFName() == sfCreatedNode) + { + foundCreatedRippleState = true; + foundPreviousTxnID = node.isFieldPresent(sfPreviousTxnID); + } + } + } + + BEAST_EXPECT(foundCreatedRippleState); + // Why we expect NO PreviousTxnID in newly created trustline metadata: + // + // PreviousTxnID in metadata indicates which transaction PREVIOUSLY + // modified an object, not which transaction created it. For a newly + // created object, there is no previous transaction - the current + // transaction is the first one to touch this object. + // + // While the SLE itself will have PreviousTxnID set to the creating + // transaction (by ApplyStateTable::threadItem), the metadata correctly + // omits PreviousTxnID from CreatedNode because there was no previous + // modification to reference. + // + // Technical detail: trustCreate() creates the RippleState with + // PreviousTxnID as a defaultObject placeholder (since it's soeREQUIRED + // in LedgerFormats.cpp). ApplyStateTable::generateTxMeta skips fields + // with zero/empty values when generating metadata, so the unset + // PreviousTxnID doesn't appear in the CreatedNode output. + BEAST_EXPECT(foundPreviousTxnID == false); + BEAST_EXPECT(foundModifiedRippleState == false); + + // Now let's check the actual ledger entry to see if PreviousTxnID was + // set Get the trustline from the ledger + auto const trustlineKey = keylet::line(alice, bob, USD.currency); + auto const sleTrustline = env.le(trustlineKey); + BEAST_EXPECT(sleTrustline); + + if (sleTrustline) + { + // Check if the SLE itself has PreviousTxnID set + // Even though it didn't appear in metadata, the SLE should have it + // because ApplyStateTable::threadItem() sets it after creation + bool sleHasPrevTxnID = + sleTrustline->isFieldPresent(sfPreviousTxnID); + bool sleHasPrevTxnSeq = + sleTrustline->isFieldPresent(sfPreviousTxnLgrSeq); + + JLOG(j.info()) << "SLE has PreviousTxnID: " << sleHasPrevTxnID; + JLOG(j.info()) << "SLE has PreviousTxnLgrSeq: " << sleHasPrevTxnSeq; + + if (sleHasPrevTxnID && sleHasPrevTxnSeq) + { + auto const prevTxnID = + sleTrustline->getFieldH256(sfPreviousTxnID); + auto const prevTxnSeq = + sleTrustline->getFieldU32(sfPreviousTxnLgrSeq); + auto const creatingTxnID = env.tx()->getTransactionID(); + + JLOG(j.info()) << "SLE PreviousTxnID: " << prevTxnID; + JLOG(j.info()) << "Creating TxnID: " << creatingTxnID; + JLOG(j.info()) << "SLE PreviousTxnLgrSeq: " << prevTxnSeq; + + // The PreviousTxnID in the SLE should match the transaction + // that created it + BEAST_EXPECT(prevTxnID == creatingTxnID); + BEAST_EXPECT(prevTxnSeq == env.closed()->seq()); + } + else + { + // This would indicate that ApplyStateTable::threadItem() didn't + // set PreviousTxnID on the newly created trustline + JLOG(j.warn()) + << "Newly created trustline SLE missing PreviousTxnID!"; + BEAST_EXPECT(false); + } + } + + // Now modify the trustline with a payment + env(pay(bob, alice, USD(100))); + env.close(); + + // Get the second transaction metadata + auto const meta2 = env.meta(); + BEAST_EXPECT(meta2); + + // Check ModifiedNode for PreviousTxnID + auto const& affectedNodes2 = meta2->getFieldArray(sfAffectedNodes); + bool foundPreviousTxnIDInModified = false; + + for (auto const& node : affectedNodes2) + { + if (node.getFName() == sfModifiedNode && + node.getFieldU16(sfLedgerEntryType) == ltRIPPLE_STATE) + { + auto json = node.getJson({}); + JLOG(j.trace()) << json; + + // Why we expect PreviousTxnID in ModifiedNode metadata: + // + // When a transaction modifies an existing RippleState, the + // ApplyView tracks the modification. During metadata + // generation, ApplyStateTable::generateTxMeta compares the + // before and after states of the SLE. + // + // For ModifiedNode entries, the metadata should include + // PreviousTxnID and PreviousTxnLgrSeq at the root level to + // indicate which transaction last modified this object. + // This is different from PreviousFields, which shows what + // field values changed. + // + // The bug was that ApplyStateTable::threadItem() was + // modifying the original SLE during provisional metadata + // generation, contaminating the "before" state. When final + // metadata was generated, the comparison didn't see + // PreviousTxnID as a change because both states had the new + // value. + if (node.isFieldPresent(sfPreviousTxnID)) + { + foundPreviousTxnIDInModified = true; + auto prevTxnID = node.getFieldH256(sfPreviousTxnID); + auto prevLgrSeq = node.getFieldU32(sfPreviousTxnLgrSeq); + + BEAST_EXPECT(node.isFieldPresent(sfPreviousTxnLgrSeq)); + BEAST_EXPECT(prevTxnID != beast::zero); + BEAST_EXPECT(prevLgrSeq > 0); + + JLOG(j.info()) << "Found PreviousTxnID: " << prevTxnID + << " at ledger: " << prevLgrSeq << std::endl; + } + else + { + BEAST_EXPECT(false); + } + } + } + + BEAST_EXPECT(foundPreviousTxnIDInModified); + } + + void + run() override + { + using namespace test::jtx; + auto const sa = supported_amendments(); + testPreviousTxnID(sa); + } +}; + +BEAST_DEFINE_TESTSUITE(PreviousTxnID, app, ripple); + +} // namespace test +} // namespace ripple \ No newline at end of file diff --git a/src/test/app/SetTrust_test.cpp b/src/test/app/SetTrust_test.cpp index db02e83ed8..fce9c4295c 100644 --- a/src/test/app/SetTrust_test.cpp +++ b/src/test/app/SetTrust_test.cpp @@ -256,145 +256,6 @@ class SetTrust_test : public beast::unit_test::suite check_quality(!createQuality); } - void - testPreviousTxnID(FeatureBitset features) - { - testcase("Check PreviousTxnID in trustline metadata"); - - using namespace test::jtx; - Env env{*this, features}; - - auto const alice = Account{"alice"}; - auto const bob = Account{"bob"}; - auto const USD = bob["USD"]; - - env.fund(XRP(10000), alice, bob); - env.close(); - - // Create a trustline - env(trust(alice, USD(1000))); - env.close(); - - // Get the transaction metadata - auto const meta1 = env.meta(); - BEAST_EXPECT(meta1); - - // Check if ModifiedNode has PreviousTxnID at root level - auto const& affectedNodes = meta1->getFieldArray(sfAffectedNodes); - bool foundPreviousTxnID = false; - - for (auto const& node : affectedNodes) - { - if (node.isFieldPresent(sfModifiedNode)) - { - auto const& modNode = const_cast(node) - .getField(sfModifiedNode) - .downcast(); - - if (modNode.getFieldU16(sfLedgerEntryType) == ltRIPPLE_STATE) - { - // PreviousTxnID should be at the root of ModifiedNode - if (modNode.isFieldPresent(sfPreviousTxnID)) - { - foundPreviousTxnID = true; - BEAST_EXPECT( - modNode.isFieldPresent(sfPreviousTxnLgrSeq)); - } - } - } - else if (node.isFieldPresent(sfCreatedNode)) - { - auto const& createdNode = const_cast(node) - .getField(sfCreatedNode) - .downcast(); - - if (createdNode.getFieldU16(sfLedgerEntryType) == - ltRIPPLE_STATE) - { - // For created nodes, PreviousTxnID might not exist yet - // but we should still check - if (createdNode.isFieldPresent(sfPreviousTxnID)) - { - foundPreviousTxnID = true; - } - } - } - } - - // Now modify the trustline with a payment - env(pay(bob, alice, USD(100))); - env.close(); - - // Create another transaction that modifies the trustline - env(trust(alice, USD(2000))); - env.close(); - - // Get the second transaction metadata - auto const meta2 = env.meta(); - BEAST_EXPECT(meta2); - - // Check ModifiedNode for PreviousTxnID - auto const& affectedNodes2 = meta2->getFieldArray(sfAffectedNodes); - bool foundPreviousTxnIDInModified = false; - - for (auto const& node : affectedNodes2) - { - std::cout << "something" << std::endl; - - if (node.getFName() == sfModifiedNode) - { - auto const& modNode = node; - - auto json = modNode.getJson({}); - std::cout << json << std::endl; - - if (modNode.getFieldU16(sfLedgerEntryType) == ltRIPPLE_STATE) - { - // This SHOULD have PreviousTxnID at root level - if (modNode.isFieldPresent(sfPreviousTxnID)) - { - foundPreviousTxnIDInModified = true; - auto prevTxnID = modNode.getFieldH256(sfPreviousTxnID); - auto prevLgrSeq = - modNode.getFieldU32(sfPreviousTxnLgrSeq); - - BEAST_EXPECT( - modNode.isFieldPresent(sfPreviousTxnLgrSeq)); - BEAST_EXPECT(prevTxnID != beast::zero); - BEAST_EXPECT(prevLgrSeq > 0); - - // Log for debugging - std::cout << "Found PreviousTxnID: " << prevTxnID - << " at ledger: " << prevLgrSeq << std::endl; - } - else - { - std::cout - << "ERROR: ModifiedNode missing PreviousTxnID!" - << std::endl; - - // Check if it's incorrectly in PreviousFields - if (modNode.isFieldPresent(sfPreviousFields)) - { - auto const& prevFields = - const_cast(modNode) - .getField(sfPreviousFields) - .downcast(); - if (prevFields.isFieldPresent(sfPreviousTxnID)) - { - std::cout << "WARNING: PreviousTxnID found in " - "PreviousFields instead of root!" - << std::endl; - } - } - } - } - } - } - - BEAST_EXPECT(foundPreviousTxnIDInModified); - } - void testDisallowIncoming(FeatureBitset features) { @@ -507,10 +368,10 @@ class SetTrust_test : public beast::unit_test::suite { using namespace test::jtx; auto const sa = supported_amendments(); - // Just run the PreviousTxnID test for debugging - testPreviousTxnID(sa); + testWithFeats(sa - disallowIncoming); + testWithFeats(sa); } }; BEAST_DEFINE_TESTSUITE(SetTrust, app, ripple); } // namespace test -} // namespace ripple \ No newline at end of file +} // namespace ripple diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 8628d2f025..045e94cc78 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -5049,7 +5049,7 @@ class TxQ1_test : public beast::unit_test::suite testFailInPreclaim(all); testQueuedTxFails(all); testMultiTxnPerAccount(all); - testTieBreaking(all); + testTieBreaking(all - fixPreviousTxnID); testAcctTxnID(all); testMaximum(all); testUnexpectedBalanceChange(all); From c9a61f7d76c7ad9db756a63bc4243887811173ba Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Fri, 6 Jun 2025 13:54:43 +0700 Subject: [PATCH 03/18] refactor: s/fixPreviousTxnID/fixProvisionalDoubleThreading/ --- src/ripple/ledger/impl/ApplyStateTable.cpp | 2 +- src/ripple/protocol/Feature.h | 2 +- src/ripple/protocol/impl/Feature.cpp | 2 +- src/test/app/TxQ_test.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index 8b1e7aa80f..e651da24ea 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -555,7 +555,7 @@ ApplyStateTable::threadItem( std::shared_ptr const& sle, const Rules& rules) { - if (rules.enabled(fixPreviousTxnID)) + if (rules.enabled(fixProvisionalDoubleThreading)) { auto const key = sle->key(); auto iter = originalThreadingState_.find(key); diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index ea8da43490..800bbef4d2 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -369,7 +369,7 @@ extern uint256 const fixXahauV3; extern uint256 const fix20250131; extern uint256 const featureHookCanEmit; extern uint256 const fixRewardClaimFlags; -extern uint256 const fixPreviousTxnID; +extern uint256 const fixProvisionalDoubleThreading; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index f23f958fbb..b752c80930 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -475,7 +475,7 @@ REGISTER_FIX (fixXahauV3, Supported::yes, VoteBehavior::De REGISTER_FIX (fix20250131, Supported::yes, VoteBehavior::DefaultYes); REGISTER_FEATURE(HookCanEmit, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixRewardClaimFlags, Supported::yes, VoteBehavior::DefaultYes); -REGISTER_FIX (fixPreviousTxnID, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixProvisionalDoubleThreading, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 045e94cc78..89e0432753 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -5049,7 +5049,7 @@ class TxQ1_test : public beast::unit_test::suite testFailInPreclaim(all); testQueuedTxFails(all); testMultiTxnPerAccount(all); - testTieBreaking(all - fixPreviousTxnID); + testTieBreaking(all - fixProvisionalDoubleThreading); testAcctTxnID(all); testMaximum(all); testUnexpectedBalanceChange(all); From d4eb995763f96209ded93e475e06e46e1447d701 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Fri, 6 Jun 2025 15:09:34 +0700 Subject: [PATCH 04/18] fix: disable fixProvisionalDoubleThreading in more Q tests --- src/test/app/TxQ_test.cpp | 2 +- src/test/rpc/LedgerRPC_test.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 89e0432753..32c3d47087 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -5067,7 +5067,7 @@ class TxQ1_test : public beast::unit_test::suite testAcctInQueueButEmpty(all); testRPC(all); testExpirationReplacement(all); - testFullQueueGapFill(all); + testFullQueueGapFill(all - fixProvisionalDoubleThreading); testSignAndSubmitSequence(all); testAccountInfo(all); testServerInfo(all); diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 116aa4d516..e6f7669162 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -2088,7 +2088,8 @@ class LedgerRPC_test : public beast::unit_test::suite section.set("normal_consensus_increase_percent", "0"); return cfg; }), - supported_amendments() - featureXahauGenesis}; + supported_amendments() - featureXahauGenesis - + fixProvisionalDoubleThreading}; Json::Value jv; jv[jss::ledger_index] = "current"; From 7efa1d1153c6179b8bcb59906693aca6bcf1c4bc Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Sat, 7 Jun 2025 10:02:20 +0700 Subject: [PATCH 05/18] chore: add suspicious_patterns to .scripts/pre-hook and not-suspicious filter --- .githooks/pre-commit | 12 ++++ .githooks/setup.sh | 4 ++ src/test/app/TxQ_test.cpp | 3 +- suspicious_patterns.sh | 114 +++++++++++++++++++++++++++++++++++--- 4 files changed, 124 insertions(+), 9 deletions(-) create mode 100755 .githooks/pre-commit create mode 100644 .githooks/setup.sh diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 0000000000..7cef4de6bf --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,12 @@ +#!/bin/bash + +# Pre-commit hook that runs the suspicious patterns check on staged files + +# Get the repository's root directory +repo_root=$(git rev-parse --show-toplevel) + +# Run the suspicious patterns script in pre-commit mode +"$repo_root/suspicious_patterns.sh" --pre-commit + +# Exit with the same code as the script +exit $? \ No newline at end of file diff --git a/.githooks/setup.sh b/.githooks/setup.sh new file mode 100644 index 0000000000..eb77aa11c5 --- /dev/null +++ b/.githooks/setup.sh @@ -0,0 +1,4 @@ +#!/bin/bash + +echo "Configuring git to use .githooks directory..." +git config core.hooksPath .githooks diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 32c3d47087..71d25bfe6a 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -186,7 +186,8 @@ class TxQ1_test : public beast::unit_test::suite // In order for the vote to occur, we must run as a validator p->section("validation_seed") - .legacy("shUwVw52ofnCUX5m7kPTKzJdr4HEH"); + .legacy("shUwVw52ofnCUX5m7kPTKzJdr4HEH"); // not-suspicious + // test seed } return p; } diff --git a/suspicious_patterns.sh b/suspicious_patterns.sh index 39287a6a19..421919d948 100755 --- a/suspicious_patterns.sh +++ b/suspicious_patterns.sh @@ -1,25 +1,123 @@ #!/bin/bash +# Exit on error, undefined variables, and pipe failures +set -euo pipefail + +# Enable debug mode if DEBUG environment variable is set +[[ "${DEBUG:-}" == "1" ]] && set -x + +# This script prevents accidental commits of XRPL/Ripple cryptographic keys +# It searches for: +# - Secret seeds (s...) - can derive keypairs +# - Private keys (p...) - validator private keys! +# - Raw key material (02/03/ED + hex) - compressed public keys or Ed25519 keys +# +# Usage: +# suspicious_patterns.sh # Check last commit (for CI) +# suspicious_patterns.sh --pre-commit # Check staged files (for pre-commit hook) +# +# WARNING: If this catches a real key in CI, that key is already compromised! +# The key has been pushed to the git history and must be immediately decommissioned. +# +# To mark keys as safe, add comment: // not-suspicious +# Files excluded: See exclude_files array below +# Lines excluded: Matching exclude_pattern regex below + +# Pattern for lines to exclude from checking +exclude_pattern="public_key|not-suspicious" + +# Array of files to exclude from checking (paths relative to repo root) +exclude_files=( + "src/test/app/Import_test.cpp" + "cfg/validators-example.txt" +) + # Get the repository's root directory repo_root=$(git rev-parse --show-toplevel) -# Get a list of files changed in the last commit with their relative paths -files_changed=$(git diff --name-only --relative HEAD~1 HEAD) +# Determine which files to check based on context: +# 1. Pre-commit hook: Check staged files before commit +# 2. GitHub PR: Check all files changed in the PR (HEAD is a synthetic merge commit) +# 3. Regular push/CI: Check files in the last real commit +if [[ "${1:-}" == "--pre-commit" ]]; then + # Pre-commit mode: Check what's about to be committed + files_changed=$(git diff --cached --name-only --relative) + mode="staged files" +else + # CI mode - need to handle two different scenarios + if [[ "${GITHUB_EVENT_NAME:-}" == "pull_request" ]]; then + # GitHub PR event: HEAD is a synthetic merge commit created by GitHub + # that merges PR branch into base. Must diff against base to get only PR files. + base_ref="${GITHUB_BASE_REF:-dev}" + + # Ensure we have the base branch (GitHub Actions shallow clone might not have it) + if ! git rev-parse --verify "origin/$base_ref" >/dev/null 2>&1; then + echo "Fetching base branch origin/$base_ref..." + git fetch --depth=1 origin "$base_ref" + fi + + # Since there's no merge base in shallow clones, we need to be creative + # Save current HEAD, switch to base, then diff the trees + current_head=$(git rev-parse HEAD) + echo "Comparing against base branch origin/$base_ref..." + + # Get the tree objects to compare (this works even without shared history) + base_tree=$(git rev-parse "origin/$base_ref^{tree}") + head_tree=$(git rev-parse "$current_head^{tree}") + + # Compare the two trees directly + files_changed=$(git diff --name-only "$base_tree" "$head_tree") + mode="PR changes" + else + # Regular push event: Check the actual commit that was pushed + # Using 'git show' works even with shallow clones (no HEAD~1 needed) + # See: https://github.com/Xahau/xahaud/actions/runs/15492442104/job/43620965462#step:3:11 + files_changed=$(git show --name-only --pretty=format:'' HEAD) + mode="last commit" + fi +fi + +echo "Checking $mode for suspicious patterns..." + +# Show additional info in CI or when verbose mode is enabled +if [[ -n "${CI:-}" ]] || [[ "${VERBOSE:-}" == "1" ]]; then + if [[ "${1:-}" != "--pre-commit" ]]; then + echo "Commit: $(git rev-parse HEAD)" + fi + echo "Files to check:" + if [[ -n "$files_changed" ]]; then + echo "$files_changed" | nl + else + echo " (none)" + fi +fi # Loop through each file and search for the patterns for file in $files_changed; do - # Skip if the file is Import_test.cpp (exact filename match regardless of path) - if [[ "$(basename "$file")" == "Import_test.cpp" ]]; then - continue - fi + # Check if file should be excluded (exact path match) + for excluded in "${exclude_files[@]}"; do + if [[ "$file" == "$excluded" ]]; then + continue 2 # Continue outer loop + fi + done # Construct the absolute path absolute_path="$repo_root/$file" # Check if the file exists (it might have been deleted) if [ -f "$absolute_path" ]; then - # Search the file for the given patterns, but exclude lines containing 'public_key' - grep_output=$(grep -n -E '(([^rpshnaf39wBUDNEGHJKLM4PQRST7VWXYZ2bcdeCg65jkm8oFqi1tuvAxyz]|^)(s|p)[rpshnaf39wBUDNEGHJKLM4PQRST7VWXYZ2bcdeCg65jkm8oFqi1tuvAxyz]{25,60}([^(]|$)))|([^A-Fa-f0-9](02|03|ED)[A-Fa-f0-9]{64})' "$absolute_path" | grep -v "public_key") + # Get file content based on mode + if [[ "${1:-}" == "--pre-commit" ]]; then + # For staged files, use git show with the staging area + file_content=$(git show ":$file" 2>/dev/null) + else + # For committed files, read from disk + file_content=$(cat "$absolute_path") + fi + + # Search the file content for the given patterns, but exclude lines matching the exclusion pattern + # Use || true to prevent grep from failing the script when no matches are found + grep_output=$(echo "$file_content" | grep -n -E '(([^rpshnaf39wBUDNEGHJKLM4PQRST7VWXYZ2bcdeCg65jkm8oFqi1tuvAxyz]|^)(s|p)[rpshnaf39wBUDNEGHJKLM4PQRST7VWXYZ2bcdeCg65jkm8oFqi1tuvAxyz]{25,60}([^(]|$)))|([^A-Fa-f0-9](02|03|ED)[A-Fa-f0-9]{64})' | grep -vE "$exclude_pattern" || true) # Check if grep found any matches if [ ! -z "$grep_output" ]; then From 17f86a83cf4cb07f6f47ecb7947b12b325017067 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Sun, 8 Jun 2025 12:32:46 +0700 Subject: [PATCH 06/18] rm: kill annoying checkpatterns job --- .github/workflows/checkpatterns.yml | 20 -------------------- 1 file changed, 20 deletions(-) delete mode 100644 .github/workflows/checkpatterns.yml diff --git a/.github/workflows/checkpatterns.yml b/.github/workflows/checkpatterns.yml deleted file mode 100644 index 8b60a12e55..0000000000 --- a/.github/workflows/checkpatterns.yml +++ /dev/null @@ -1,20 +0,0 @@ -name: checkpatterns - -on: [push, pull_request] - -jobs: - checkpatterns: - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Check for suspicious patterns - run: | - if [ -f "suspicious_patterns.sh" ]; then - bash suspicious_patterns.sh - else - echo "Warning: suspicious_patterns.sh not found, skipping check" - # Still exit with success for compatibility with dependent jobs - exit 0 - fi From 9782d02039d1afec61a2ffd242326e969e76ee36 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Mon, 9 Jun 2025 12:13:34 +0700 Subject: [PATCH 07/18] fix: restore state eagerly after provisional tx meta generation --- src/ripple/ledger/detail/ApplyStateTable.h | 3 +- src/ripple/ledger/impl/ApplyStateTable.cpp | 108 +++++++++++++++++---- src/ripple/ledger/impl/ApplyViewImpl.cpp | 8 +- 3 files changed, 97 insertions(+), 22 deletions(-) diff --git a/src/ripple/ledger/detail/ApplyStateTable.h b/src/ripple/ledger/detail/ApplyStateTable.h index c64f03469f..984db370b7 100644 --- a/src/ripple/ledger/detail/ApplyStateTable.h +++ b/src/ripple/ledger/detail/ApplyStateTable.h @@ -84,7 +84,8 @@ class ApplyStateTable std::optional const& deliver, std::vector const& hookExecution, std::vector const& hookEmission, - beast::Journal j); + beast::Journal j, + bool isProvisional = false); void apply( diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index e651da24ea..f482f6ef8b 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -118,7 +118,8 @@ ApplyStateTable::generateTxMeta( std::optional const& deliver, std::vector const& hookExecution, std::vector const& hookEmission, - beast::Journal j) + beast::Journal j, + bool isProvisional) { TxMeta meta(tx.getTransactionID(), to.seq()); if (deliver) @@ -254,6 +255,44 @@ ApplyStateTable::generateTxMeta( } } + // After provisional metadata generation, restore the original PreviousTxnID + // values to prevent contamination of the "before" state used for final + // metadata comparison. This ensures PreviousTxnID appears correctly in + // ModifiedNode metadata. + if (isProvisional && to.rules().enabled(fixProvisionalDoubleThreading)) + { + for (auto const& [key, state] : originalThreadingState_) + { + auto iter = items_.find(key); + if (iter != items_.end()) + { + auto sle = + iter->second.second; // This is already a shared_ptr + if (state.hasPrevTxnID) + { + sle->setFieldH256(sfPreviousTxnID, state.prevTxnID); + sle->setFieldU32(sfPreviousTxnLgrSeq, state.prevTxnLgrSeq); + // Restored to original PreviousTxnID + } + else + { + sle->makeFieldAbsent(sfPreviousTxnID); + sle->makeFieldAbsent(sfPreviousTxnLgrSeq); + // Restored to no PreviousTxnID + } + } + } + } + else if ( + !isProvisional && to.rules().enabled(fixProvisionalDoubleThreading)) + { + // For final metadata generation, clear the tracking state + // This prevents any confusion if the same ApplyStateTable is reused + // Clear the tracking state to prevent any confusion if this + // ApplyStateTable instance is reused + originalThreadingState_.clear(); + } + return {meta, newMod}; } @@ -287,6 +326,8 @@ ApplyStateTable::apply( // VFALCO For diagnostics do we want to show // metadata even when the base view is open? JLOG(j.trace()) << "metadata " << meta.getJson(JsonOptions::none); + + // Metadata has been generated } to.rawTxInsert(tx.getTransactionID(), sTx, sMeta); apply(to); @@ -549,12 +590,31 @@ ApplyStateTable::destroyXRP(XRPAmount const& fee) //------------------------------------------------------------------------------ // Insert this transaction to the SLE's threading list +// +// This method is called during metadata generation to update the +// PreviousTxnID/PreviousTxnLgrSeq fields on SLEs. However, it's called +// twice for each transaction: +// 1. During provisional metadata generation (for hooks to see) +// 2. During final metadata generation (for the actual ledger) +// +// The fixProvisionalDoubleThreading amendment fixes a bug where the +// provisional threading would contaminate the "original" state used +// for metadata comparison, causing PreviousTxnID to be missing from +// the final metadata. +// +// The fix works by: +// - Saving the original PreviousTxnID state before provisional threading +// - Restoring it after provisional metadata generation +// - Allowing final threading to proceed normally void ApplyStateTable::threadItem( TxMeta& meta, std::shared_ptr const& sle, const Rules& rules) { + key_type prevTxID; + LedgerIndex prevLgrID; + if (rules.enabled(fixProvisionalDoubleThreading)) { auto const key = sle->key(); @@ -571,30 +631,33 @@ ApplyStateTable::threadItem( state.prevTxnLgrSeq = sle->getFieldU32(sfPreviousTxnLgrSeq); } originalThreadingState_[key] = state; + + // Thread to get the values for metadata + if (!sle->thread( + meta.getTxID(), meta.getLgrSeq(), prevTxID, prevLgrID)) + return; + + // Debug logging + // Don't restore yet - we'll restore all threaded SLEs after + // provisional metadata generation completes } else { - // Subsequent call (final metadata) - restore to pristine state - // before threading - auto const& origState = iter->second; - if (origState.hasPrevTxnID) - { - sle->setFieldH256(sfPreviousTxnID, origState.prevTxnID); - sle->setFieldU32(sfPreviousTxnLgrSeq, origState.prevTxnLgrSeq); - } - else - { - sle->makeFieldAbsent(sfPreviousTxnID); - sle->makeFieldAbsent(sfPreviousTxnLgrSeq); - } + // Subsequent call (final metadata) - just thread normally + // No restore needed since we eagerly restored after provisional + if (!sle->thread( + meta.getTxID(), meta.getLgrSeq(), prevTxID, prevLgrID)) + return; + + // Final threading - this will persist in the ledger } } - - key_type prevTxID; - LedgerIndex prevLgrID; - - if (!sle->thread(meta.getTxID(), meta.getLgrSeq(), prevTxID, prevLgrID)) - return; + else + { + // Amendment not enabled - use original behavior + if (!sle->thread(meta.getTxID(), meta.getLgrSeq(), prevTxID, prevLgrID)) + return; + } if (!prevTxID.isZero()) { @@ -605,6 +668,11 @@ ApplyStateTable::threadItem( assert(node.getFieldIndex(sfPreviousTxnLgrSeq) == -1); node.setFieldH256(sfPreviousTxnID, prevTxID); node.setFieldU32(sfPreviousTxnLgrSeq, prevLgrID); + // Added PreviousTxnID to metadata + } + else + { + // PreviousTxnID already present in metadata } assert(node.getFieldH256(sfPreviousTxnID) == prevTxID); diff --git a/src/ripple/ledger/impl/ApplyViewImpl.cpp b/src/ripple/ledger/impl/ApplyViewImpl.cpp index e21c361c5d..fc68d87c87 100644 --- a/src/ripple/ledger/impl/ApplyViewImpl.cpp +++ b/src/ripple/ledger/impl/ApplyViewImpl.cpp @@ -41,7 +41,13 @@ ApplyViewImpl::generateProvisionalMeta( beast::Journal j) { auto [meta, _] = items_.generateTxMeta( - to, tx, deliver_, hookExecution_, hookEmission_, j); + to, + tx, + deliver_, + hookExecution_, + hookEmission_, + j, + true); // isProvisional = true return meta; } From fcd7506bea8ff5b56b5032c0045b26e00df2e999 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Thu, 12 Jun 2025 12:12:54 +0700 Subject: [PATCH 08/18] refactor: apply @tequdev's suggestion --- src/ripple/ledger/detail/ApplyStateTable.h | 3 +-- src/ripple/ledger/impl/ApplyStateTable.cpp | 25 ++++++---------------- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/src/ripple/ledger/detail/ApplyStateTable.h b/src/ripple/ledger/detail/ApplyStateTable.h index 984db370b7..1dd0375e64 100644 --- a/src/ripple/ledger/detail/ApplyStateTable.h +++ b/src/ripple/ledger/detail/ApplyStateTable.h @@ -169,8 +169,7 @@ class ApplyStateTable TxMeta& meta, AccountID const& to, Mods& mods, - beast::Journal j, - Rules const& rules); + beast::Journal j); void threadOwners( diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index f482f6ef8b..37bb4c99a1 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -735,8 +735,7 @@ ApplyStateTable::threadTx( TxMeta& meta, AccountID const& to, Mods& mods, - beast::Journal j, - Rules const& rules) + beast::Journal j) { auto const sle = getForMod(base, keylet::account(to).key, mods, j); if (!sle) @@ -747,7 +746,7 @@ ApplyStateTable::threadTx( JLOG(j.warn()) << "Threading to non-existent account: " << toBase58(to); return; } - threadItem(meta, sle, rules); + threadItem(meta, sle, base.rules()); } void @@ -766,26 +765,14 @@ ApplyStateTable::threadOwners( break; } case ltRIPPLE_STATE: { - threadTx( - base, - meta, - (*sle)[sfLowLimit].getIssuer(), - mods, - j, - base.rules()); - threadTx( - base, - meta, - (*sle)[sfHighLimit].getIssuer(), - mods, - j, - base.rules()); + threadTx(base, meta, (*sle)[sfLowLimit].getIssuer(), mods, j); + threadTx(base, meta, (*sle)[sfHighLimit].getIssuer(), mods, j); break; } default: { // If sfAccount is present, thread to that account if (auto const optSleAcct{(*sle)[~sfAccount]}) - threadTx(base, meta, *optSleAcct, mods, j, base.rules()); + threadTx(base, meta, *optSleAcct, mods, j); // Don't thread a check's sfDestination unless the amendment is // enabled @@ -795,7 +782,7 @@ ApplyStateTable::threadOwners( // If sfDestination is present, thread to that account if (auto const optSleDest{(*sle)[~sfDestination]}) - threadTx(base, meta, *optSleDest, mods, j, base.rules()); + threadTx(base, meta, *optSleDest, mods, j); } } } From e0450d4eb5b86b48fcb1dd1037d2d59b878f03e3 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Tue, 1 Jul 2025 11:06:35 +0700 Subject: [PATCH 09/18] chore: revert suspat changes in favor of #525 --- .githooks/pre-commit | 12 --- .githooks/setup.sh | 4 - .github/workflows/checkpatterns.yml | 20 +++++ suspicious_patterns.sh | 114 ++-------------------------- 4 files changed, 28 insertions(+), 122 deletions(-) delete mode 100755 .githooks/pre-commit delete mode 100644 .githooks/setup.sh create mode 100644 .github/workflows/checkpatterns.yml diff --git a/.githooks/pre-commit b/.githooks/pre-commit deleted file mode 100755 index 7cef4de6bf..0000000000 --- a/.githooks/pre-commit +++ /dev/null @@ -1,12 +0,0 @@ -#!/bin/bash - -# Pre-commit hook that runs the suspicious patterns check on staged files - -# Get the repository's root directory -repo_root=$(git rev-parse --show-toplevel) - -# Run the suspicious patterns script in pre-commit mode -"$repo_root/suspicious_patterns.sh" --pre-commit - -# Exit with the same code as the script -exit $? \ No newline at end of file diff --git a/.githooks/setup.sh b/.githooks/setup.sh deleted file mode 100644 index eb77aa11c5..0000000000 --- a/.githooks/setup.sh +++ /dev/null @@ -1,4 +0,0 @@ -#!/bin/bash - -echo "Configuring git to use .githooks directory..." -git config core.hooksPath .githooks diff --git a/.github/workflows/checkpatterns.yml b/.github/workflows/checkpatterns.yml new file mode 100644 index 0000000000..8b60a12e55 --- /dev/null +++ b/.github/workflows/checkpatterns.yml @@ -0,0 +1,20 @@ +name: checkpatterns + +on: [push, pull_request] + +jobs: + checkpatterns: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Check for suspicious patterns + run: | + if [ -f "suspicious_patterns.sh" ]; then + bash suspicious_patterns.sh + else + echo "Warning: suspicious_patterns.sh not found, skipping check" + # Still exit with success for compatibility with dependent jobs + exit 0 + fi diff --git a/suspicious_patterns.sh b/suspicious_patterns.sh index 421919d948..39287a6a19 100755 --- a/suspicious_patterns.sh +++ b/suspicious_patterns.sh @@ -1,123 +1,25 @@ #!/bin/bash -# Exit on error, undefined variables, and pipe failures -set -euo pipefail - -# Enable debug mode if DEBUG environment variable is set -[[ "${DEBUG:-}" == "1" ]] && set -x - -# This script prevents accidental commits of XRPL/Ripple cryptographic keys -# It searches for: -# - Secret seeds (s...) - can derive keypairs -# - Private keys (p...) - validator private keys! -# - Raw key material (02/03/ED + hex) - compressed public keys or Ed25519 keys -# -# Usage: -# suspicious_patterns.sh # Check last commit (for CI) -# suspicious_patterns.sh --pre-commit # Check staged files (for pre-commit hook) -# -# WARNING: If this catches a real key in CI, that key is already compromised! -# The key has been pushed to the git history and must be immediately decommissioned. -# -# To mark keys as safe, add comment: // not-suspicious -# Files excluded: See exclude_files array below -# Lines excluded: Matching exclude_pattern regex below - -# Pattern for lines to exclude from checking -exclude_pattern="public_key|not-suspicious" - -# Array of files to exclude from checking (paths relative to repo root) -exclude_files=( - "src/test/app/Import_test.cpp" - "cfg/validators-example.txt" -) - # Get the repository's root directory repo_root=$(git rev-parse --show-toplevel) -# Determine which files to check based on context: -# 1. Pre-commit hook: Check staged files before commit -# 2. GitHub PR: Check all files changed in the PR (HEAD is a synthetic merge commit) -# 3. Regular push/CI: Check files in the last real commit -if [[ "${1:-}" == "--pre-commit" ]]; then - # Pre-commit mode: Check what's about to be committed - files_changed=$(git diff --cached --name-only --relative) - mode="staged files" -else - # CI mode - need to handle two different scenarios - if [[ "${GITHUB_EVENT_NAME:-}" == "pull_request" ]]; then - # GitHub PR event: HEAD is a synthetic merge commit created by GitHub - # that merges PR branch into base. Must diff against base to get only PR files. - base_ref="${GITHUB_BASE_REF:-dev}" - - # Ensure we have the base branch (GitHub Actions shallow clone might not have it) - if ! git rev-parse --verify "origin/$base_ref" >/dev/null 2>&1; then - echo "Fetching base branch origin/$base_ref..." - git fetch --depth=1 origin "$base_ref" - fi - - # Since there's no merge base in shallow clones, we need to be creative - # Save current HEAD, switch to base, then diff the trees - current_head=$(git rev-parse HEAD) - echo "Comparing against base branch origin/$base_ref..." - - # Get the tree objects to compare (this works even without shared history) - base_tree=$(git rev-parse "origin/$base_ref^{tree}") - head_tree=$(git rev-parse "$current_head^{tree}") - - # Compare the two trees directly - files_changed=$(git diff --name-only "$base_tree" "$head_tree") - mode="PR changes" - else - # Regular push event: Check the actual commit that was pushed - # Using 'git show' works even with shallow clones (no HEAD~1 needed) - # See: https://github.com/Xahau/xahaud/actions/runs/15492442104/job/43620965462#step:3:11 - files_changed=$(git show --name-only --pretty=format:'' HEAD) - mode="last commit" - fi -fi - -echo "Checking $mode for suspicious patterns..." - -# Show additional info in CI or when verbose mode is enabled -if [[ -n "${CI:-}" ]] || [[ "${VERBOSE:-}" == "1" ]]; then - if [[ "${1:-}" != "--pre-commit" ]]; then - echo "Commit: $(git rev-parse HEAD)" - fi - echo "Files to check:" - if [[ -n "$files_changed" ]]; then - echo "$files_changed" | nl - else - echo " (none)" - fi -fi +# Get a list of files changed in the last commit with their relative paths +files_changed=$(git diff --name-only --relative HEAD~1 HEAD) # Loop through each file and search for the patterns for file in $files_changed; do - # Check if file should be excluded (exact path match) - for excluded in "${exclude_files[@]}"; do - if [[ "$file" == "$excluded" ]]; then - continue 2 # Continue outer loop - fi - done + # Skip if the file is Import_test.cpp (exact filename match regardless of path) + if [[ "$(basename "$file")" == "Import_test.cpp" ]]; then + continue + fi # Construct the absolute path absolute_path="$repo_root/$file" # Check if the file exists (it might have been deleted) if [ -f "$absolute_path" ]; then - # Get file content based on mode - if [[ "${1:-}" == "--pre-commit" ]]; then - # For staged files, use git show with the staging area - file_content=$(git show ":$file" 2>/dev/null) - else - # For committed files, read from disk - file_content=$(cat "$absolute_path") - fi - - # Search the file content for the given patterns, but exclude lines matching the exclusion pattern - # Use || true to prevent grep from failing the script when no matches are found - grep_output=$(echo "$file_content" | grep -n -E '(([^rpshnaf39wBUDNEGHJKLM4PQRST7VWXYZ2bcdeCg65jkm8oFqi1tuvAxyz]|^)(s|p)[rpshnaf39wBUDNEGHJKLM4PQRST7VWXYZ2bcdeCg65jkm8oFqi1tuvAxyz]{25,60}([^(]|$)))|([^A-Fa-f0-9](02|03|ED)[A-Fa-f0-9]{64})' | grep -vE "$exclude_pattern" || true) + # Search the file for the given patterns, but exclude lines containing 'public_key' + grep_output=$(grep -n -E '(([^rpshnaf39wBUDNEGHJKLM4PQRST7VWXYZ2bcdeCg65jkm8oFqi1tuvAxyz]|^)(s|p)[rpshnaf39wBUDNEGHJKLM4PQRST7VWXYZ2bcdeCg65jkm8oFqi1tuvAxyz]{25,60}([^(]|$)))|([^A-Fa-f0-9](02|03|ED)[A-Fa-f0-9]{64})' "$absolute_path" | grep -v "public_key") # Check if grep found any matches if [ ! -z "$grep_output" ]; then From 5af34c163d79bfdc70acf3dd174d07f186318934 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Tue, 1 Jul 2025 11:37:51 +0700 Subject: [PATCH 10/18] refactor(ledger): Refine provisional threading logic and comments This commit makes several small improvements to the implementation of the `fixProvisionalDoubleThreading` amendment. - The explicit call to `originalThreadingState_.clear()` is removed as it was superfluous. The `ApplyStateTable` is single-use per transaction, so its member map is destroyed along with it. - A comment is added to the `originalThreadingState_` declaration to clarify this lifecycle assumption. - The comment in `PreviousTxn_test.cpp` is updated to explicitly name the `fixProvisionalDoubleThreading` amendment for better readability. - Minor test cleanup, including adjusting the log level and ensuring a final newline. --- src/ripple/ledger/detail/ApplyStateTable.h | 4 +++- src/ripple/ledger/impl/ApplyStateTable.cpp | 12 +----------- src/test/app/PreviousTxn_test.cpp | 15 ++++++++------- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/src/ripple/ledger/detail/ApplyStateTable.h b/src/ripple/ledger/detail/ApplyStateTable.h index 1dd0375e64..793bd68d17 100644 --- a/src/ripple/ledger/detail/ApplyStateTable.h +++ b/src/ripple/ledger/detail/ApplyStateTable.h @@ -55,7 +55,9 @@ class ApplyStateTable XRPAmount dropsDestroyed_{0}; // Track original PreviousTxnID/LgrSeq values to restore after provisional - // metadata + // metadata. This map is populated during provisional metadata generation + // and consumed during final metadata generation. It is not cleared as + // the ApplyStateTable instance is single-use per transaction. struct ThreadingState { uint256 prevTxnID; diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index 37bb4c99a1..93ad6a9245 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -266,8 +266,7 @@ ApplyStateTable::generateTxMeta( auto iter = items_.find(key); if (iter != items_.end()) { - auto sle = - iter->second.second; // This is already a shared_ptr + auto sle = iter->second.second; if (state.hasPrevTxnID) { sle->setFieldH256(sfPreviousTxnID, state.prevTxnID); @@ -283,15 +282,6 @@ ApplyStateTable::generateTxMeta( } } } - else if ( - !isProvisional && to.rules().enabled(fixProvisionalDoubleThreading)) - { - // For final metadata generation, clear the tracking state - // This prevents any confusion if the same ApplyStateTable is reused - // Clear the tracking state to prevent any confusion if this - // ApplyStateTable instance is reused - originalThreadingState_.clear(); - } return {meta, newMod}; } diff --git a/src/test/app/PreviousTxn_test.cpp b/src/test/app/PreviousTxn_test.cpp index 3c82561f25..46dcf19a45 100644 --- a/src/test/app/PreviousTxn_test.cpp +++ b/src/test/app/PreviousTxn_test.cpp @@ -33,7 +33,7 @@ class PreviousTxnID_test : public beast::unit_test::suite using namespace test::jtx; Env env{ - *this, envconfig(), features, nullptr, beast::severities::kInfo}; + *this, envconfig(), features, nullptr, beast::severities::kNone}; auto j = env.app().logs().journal("PreviousTxnID_test"); auto const alice = Account{"alice"}; @@ -173,12 +173,13 @@ class PreviousTxnID_test : public beast::unit_test::suite // This is different from PreviousFields, which shows what // field values changed. // - // The bug was that ApplyStateTable::threadItem() was + // The bug fixed by the `fixProvisionalDoubleThreading` + // amendment was that ApplyStateTable::threadItem() was // modifying the original SLE during provisional metadata - // generation, contaminating the "before" state. When final - // metadata was generated, the comparison didn't see - // PreviousTxnID as a change because both states had the new - // value. + // generation. This contaminated the "before" state used for + // comparison, so when final metadata was generated, the + // comparison didn't see PreviousTxnID as a change because both + // states had the new value. if (node.isFieldPresent(sfPreviousTxnID)) { foundPreviousTxnIDInModified = true; @@ -214,4 +215,4 @@ class PreviousTxnID_test : public beast::unit_test::suite BEAST_DEFINE_TESTSUITE(PreviousTxnID, app, ripple); } // namespace test -} // namespace ripple \ No newline at end of file +} // namespace ripple From d067c2e22462be5bba8924d23cbe3e4b3a5db84d Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Tue, 1 Jul 2025 12:01:26 +0700 Subject: [PATCH 11/18] docs(ledger): clarify batch-restore logic in threadItem comment This commit improves the comment for the `fixProvisionalDoubleThreading` logic within the `threadItem` function. The new comment clarifies *why* the original SLE state is restored in a single batch after the entire provisional metadata pass is complete, rather than immediately within each `threadItem` call. It explicitly notes that `threadItem` can be called multiple times on the same SLE within a single pass, making an immediate restore incorrect. This provides better context for future developers. --- src/ripple/ledger/impl/ApplyStateTable.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index 93ad6a9245..4198b2be44 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -593,9 +593,15 @@ ApplyStateTable::destroyXRP(XRPAmount const& fee) // the final metadata. // // The fix works by: -// - Saving the original PreviousTxnID state before provisional threading -// - Restoring it after provisional metadata generation -// - Allowing final threading to proceed normally +// - Saving the original PreviousTxnID state for an SLE the first time it's +// threaded during the provisional pass. +// - Restoring the original state for all affected SLEs in a single batch +// after the entire provisional metadata generation is complete. +// +// This batch-restore is critical because threadItem() can be called on the +// same SLE multiple times within one metadata pass. Restoring immediately +// would be incorrect. This approach ensures the final metadata comparison +// starts from the correct, uncontaminated "before" state. void ApplyStateTable::threadItem( TxMeta& meta, From 4cc602af6e33bbe1dc06739ca85910b644a4a43a Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Tue, 1 Jul 2025 12:10:17 +0700 Subject: [PATCH 12/18] refactor(ledger): simplify threading logic in threadItem This commit refactors the `threadItem` function to simplify its control flow. The previous implementation had three separate branches that all contained a call to `sle->thread()`. This change consolidates them into a single, unconditional call. The conditional logic for saving the original SLE state (when the `fixProvisionalDoubleThreading` amendment is enabled) is now handled in a separate block before the common threading logic, removing redundancy and making the function's intent clearer. --- src/ripple/ledger/impl/ApplyStateTable.cpp | 37 ++++------------------ 1 file changed, 6 insertions(+), 31 deletions(-) diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index 4198b2be44..792f93c475 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -608,15 +608,10 @@ ApplyStateTable::threadItem( std::shared_ptr const& sle, const Rules& rules) { - key_type prevTxID; - LedgerIndex prevLgrID; - if (rules.enabled(fixProvisionalDoubleThreading)) { auto const key = sle->key(); - auto iter = originalThreadingState_.find(key); - - if (iter == originalThreadingState_.end()) + if (originalThreadingState_.find(key) == originalThreadingState_.end()) { // First time (provisional metadata) - save the original state ThreadingState state; @@ -627,33 +622,13 @@ ApplyStateTable::threadItem( state.prevTxnLgrSeq = sle->getFieldU32(sfPreviousTxnLgrSeq); } originalThreadingState_[key] = state; - - // Thread to get the values for metadata - if (!sle->thread( - meta.getTxID(), meta.getLgrSeq(), prevTxID, prevLgrID)) - return; - - // Debug logging - // Don't restore yet - we'll restore all threaded SLEs after - // provisional metadata generation completes - } - else - { - // Subsequent call (final metadata) - just thread normally - // No restore needed since we eagerly restored after provisional - if (!sle->thread( - meta.getTxID(), meta.getLgrSeq(), prevTxID, prevLgrID)) - return; - - // Final threading - this will persist in the ledger } } - else - { - // Amendment not enabled - use original behavior - if (!sle->thread(meta.getTxID(), meta.getLgrSeq(), prevTxID, prevLgrID)) - return; - } + + key_type prevTxID; + LedgerIndex prevLgrID; + if (!sle->thread(meta.getTxID(), meta.getLgrSeq(), prevTxID, prevLgrID)) + return; if (!prevTxID.isZero()) { From 7496a7b7ea698d551221534b055a1aa186ed2116 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Tue, 1 Jul 2025 12:22:18 +0700 Subject: [PATCH 13/18] chore: add line spacing --- commit_message.txt | 12 ++++++++++++ src/ripple/ledger/impl/ApplyStateTable.cpp | 1 + 2 files changed, 13 insertions(+) create mode 100644 commit_message.txt diff --git a/commit_message.txt b/commit_message.txt new file mode 100644 index 0000000000..8c15ff8e71 --- /dev/null +++ b/commit_message.txt @@ -0,0 +1,12 @@ +refactor(ledger): simplify threading logic in threadItem + +This commit refactors the `threadItem` function to simplify its +control flow. + +The previous implementation had three separate branches that all +contained a call to `sle->thread()`. This change consolidates them +into a single, unconditional call. The conditional logic for saving +the original SLE state (when the `fixProvisionalDoubleThreading` +amendment is enabled) is now handled in a separate block before the +common threading logic, removing redundancy and making the function's +intent clearer. \ No newline at end of file diff --git a/src/ripple/ledger/impl/ApplyStateTable.cpp b/src/ripple/ledger/impl/ApplyStateTable.cpp index 792f93c475..6a742d95de 100644 --- a/src/ripple/ledger/impl/ApplyStateTable.cpp +++ b/src/ripple/ledger/impl/ApplyStateTable.cpp @@ -627,6 +627,7 @@ ApplyStateTable::threadItem( key_type prevTxID; LedgerIndex prevLgrID; + if (!sle->thread(meta.getTxID(), meta.getLgrSeq(), prevTxID, prevLgrID)) return; From 8814a9434dcbc15458adea540be1a3085ff53b31 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Tue, 1 Jul 2025 12:25:33 +0700 Subject: [PATCH 14/18] chore: remove geminis commit quoting workaround --- commit_message.txt | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 commit_message.txt diff --git a/commit_message.txt b/commit_message.txt deleted file mode 100644 index 8c15ff8e71..0000000000 --- a/commit_message.txt +++ /dev/null @@ -1,12 +0,0 @@ -refactor(ledger): simplify threading logic in threadItem - -This commit refactors the `threadItem` function to simplify its -control flow. - -The previous implementation had three separate branches that all -contained a call to `sle->thread()`. This change consolidates them -into a single, unconditional call. The conditional logic for saving -the original SLE state (when the `fixProvisionalDoubleThreading` -amendment is enabled) is now handled in a separate block before the -common threading logic, removing redundancy and making the function's -intent clearer. \ No newline at end of file From 6299f20b16554fea5f660cb8715af2d2fcd191a3 Mon Sep 17 00:00:00 2001 From: Niq Dudfield Date: Mon, 7 Jul 2025 12:23:19 +0700 Subject: [PATCH 15/18] feat: set default voting behaviour to yes for a fix Co-authored-by: tequ --- src/ripple/protocol/impl/Feature.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index b752c80930..24ab474c17 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -475,7 +475,7 @@ REGISTER_FIX (fixXahauV3, Supported::yes, VoteBehavior::De REGISTER_FIX (fix20250131, Supported::yes, VoteBehavior::DefaultYes); REGISTER_FEATURE(HookCanEmit, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixRewardClaimFlags, Supported::yes, VoteBehavior::DefaultYes); -REGISTER_FIX (fixProvisionalDoubleThreading, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixProvisionalDoubleThreading, Supported::yes, VoteBehavior::DefaultYes); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. From 0b23d2a5819b680973761a88a1f391d470d2fc66 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Tue, 8 Jul 2025 10:02:01 +0700 Subject: [PATCH 16/18] test(app): add comprehensive tests for PreviousTxnID metadata fix - test both with and without fixProvisionalDoubleThreading amendment - verify PreviousTxnID appears in metadata only when fix is enabled - confirm SLE state is correct regardless of the bug - check that PreviousTxnID never appears in PreviousFields - validate account threading works properly in both cases --- src/test/app/PreviousTxn_test.cpp | 108 ++++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 4 deletions(-) diff --git a/src/test/app/PreviousTxn_test.cpp b/src/test/app/PreviousTxn_test.cpp index 46dcf19a45..80d17da6e0 100644 --- a/src/test/app/PreviousTxn_test.cpp +++ b/src/test/app/PreviousTxn_test.cpp @@ -29,7 +29,6 @@ class PreviousTxnID_test : public beast::unit_test::suite void testPreviousTxnID(FeatureBitset features) { - testcase("Check PreviousTxnID in trustline metadata"); using namespace test::jtx; Env env{ @@ -47,8 +46,9 @@ class PreviousTxnID_test : public beast::unit_test::suite env(trust(alice, USD(1000))); env.close(); - // Get the transaction metadata + // Get the transaction metadata and ID auto const meta1 = env.meta(); + auto const trustCreateTxID = env.tx()->getTransactionID(); BEAST_EXPECT(meta1); // Check if ModifiedNode has PreviousTxnID at root level @@ -151,6 +151,7 @@ class PreviousTxnID_test : public beast::unit_test::suite // Check ModifiedNode for PreviousTxnID auto const& affectedNodes2 = meta2->getFieldArray(sfAffectedNodes); bool foundPreviousTxnIDInModified = false; + bool foundPreviousTxnIDInPreviousFields = false; for (auto const& node : affectedNodes2) { @@ -180,6 +181,9 @@ class PreviousTxnID_test : public beast::unit_test::suite // comparison, so when final metadata was generated, the // comparison didn't see PreviousTxnID as a change because both // states had the new value. + bool expectPreviousTxnID = + features[fixProvisionalDoubleThreading]; + if (node.isFieldPresent(sfPreviousTxnID)) { foundPreviousTxnIDInModified = true; @@ -192,15 +196,106 @@ class PreviousTxnID_test : public beast::unit_test::suite JLOG(j.info()) << "Found PreviousTxnID: " << prevTxnID << " at ledger: " << prevLgrSeq << std::endl; + + // When the fix is enabled, we should see the trustline + // creation transaction ID as the previous transaction + if (expectPreviousTxnID) + { + BEAST_EXPECT(prevTxnID == trustCreateTxID); + } } else { - BEAST_EXPECT(false); + // Without the fix, we expect PreviousTxnID to be missing + // due to the provisional metadata contamination bug + JLOG(j.info()) << "PreviousTxnID missing in metadata"; + } + + // Check if PreviousTxnID appears in PreviousFields + // (it shouldn't - PreviousTxnID is a root-level field) + if (node.isFieldPresent(sfPreviousFields)) + { + auto prevFields = dynamic_cast( + node.peekAtPField(sfPreviousFields)); + if (prevFields && + prevFields->isFieldPresent(sfPreviousTxnID)) + { + foundPreviousTxnIDInPreviousFields = true; + JLOG(j.warn()) << "Found PreviousTxnID in " + "PreviousFields (unexpected)"; + } } } } - BEAST_EXPECT(foundPreviousTxnIDInModified); + // PreviousTxnID should never appear in PreviousFields + BEAST_EXPECT(!foundPreviousTxnIDInPreviousFields); + + // With the fix enabled, we expect to find PreviousTxnID + // Without the fix, we expect it to be missing (the bug) + if (features[fixProvisionalDoubleThreading]) + { + BEAST_EXPECT(foundPreviousTxnIDInModified); + } + else + { + BEAST_EXPECT(!foundPreviousTxnIDInModified); + } + + // Additional check: Verify the SLE state after the payment + auto const sleTrustlineAfter = env.le(trustlineKey); + BEAST_EXPECT(sleTrustlineAfter); + + if (sleTrustlineAfter) + { + // The SLE should always have PreviousTxnID set after modification + BEAST_EXPECT(sleTrustlineAfter->isFieldPresent(sfPreviousTxnID)); + BEAST_EXPECT( + sleTrustlineAfter->isFieldPresent(sfPreviousTxnLgrSeq)); + + auto const currentPrevTxnID = + sleTrustlineAfter->getFieldH256(sfPreviousTxnID); + auto const currentPrevTxnSeq = + sleTrustlineAfter->getFieldU32(sfPreviousTxnLgrSeq); + + // The PreviousTxnID should now point to the payment transaction + auto const paymentTxID = env.tx()->getTransactionID(); + BEAST_EXPECT(currentPrevTxnID == paymentTxID); + BEAST_EXPECT(currentPrevTxnSeq == env.closed()->seq()); + + // When the bug is present (feature disabled), the metadata won't + // show the change, but the SLE will still be updated correctly + if (!features[fixProvisionalDoubleThreading]) + { + JLOG(j.info()) + << "Bug confirmed: SLE has correct PreviousTxnID (" + << currentPrevTxnID + << ") but metadata doesn't show the change"; + } + } + + // Check account objects were threaded correctly + auto const aliceAccount = env.le(keylet::account(alice)); + auto const bobAccount = env.le(keylet::account(bob)); + + BEAST_EXPECT(aliceAccount); + BEAST_EXPECT(bobAccount); + + if (aliceAccount && bobAccount) + { + // Both accounts should have been threaded by the payment + BEAST_EXPECT(aliceAccount->isFieldPresent(sfPreviousTxnID)); + BEAST_EXPECT(bobAccount->isFieldPresent(sfPreviousTxnID)); + + auto const alicePrevTxnID = + aliceAccount->getFieldH256(sfPreviousTxnID); + auto const bobPrevTxnID = bobAccount->getFieldH256(sfPreviousTxnID); + auto const paymentTxID = env.tx()->getTransactionID(); + + // Both should point to the payment transaction + BEAST_EXPECT(alicePrevTxnID == paymentTxID); + BEAST_EXPECT(bobPrevTxnID == paymentTxID); + } } void @@ -208,7 +303,12 @@ class PreviousTxnID_test : public beast::unit_test::suite { using namespace test::jtx; auto const sa = supported_amendments(); + + testcase("With fixProvisionalDoubleThreading enabled"); testPreviousTxnID(sa); + + testcase("Without fixProvisionalDoubleThreading (bug present)"); + testPreviousTxnID(sa - fixProvisionalDoubleThreading); } }; From d62dfd35b8ee8786c1334b52fa9266d49e8253c1 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Tue, 8 Jul 2025 10:07:15 +0700 Subject: [PATCH 17/18] chore: clang format --- src/test/app/PreviousTxn_test.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/app/PreviousTxn_test.cpp b/src/test/app/PreviousTxn_test.cpp index 80d17da6e0..829b3d84dd 100644 --- a/src/test/app/PreviousTxn_test.cpp +++ b/src/test/app/PreviousTxn_test.cpp @@ -29,7 +29,6 @@ class PreviousTxnID_test : public beast::unit_test::suite void testPreviousTxnID(FeatureBitset features) { - using namespace test::jtx; Env env{ *this, envconfig(), features, nullptr, beast::severities::kNone}; From 3ec83cd6e96b8583c301a10c08b67e495958eb5b Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Tue, 8 Jul 2025 14:39:04 +0700 Subject: [PATCH 18/18] docs: add inline comments about test fragility --- src/test/app/TxQ_test.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 71d25bfe6a..7bc0ca71ec 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -5050,6 +5050,8 @@ class TxQ1_test : public beast::unit_test::suite testFailInPreclaim(all); testQueuedTxFails(all); testMultiTxnPerAccount(all); + // fragile: hardcoded ordering by txID XOR parentHash + // parentHash < txTree Hash < txMeta < PreviousTxnID testTieBreaking(all - fixProvisionalDoubleThreading); testAcctTxnID(all); testMaximum(all); @@ -5068,6 +5070,8 @@ class TxQ1_test : public beast::unit_test::suite testAcctInQueueButEmpty(all); testRPC(all); testExpirationReplacement(all); + // fragile: hardcoded ordering by txID XOR parentHash + // parentHash < txTree Hash < txMeta < PreviousTxnID testFullQueueGapFill(all - fixProvisionalDoubleThreading); testSignAndSubmitSequence(all); testAccountInfo(all);