From c3cff1ed5c666817a7b05966df34946c266361b6 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Wed, 18 Mar 2026 02:04:57 +0100 Subject: [PATCH] review comments --- include/xrpl/ledger/OpenView.h | 2 +- include/xrpl/protocol/Batch.h | 2 + include/xrpl/protocol/STTx.h | 5 ++- include/xrpl/tx/ApplyContext.h | 2 +- include/xrpl/tx/transactors/Batch.h | 2 - src/libxrpl/protocol/STTx.cpp | 51 +++++++++-------------- src/libxrpl/tx/Transactor.cpp | 4 ++ src/libxrpl/tx/transactors/Batch.cpp | 8 ++-- src/test/app/Batch_test.cpp | 61 ++++++++++++++++++++++++++++ src/test/jtx/batch.h | 8 ++-- src/test/rpc/Simulate_test.cpp | 16 +++++++- src/xrpld/rpc/handlers/Simulate.cpp | 8 ++++ 12 files changed, 123 insertions(+), 46 deletions(-) diff --git a/include/xrpl/ledger/OpenView.h b/include/xrpl/ledger/OpenView.h index 5c942ce5e3..3e0974af39 100644 --- a/include/xrpl/ledger/OpenView.h +++ b/include/xrpl/ledger/OpenView.h @@ -28,7 +28,7 @@ inline constexpr struct open_ledger_t /** Batch view construction tag. Views constructed with this tag are part of a stack of views - used during batch transaction applied. + used during batch transaction application. */ inline constexpr struct batch_view_t { diff --git a/include/xrpl/protocol/Batch.h b/include/xrpl/protocol/Batch.h index 91fb8e5a9d..00cbf641da 100644 --- a/include/xrpl/protocol/Batch.h +++ b/include/xrpl/protocol/Batch.h @@ -1,3 +1,5 @@ +#pragma once + #include #include #include diff --git a/include/xrpl/protocol/STTx.h b/include/xrpl/protocol/STTx.h index 72bd38d5d6..15978efded 100644 --- a/include/xrpl/protocol/STTx.h +++ b/include/xrpl/protocol/STTx.h @@ -150,13 +150,16 @@ private: Expected checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const; + void + buildBatchTxnIds(); + STBase* copy(std::size_t n, void* buf) const override; STBase* move(std::size_t n, void* buf) override; friend class detail::STVar; - mutable std::vector batchTxnIds_; + std::vector batchTxnIds_; }; bool diff --git a/include/xrpl/tx/ApplyContext.h b/include/xrpl/tx/ApplyContext.h index 9e382556c2..2326ff45e9 100644 --- a/include/xrpl/tx/ApplyContext.h +++ b/include/xrpl/tx/ApplyContext.h @@ -122,7 +122,7 @@ private: ApplyFlags flags_; std::optional view_; - // The ID of the batch transaction we are executing under, if seated. + // The ID of the batch transaction we are executing under, if set. std::optional parentBatchId_; }; diff --git a/include/xrpl/tx/transactors/Batch.h b/include/xrpl/tx/transactors/Batch.h index 901677ea2e..4220b4aa2c 100644 --- a/include/xrpl/tx/transactors/Batch.h +++ b/include/xrpl/tx/transactors/Batch.h @@ -1,7 +1,5 @@ #pragma once -#include -#include #include namespace xrpl { diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index 93311174a6..a3b79f775f 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -73,6 +73,7 @@ STTx::STTx(STObject&& object) : STObject(std::move(object)) tx_type_ = safe_cast(getFieldU16(sfTransactionType)); applyTemplate(getTxFormat(tx_type_)->getSOTemplate()); // may throw tid_ = getHash(HashPrefix::transactionID); + buildBatchTxnIds(); } STTx::STTx(SerialIter& sit) : STObject(sfTransaction) @@ -89,6 +90,7 @@ STTx::STTx(SerialIter& sit) : STObject(sfTransaction) applyTemplate(getTxFormat(tx_type_)->getSOTemplate()); // May throw tid_ = getHash(HashPrefix::transactionID); + buildBatchTxnIds(); } STTx::STTx(TxType type, std::function assembler) : STObject(sfTransaction) @@ -106,6 +108,7 @@ STTx::STTx(TxType type, std::function assembler) : STObject(sfT LogicError("Transaction type was mutated during assembly"); tid_ = getHash(HashPrefix::transactionID); + buildBatchTxnIds(); } STBase* @@ -538,41 +541,24 @@ STTx::checkMultiSign(Rules const& rules, STObject const& sigObject) const rules); } -/** - * @brief Retrieves a batch of transaction IDs from the STTx. - * - * This function returns a vector of transaction IDs by extracting them from - * the field array `sfRawTransactions` within the STTx. If the batch - * transaction IDs have already been computed and cached in `batchTxnIds_`, - * it returns the cached vector. Otherwise, it computes the transaction IDs, - * caches them, and then returns the vector. - * - * @return A vector of `uint256` containing the batch transaction IDs. - * - * @note The function asserts that the `sfRawTransactions` field array is not - * empty and that the size of the computed batch transaction IDs matches the - * size of the `sfRawTransactions` field array. - */ +void +STTx::buildBatchTxnIds() +{ + if (tx_type_ != ttBATCH || !isFieldPresent(sfRawTransactions)) + return; + + auto const& raw = getFieldArray(sfRawTransactions); + batchTxnIds_.reserve(raw.size()); + for (STObject const& rb : raw) + batchTxnIds_.push_back(rb.getHash(HashPrefix::transactionID)); +} + std::vector const& STTx::getBatchTransactionIDs() const { XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::getBatchTransactionIDs : not a batch transaction"); XRPL_ASSERT( - getFieldArray(sfRawTransactions).size() != 0, - "STTx::getBatchTransactionIDs : empty raw transactions"); - - // The list of inner ids is built once, then reused on subsequent calls. - // After the list is built, it must always have the same size as the array - // `sfRawTransactions`. The assert below verifies that. - if (batchTxnIds_.size() == 0) - { - for (STObject const& rb : getFieldArray(sfRawTransactions)) - batchTxnIds_.push_back(rb.getHash(HashPrefix::transactionID)); - } - - XRPL_ASSERT( - batchTxnIds_.size() == getFieldArray(sfRawTransactions).size(), - "STTx::getBatchTransactionIDs : batch transaction IDs size mismatch"); + !batchTxnIds_.empty(), "STTx::getBatchTransactionIDs : batch transaction IDs not built"); return batchTxnIds_; } @@ -728,7 +714,7 @@ isRawTransactionOkay(STObject const& st, std::string& reason) reason = "Raw Transactions array exceeds max entries."; return false; } - for (STObject raw : rawTxns) + for (auto raw : rawTxns) { try { @@ -740,6 +726,9 @@ isRawTransactionOkay(STObject const& st, std::string& reason) } raw.applyTemplate(getTxFormat(tt)->getSOTemplate()); + + if (!passesLocalChecks(raw, reason)) + return false; } catch (std::exception const& e) { diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 3f1c600fe1..25171b055a 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -178,6 +178,10 @@ Transactor::preflight1(PreflightContext const& ctx, std::uint32_t flagMask) if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatchV1_1)) return temINVALID_FLAG; + if (ctx.rules.enabled(featureBatchV1_1) && + ctx.tx.isFlag(tfInnerBatchTxn) != ctx.parentBatchId.has_value()) + return temINVALID_INNER_BATCH; + XRPL_ASSERT( ctx.tx.isFlag(tfInnerBatchTxn) == ctx.parentBatchId.has_value() || !ctx.rules.enabled(featureBatchV1_1), diff --git a/src/libxrpl/tx/transactors/Batch.cpp b/src/libxrpl/tx/transactors/Batch.cpp index c488fff6da..f9b3036e57 100644 --- a/src/libxrpl/tx/transactors/Batch.cpp +++ b/src/libxrpl/tx/transactors/Batch.cpp @@ -63,9 +63,9 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) } // LCOV_EXCL_STOP - for (STObject txn : txns) + for (auto const& txn : txns) { - STTx const stx = STTx{std::move(txn)}; + STTx const stx = STTx{STObject(txn)}; // LCOV_EXCL_START if (stx.getTxnType() == ttBATCH) @@ -256,9 +256,9 @@ Batch::preflight(PreflightContext const& ctx) return tesSUCCESS; }; - for (STObject rb : rawTxns) + for (auto const& rb : rawTxns) { - STTx const stx = STTx{std::move(rb)}; + STTx const stx = STTx{STObject(rb)}; auto const hash = stx.getTransactionID(); if (!uniqueHashes.emplace(hash).second) { diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 0bd24cab5c..420999c192 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -14,6 +14,7 @@ #include #include #include +#include namespace xrpl { namespace test { @@ -369,6 +370,25 @@ class Batch_test : public beast::unit_test::suite env.close(); } + // temINVALID_INNER_BATCH: tfInnerBatchTxn set but no parentBatchId. + { + auto jtx = env.jt(pay(alice, bob, XRP(1)), txflags(tfInnerBatchTxn)); + PreflightContext pfCtx( + env.app(), *jtx.stx, env.current()->rules(), tapNONE, env.journal); + auto const pf = Transactor::invokePreflight(pfCtx); + BEAST_EXPECT(pf == temINVALID_INNER_BATCH); + } + + // temINVALID_INNER_BATCH: parentBatchId set but tfInnerBatchTxn not + // set. + { + auto jtx = env.jt(pay(alice, bob, XRP(1))); + PreflightContext pfCtx( + env.app(), *jtx.stx, uint256{1}, env.current()->rules(), tapBATCH, env.journal); + auto const pf = Transactor::invokePreflight(pfCtx); + BEAST_EXPECT(pf == temINVALID_INNER_BATCH); + } + // temBAD_FEE: Batch: inner txn must have a fee of 0. { auto const seq = env.seq(alice); @@ -910,6 +930,47 @@ class Batch_test : public beast::unit_test::suite env(jt.jv, batch::sig(bob), ter(telENV_RPC_FAILED)); env.close(); } + + // Invalid: inner txn with MPT amount on tx type that doesn't + // support MPT (OfferCreate TakerPays) + { + MPTIssue issue(makeMptID(1, alice)); + STAmount mptAmt{issue, UINT64_C(100)}; + + auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const seq = env.seq(alice); + + Json::Value tx1; + tx1[jss::TransactionType] = jss::OfferCreate; + tx1[jss::Account] = alice.human(); + tx1[jss::TakerPays] = mptAmt.getJson(JsonOptions::none); + tx1[jss::TakerGets] = XRP(10).value().getJson(JsonOptions::none); + + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::inner(tx1, seq + 1), + batch::inner(pay(alice, bob, XRP(1)), seq + 2), + ter(telENV_RPC_FAILED)); + env.close(); + } + + // Invalid: inner txn with invalid memo (non-URL-safe MemoType) + { + auto const batchFee = batch::calcBatchFee(env, 0, 2); + auto const seq = env.seq(alice); + + auto tx1 = pay(alice, bob, XRP(10)); + auto& ma = tx1["Memos"]; + auto& mi = ma[ma.size()]; + auto& m = mi["Memo"]; + m["MemoType"] = strHex(std::string("\x01\x02\x03", 3)); + m["MemoData"] = strHex(std::string("test")); + + env(batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::inner(tx1, seq + 1), + batch::inner(pay(alice, bob, XRP(1)), seq + 2), + ter(telENV_RPC_FAILED)); + env.close(); + } } void diff --git a/src/test/jtx/batch.h b/src/test/jtx/batch.h index f81bed3c6e..bc08a43689 100644 --- a/src/test/jtx/batch.h +++ b/src/test/jtx/batch.h @@ -2,14 +2,13 @@ #include #include +#include #include #include #include #include -#include "test/jtx/SignerUtils.h" - #include #include #include @@ -34,7 +33,6 @@ class inner { private: Json::Value txn_; - std::uint32_t seq_; std::optional ticket_; public: @@ -42,10 +40,10 @@ public: Json::Value const& txn, std::uint32_t const& sequence, std::optional const& ticket = std::nullopt) - : txn_(txn), seq_(sequence), ticket_(ticket) + : txn_(txn), ticket_(ticket) { txn_[jss::SigningPubKey] = ""; - txn_[jss::Sequence] = seq_; + txn_[jss::Sequence] = sequence; txn_[jss::Fee] = "0"; txn_[jss::Flags] = txn_[jss::Flags].asUInt() | tfInnerBatchTxn; diff --git a/src/test/rpc/Simulate_test.cpp b/src/test/rpc/Simulate_test.cpp index d1b1cd02fc..fc96e76c90 100644 --- a/src/test/rpc/Simulate_test.cpp +++ b/src/test/rpc/Simulate_test.cpp @@ -394,6 +394,20 @@ class Simulate_test : public beast::unit_test::suite BEAST_EXPECT( resp[jss::result][jss::error_message] == "Transaction should not be signed."); } + { + // tfInnerBatchTxn flag on top-level transaction + Json::Value params; + Json::Value tx_json = Json::objectValue; + tx_json[jss::TransactionType] = jss::AccountSet; + tx_json[jss::Account] = env.master.human(); + tx_json[jss::Flags] = tfInnerBatchTxn; + params[jss::tx_json] = tx_json; + + auto const resp = env.rpc("json", "simulate", to_string(params)); + BEAST_EXPECT( + resp[jss::result][jss::error_message] == + "tfInnerBatchTxn flag is not allowed on top-level transactions."); + } } void @@ -450,7 +464,7 @@ class Simulate_test : public beast::unit_test::suite auto jt = env.jtnofill( batch::outer(alice, env.seq(alice), batchFee, tfAllOrNothing), batch::inner(pay(alice, bob, XRP(10)), seq + 1), - batch::inner(pay(alice, bob, XRP(10)), seq + 1)); + batch::inner(pay(alice, bob, XRP(10)), seq + 2)); jt.jv.removeMember(jss::TxnSignature); Json::Value params; diff --git a/src/xrpld/rpc/handlers/Simulate.cpp b/src/xrpld/rpc/handlers/Simulate.cpp index 5703597beb..b1be45c2ab 100644 --- a/src/xrpld/rpc/handlers/Simulate.cpp +++ b/src/xrpld/rpc/handlers/Simulate.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -332,6 +333,13 @@ doSimulate(RPC::JsonContext& context) return RPC::make_error(rpcNOT_IMPL); } + // Reject transactions with the tfInnerBatchTxn flag. + if (stTx->isFlag(tfInnerBatchTxn)) + { + return RPC::make_error( + rpcINVALID_PARAMS, "tfInnerBatchTxn flag is not allowed on top-level transactions."); + } + std::string reason; auto transaction = std::make_shared(stTx, reason, context.app); // Actually run the transaction through the transaction processor