diff --git a/include/xrpl/protocol/STTx.h b/include/xrpl/protocol/STTx.h index 989bd11c10..e2b6301edf 100644 --- a/include/xrpl/protocol/STTx.h +++ b/include/xrpl/protocol/STTx.h @@ -145,6 +145,13 @@ public: [[nodiscard]] std::vector const& getBatchTransactionIDs() const; + /** + * The inner transactions of a Batch, built and validated at construction. + * Seated only when the batch is within the size cap. + */ + [[nodiscard]] std::vector> const& + getBatchTransactions() const; + [[nodiscard]] AccountID getFeePayerID() const; @@ -172,7 +179,7 @@ private: checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const; void - buildBatchTxnIds(); + buildBatchTxns(); STBase* copy(std::size_t n, void* buf) const override; @@ -181,6 +188,7 @@ private: friend class detail::STVar; std::optional> batchTxnIds_; + std::optional>> batchTxns_; }; bool diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index be4ba2e8e5..988034cf5e 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* @@ -281,10 +281,14 @@ STTx::checkSign(Rules const& rules) const // 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)) + // the transaction engine. Keyed only on sfBatchSigners being present (which + // only a Batch can carry). Deliberately NOT gated on batchTxnIds_ or any + // other transaction data: signature checking must never be skipped based on + // whether some size check passed, otherwise an oversized batch could slip + // through as validly signed by leaning on the later local / preflight + // checks (which are bypassed on several paths). A batch whose inners are all + // from the outer account has no sfBatchSigners and needs no signer crypto. + if (isFieldPresent(sfBatchSigners)) { if (auto const ret = checkBatchSign(rules); !ret) return ret; @@ -307,6 +311,24 @@ 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."); + // The signers sign over the inner transaction ids, which are seated at + // construction whenever sfRawTransactions is present (required for a + // Batch). Guard against a future format change that made it optional. + if (!batchTxnIds_) + { + // LCOV_EXCL_START + UNREACHABLE("STTx::checkBatchSign : batch transaction IDs not built"); + return std::unexpected("Missing inner transactions."); + // LCOV_EXCL_STOP + } for (auto const& signer : signers) { Blob const& signingPubKey = signer.getFieldVL(sfSigningPubKey); @@ -577,25 +599,45 @@ 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 ids and 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(). + // 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). ids are cached alongside the + // transactions so checkBatchSign, which signs over them once per signer, + // does not reallocate. auto& ids = batchTxnIds_.emplace(); + auto& txns = batchTxns_.emplace(); ids.reserve(raw.size()); + 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."); + + auto stx = std::make_shared(STObject{rb}); + ids.push_back(stx->getTransactionID()); + txns.push_back(std::move(stx)); + } } std::vector const& @@ -611,6 +653,18 @@ STTx::getBatchTransactionIDs() const return *batchTxnIds_; } +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( + batchTxns_->size() == getFieldArray(sfRawTransactions).size(), + "STTx::getBatchTransactions : batch transactions size mismatch"); + return *batchTxns_; +} + AccountID STTx::getFeePayerID() const { @@ -775,36 +829,12 @@ isBatchRawTransactionOkay(STObject const& st, std::string& reason) 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 (STObject const& raw : st.getFieldArray(sfRawTransactions)) { - 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(raw, 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/applySteps.cpp b/src/libxrpl/tx/applySteps.cpp index 5af4f621a7..f4e343b3bc 100644 --- a/src/libxrpl/tx/applySteps.cpp +++ b/src/libxrpl/tx/applySteps.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -419,7 +420,19 @@ preclaim(PreflightResult const& preflightResult, ServiceRegistry& registry, Open XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx) { - return invokeCalculateBaseFee(view, tx); + try + { + return invokeCalculateBaseFee(view, tx); + } + catch (std::exception const& e) + { + // LCOV_EXCL_START + // A fee calc should not throw; return an unpayable fee so the + // transaction fails the fee check. + JLOG(debugLog().error()) << "apply (calculateBaseFee): " << e.what(); + return XRPAmount{kInitialXrp}; + // LCOV_EXCL_STOP + } } XRPAmount diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index dee465cb88..7838698d1a 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/src/libxrpl/tx/transactors/system/Batch.cpp @@ -73,43 +73,20 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) 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."; + JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in txnFees calculation."; return XRPAmount{kInitialXrp}; } // 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 @@ -140,7 +117,7 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) { JLOG(debugLog().error()) << "BatchTrace: Nested Signers array exceeds max entries."; - return kInitialXrp; + return XRPAmount{kInitialXrp}; } // LCOV_EXCL_STOP signerCount += nestedSigners.size(); @@ -246,13 +223,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 +263,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 +276,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 +396,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..d28d40d36a 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -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. diff --git a/src/test/protocol/STTx_test.cpp b/src/test/protocol/STTx_test.cpp index 45bd2729c4..4d03cbbb58 100644 --- a/src/test/protocol/STTx_test.cpp +++ b/src/test/protocol/STTx_test.cpp @@ -59,6 +59,9 @@ public: testcase("STObject constructor errors"); testObjectCtorErrors(); + + testcase("Batch inner transaction validation"); + testBatchInnerCtorErrors(); } void @@ -1463,6 +1466,73 @@ public: BEAST_EXPECT(got == "Field 'Fee' is required but missing."); } } + + void + testBatchInnerCtorErrors() + { + 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 got; + try + { + STTx{makeBatch(makeInner(ttPAYMENT))}; + } + catch (std::exception const& err) + { + got = err.what(); + } + BEAST_EXPECT(got.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 got; + try + { + STTx{makeBatch(makeInner(60000))}; + } + catch (std::exception const& err) + { + got = err.what(); + } + BEAST_EXPECT(matches(got.c_str(), "Invalid transaction type 60000")); + } + } }; class InnerObjectFormatsSerializer_test : public beast::unit_test::Suite