From 095389672a1b4d201d481b1e8c43540db2e314f4 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Mon, 29 Jun 2026 18:15:15 -0400 Subject: [PATCH] fix: refactor checkBatchSign & checkSign --- include/xrpl/core/HashRouter.h | 4 +- include/xrpl/protocol/STTx.h | 3 +- include/xrpl/tx/Transactor.h | 7 +- src/libxrpl/protocol/STTx.cpp | 40 +++++--- src/libxrpl/tx/Transactor.cpp | 31 ++++-- src/libxrpl/tx/apply.cpp | 14 +-- src/libxrpl/tx/transactors/system/Batch.cpp | 83 ++++------------ src/test/app/Batch_test.cpp | 104 +++++++------------- 8 files changed, 118 insertions(+), 168 deletions(-) diff --git a/include/xrpl/core/HashRouter.h b/include/xrpl/core/HashRouter.h index b2057283c1..c8b34d8e93 100644 --- a/include/xrpl/core/HashRouter.h +++ b/include/xrpl/core/HashRouter.h @@ -28,9 +28,7 @@ enum class HashRouterFlags : std::uint16_t { PRIVATE4 = 0x0800, // Used in EscrowFinish.cpp PRIVATE5 = 0x1000, - PRIVATE6 = 0x2000, - // Used in Batch.cpp - PRIVATE7 = 0x4000 + PRIVATE6 = 0x2000 }; constexpr HashRouterFlags diff --git a/include/xrpl/protocol/STTx.h b/include/xrpl/protocol/STTx.h index 7cc1dd9b49..95f31cc711 100644 --- a/include/xrpl/protocol/STTx.h +++ b/include/xrpl/protocol/STTx.h @@ -12,6 +12,7 @@ #include #include +#include namespace xrpl { @@ -162,7 +163,7 @@ private: move(std::size_t n, void* buf) override; friend class detail::STVar; - std::vector batchTxnIds_; + std::optional> batchTxnIds_; }; bool diff --git a/include/xrpl/tx/Transactor.h b/include/xrpl/tx/Transactor.h index 6144025ac2..2b50cfa6e7 100644 --- a/include/xrpl/tx/Transactor.h +++ b/include/xrpl/tx/Transactor.h @@ -370,7 +370,12 @@ protected: std::optional const& parentBatchId, AccountID const& idAccount, STObject const& sigObject, - beast::Journal const j); + beast::Journal const j, + // A batch may carry an inner from an account that an earlier inner + // creates, so the signer account need not exist yet; when it does not, + // only its own master key may authorize it. Normal transactions require + // the account to already exist. + bool permitUncreatedAccount = false); // Base class always returns true static bool diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index 2b2a7ba8ba..b95c93d655 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -281,6 +281,17 @@ STTx::checkSign(Rules const& rules) const if (auto const ret = checkSign(rules, counterSig); !ret) return std::unexpected("Counterparty: " + ret.error()); } + + // Verify the batch signer signatures here too, so they are cached with the + // rest of signature checking (checkValidity / SF_SIGGOOD) and stay out of + // the transaction engine. Gated on a batch (batchTxnIds_ seated) that + // actually carries signers; a batch whose inners are all from the outer + // account has no sfBatchSigners and needs no signer crypto. + if (batchTxnIds_ && isFieldPresent(sfBatchSigners)) + { + if (auto const ret = checkBatchSign(rules); !ret) + return ret; + } return {}; } @@ -581,12 +592,13 @@ STTx::buildBatchTxnIds() auto const& raw = getFieldArray(sfRawTransactions); - if (raw.size() > kMaxBatchTxCount) - return; - - batchTxnIds_.reserve(raw.size()); + // Seated for any batch with raw transactions. The count is validated in + // preflight and at the relay boundary, so build every id here; this keeps + // the invariant batchTxnIds_->size() == rawTransactions.size(). + auto& ids = batchTxnIds_.emplace(); + ids.reserve(raw.size()); for (STObject const& rb : raw) - batchTxnIds_.push_back(rb.getHash(HashPrefix::TransactionId)); + ids.push_back(rb.getHash(HashPrefix::TransactionId)); } std::vector const& @@ -594,11 +606,11 @@ STTx::getBatchTransactionIDs() const { XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::getBatchTransactionIDs : batch transaction"); XRPL_ASSERT( - !batchTxnIds_.empty(), "STTx::getBatchTransactionIDs : batch transaction IDs not built"); + batchTxnIds_.has_value(), "STTx::getBatchTransactionIDs : batch transaction IDs built"); XRPL_ASSERT( - batchTxnIds_.size() == getFieldArray(sfRawTransactions).size(), + batchTxnIds_->size() == getFieldArray(sfRawTransactions).size(), "STTx::getBatchTransactionIDs : batch transaction IDs size mismatch"); - return batchTxnIds_; + return *batchTxnIds_; } //------------------------------------------------------------------------------ @@ -740,10 +752,14 @@ isBatchRawTransactionOkay(STObject const& st, std::string& reason) if (!st.isFieldPresent(sfRawTransactions)) return true; - // sfRawTransactions only appears on a Batch. - XRPL_ASSERT( - safeCast(st.getFieldU16(sfTransactionType)) == ttBATCH, - "xrpl::isBatchRawTransactionOkay : batch transaction"); + // sfRawTransactions only appears on a Batch. passesLocalChecks runs on + // unverified user and peer input, so reject (rather than assert) a non-batch + // transaction that carries it. + if (safeCast(st.getFieldU16(sfTransactionType)) != ttBATCH) + { + reason = "Only Batch transactions may contain raw transactions."; + return false; + } if (st.isFieldPresent(sfBatchSigners) && st.getFieldArray(sfBatchSigners).size() > kMaxBatchSigners) diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index d7ffaf8757..0f6988543e 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -244,8 +244,11 @@ Transactor::preflight2(PreflightContext const& ctx) return *ret; } - // Skip signature check on batch inner transactions. These flag and - // parentBatchId checks duplicate preflight1 as defense in depth. + // Skip the signature check on batch inner transactions. preflight1 already + // enforces both conditions; re-checking them as defense in depth guarantees + // we never return success (and so skip signature validation) for an inner + // transaction unless the amendment is enabled and it really sits inside a + // batch. if (ctx.tx.isFlag(tfInnerBatchTxn)) { if (!ctx.rules.enabled(featureBatchV1_1)) @@ -710,16 +713,19 @@ Transactor::checkSign( std::optional const& parentBatchId, AccountID const& idAccount, STObject const& sigObject, - beast::Journal const j) + beast::Journal const j, + bool permitUncreatedAccount) { { auto const sle = view.read(keylet::account(idAccount)); - if (view.rules().enabled(featureLendingProtocol) && isPseudoAccount(sle)) + if ((view.rules().enabled(featureLendingProtocol) || + view.rules().enabled(featureBatchV1_1) || view.rules().enabled(fixCleanup3_3_0)) && + isPseudoAccount(sle)) { - // Pseudo-accounts can't sign transactions. This check is gated on - // the Lending Protocol amendment because that's the project it was - // added under, and it doesn't justify another amendment + // Pseudo-accounts can't sign transactions. This check is gated on a + // few different amendments so that it takes effect as soon as any of + // them is activated. return tefBAD_AUTH; } } @@ -764,7 +770,16 @@ Transactor::checkSign( auto const idSigner = calcAccountID(PublicKey(makeSlice(pkSigner))); auto const sleAccount = view.read(keylet::account(idAccount)); if (!sleAccount) - return terNO_ACCOUNT; + { + // An account that does not exist yet can only be authorized by its own + // master key, and only where an un-created signer is permitted (a batch + // whose earlier inner creates the account). Otherwise it cannot sign. + if (!permitUncreatedAccount) + return terNO_ACCOUNT; + if (idAccount != idSigner) + return tefBAD_AUTH; + return tesSUCCESS; + } return checkSingleSign(view, idSigner, idAccount, sleAccount, j); } diff --git a/src/libxrpl/tx/apply.cpp b/src/libxrpl/tx/apply.cpp index 4eb9b39780..d85c4cfc40 100644 --- a/src/libxrpl/tx/apply.cpp +++ b/src/libxrpl/tx/apply.cpp @@ -8,7 +8,6 @@ #include #include #include -#include #include #include #include @@ -39,13 +38,14 @@ checkValidity(HashRouter& router, STTx const& tx, Rules const& rules) auto const id = tx.getTransactionID(); auto const flags = router.getFlags(id); - // Ignore signature check on batch inner transactions - if (tx.isFlag(tfInnerBatchTxn) && rules.enabled(featureBatchV1_1)) + // Batch inner transactions are never independently valid: they are applied + // within their batch, not through checkValidity. Reaching here means one was + // relayed or submitted on its own, so mark it bad regardless of the + // amendment (like PeerImp and NetworkOPs). + if (tx.isFlag(tfInnerBatchTxn)) { - // 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."}; + router.setFlags(id, kSfSigbad); + return {Validity::SigBad, "Batch inner transactions are never considered validly signed."}; } if (any(flags & kSfSigbad)) diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index c11a2c0684..4cb44513bd 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/src/libxrpl/tx/transactors/system/Batch.cpp @@ -1,19 +1,14 @@ #include #include -#include #include #include #include -#include #include #include #include -#include #include -#include #include -#include #include #include #include @@ -38,10 +33,6 @@ namespace xrpl { -// Set on a batch's tx id once its signer signatures verify, so -// preflightSigValidated can skip the expensive re-check on later preflights. -constexpr HashRouterFlags kSfBatchSigGood = HashRouterFlags::PRIVATE7; - /** * @brief Calculates the total base fee for a batch transaction. * @@ -171,7 +162,8 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in signerFees calculation."; return XRPAmount{kInitialXrp}; } - if (txnFees + signerFees > maxAmount - batchBase) + XRPAmount const innerFees = txnFees + signerFees; + if (innerFees > maxAmount - batchBase) { JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in total fee calculation."; return XRPAmount{kInitialXrp}; @@ -179,7 +171,7 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) // LCOV_EXCL_STOP // 10 drops per batch signature + sum of inner tx fees + batchBase - return signerFees + txnFees + batchBase; + return innerFees + batchBase; } std::uint32_t @@ -485,7 +477,7 @@ Batch::preflightSigValidated(PreflightContext const& ctx) requiredSigners[numReqSignersMatched] != signerAccount) { JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " - << "extra signer provided: " << signerAccount; + << "missing signer or extra signer provided: " << signerAccount; return temBAD_SIGNER; } ++numReqSignersMatched; @@ -502,25 +494,6 @@ Batch::preflightSigValidated(PreflightContext const& ctx) return temBAD_SIGNER; } - // Check the batch signer signatures (only present when there are signers). - if (ctx.tx.isFieldPresent(sfBatchSigners)) - { - // Caches only the cryptographic signature check, not whether each - // signer is authorized to sign for its account - that ledger-dependent - // check is Batch::checkBatchSign in preclaim and is never cached. - auto& router = ctx.registry.get().getHashRouter(); - if (!any(router.getFlags(parentBatchId) & kSfBatchSigGood)) - { - if (auto const sigResult = ctx.tx.checkBatchSign(ctx.rules); !sigResult) - { - JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " - << "invalid batch txn signature: " << sigResult.error(); - return temBAD_SIGNATURE; - } - router.setFlags(parentBatchId, kSfBatchSigGood); - } - } - return tesSUCCESS; } @@ -530,41 +503,21 @@ Batch::checkBatchSign(PreclaimContext const& ctx) STArray const& signers{ctx.tx.getFieldArray(sfBatchSigners)}; for (auto const& signer : signers) { + // Reuse the standard signer-authorization rules so the batch and + // non-batch paths cannot drift. permitUncreatedAccount allows an inner + // from an account that an earlier inner creates, which must be + // authorized by its own master key. auto const idAccount = signer.getAccountID(sfAccount); - Blob const& pkSigner = signer.getFieldVL(sfSigningPubKey); - if (pkSigner.empty()) - { - if (auto const ret = checkMultiSign(ctx.view, ctx.flags, idAccount, signer, ctx.j); - !isTesSuccess(ret)) - return ret; - } - else - { - if (!publicKeyType(makeSlice(pkSigner))) - return tefBAD_AUTH; // LCOV_EXCL_LINE - - auto const idSigner = calcAccountID(PublicKey(makeSlice(pkSigner))); - auto const sleAccount = ctx.view.read(keylet::account(idAccount)); - - if (sleAccount) - { - if (isPseudoAccount(sleAccount)) - return tefBAD_AUTH; // LCOV_EXCL_LINE - - if (auto const ret = - checkSingleSign(ctx.view, idSigner, idAccount, sleAccount, ctx.j); - !isTesSuccess(ret)) - return ret; - } - else - { - if (idAccount != idSigner) - return tefBAD_AUTH; - - // A batch can include transactions from an un-created account ONLY - // when the account master key is the signer - } - } + if (auto const ret = Transactor::checkSign( + ctx.view, + ctx.flags, + ctx.parentBatchId, + idAccount, + signer, + ctx.j, + /*permitUncreatedAccount=*/true); + !isTesSuccess(ret)) + return ret; } return tesSUCCESS; } diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 443be72458..f9c156fe06 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -232,12 +232,10 @@ class Batch_test : public beast::unit_test::Suite } // tfInnerBatchTxn - // If the feature is disabled, the transaction fails with - // temINVALID_FLAG. If the feature is enabled, the transaction fails - // early in checkValidity() + // A standalone transaction carrying this flag is never valid, so it + // is rejected early in checkValidity() regardless of the amendment. { - auto const txResult = withBatch ? Ter(telENV_RPC_FAILED) : Ter(temINVALID_FLAG); - env(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn), txResult); + env(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn), Ter(telENV_RPC_FAILED)); env.close(); } @@ -663,7 +661,7 @@ class Batch_test : public beast::unit_test::Suite jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfTxnSignature.jsonName] = strHex(Slice{sig.data(), sig.size()}); - env(jt.jv, Ter(temBAD_SIGNATURE)); + env(jt.jv, Ter(telENV_RPC_FAILED)); env.close(); } @@ -2650,37 +2648,20 @@ class Batch_test : public beast::unit_test::Suite env.fund(XRP(10000), alice, bob); env.close(); - auto submitAndValidate = [&](std::string caseName, - Slice const& slice, - int line, - std::optional expectedEnabled = std::nullopt, - std::optional expectedDisabled = std::nullopt, - bool expectInvalidFlag = false) { - testcase << testName << caseName - << (expectInvalidFlag ? " - Expected to reach tx engine!" : ""); + // Any transaction carrying tfInnerBatchTxn is rejected in checkValidity() + // before it reaches the tx engine, regardless of its signing fields or + // whether the amendment is enabled. + auto submitAndValidate = [&](std::string caseName, Slice const& slice, int line) { + testcase << testName << caseName; auto const jrr = env.rpc("submit", strHex(slice))[jss::result]; - auto const expected = withBatch - ? expectedEnabled.value_or( - "fails local checks: Malformed: Invalid inner batch " - "transaction.") - : expectedDisabled.value_or("fails local checks: Empty SigningPubKey."); - if (expectInvalidFlag) - { - expect( - jrr[jss::status] == "success" && jrr[jss::engine_result] == "temINVALID_FLAG", - pretty(jrr), - __FILE__, - line); - } - else - { - expect( - jrr[jss::status] == "error" && jrr[jss::error] == "invalidTransaction" && - jrr[jss::error_exception] == expected, - pretty(jrr), - __FILE__, - line); - } + expect( + jrr[jss::status] == "error" && jrr[jss::error] == "invalidTransaction" && + jrr[jss::error_exception] == + "fails local checks: Batch inner transactions are never " + "considered validly signed.", + pretty(jrr), + __FILE__, + line); env.close(); }; @@ -2709,12 +2690,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "SigningPubKey set", - s.slice(), - __LINE__, - std::nullopt, - "fails local checks: Invalid signature."); + submitAndValidate("SigningPubKey set", s.slice(), __LINE__); } // Invalid RPC Submission: Signers @@ -2728,12 +2704,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "Signers set", - s.slice(), - __LINE__, - std::nullopt, - "fails local checks: Invalid Signers array size."); + submitAndValidate("Signers set", s.slice(), __LINE__); } { @@ -2744,8 +2715,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", jt.jv); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "Fully signed", s.slice(), __LINE__, std::nullopt, std::nullopt, !withBatch); + submitAndValidate("Fully signed", s.slice(), __LINE__); } // Invalid RPC Submission: tfInnerBatchTxn @@ -2758,12 +2728,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "No signing fields set", - s.slice(), - __LINE__, - "fails local checks: Empty SigningPubKey.", - "fails local checks: Empty SigningPubKey."); + submitAndValidate("No signing fields set", s.slice(), __LINE__); } // Invalid RPC Submission: tfInnerBatchTxn pseudo-transaction @@ -2782,12 +2747,7 @@ class Batch_test : public beast::unit_test::Suite STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access) - submitAndValidate( - "Pseudo-transaction", - s.slice(), - __LINE__, - "fails local checks: Empty SigningPubKey.", - "fails local checks: Empty SigningPubKey."); + submitAndValidate("Pseudo-transaction", s.slice(), __LINE__); } } @@ -5716,7 +5676,7 @@ class Batch_test : public beast::unit_test::Suite batch::Inner(pay(carol, alice, XRP(50)), carolSeq)); jt2.jv[sfBatchSigners.jsonName] = capturedSigners; - env(jt2.jv, Ter(temBAD_SIGNATURE)); + env(jt2.jv, Ter(telENV_RPC_FAILED)); env.close(); BEAST_EXPECT(env.seq(carol) == carolSeq); @@ -5760,7 +5720,7 @@ class Batch_test : public beast::unit_test::Suite batch::Inner(pay(carol, alice, XRP(500)), carolSeq)); jt2.jv[sfBatchSigners.jsonName] = capturedSigners; - env(jt2.jv, Ter(temBAD_SIGNATURE)); + env(jt2.jv, Ter(telENV_RPC_FAILED)); env.close(); BEAST_EXPECT(env.balance(carol) == preCarol); @@ -5813,7 +5773,7 @@ class Batch_test : public beast::unit_test::Suite bobSignerEntry[sfBatchSigner.jsonName][sfSigners.jsonName]; jt2.jv[sfBatchSigners.jsonName][0u] = carolSigner; - env(jt2.jv, Ter(temBAD_SIGNATURE)); + env(jt2.jv, Ter(telENV_RPC_FAILED)); env.close(); } } @@ -5860,8 +5820,10 @@ class Batch_test : public beast::unit_test::Suite using namespace test::jtx; - // Mirrors Batch.cpp's file-local kSfBatchSigGood; keep in sync. - constexpr HashRouterFlags kSfBatchSigGood = HashRouterFlags::PRIVATE7; + // Mirrors apply.cpp's file-local kSfSiggood (the standard signature-good + // cache); batch signer sigs are now verified and cached alongside the + // outer signature in checkSign/checkValidity. + constexpr HashRouterFlags kSfSiggood = HashRouterFlags::PRIVATE2; // Valid batch: alice (outer) + an inner from bob, who co-signs. auto buildValidBatch = [](Env& env) { @@ -5885,9 +5847,9 @@ class Batch_test : public beast::unit_test::Suite auto jt = buildValidBatch(env); auto const txid = jt.stx->getTransactionID(); - BEAST_EXPECT(!any(env.app().getHashRouter().getFlags(txid) & kSfBatchSigGood)); + BEAST_EXPECT(!any(env.app().getHashRouter().getFlags(txid) & kSfSiggood)); env(jt, Ter(tesSUCCESS)); - BEAST_EXPECT(any(env.app().getHashRouter().getFlags(txid) & kSfBatchSigGood)); + BEAST_EXPECT(any(env.app().getHashRouter().getFlags(txid) & kSfSiggood)); env.close(); } @@ -5901,7 +5863,7 @@ class Batch_test : public beast::unit_test::Suite auto jt = buildValidBatch(env); jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfTxnSignature.jsonName] = "00"; - env(jt.jv, Ter(temBAD_SIGNATURE)); + env(jt.jv, Ter(telENV_RPC_FAILED)); env.close(); } { @@ -5914,7 +5876,7 @@ class Batch_test : public beast::unit_test::Suite jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfTxnSignature.jsonName] = "00"; auto const txid = STTx{parse(jt.jv)}.getTransactionID(); - env.app().getHashRouter().setFlags(txid, kSfBatchSigGood); + env.app().getHashRouter().setFlags(txid, kSfSiggood); env(jt.jv, Ter(tesSUCCESS)); env.close(); }