From 69084a6ff5daeb753428dbeedba1104400a14802 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Fri, 20 Feb 2026 22:04:43 +0100 Subject: [PATCH] feature BatchV1_1 --- include/xrpl/protocol/detail/features.macro | 4 +- .../xrpl/protocol/detail/transactions.macro | 2 +- src/libxrpl/tx/Transactor.cpp | 25 +++--- src/libxrpl/tx/apply.cpp | 19 +---- .../tx/transactors/Lending/LoanSet.cpp | 3 +- src/test/app/Batch_test.cpp | 78 ++++++++++++++----- src/test/rpc/Feature_test.cpp | 5 +- src/xrpld/app/misc/NetworkOPs.cpp | 4 +- src/xrpld/overlay/detail/PeerImp.cpp | 4 +- 9 files changed, 80 insertions(+), 64 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index a40d524c70..bafe93f4d9 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,10 +15,9 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. - +XRPL_FEATURE(BatchV1_1, Supported::no, VoteBehavior::DefaultNo) XRPL_FIX (PermissionedDomainInvariant, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo) -XRPL_FIX (BatchInnerSigs, Supported::no, VoteBehavior::DefaultNo) XRPL_FEATURE(LendingProtocol, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(PermissionDelegationV1_1, Supported::no, VoteBehavior::DefaultNo) XRPL_FIX (DirectoryLimit, Supported::yes, VoteBehavior::DefaultNo) @@ -32,7 +31,6 @@ XRPL_FEATURE(TokenEscrow, Supported::yes, VoteBehavior::DefaultNo XRPL_FIX (EnforceNFTokenTrustlineV2, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (AMMv1_3, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(PermissionedDEX, Supported::yes, VoteBehavior::DefaultNo) -XRPL_FEATURE(Batch, Supported::no, VoteBehavior::DefaultNo) XRPL_FEATURE(SingleAssetVault, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (PayChanCancelAfter, Supported::yes, VoteBehavior::DefaultNo) // Check flags in Credential transactions diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index b696a1d1c2..973a51d912 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -918,7 +918,7 @@ TRANSACTION(ttVAULT_CLAWBACK, 70, VaultClawback, #endif TRANSACTION(ttBATCH, 71, Batch, Delegation::notDelegable, - featureBatch, + featureBatchV1_1, noPriv, ({ {sfRawTransactions, soeREQUIRED}, diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 9da1028b3a..4e27a171d1 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -175,12 +175,11 @@ Transactor::preflight1(PreflightContext const& ctx, std::uint32_t flagMask) if (ctx.tx.getSeqProxy().isTicket() && ctx.tx.isFieldPresent(sfAccountTxnID)) return temINVALID; - if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatch)) + if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatchV1_1)) return temINVALID_FLAG; XRPL_ASSERT( - ctx.tx.isFlag(tfInnerBatchTxn) == ctx.parentBatchId.has_value() || - !ctx.rules.enabled(featureBatch), + ctx.tx.isFlag(tfInnerBatchTxn) == ctx.parentBatchId.has_value() || !ctx.rules.enabled(featureBatchV1_1), "Inner batch transaction must have a parent batch ID."); return tesSUCCESS; @@ -196,13 +195,13 @@ Transactor::preflight2(PreflightContext const& ctx) return *ret; // It should be impossible for the InnerBatchTxn flag to be set without - // featureBatch being enabled + // featureBatchV1_1 being enabled XRPL_ASSERT_PARTS( - !ctx.tx.isFlag(tfInnerBatchTxn) || ctx.rules.enabled(featureBatch), + !ctx.tx.isFlag(tfInnerBatchTxn) || ctx.rules.enabled(featureBatchV1_1), "xrpl::Transactor::preflight2", "InnerBatch flag only set if feature enabled"); // Skip signature check on batch inner transactions - if (ctx.tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatch)) + if (ctx.tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatchV1_1)) return tesSUCCESS; // Do not add any checks after this point that are relevant for // batch inner transactions. They will be skipped. @@ -647,7 +646,7 @@ Transactor::checkSign( auto const pkSigner = sigObject.getFieldVL(sfSigningPubKey); // Ignore signature check on batch inner transactions - if (parentBatchId && view.rules().enabled(featureBatch)) + if (parentBatchId && view.rules().enabled(featureBatchV1_1)) { // Defensive Check: These values are also checked in Batch::preflight if (sigObject.isFieldPresent(sfTxnSignature) || !pkSigner.empty() || @@ -731,13 +730,15 @@ Transactor::checkBatchSign(PreclaimContext const& ctx) { if (idAccount != idSigner) return tefBAD_AUTH; - - return tesSUCCESS; } + else + { + if (isPseudoAccount(sleAccount)) + return tefBAD_AUTH; - if (ret = checkSingleSign(ctx.view, idSigner, idAccount, sleAccount, ctx.j); - !isTesSuccess(ret)) - return ret; + if (ret = checkSingleSign(ctx.view, idSigner, idAccount, sleAccount, ctx.j); !isTesSuccess(ret)) + return ret; + } } } return ret; diff --git a/src/libxrpl/tx/apply.cpp b/src/libxrpl/tx/apply.cpp index def82aca18..9ff1d4b473 100644 --- a/src/libxrpl/tx/apply.cpp +++ b/src/libxrpl/tx/apply.cpp @@ -24,29 +24,12 @@ checkValidity(HashRouter& router, STTx const& tx, Rules const& rules) auto const flags = router.getFlags(id); // Ignore signature check on batch inner transactions - if (tx.isFlag(tfInnerBatchTxn) && rules.enabled(featureBatch)) + if (tx.isFlag(tfInnerBatchTxn) && rules.enabled(featureBatchV1_1)) { // Defensive Check: These values are also checked in Batch::preflight if (tx.isFieldPresent(sfTxnSignature) || !tx.getSigningPubKey().empty() || tx.isFieldPresent(sfSigners)) return {Validity::SigBad, "Malformed: Invalid inner batch transaction."}; - - // This block should probably have never been included in the - // original `Batch` implementation. An inner transaction never - // has a valid signature. - bool const neverValid = rules.enabled(fixBatchInnerSigs); - if (!neverValid) - { - std::string reason; - if (!passesLocalChecks(tx, reason)) - { - router.setFlags(id, SF_LOCALBAD); - return {Validity::SigGoodOnly, reason}; - } - - router.setFlags(id, SF_SIGGOOD); - return {Validity::Valid, ""}; - } } if (any(flags & SF_SIGBAD)) diff --git a/src/libxrpl/tx/transactors/Lending/LoanSet.cpp b/src/libxrpl/tx/transactors/Lending/LoanSet.cpp index 82a64bc89b..4cf13744a0 100644 --- a/src/libxrpl/tx/transactors/Lending/LoanSet.cpp +++ b/src/libxrpl/tx/transactors/Lending/LoanSet.cpp @@ -26,8 +26,7 @@ LoanSet::preflight(PreflightContext const& ctx) auto const& tx = ctx.tx; // Special case for Batch inner transactions - if (tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatch) && - !tx.isFieldPresent(sfCounterparty)) + if (tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatchV1_1) && !tx.isFieldPresent(sfCounterparty)) { auto const parentBatchId = ctx.parentBatchId.value_or(uint256{0}); JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index bd1d701381..21d34b849d 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -141,14 +141,11 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - bool const withInnerSigFix = features[fixBatchInnerSigs]; - for (bool const withBatch : {true, false}) { - testcase << "enabled: Batch " << (withBatch ? "enabled" : "disabled") - << ", Inner Sig Fix: " << (withInnerSigFix ? "enabled" : "disabled"); + testcase << "enabled: Batch " << (withBatch ? "enabled" : "disabled"); - auto const amend = withBatch ? features : features - featureBatch; + auto const amend = withBatch ? features : features - featureBatchV1_1; test::jtx::Env env{*this, amend}; @@ -2191,22 +2188,16 @@ class Batch_test : public beast::unit_test::suite void doTestInnerSubmitRPC(FeatureBitset features, bool withBatch) { - bool const withInnerSigFix = features[fixBatchInnerSigs]; + std::string const testName = + std::string("inner submit rpc: batch ") + (withBatch ? "enabled" : "disabled") + ": "; - std::string const testName = [&]() { - std::stringstream ss; - ss << "inner submit rpc: batch " << (withBatch ? "enabled" : "disabled") - << ", inner sig fix: " << (withInnerSigFix ? "enabled" : "disabled") << ": "; - return ss.str(); - }(); - - auto const amend = withBatch ? features : features - featureBatch; + auto const amend = withBatch ? features : features - featureBatchV1_1; using namespace test::jtx; using namespace std::literals; test::jtx::Env env{*this, amend}; - if (!BEAST_EXPECT(amend[featureBatch] == withBatch)) + if (!BEAST_EXPECT(amend[featureBatchV1_1] == withBatch)) return; auto const alice = Account("alice"); @@ -2328,8 +2319,7 @@ class Batch_test : public beast::unit_test::suite s.slice(), __LINE__, "fails local checks: Empty SigningPubKey.", - "fails local checks: Empty SigningPubKey.", - withBatch && !withInnerSigFix); + "fails local checks: Empty SigningPubKey."); } // Invalid RPC Submission: tfInnerBatchTxn pseudo-transaction @@ -2340,7 +2330,7 @@ class Batch_test : public beast::unit_test::suite { STTx amendTx(ttAMENDMENT, [seq = env.closed()->header().seq + 1](auto& obj) { obj.setAccountID(sfAccount, AccountID()); - obj.setFieldH256(sfAmendment, fixBatchInnerSigs); + obj.setFieldH256(sfAmendment, featureBatchV1_1); obj.setFieldU32(sfLedgerSequence, seq); obj.setFieldU32(sfFlags, tfInnerBatchTxn); }); @@ -2352,8 +2342,7 @@ class Batch_test : public beast::unit_test::suite "Pseudo-transaction", s.slice(), __LINE__, - withInnerSigFix ? "fails local checks: Empty SigningPubKey." - : "fails local checks: Cannot submit pseudo transactions.", + "fails local checks: Empty SigningPubKey.", "fails local checks: Empty SigningPubKey."); } } @@ -2414,6 +2403,53 @@ class Batch_test : public beast::unit_test::suite BEAST_EXPECT(env.balance(bob) == XRP(1000)); } + void + testCheckAllSignatures(FeatureBitset features) + { + testcase("check all signatures"); + + using namespace test::jtx; + using namespace std::literals; + + // Verifies that checkBatchSign validates all signers even when an + // unfunded account (signed with its master key) appears first in the + // sorted signer list. A funded account with an invalid signature must + // still be rejected with tefBAD_AUTH. + + test::jtx::Env env{*this, features}; + + auto const alice = Account("alice"); + // "aaa" sorts before other accounts alphabetically, ensuring the + // unfunded account is checked first in the sorted signer list + auto const unfunded = Account("aaa"); + auto const carol = Account("carol"); + env.fund(XRP(10000), alice, carol); + env.close(); + + // Verify sort order: unfunded.id() < carol.id() + BEAST_EXPECT(unfunded.id() < carol.id()); + + auto const seq = env.seq(alice); + auto const ledSeq = env.current()->seq(); + auto const batchFee = batch::calcBatchFee(env, 2, 3); + + // The batch includes: + // 1. alice pays unfunded (to create unfunded's account) + // 2. unfunded does a noop (signed by unfunded's master key - valid) + // 3. carol pays alice (signed by alice's key - INVALID since alice is + // not carol's regular key) + // + // checkBatchSign must validate all signers regardless of order. + // This must fail with tefBAD_AUTH. + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::inner(pay(alice, unfunded, XRP(100)), seq + 1), + batch::inner(noop(unfunded), ledSeq), + batch::inner(pay(carol, alice, XRP(1000)), env.seq(carol)), + batch::sig(unfunded, Reg{carol, alice}), + ter(tefBAD_AUTH)); + env.close(); + } + void testAccountSet(FeatureBitset features) { @@ -4331,6 +4367,7 @@ class Batch_test : public beast::unit_test::suite testIndependent(features); testInnerSubmitRPC(features); testAccountActivation(features); + testCheckAllSignatures(features); testAccountSet(features); testAccountDelete(features); testLoan(features); @@ -4356,7 +4393,6 @@ public: { using namespace test::jtx; auto const sa = testable_amendments(); - testWithFeats(sa - fixBatchInnerSigs); testWithFeats(sa); } }; diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index a802aed482..1773b2e8ff 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -114,9 +114,8 @@ class Feature_test : public beast::unit_test::suite // Test a random sampling of the variables. If any of these get retired // or removed, swap out for any other feature. - BEAST_EXPECT( - featureToName(fixRemoveNFTokenAutoTrustLine) == "fixRemoveNFTokenAutoTrustLine"); - BEAST_EXPECT(featureToName(featureBatch) == "Batch"); + BEAST_EXPECT(featureToName(fixRemoveNFTokenAutoTrustLine) == "fixRemoveNFTokenAutoTrustLine"); + BEAST_EXPECT(featureToName(featureBatchV1_1) == "BatchV1_1"); BEAST_EXPECT(featureToName(featureDID) == "DID"); BEAST_EXPECT(featureToName(fixIncludeKeyletFields) == "fixIncludeKeyletFields"); BEAST_EXPECT(featureToName(featureTokenEscrow) == "TokenEscrow"); diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 8178611c61..277f76a2f3 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1119,7 +1119,7 @@ NetworkOPsImp::submitTransaction(std::shared_ptr const& iTrans) } // Enforce Network bar for batch txn - if (iTrans->isFlag(tfInnerBatchTxn) && m_ledgerMaster.getValidatedRules().enabled(featureBatch)) + if (iTrans->isFlag(tfInnerBatchTxn) && m_ledgerMaster.getValidatedRules().enabled(featureBatchV1_1)) { JLOG(m_journal.error()) << "Submitted transaction invalid: tfInnerBatchTxn flag present."; return; @@ -1185,7 +1185,7 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr& transaction) // under no circumstances will we ever accept an inner txn within a batch // txn from the network. auto const sttx = *transaction->getSTransaction(); - if (sttx.isFlag(tfInnerBatchTxn) && view->rules().enabled(featureBatch)) + if (sttx.isFlag(tfInnerBatchTxn) && view->rules().enabled(featureBatchV1_1)) { transaction->setStatus(INVALID); transaction->setResult(temINVALID_FLAG); diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index dc7c350697..139e765266 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1291,7 +1291,7 @@ PeerImp::handleTransaction( // Charge strongly for attempting to relay a txn with tfInnerBatchTxn // LCOV_EXCL_START /* - There is no need to check whether the featureBatch amendment is + There is no need to check whether the featureBatchV1_1 amendment is enabled. * If the `tfInnerBatchTxn` flag is set, and the amendment is @@ -2740,7 +2740,7 @@ PeerImp::checkTransaction( // charge strongly for relaying batch txns // LCOV_EXCL_START /* - There is no need to check whether the featureBatch amendment is + There is no need to check whether the featureBatchV1_1 amendment is enabled. * If the `tfInnerBatchTxn` flag is set, and the amendment is