Fix: Improve error handling in Batch RPC response (#5503)

src/libxrpl/protocol/STTx.cpp

@@ -760,6 +760,12 @@ isRawTransactionOkay(STObject const& st, std::string& reason)
         {
             TxType const tt =
                 safe_cast<TxType>(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)

src/test/app/Batch_test.cpp

@@ -24,6 +24,7 @@
 #include <xrpld/app/misc/HashRouter.h>
 #include <xrpld/app/misc/Transaction.h>
 #include <xrpld/app/tx/apply.h>
+#include <xrpld/app/tx/detail/Batch.h>
 
 #include <xrpl/protocol/Batch.h>
 #include <xrpl/protocol/Feature.h>
@@ -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:

src/xrpld/app/misc/NetworkOPs.cpp

@@ -1701,7 +1701,7 @@ NetworkOPsImp::apply(std::unique_lock<std::mutex>& batchLock)
                 }
             }
 
-            if (!isTemMalformed(e.result) && validatedLedgerIndex)
+            if (validatedLedgerIndex)
             {
                 auto [fee, accountSeq, availableSeq] =
                     app_.getTxQ().getTxRequiredFeeAndSeq(

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