From 8f2f5310e216d0ca6a068132730338f79686a709 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Thu, 19 Jun 2025 04:46:45 +0700 Subject: [PATCH] Fix: Improve error handling in Batch RPC response (#5503) --- src/libxrpl/protocol/STTx.cpp | 6 + src/test/app/Batch_test.cpp | 178 +++++++++++++++++++++++++++++- src/xrpld/app/misc/NetworkOPs.cpp | 2 +- src/xrpld/app/tx/detail/Batch.cpp | 58 ++++++---- 4 files changed, 222 insertions(+), 22 deletions(-) diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index ee26dd69de..615012dba4 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -760,6 +760,12 @@ isRawTransactionOkay(STObject const& st, std::string& reason) { TxType const tt = safe_cast(raw.getFieldU16(sfTransactionType)); + if (tt == ttBATCH) + { + reason = "Raw Transactions may not contain batch transactions."; + return false; + } + raw.applyTemplate(getTxFormat(tt)->getSOTemplate()); } catch (std::exception const& e) diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 0866bca2ef..6ce95c56d0 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -317,7 +318,8 @@ class Batch_test : public beast::unit_test::suite env.close(); } - // temINVALID: Batch: batch cannot have inner batch txn. + // DEFENSIVE: temINVALID: Batch: batch cannot have inner batch txn. + // ACTUAL: telENV_RPC_FAILED: isRawTransactionOkay() { auto const seq = env.seq(alice); auto const batchFee = batch::calcBatchFee(env, 0, 2); @@ -325,7 +327,7 @@ class Batch_test : public beast::unit_test::suite batch::inner( batch::outer(alice, seq, batchFee, tfAllOrNothing), seq), batch::inner(pay(alice, bob, XRP(1)), seq + 2), - ter(temINVALID)); + ter(telENV_RPC_FAILED)); env.close(); } @@ -3953,6 +3955,176 @@ class Batch_test : public beast::unit_test::suite } } + void + testValidateRPCResponse(FeatureBitset features) + { + // Verifying that the RPC response from submit includes + // the account_sequence_available, account_sequence_next, + // open_ledger_cost and validated_ledger_index fields. + testcase("Validate RPC response"); + + using namespace jtx; + Env env(*this); + Account const alice("alice"); + Account const bob("bob"); + env.fund(XRP(10000), alice, bob); + env.close(); + + // tes + { + auto const baseFee = env.current()->fees().base; + auto const aliceSeq = env.seq(alice); + auto jtx = env.jt(pay(alice, bob, XRP(1))); + + Serializer s; + jtx.stx->add(s); + auto const jr = env.rpc("submit", strHex(s.slice()))[jss::result]; + env.close(); + + BEAST_EXPECT(jr.isMember(jss::account_sequence_available)); + BEAST_EXPECT( + jr[jss::account_sequence_available].asUInt() == aliceSeq + 1); + BEAST_EXPECT(jr.isMember(jss::account_sequence_next)); + BEAST_EXPECT( + jr[jss::account_sequence_next].asUInt() == aliceSeq + 1); + BEAST_EXPECT(jr.isMember(jss::open_ledger_cost)); + BEAST_EXPECT(jr[jss::open_ledger_cost] == to_string(baseFee)); + BEAST_EXPECT(jr.isMember(jss::validated_ledger_index)); + } + + // tec failure + { + auto const baseFee = env.current()->fees().base; + auto const aliceSeq = env.seq(alice); + env(fset(bob, asfRequireDest)); + auto jtx = env.jt(pay(alice, bob, XRP(1)), seq(aliceSeq)); + + Serializer s; + jtx.stx->add(s); + auto const jr = env.rpc("submit", strHex(s.slice()))[jss::result]; + env.close(); + + BEAST_EXPECT(jr.isMember(jss::account_sequence_available)); + BEAST_EXPECT( + jr[jss::account_sequence_available].asUInt() == aliceSeq + 1); + BEAST_EXPECT(jr.isMember(jss::account_sequence_next)); + BEAST_EXPECT( + jr[jss::account_sequence_next].asUInt() == aliceSeq + 1); + BEAST_EXPECT(jr.isMember(jss::open_ledger_cost)); + BEAST_EXPECT(jr[jss::open_ledger_cost] == to_string(baseFee)); + BEAST_EXPECT(jr.isMember(jss::validated_ledger_index)); + } + + // tem failure + { + auto const baseFee = env.current()->fees().base; + auto const aliceSeq = env.seq(alice); + auto jtx = env.jt(pay(alice, bob, XRP(1)), seq(aliceSeq + 1)); + + Serializer s; + jtx.stx->add(s); + auto const jr = env.rpc("submit", strHex(s.slice()))[jss::result]; + env.close(); + + BEAST_EXPECT(jr.isMember(jss::account_sequence_available)); + BEAST_EXPECT( + jr[jss::account_sequence_available].asUInt() == aliceSeq); + BEAST_EXPECT(jr.isMember(jss::account_sequence_next)); + BEAST_EXPECT(jr[jss::account_sequence_next].asUInt() == aliceSeq); + BEAST_EXPECT(jr.isMember(jss::open_ledger_cost)); + BEAST_EXPECT(jr[jss::open_ledger_cost] == to_string(baseFee)); + BEAST_EXPECT(jr.isMember(jss::validated_ledger_index)); + } + } + + void + testBatchCalculateBaseFee(FeatureBitset features) + { + using namespace jtx; + Env env(*this); + Account const alice("alice"); + Account const bob("bob"); + Account const carol("carol"); + env.fund(XRP(10000), alice, bob, carol); + env.close(); + + auto getBaseFee = [&](JTx const& jtx) -> XRPAmount { + Serializer s; + jtx.stx->add(s); + return Batch::calculateBaseFee(*env.current(), *jtx.stx); + }; + + // bad: Inner Batch transaction found + { + auto const seq = env.seq(alice); + XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2); + auto jtx = env.jt( + 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(INITIAL_XRP)); + } + + // bad: Raw Transactions array exceeds max entries. + { + auto const seq = env.seq(alice); + XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2); + + auto jtx = env.jt( + batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::inner(pay(alice, bob, XRP(1)), seq + 1), + batch::inner(pay(alice, bob, XRP(1)), seq + 2), + batch::inner(pay(alice, bob, XRP(1)), seq + 3), + batch::inner(pay(alice, bob, XRP(1)), seq + 4), + batch::inner(pay(alice, bob, XRP(1)), seq + 5), + batch::inner(pay(alice, bob, XRP(1)), seq + 6), + 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)); + + XRPAmount const txBaseFee = getBaseFee(jtx); + BEAST_EXPECT(txBaseFee == XRPAmount(INITIAL_XRP)); + } + + // bad: Signers array exceeds max entries. + { + auto const seq = env.seq(alice); + XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2); + + auto jtx = env.jt( + batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::inner(pay(alice, bob, XRP(10)), seq + 1), + batch::inner(pay(alice, bob, XRP(5)), seq + 2), + batch::sig( + bob, + carol, + alice, + bob, + carol, + alice, + bob, + carol, + alice, + alice)); + XRPAmount const txBaseFee = getBaseFee(jtx); + BEAST_EXPECT(txBaseFee == XRPAmount(INITIAL_XRP)); + } + + // good: + { + auto const seq = env.seq(alice); + XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2); + auto jtx = env.jt( + batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::inner(pay(alice, bob, XRP(1)), seq + 1), + batch::inner(pay(bob, alice, XRP(2)), seq + 2)); + XRPAmount const txBaseFee = getBaseFee(jtx); + BEAST_EXPECT(txBaseFee == batchFee); + } + } + void testWithFeats(FeatureBitset features) { @@ -3983,6 +4155,8 @@ class Batch_test : public beast::unit_test::suite testBatchTxQueue(features); testBatchNetworkOps(features); testBatchDelegate(features); + testValidateRPCResponse(features); + testBatchCalculateBaseFee(features); } public: diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index c8197b2219..1b1bea3ad9 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1701,7 +1701,7 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) } } - if (!isTemMalformed(e.result) && validatedLedgerIndex) + if (validatedLedgerIndex) { auto [fee, accountSeq, availableSeq] = app_.getTxQ().getTxRequiredFeeAndSeq( diff --git a/src/xrpld/app/tx/detail/Batch.cpp b/src/xrpld/app/tx/detail/Batch.cpp index dcac889a5a..40991ea99a 100644 --- a/src/xrpld/app/tx/detail/Batch.cpp +++ b/src/xrpld/app/tx/detail/Batch.cpp @@ -61,7 +61,10 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) // LCOV_EXCL_START if (baseFee > maxAmount - view.fees().base) - throw std::overflow_error("XRPAmount overflow"); + { + JLOG(debugLog().error()) << "BatchTrace: Base fee overflow detected."; + return XRPAmount{INITIAL_XRP}; + } // LCOV_EXCL_STOP XRPAmount const batchBase = view.fees().base + baseFee; @@ -72,32 +75,36 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) { auto const& txns = tx.getFieldArray(sfRawTransactions); - XRPL_ASSERT( - txns.size() <= maxBatchTxCount, - "Raw Transactions array exceeds max entries."); - // LCOV_EXCL_START if (txns.size() > maxBatchTxCount) - throw std::length_error( - "Raw Transactions array exceeds max entries"); + { + JLOG(debugLog().error()) + << "BatchTrace: Raw Transactions array exceeds max entries."; + return XRPAmount{INITIAL_XRP}; + } // LCOV_EXCL_STOP for (STObject txn : txns) { STTx const stx = STTx{std::move(txn)}; - XRPL_ASSERT( - stx.getTxnType() != ttBATCH, "Inner Batch transaction found."); - // LCOV_EXCL_START if (stx.getTxnType() == ttBATCH) - throw std::invalid_argument("Inner Batch transaction found"); + { + JLOG(debugLog().error()) + << "BatchTrace: Inner Batch transaction found."; + return XRPAmount{INITIAL_XRP}; + } // LCOV_EXCL_STOP auto const fee = ripple::calculateBaseFee(view, stx); // LCOV_EXCL_START if (txnFees > maxAmount - fee) - throw std::overflow_error("XRPAmount overflow"); + { + JLOG(debugLog().error()) + << "BatchTrace: XRPAmount overflow in txnFees calculation."; + return XRPAmount{INITIAL_XRP}; + } // LCOV_EXCL_STOP txnFees += fee; } @@ -108,13 +115,14 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) if (tx.isFieldPresent(sfBatchSigners)) { auto const& signers = tx.getFieldArray(sfBatchSigners); - XRPL_ASSERT( - signers.size() <= maxBatchTxCount, - "Batch Signers array exceeds max entries."); // LCOV_EXCL_START if (signers.size() > maxBatchTxCount) - throw std::length_error("Batch Signers array exceeds max entries"); + { + JLOG(debugLog().error()) + << "BatchTrace: Batch Signers array exceeds max entries."; + return XRPAmount{INITIAL_XRP}; + } // LCOV_EXCL_STOP for (STObject const& signer : signers) @@ -128,16 +136,28 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) // LCOV_EXCL_START if (signerCount > 0 && view.fees().base > maxAmount / signerCount) - throw std::overflow_error("XRPAmount overflow"); + { + JLOG(debugLog().error()) + << "BatchTrace: XRPAmount overflow in signerCount calculation."; + return XRPAmount{INITIAL_XRP}; + } // LCOV_EXCL_STOP XRPAmount signerFees = signerCount * view.fees().base; // LCOV_EXCL_START if (signerFees > maxAmount - txnFees) - throw std::overflow_error("XRPAmount overflow"); + { + JLOG(debugLog().error()) + << "BatchTrace: XRPAmount overflow in signerFees calculation."; + return XRPAmount{INITIAL_XRP}; + } if (txnFees + signerFees > maxAmount - batchBase) - throw std::overflow_error("XRPAmount overflow"); + { + JLOG(debugLog().error()) + << "BatchTrace: XRPAmount overflow in total fee calculation."; + return XRPAmount{INITIAL_XRP}; + } // LCOV_EXCL_STOP // 10 drops per batch signature + sum of inner tx fees + batchBase