diff --git a/include/xrpl/protocol/STObject.h b/include/xrpl/protocol/STObject.h index a1cdff22e0..ad87d106c4 100644 --- a/include/xrpl/protocol/STObject.h +++ b/include/xrpl/protocol/STObject.h @@ -229,13 +229,6 @@ public: [[nodiscard]] AccountID getAccountID(SField const& field) const; - /** - * The account responsible for the authorization: the delegate when - * sfDelegate is present, otherwise the account. - */ - [[nodiscard]] AccountID - getInitiator() const; - [[nodiscard]] Blob getFieldVL(SField const& field) const; [[nodiscard]] STAmount const& diff --git a/include/xrpl/protocol/STTx.h b/include/xrpl/protocol/STTx.h index 989bd11c10..d329d42eee 100644 --- a/include/xrpl/protocol/STTx.h +++ b/include/xrpl/protocol/STTx.h @@ -142,9 +142,26 @@ public: TxnSql status, std::string const& escapedMetaData) const; - [[nodiscard]] std::vector const& + /** + * The IDs of the inner transactions of a Batch. + */ + [[nodiscard]] std::vector getBatchTransactionIDs() const; + /** + * The inner transactions of a Batch, built and validated at construction. + * Always seated for Batch STTx instances (construction throws if oversized). + */ + [[nodiscard]] std::vector> const& + getBatchTransactions() const; + + /** + * The account responsible for the authorization: the delegate when + * sfDelegate is present, otherwise the account. + */ + [[nodiscard]] AccountID + getInitiator() const; + [[nodiscard]] AccountID getFeePayerID() const; @@ -166,13 +183,16 @@ private: checkMultiSign(Rules const& rules, STObject const& sigObject) const; [[nodiscard]] std::expected - checkBatchSingleSign(STObject const& batchSigner) const; + checkBatchSingleSign(STObject const& batchSigner, std::vector const& txIds) const; [[nodiscard]] std::expected - checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const; + checkBatchMultiSign( + STObject const& batchSigner, + Rules const& rules, + std::vector const& txIds) const; void - buildBatchTxnIds(); + buildBatchTxns(); STBase* copy(std::size_t n, void* buf) const override; @@ -180,11 +200,11 @@ private: move(std::size_t n, void* buf) override; friend class detail::STVar; - std::optional> batchTxnIds_; + std::optional>> batchTxns_; }; bool -passesLocalChecks(STObject const& st, std::string&); +passesLocalChecks(STTx const& tx, std::string&); /** * Sterilize a transaction. diff --git a/include/xrpl/tx/transactors/system/Batch.h b/include/xrpl/tx/transactors/system/Batch.h index 6540005877..ceaa42e8e0 100644 --- a/include/xrpl/tx/transactors/system/Batch.h +++ b/include/xrpl/tx/transactors/system/Batch.h @@ -12,6 +12,7 @@ #include #include +#include namespace xrpl { @@ -39,6 +40,9 @@ public: static NotTEC checkSign(PreclaimContext const& ctx); + static TER + preclaim(PreclaimContext const& ctx); + TER doApply() override; @@ -76,6 +80,10 @@ private: // only be reached through Batch::checkSign. static NotTEC checkBatchSign(PreclaimContext const& ctx); + + // nullopt on overflow or oversized signer arrays. + static std::optional + calculateBaseFeeImpl(ReadView const& view, STTx const& tx); }; } // namespace xrpl diff --git a/src/libxrpl/protocol/STObject.cpp b/src/libxrpl/protocol/STObject.cpp index ab9ae7d4d7..4b3ace2be3 100644 --- a/src/libxrpl/protocol/STObject.cpp +++ b/src/libxrpl/protocol/STObject.cpp @@ -633,20 +633,6 @@ STObject::getAccountID(SField const& field) const return getFieldByValue(field); } -AccountID -STObject::getInitiator() const -{ - // If sfDelegate is present, the delegate account is the initiator - // note: if a delegate is specified, its authorization to act on behalf of the account is - // enforced in `Transactor::invokeCheckPermission` - // cryptographic signature validity is checked separately (e.g., in `Transactor::checkSign`) - if (isFieldPresent(sfDelegate)) - return getAccountID(sfDelegate); - - // Default initiator - return getAccountID(sfAccount); -} - Blob STObject::getFieldVL(SField const& field) const { diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index be4ba2e8e5..17d7617590 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -72,7 +72,7 @@ STTx::STTx(STObject&& object) { applyTemplate(getTxFormat(txType_)->getSOTemplate()); // may throw tid_ = getHash(HashPrefix::TransactionId); - buildBatchTxnIds(); + buildBatchTxns(); } STTx::STTx(SerialIter& sit) : STObject(sfTransaction) @@ -89,7 +89,7 @@ STTx::STTx(SerialIter& sit) : STObject(sfTransaction) applyTemplate(getTxFormat(txType_)->getSOTemplate()); // May throw tid_ = getHash(HashPrefix::TransactionId); - buildBatchTxnIds(); + buildBatchTxns(); } STTx::STTx(TxType type, std::function assembler) : STObject(sfTransaction) @@ -110,7 +110,7 @@ STTx::STTx(TxType type, std::function assembler) : STObject(sfT logicError("Transaction type was mutated during assembly"); tid_ = getHash(HashPrefix::TransactionId); - buildBatchTxnIds(); + buildBatchTxns(); } STBase* @@ -279,12 +279,9 @@ STTx::checkSign(Rules const& rules) const return std::unexpected("Sponsor: " + 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)) + // Verify batch signer signatures here so they are cached with the rest + // of signature checking. + if (isFieldPresent(sfBatchSigners)) { if (auto const ret = checkBatchSign(rules); !ret) return ret; @@ -307,11 +304,28 @@ STTx::checkBatchSign(Rules const& rules) const if (!isFieldPresent(sfBatchSigners)) return std::unexpected("Missing BatchSigners field."); // LCOV_EXCL_LINE STArray const& signers{getFieldArray(sfBatchSigners)}; + // Bound signature verification to the protocol cap. This runs in + // checkValidity (via checkSign) at relay / submit time, BEFORE preflight + // and passesLocalChecks enforce the cap. Without this guard a malicious + // peer could put an oversized sfBatchSigners array in a 1 MB blob and + // force one signature verification per entry before any of those checks + // (or the fee charge) runs. + if (signers.size() > kMaxBatchSigners) + return std::unexpected("BatchSigners array exceeds max entries."); + // Defensive. + if (!batchTxns_) + { + // LCOV_EXCL_START + UNREACHABLE("STTx::checkBatchSign : batch transactions not built"); + return std::unexpected("Missing inner transactions."); + // LCOV_EXCL_STOP + } + auto const txIds = getBatchTransactionIDs(); for (auto const& signer : signers) { Blob const& signingPubKey = signer.getFieldVL(sfSigningPubKey); - auto const result = signingPubKey.empty() ? checkBatchMultiSign(signer, rules) - : checkBatchSingleSign(signer); + auto const result = signingPubKey.empty() ? checkBatchMultiSign(signer, rules, txIds) + : checkBatchSingleSign(signer, txIds); if (!result) return result; @@ -441,12 +455,11 @@ STTx::checkSingleSign(STObject const& sigObject) const } std::expected -STTx::checkBatchSingleSign(STObject const& batchSigner) const +STTx::checkBatchSingleSign(STObject const& batchSigner, std::vector const& txIds) const { XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::checkBatchSingleSign : batch transaction"); Serializer msg; - serializeBatch( - msg, getAccountID(sfAccount), getSeqValue(), getFlags(), getBatchTransactionIDs()); + serializeBatch(msg, getAccountID(sfAccount), getSeqValue(), getFlags(), txIds); finishMultiSigningData(batchSigner.getAccountID(sfAccount), msg); return singleSignHelper(batchSigner, msg.slice()); } @@ -529,7 +542,10 @@ multiSignHelper( } std::expected -STTx::checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const +STTx::checkBatchMultiSign( + STObject const& batchSigner, + Rules const& rules, + std::vector const& txIds) const { XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::checkBatchMultiSign : batch transaction"); // We can ease the computational load inside the loop a bit by @@ -537,8 +553,7 @@ STTx::checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const // with the stuff that stays constant from signature to signature. auto const batchSignerAccount = batchSigner.getAccountID(sfAccount); Serializer dataStart; - serializeBatch( - dataStart, getAccountID(sfAccount), getSeqValue(), getFlags(), getBatchTransactionIDs()); + serializeBatch(dataStart, getAccountID(sfAccount), getSeqValue(), getFlags(), txIds); dataStart.addBitString(batchSignerAccount); return multiSignHelper( batchSigner, @@ -577,38 +592,79 @@ STTx::checkMultiSign(Rules const& rules, STObject const& sigObject) const } void -STTx::buildBatchTxnIds() +STTx::buildBatchTxns() { // Precondition: the template must have been applied first, so the fields // (including sfRawTransactions) are canonical before the inner txns are // hashed. The constructors call this immediately after applying the // template; isFree() being false confirms a template is set. - XRPL_ASSERT(!isFree(), "STTx::buildBatchTxnIds : template applied"); - if (getTxnType() != ttBATCH || !isFieldPresent(sfRawTransactions)) + XRPL_ASSERT(!isFree(), "STTx::buildBatchTxns : template applied"); + if (getTxnType() != ttBATCH) return; + // A Batch always seats its inner transactions here, so every downstream + // consumer can rely on them. sfRawTransactions is required by the format + // (applyTemplate rejects a Batch without it); this guards a future change + // that made it optional. + if (!isFieldPresent(sfRawTransactions)) + { + // LCOV_EXCL_START + UNREACHABLE("STTx::buildBatchTxns : missing RawTransactions"); + Throw("Batch has no RawTransactions."); + // LCOV_EXCL_STOP + } auto const& raw = getFieldArray(sfRawTransactions); + if (raw.size() > kMaxBatchTxCount) + Throw("Batch has too many inner transactions."); - // 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()); + // Build and validate each inner as an STTx once. A malformed inner throws; + // a nested batch is rejected before building it (a batch cannot contain a + // batch, and building one would recurse). + auto& txns = batchTxns_.emplace(); + txns.reserve(raw.size()); for (STObject const& rb : raw) - ids.push_back(rb.getHash(HashPrefix::TransactionId)); + { + if (rb.getFieldU16(sfTransactionType) == ttBATCH) + Throw("Batch inner transaction cannot be a Batch."); + + txns.push_back(std::make_shared(STObject{rb})); + } } -std::vector const& +std::vector STTx::getBatchTransactionIDs() const { - XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::getBatchTransactionIDs : batch transaction"); + auto const& txns = getBatchTransactions(); + std::vector ids; + ids.reserve(txns.size()); + for (auto const& stx : txns) + ids.push_back(stx->getTransactionID()); + return ids; +} + +std::vector> const& +STTx::getBatchTransactions() const +{ + XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::getBatchTransactions : batch transaction"); + XRPL_ASSERT(batchTxns_.has_value(), "STTx::getBatchTransactions : batch transactions built"); XRPL_ASSERT( - batchTxnIds_.has_value(), "STTx::getBatchTransactionIDs : batch transaction IDs built"); - XRPL_ASSERT( - batchTxnIds_->size() == getFieldArray(sfRawTransactions).size(), - "STTx::getBatchTransactionIDs : batch transaction IDs size mismatch"); - // NOLINTNEXTLINE(bugprone-unchecked-optional-access): guarded by assert above - return *batchTxnIds_; + batchTxns_->size() == getFieldArray(sfRawTransactions).size(), + "STTx::getBatchTransactions : batch transactions size mismatch"); + return *batchTxns_; +} + +AccountID +STTx::getInitiator() const +{ + // If sfDelegate is present, the delegate account is the initiator + // note: if a delegate is specified, its authorization to act on behalf of the account is + // enforced in `Transactor::invokeCheckPermission` + // cryptographic signature validity is checked separately (e.g., in `Transactor::checkSign`) + if (isFieldPresent(sfDelegate)) + return getAccountID(sfDelegate); + + // Default initiator + return getAccountID(sfAccount); } AccountID @@ -754,86 +810,62 @@ invalidMPTAmountInTx(STObject const& tx) } static bool -isBatchRawTransactionOkay(STObject const& st, std::string& reason) +isBatchRawTransactionOkay(STTx const& tx, std::string& reason) { - if (!st.isFieldPresent(sfRawTransactions)) + if (!tx.isFieldPresent(sfRawTransactions)) return true; // 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 (st.getFieldU16(sfTransactionType) != ttBATCH) + if (tx.getTxnType() != ttBATCH) { reason = "Only Batch transactions may contain raw transactions."; return false; } - if (st.isFieldPresent(sfBatchSigners) && - st.getFieldArray(sfBatchSigners).size() > kMaxBatchSigners) + if (tx.isFieldPresent(sfBatchSigners) && + tx.getFieldArray(sfBatchSigners).size() > kMaxBatchSigners) { - reason = "Batch Signers array exceeds max entries."; + reason = "BatchSigners array exceeds max entries."; return false; } - auto const& rawTxns = st.getFieldArray(sfRawTransactions); - if (rawTxns.size() > kMaxBatchTxCount) + // Inner structure (type, template, no nesting, count) is validated when the + // batch STTx is constructed; here we only run each inner's local checks. + for (auto const& inner : tx.getBatchTransactions()) { - reason = "Raw Transactions array exceeds max entries."; - return false; - } - for (STObject raw : rawTxns) - { - try - { - auto const tt = safeCast(raw.getFieldU16(sfTransactionType)); - if (tt == ttBATCH) - { - reason = "Raw Transactions may not contain batch transactions."; - return false; - } - - raw.applyTemplate(getTxFormat(tt)->getSOTemplate()); - - // passesLocalChecks recurses back into isBatchRawTransactionOkay, - // but an inner can never be a batch (rejected above), so the - // recursion terminates at depth 1. - if (!passesLocalChecks(raw, reason)) - return false; - } - catch (std::exception const& e) - { - reason = e.what(); + if (!passesLocalChecks(*inner, reason)) return false; - } } return true; } bool -passesLocalChecks(STObject const& st, std::string& reason) +passesLocalChecks(STTx const& tx, std::string& reason) { - if (!isMemoOkay(st, reason)) + if (!isMemoOkay(tx, reason)) return false; - if (!isAccountFieldOkay(st)) + if (!isAccountFieldOkay(tx)) { reason = "An account field is invalid."; return false; } - if (isPseudoTx(st)) + if (isPseudoTx(tx)) { reason = "Cannot submit pseudo transactions."; return false; } - if (invalidMPTAmountInTx(st)) + if (invalidMPTAmountInTx(tx)) { reason = "Amount can not be MPT."; return false; } - if (!isBatchRawTransactionOkay(st, reason)) + if (!isBatchRawTransactionOkay(tx, reason)) return false; return true; diff --git a/src/libxrpl/tx/apply.cpp b/src/libxrpl/tx/apply.cpp index d85c4cfc40..f93b19a158 100644 --- a/src/libxrpl/tx/apply.cpp +++ b/src/libxrpl/tx/apply.cpp @@ -177,9 +177,9 @@ applyBatchTransactions( int applied = 0; - for (STObject rb : batchTxn.getFieldArray(sfRawTransactions)) + for (auto const& stx : batchTxn.getBatchTransactions()) { - auto const result = applyOneTransaction(STTx{std::move(rb)}); + auto const result = applyOneTransaction(*stx); XRPL_ASSERT( result.applied == (isTesSuccess(result.ter) || isTecClaim(result.ter)), "Outer Batch failure, inner transaction should not be applied"); diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index dee465cb88..ccb113e07b 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/src/libxrpl/tx/transactors/system/Batch.cpp @@ -15,7 +15,6 @@ #include #include #include -#include #include #include #include @@ -28,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -46,17 +46,11 @@ namespace xrpl { * * @param view The ledger view providing fee and state information. * @param tx The batch transaction to calculate the fee for. - * @return XRPAmount The total base fee required for the batch transaction. - * - * @throws std::overflow_error If any fee calculation would overflow the - * XRPAmount type. - * @throws std::length_error If the number of inner transactions or signers - * exceeds the allowed maximum. - * @throws std::invalid_argument If an inner transaction is itself a batch - * transaction. + * @return XRPAmount The total base fee required for the batch transaction, + * or std::nullopt on failure (overflow, oversized arrays). */ -XRPAmount -Batch::calculateBaseFee(ReadView const& view, STTx const& tx) +std::optional +Batch::calculateBaseFeeImpl(ReadView const& view, STTx const& tx) { XRPAmount const maxAmount{std::numeric_limits::max()}; @@ -67,49 +61,26 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) if (baseFee > maxAmount - view.fees().base) { JLOG(debugLog().error()) << "BatchTrace: Base fee overflow detected."; - return XRPAmount{kInitialXrp}; + return std::nullopt; } // LCOV_EXCL_STOP XRPAmount const batchBase = view.fees().base + baseFee; - // Calculate the Inner Txn Fees + // Calculate the Inner Txn Fees. Inners are built and validated (count, + // no nesting) at construction, so they are reused here directly. XRPAmount txnFees{0}; - if (tx.isFieldPresent(sfRawTransactions)) + for (auto const& stx : tx.getBatchTransactions()) { - auto const& txns = tx.getFieldArray(sfRawTransactions); - + auto const fee = xrpl::calculateBaseFee(view, *stx); // LCOV_EXCL_START - if (txns.size() > kMaxBatchTxCount) + if (txnFees > maxAmount - fee) { - JLOG(debugLog().error()) << "BatchTrace: Raw Transactions array exceeds max entries."; - return XRPAmount{kInitialXrp}; + JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in txnFees calculation."; + return std::nullopt; } // LCOV_EXCL_STOP - - for (STObject txn : txns) - { - STTx const stx = STTx{std::move(txn)}; - - // LCOV_EXCL_START - if (stx.getTxnType() == ttBATCH) - { - JLOG(debugLog().error()) << "BatchTrace: Inner Batch transaction found."; - return XRPAmount{kInitialXrp}; - } - // LCOV_EXCL_STOP - - auto const fee = xrpl::calculateBaseFee(view, stx); - // LCOV_EXCL_START - if (txnFees > maxAmount - fee) - { - JLOG(debugLog().error()) - << "BatchTrace: XRPAmount overflow in txnFees calculation."; - return XRPAmount{kInitialXrp}; - } - // LCOV_EXCL_STOP - txnFees += fee; - } + txnFees += fee; } // Calculate the Signers/BatchSigners Fees @@ -122,7 +93,7 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) if (signers.size() > kMaxBatchSigners) { JLOG(debugLog().error()) << "BatchTrace: Batch Signers array exceeds max entries."; - return XRPAmount{kInitialXrp}; + return std::nullopt; } // LCOV_EXCL_STOP @@ -140,7 +111,7 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) { JLOG(debugLog().error()) << "BatchTrace: Nested Signers array exceeds max entries."; - return kInitialXrp; + return std::nullopt; } // LCOV_EXCL_STOP signerCount += nestedSigners.size(); @@ -152,7 +123,7 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) if (signerCount > 0 && view.fees().base > maxAmount / signerCount) { JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in signerCount calculation."; - return XRPAmount{kInitialXrp}; + return std::nullopt; } // LCOV_EXCL_STOP @@ -162,13 +133,13 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) if (signerFees > maxAmount - txnFees) { JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in signerFees calculation."; - return XRPAmount{kInitialXrp}; + return std::nullopt; } XRPAmount const innerFees = txnFees + signerFees; if (innerFees > maxAmount - batchBase) { JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in total fee calculation."; - return XRPAmount{kInitialXrp}; + return std::nullopt; } // LCOV_EXCL_STOP @@ -176,6 +147,24 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) return innerFees + batchBase; } +XRPAmount +Batch::calculateBaseFee(ReadView const& view, STTx const& tx) +{ + if (auto const fee = calculateBaseFeeImpl(view, tx)) + return *fee; + // The fee could not be computed, so return a placeholder the account can + // pay; preclaim rejects the transaction with tecINSUFF_FEE. + return view.fees().base; // LCOV_EXCL_LINE +} + +TER +Batch::preclaim(PreclaimContext const& ctx) +{ + if (!calculateBaseFeeImpl(ctx.view, ctx.tx)) + return tecINSUFF_FEE; // LCOV_EXCL_LINE + return tesSUCCESS; +} + std::uint32_t Batch::getFlagsMask(PreflightContext const& ctx) { @@ -246,13 +235,6 @@ Batch::preflight(PreflightContext const& ctx) return temARRAY_EMPTY; } - if (rawTxns.size() > kMaxBatchTxCount) - { - JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]:" - << "txns array exceeds 8 entries."; - return temARRAY_TOO_LARGE; - } - if (ctx.tx.isFieldPresent(sfBatchSigners) && ctx.tx.getFieldArray(sfBatchSigners).size() > kMaxBatchSigners) { @@ -293,9 +275,9 @@ Batch::preflight(PreflightContext const& ctx) return tesSUCCESS; }; - for (STObject rb : rawTxns) + for (auto const& stxPtr : ctx.tx.getBatchTransactions()) { - STTx const stx = STTx{std::move(rb)}; + STTx const& stx = *stxPtr; auto const hash = stx.getTransactionID(); if (!uniqueHashes.emplace(hash).second) { @@ -306,14 +288,6 @@ Batch::preflight(PreflightContext const& ctx) } auto const txType = stx.getFieldU16(sfTransactionType); - if (txType == ttBATCH) - { - JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " - << "batch cannot have an inner batch txn. " - << "txID: " << hash; - return temINVALID; - } - if (std::ranges::any_of( kDisabledTxTypes, [txType](auto const& disabled) { return txType == disabled; })) { @@ -434,15 +408,14 @@ Batch::preflightSigValidated(PreflightContext const& ctx) ctx.tx.getTxnType() == ttBATCH, "xrpl::Batch::preflightSigValidated : batch transaction"); auto const parentBatchId = ctx.tx.getTransactionID(); auto const outerAccount = ctx.tx.getAccountID(sfAccount); - auto const& rawTxns = ctx.tx.getFieldArray(sfRawTransactions); - // Accounts that must sign the batch: each inner authorizer and counterparty // (excluding the outer account), sorted and de-duplicated to match against // the ascending, unique batch signers. std::vector requiredSigners; requiredSigners.reserve(kMaxBatchSigners); - for (STObject const& rb : rawTxns) + for (auto const& stxPtr : ctx.tx.getBatchTransactions()) { + STTx const& rb = *stxPtr; // A delegated inner is signed by the delegate, not the account holder, // so the delegate is the required signer when present. AccountID const authorizer = rb.getInitiator(); diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index f9c156fe06..5085ad6172 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -56,7 +56,6 @@ #include #include #include -#include #include #include #include @@ -72,6 +71,7 @@ #include #include #include +#include #include #include #include @@ -313,8 +313,8 @@ class Batch_test : public beast::unit_test::Suite env.close(); } - // DEFENSIVE: temARRAY_TOO_LARGE: Batch: txns array exceeds 8 entries. - // ACTUAL: telENV_RPC_FAILED: isRawTransactionOkay() + // An oversized batch (more than kMaxBatchTxCount inners) fails STTx + // construction, so the transaction cannot be built. { auto const seq = env.seq(alice); auto const batchFee = batch::calcBatchFee(env, 0, 9); @@ -328,7 +328,7 @@ class Batch_test : public beast::unit_test::Suite batch::Inner(pay(alice, bob, XRP(1)), seq + 7), batch::Inner(pay(alice, bob, XRP(1)), seq + 8), batch::Inner(pay(alice, bob, XRP(1)), seq + 9), - Ter(telENV_RPC_FAILED)); + Ter(temMALFORMED)); env.close(); } @@ -345,15 +345,15 @@ class Batch_test : public beast::unit_test::Suite env.close(); } - // DEFENSIVE: temINVALID: Batch: batch cannot have inner batch txn. - // ACTUAL: telENV_RPC_FAILED: isRawTransactionOkay() + // A batch may not contain a batch: the nested inner fails STTx + // construction, so the transaction cannot be built. { auto const seq = env.seq(alice); auto const batchFee = batch::calcBatchFee(env, 0, 2); env(batch::outer(alice, seq, batchFee, tfAllOrNothing), batch::Inner(batch::outer(alice, seq, batchFee, tfAllOrNothing), seq), batch::Inner(pay(alice, bob, XRP(1)), seq + 2), - Ter(telENV_RPC_FAILED)); + Ter(temMALFORMED)); env.close(); } @@ -938,80 +938,41 @@ class Batch_test : public beast::unit_test::Suite env.fund(XRP(10000), alice, bob); + // An inner missing a required field can no longer be submitted: the + // outer STTx builds and validates each inner at construction, so + // building the batch (as signing does) throws. Returns true if the + // build fails. + auto batchCtorFails = [&](json::StaticString const& field) -> bool { + auto const batchFee = batch::calcBatchFee(env, 1, 2); + auto const seq = env.seq(alice); + auto tx1 = batch::Inner(pay(alice, bob, XRP(10)), seq + 1); + tx1.removeMember(field); + try + { + // Env::st swallows the construction failure and yields a null + // stx, so a malformed inner shows up as no transaction built. + auto const jt = env.jtnofill( + batch::outer(alice, seq, batchFee, tfAllOrNothing), + tx1, + batch::Inner(pay(alice, bob, XRP(10)), seq + 2)); + return jt.stx == nullptr; + } + catch (std::exception const&) + { + return true; + } + }; + // Invalid: sfTransactionType - { - auto const batchFee = batch::calcBatchFee(env, 1, 2); - auto const seq = env.seq(alice); - auto tx1 = batch::Inner(pay(alice, bob, XRP(10)), seq + 1); - tx1.removeMember(jss::TransactionType); - auto jt = env.jtnofill( - batch::outer(alice, seq, batchFee, tfAllOrNothing), - tx1, - batch::Inner(pay(alice, bob, XRP(10)), seq + 2)); - - env(jt.jv, batch::Sig(bob), Ter(telENV_RPC_FAILED)); - env.close(); - } - + BEAST_EXPECT(batchCtorFails(jss::TransactionType)); // Invalid: sfAccount - { - auto const batchFee = batch::calcBatchFee(env, 1, 2); - auto const seq = env.seq(alice); - auto tx1 = batch::Inner(pay(alice, bob, XRP(10)), seq + 1); - tx1.removeMember(jss::Account); - auto jt = env.jtnofill( - batch::outer(alice, seq, batchFee, tfAllOrNothing), - tx1, - batch::Inner(pay(alice, bob, XRP(10)), seq + 2)); - - env(jt.jv, batch::Sig(bob), Ter(telENV_RPC_FAILED)); - env.close(); - } - + BEAST_EXPECT(batchCtorFails(jss::Account)); // Invalid: sfSequence - { - auto const batchFee = batch::calcBatchFee(env, 1, 2); - auto const seq = env.seq(alice); - auto tx1 = batch::Inner(pay(alice, bob, XRP(10)), seq + 1); - tx1.removeMember(jss::Sequence); - auto jt = env.jtnofill( - batch::outer(alice, seq, batchFee, tfAllOrNothing), - tx1, - batch::Inner(pay(alice, bob, XRP(10)), seq + 2)); - - env(jt.jv, batch::Sig(bob), Ter(telENV_RPC_FAILED)); - env.close(); - } - + BEAST_EXPECT(batchCtorFails(jss::Sequence)); // Invalid: sfFee - { - auto const batchFee = batch::calcBatchFee(env, 1, 2); - auto const seq = env.seq(alice); - auto tx1 = batch::Inner(pay(alice, bob, XRP(10)), seq + 1); - tx1.removeMember(jss::Fee); - auto jt = env.jtnofill( - batch::outer(alice, seq, batchFee, tfAllOrNothing), - tx1, - batch::Inner(pay(alice, bob, XRP(10)), seq + 2)); - - env(jt.jv, batch::Sig(bob), Ter(telENV_RPC_FAILED)); - env.close(); - } - + BEAST_EXPECT(batchCtorFails(jss::Fee)); // Invalid: sfSigningPubKey - { - auto const batchFee = batch::calcBatchFee(env, 1, 2); - auto const seq = env.seq(alice); - auto tx1 = batch::Inner(pay(alice, bob, XRP(10)), seq + 1); - tx1.removeMember(jss::SigningPubKey); - auto jt = env.jtnofill( - batch::outer(alice, seq, batchFee, tfAllOrNothing), - tx1, - batch::Inner(pay(alice, bob, XRP(10)), seq + 2)); - - env(jt.jv, batch::Sig(bob), Ter(telENV_RPC_FAILED)); - env.close(); - } + BEAST_EXPECT(batchCtorFails(jss::SigningPubKey)); // Inner OfferCreate with MPT TakerPays. Valid under featureMPTokensV2. { @@ -1512,11 +1473,13 @@ class Batch_test : public beast::unit_test::Suite batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), - Ter(telENV_RPC_FAILED)); + Ter(temMALFORMED)); env.close(); } - // temARRAY_TOO_LARGE: Batch: txns array exceeds 8 entries. + // An oversized batch (more than kMaxBatchTxCount inners) fails STTx + // construction, so it never reaches apply or checkValidity - Env::st + // swallows the failure and yields a null stx. { Env env{*this, features}; @@ -1539,11 +1502,44 @@ class Batch_test : public beast::unit_test::Suite batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), batch::Inner(pay(alice, bob, XRP(1)), aliceSeq)); - env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal j) { - auto const result = xrpl::apply(env.app(), view, *jt.stx, TapNone, j); - BEAST_EXPECT(!result.applied && result.ter == temARRAY_TOO_LARGE); - return result.applied; - }); + BEAST_EXPECT(jt.stx == nullptr); + } + + // An oversized batch cannot slip through as validly signed even when it + // carries a BatchSigners array: the oversized inner array fails STTx + // construction before any signature is checked, so the batch can't be + // built at all (building it - as signing does - throws). + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + env.fund(XRP(10000), alice, bob); + env.close(); + + auto const aliceSeq = env.seq(alice); + auto const batchFee = batch::calcBatchFee(env, kMaxBatchSigners + 1, 9); + bool threw = false; + try + { + env.jtnofill( + batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Sig(std::vector(kMaxBatchSigners + 1, bob))); + } + catch (std::exception const&) + { + threw = true; + } + BEAST_EXPECT(threw); } // Regression: the relay-boundary local check (isBatchRawTransactionOkay) @@ -1598,6 +1594,14 @@ class Batch_test : public beast::unit_test::Suite BEAST_EXPECT(!result.applied && result.ter == temARRAY_TOO_LARGE); return result.applied; }); + + // Regression (uncapped batch-signer verification): the relay + // boundary (checkValidity) rejects the oversized signers array via + // the checkBatchSign guard, BEFORE verifying a single signature. + auto const [valid, reason] = + xrpl::checkValidity(env.app().getHashRouter(), *jt.stx, env.current()->rules()); + BEAST_EXPECT(valid == xrpl::Validity::SigBad); + BEAST_EXPECT(reason == "BatchSigners array exceeds max entries."); } } @@ -5524,7 +5528,8 @@ class Batch_test : public beast::unit_test::Suite return Batch::calculateBaseFee(*env.current(), *jtx.stx); }; - // bad: Inner Batch transaction found + // bad: a batch may not contain a batch - the nested inner fails STTx + // construction, so the transaction cannot be built. { auto const seq = env.seq(alice); XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2); @@ -5532,11 +5537,11 @@ class Batch_test : public beast::unit_test::Suite batch::outer(alice, seq, batchFee, tfAllOrNothing), batch::Inner(batch::outer(alice, seq, batchFee, tfAllOrNothing), seq), batch::Inner(pay(alice, bob, XRP(1)), seq + 2)); - XRPAmount const txBaseFee = getBaseFee(jtx); - BEAST_EXPECT(txBaseFee == XRPAmount(kInitialXrp)); + BEAST_EXPECT(jtx.stx == nullptr); } - // bad: Raw Transactions array exceeds max entries. + // bad: an oversized batch (more than kMaxBatchTxCount inners) fails + // STTx construction, so it cannot be built. { auto const seq = env.seq(alice); XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2); @@ -5553,8 +5558,7 @@ class Batch_test : public beast::unit_test::Suite batch::Inner(pay(alice, bob, XRP(1)), seq + 8), batch::Inner(pay(alice, bob, XRP(1)), seq + 9)); - XRPAmount const txBaseFee = getBaseFee(jtx); - BEAST_EXPECT(txBaseFee == XRPAmount(kInitialXrp)); + BEAST_EXPECT(jtx.stx == nullptr); } // bad: Signers array exceeds max entries. @@ -5567,8 +5571,9 @@ class Batch_test : public beast::unit_test::Suite batch::Inner(pay(alice, bob, XRP(10)), seq + 1), batch::Inner(pay(alice, bob, XRP(5)), seq + 2), batch::Sig(std::vector(kMaxBatchSigners + 1, bob))); + // Failure paths fall back to the ledger base fee. XRPAmount const txBaseFee = getBaseFee(jtx); - BEAST_EXPECT(txBaseFee == XRPAmount(kInitialXrp)); + BEAST_EXPECT(txBaseFee == env.current()->fees().base); } // good: diff --git a/src/test/protocol/STTx_test.cpp b/src/test/protocol/STTx_test.cpp index 45bd2729c4..777234be83 100644 --- a/src/test/protocol/STTx_test.cpp +++ b/src/test/protocol/STTx_test.cpp @@ -21,6 +21,7 @@ #include +#include #include #include #include @@ -50,15 +51,10 @@ public: run() override { testMalformedSerializedForm(); - - testcase("secp256k1 signatures"); testSTTx(KeyType::Secp256k1); - - testcase("ed25519 signatures"); testSTTx(KeyType::Ed25519); - - testcase("STObject constructor errors"); testObjectCtorErrors(); + testBatchInnerCtorErrors(); } void @@ -1328,6 +1324,8 @@ public: void testSTTx(KeyType keyType) { + testcase(std::string(to_string(keyType)) + " signatures"); + auto const keypair = randomKeyPair(keyType); STTx j(ttACCOUNT_SET, [&keypair](auto& obj) { @@ -1382,6 +1380,8 @@ public: void testObjectCtorErrors() { + testcase("STObject constructor errors"); + auto const kp1 = randomKeyPair(KeyType::Secp256k1); auto const id1 = calcAccountID(kp1.first); @@ -1463,6 +1463,75 @@ public: BEAST_EXPECT(got == "Field 'Fee' is required but missing."); } } + + void + testBatchInnerCtorErrors() + { + testcase("Batch inner transaction validation"); + + auto const kp1 = randomKeyPair(KeyType::Secp256k1); + auto const id1 = calcAccountID(kp1.first); + + auto const kp2 = randomKeyPair(KeyType::Secp256k1); + auto const id2 = calcAccountID(kp2.first); + + // A raw inner transaction object of the given transaction type. + auto makeInner = [&](std::uint16_t txType) { + STObject inner(sfRawTransaction); + inner.setFieldU16(sfTransactionType, txType); + inner.setAccountID(sfAccount, id1); + inner.setAccountID(sfDestination, id2); + inner.setFieldAmount(sfAmount, STAmount(10000000000ull)); + inner.setFieldAmount(sfFee, STAmount(0ull)); + inner.setFieldU32(sfSequence, 1); + inner.setFieldVL(sfSigningPubKey, Slice(kp1.first.data(), kp1.first.size())); + return inner; + }; + + // An outer Batch STObject wrapping the given inner. + auto makeBatch = [&](STObject inner) { + STArray rawTxns(sfRawTransactions); + rawTxns.push_back(std::move(inner)); + + STObject batch(sfGeneric); + batch.setFieldU16(sfTransactionType, ttBATCH); + batch.setAccountID(sfAccount, id1); + batch.setFieldAmount(sfFee, STAmount(20ull)); + batch.setFieldU32(sfSequence, 1); + batch.setFieldVL(sfSigningPubKey, Slice(kp1.first.data(), kp1.first.size())); + batch.setFieldArray(sfRawTransactions, rawTxns); + return batch; + }; + + { + // A batch whose inner is a well-formed transaction constructs. + std::string errorMsg; + try + { + STTx{makeBatch(makeInner(ttPAYMENT))}; + } + catch (std::exception const& err) + { + errorMsg = err.what(); + } + BEAST_EXPECT(errorMsg.empty()); + } + { + // A batch whose inner carries an unregistered transaction type is + // rejected at construction, rather than surviving as a raw STObject + // and throwing later from an unprotected fee-calculation path. + std::string errorMsg; + try + { + STTx{makeBatch(makeInner(60000))}; + } + catch (std::exception const& err) + { + errorMsg = err.what(); + } + BEAST_EXPECT(matches(errorMsg.c_str(), "Invalid transaction type 60000")); + } + } }; class InnerObjectFormatsSerializer_test : public beast::unit_test::Suite