diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 9bac9ef7db..9797fb837d 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -936,7 +936,7 @@ TRANSACTION(ttVAULT_CLAWBACK, 70, VaultClawback, #endif TRANSACTION(ttBATCH, 71, Batch, Delegation::NotDelegable, - featureBatch, + featureBatchV1_1, NoPriv, ({ {sfRawTransactions, SoeRequired}, diff --git a/include/xrpl/protocol_autogen/transactions/Batch.h b/include/xrpl/protocol_autogen/transactions/Batch.h index 55b99d0eef..00f553ada7 100644 --- a/include/xrpl/protocol_autogen/transactions/Batch.h +++ b/include/xrpl/protocol_autogen/transactions/Batch.h @@ -20,7 +20,7 @@ class BatchBuilder; * * Type: ttBATCH (71) * Delegable: Delegation::NotDelegable - * Amendment: featureBatch + * Amendment: featureBatchV1_1 * Privileges: NoPriv * * Immutable wrapper around STTx providing type-safe field access. diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index 689155839a..7af3877660 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -570,7 +570,7 @@ STTx::buildBatchTxnIds() auto const& raw = getFieldArray(sfRawTransactions); - if (raw.size() > maxBatchTxCount) + if (raw.size() > kMaxBatchTxCount) return; batchTxnIds_.reserve(raw.size()); diff --git a/src/libxrpl/tx/transactors/payment/Payment.cpp b/src/libxrpl/tx/transactors/payment/Payment.cpp index 3334bc187b..146f5a0ff2 100644 --- a/src/libxrpl/tx/transactors/payment/Payment.cpp +++ b/src/libxrpl/tx/transactors/payment/Payment.cpp @@ -330,7 +330,11 @@ Payment::preclaim(PreclaimContext const& ctx) // transaction would succeed. return tecNO_DST; } - if (partialPaymentAllowed) + // Prior to featureBatchV1_1 this check was skipped on closed views, + // which let inner batch payments slip through. Gate the broadened + // check on the amendment so behavior is preserved pre-amendment. + if (partialPaymentAllowed && + (ctx.view.open() || ctx.view.rules().enabled(featureBatchV1_1))) { // You cannot fund an account with a partial payment. // Make retry work smaller, by rejecting this. @@ -367,7 +371,11 @@ Payment::preclaim(PreclaimContext const& ctx) } // Payment with at least one intermediate step and uses transitive balances. - if (hasPaths || sendMax || !dstAmount.native()) + // Prior to featureBatchV1_1 the path size check below was only enforced on + // open views; gate the broadened enforcement on the amendment so + // pre-amendment behavior is preserved. + if ((hasPaths || sendMax || !dstAmount.native()) && + (ctx.view.open() || ctx.view.rules().enabled(featureBatchV1_1))) { STPathSet const& paths = ctx.tx.getFieldPathSet(sfPaths); diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index 4808a5333e..c04e3cbbe7 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/src/libxrpl/tx/transactors/system/Batch.cpp @@ -136,11 +136,11 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) { auto const& nestedSigners = signer.getFieldArray(sfSigners); // LCOV_EXCL_START - if (nestedSigners.size() > STTx::maxMultiSigners) + if (nestedSigners.size() > STTx::kMaxMultiSigners) { JLOG(debugLog().error()) << "BatchTrace: Nested Signers array exceeds max entries."; - return XRPAmount{INITIAL_XRP}; + return kInitialXrp; } // LCOV_EXCL_STOP signerCount += nestedSigners.size(); @@ -243,7 +243,7 @@ Batch::preflight(PreflightContext const& ctx) } if (ctx.tx.isFieldPresent(sfBatchSigners) && - ctx.tx.getFieldArray(sfBatchSigners).size() > maxBatchTxCount) + ctx.tx.getFieldArray(sfBatchSigners).size() > kMaxBatchTxCount) { JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]:" << "signers array exceeds 8 entries."; @@ -442,7 +442,7 @@ Batch::preflightSigValidated(PreflightContext const& ctx) } // Batch signers must be in strictly ascending order by AccountID. - AccountID lastBatchSigner(beast::zero); + AccountID lastBatchSigner{beast::kZero}; for (auto const& signer : signers) { AccountID const signerAccount = signer.getAccountID(sfAccount); diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index dffb04bae3..a453e8840e 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -51,6 +51,7 @@ #include #include #include +#include #include #include #include @@ -429,9 +430,9 @@ class Batch_test : public beast::unit_test::Suite // temINVALID_INNER_BATCH: tfInnerBatchTxn set but no parentBatchId. { - auto jtx = env.jt(pay(alice, bob, XRP(1)), txflags(tfInnerBatchTxn)); + auto jtx = env.jt(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn)); PreflightContext pfCtx( - env.app(), *jtx.stx, env.current()->rules(), tapNONE, env.journal); + env.app(), *jtx.stx, env.current()->rules(), TapNone, env.journal); auto const pf = Transactor::invokePreflight(pfCtx); BEAST_EXPECT(pf == temINVALID_INNER_BATCH); } @@ -441,7 +442,7 @@ class Batch_test : public beast::unit_test::Suite { auto jtx = env.jt(pay(alice, bob, XRP(1))); PreflightContext pfCtx( - env.app(), *jtx.stx, uint256{1}, env.current()->rules(), tapBATCH, env.journal); + env.app(), *jtx.stx, uint256{1}, env.current()->rules(), TapBatch, env.journal); auto const pf = Transactor::invokePreflight(pfCtx); BEAST_EXPECT(pf == temINVALID_INNER_BATCH); } @@ -1002,16 +1003,16 @@ class Batch_test : public beast::unit_test::Suite auto const batchFee = batch::calcBatchFee(env, 0, 2); auto const seq = env.seq(alice); - Json::Value tx1; + 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); + tx1[jss::TakerPays] = mptAmt.getJson(JsonOptions::Values::None); + tx1[jss::TakerGets] = XRP(10).value().getJson(JsonOptions::Values::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)); + batch::Inner(tx1, seq + 1), + batch::Inner(pay(alice, bob, XRP(1)), seq + 2), + Ter(telENV_RPC_FAILED)); env.close(); } @@ -1028,9 +1029,9 @@ class Batch_test : public beast::unit_test::Suite 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)); + batch::Inner(tx1, seq + 1), + batch::Inner(pay(alice, bob, XRP(1)), seq + 2), + Ter(telENV_RPC_FAILED)); env.close(); } } @@ -2566,11 +2567,11 @@ class Batch_test : public beast::unit_test::Suite // checkBatchSign must validate all signers regardless of order. // This must fail with tefBAD_AUTH. env(batch::outer(alice, seq, batchFee, tfAllOrNothing), - batch::inner(pay(alice, unfunded, XRP(100)), seq + 1), - batch::inner(noop(unfunded), ledSeq), - batch::inner(pay(carol, alice, XRP(1000)), env.seq(carol)), - batch::sig(unfunded, Reg{carol, alice}), - ter(tefBAD_AUTH)); + batch::Inner(pay(alice, unfunded, XRP(100)), seq + 1), + batch::Inner(noop(unfunded), ledSeq), + batch::Inner(pay(carol, alice, XRP(1000)), env.seq(carol)), + batch::Sig(unfunded, Reg{carol, alice}), + Ter(tefBAD_AUTH)); env.close(); } @@ -4497,7 +4498,7 @@ class Batch_test : public beast::unit_test::Suite // Submit a normal Payment with tfInnerBatchTxn flag. // preflight1 must reject with temINVALID_INNER_BATCH because // the flag is set but no parentBatchId exists. - env(pay(alice, bob, XRP(1)), txflags(tfInnerBatchTxn), ter(telENV_RPC_FAILED)); + env(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn), Ter(telENV_RPC_FAILED)); env.close(); // Verify via direct apply path (bypassing RPC layer) @@ -4514,7 +4515,7 @@ class Batch_test : public beast::unit_test::Suite obj.setFieldU32(sfFlags, tfInnerBatchTxn); }); - auto const result = xrpl::apply(env.app(), view, stx, tapNONE, j); + auto const result = xrpl::apply(env.app(), view, stx, TapNone, j); // Must NOT be applied — signature was never checked BEAST_EXPECT(!result.applied); return false; @@ -4549,13 +4550,13 @@ class Batch_test : public beast::unit_test::Suite auto const batchFee1 = batch::calcBatchFee(env, 2, 2); auto jt1 = env.jt( batch::outer(alice, aliceSeq, batchFee1, tfOnlyOne), - batch::inner(pay(bob, alice, XRP(100)), bobSeq), - batch::inner(pay(carol, alice, XRP(50)), carolSeq), - batch::sig(bob, carol)); + batch::Inner(pay(bob, alice, XRP(100)), bobSeq), + batch::Inner(pay(carol, alice, XRP(50)), carolSeq), + batch::Sig(bob, carol)); auto const capturedSigners = jt1.jv[sfBatchSigners.jsonName]; - env(jt1, ter(tesSUCCESS)); + env(jt1, Ter(tesSUCCESS)); env.close(); BEAST_EXPECT(env.seq(bob) == bobSeq + 1); BEAST_EXPECT(env.seq(carol) == carolSeq); @@ -4563,11 +4564,11 @@ class Batch_test : public beast::unit_test::Suite auto const batchFee2 = batch::calcBatchFee(env, 2, 2); auto jt2 = env.jtnofill( batch::outer(eve, env.seq(eve), batchFee2, tfOnlyOne), - batch::inner(pay(bob, alice, XRP(100)), bobSeq), - batch::inner(pay(carol, alice, XRP(50)), carolSeq)); + batch::Inner(pay(bob, alice, XRP(100)), bobSeq), + batch::Inner(pay(carol, alice, XRP(50)), carolSeq)); jt2.jv[sfBatchSigners.jsonName] = capturedSigners; - env(jt2.jv, ter(temBAD_SIGNATURE)); + env(jt2.jv, Ter(temBAD_SIGNATURE)); env.close(); BEAST_EXPECT(env.seq(carol) == carolSeq); @@ -4593,13 +4594,13 @@ class Batch_test : public beast::unit_test::Suite auto const batchFee1 = batch::calcBatchFee(env, 2, 2); auto jt1 = env.jt( batch::outer(alice, aliceSeq1, batchFee1, tfOnlyOne), - batch::inner(pay(bob, alice, XRP(500)), bobSeq), - batch::inner(pay(carol, alice, XRP(500)), carolSeq), - batch::sig(bob, carol)); + batch::Inner(pay(bob, alice, XRP(500)), bobSeq), + batch::Inner(pay(carol, alice, XRP(500)), carolSeq), + batch::Sig(bob, carol)); auto const capturedSigners = jt1.jv[sfBatchSigners.jsonName]; - env(jt1, ter(tesSUCCESS)); + env(jt1, Ter(tesSUCCESS)); env.close(); BEAST_EXPECT(env.seq(bob) == bobSeq + 1); BEAST_EXPECT(env.seq(carol) == carolSeq); @@ -4607,11 +4608,11 @@ class Batch_test : public beast::unit_test::Suite auto const batchFee2 = batch::calcBatchFee(env, 2, 2); auto jt2 = env.jtnofill( batch::outer(alice, env.seq(alice), batchFee2, tfOnlyOne), - batch::inner(pay(bob, alice, XRP(500)), bobSeq), - batch::inner(pay(carol, alice, XRP(500)), carolSeq)); + batch::Inner(pay(bob, alice, XRP(500)), bobSeq), + batch::Inner(pay(carol, alice, XRP(500)), carolSeq)); jt2.jv[sfBatchSigners.jsonName] = capturedSigners; - env(jt2.jv, ter(temBAD_SIGNATURE)); + env(jt2.jv, Ter(temBAD_SIGNATURE)); env.close(); BEAST_EXPECT(env.balance(carol) == preCarol); @@ -4640,31 +4641,31 @@ class Batch_test : public beast::unit_test::Suite auto const batchFee = batch::calcBatchFee(env, 3, 2); auto jt1 = env.jt( batch::outer(alice, seq, batchFee, tfAllOrNothing), - batch::inner(pay(alice, bob, XRP(10)), seq + 1), - batch::inner(pay(bob, alice, XRP(5)), env.seq(bob)), - batch::msig(bob, {dave, elsa}), - ter(tesSUCCESS)); + batch::Inner(pay(alice, bob, XRP(10)), seq + 1), + batch::Inner(pay(bob, alice, XRP(5)), env.seq(bob)), + batch::Msig(bob, {dave, elsa}), + Ter(tesSUCCESS)); auto const bobSignerEntry = jt1.jv[sfBatchSigners.jsonName][0u]; - env(jt1, ter(tesSUCCESS)); + env(jt1, Ter(tesSUCCESS)); env.close(); auto const seq2 = env.seq(alice); auto const batchFee2 = batch::calcBatchFee(env, 3, 2); auto jt2 = env.jtnofill( batch::outer(alice, seq2, batchFee2, tfAllOrNothing), - batch::inner(pay(alice, carol, XRP(10)), seq2 + 1), - batch::inner(pay(carol, alice, XRP(5)), env.seq(carol))); + batch::Inner(pay(alice, carol, XRP(10)), seq2 + 1), + batch::Inner(pay(carol, alice, XRP(5)), env.seq(carol))); - Json::Value carolSigner; + json::Value carolSigner; carolSigner[sfBatchSigner.jsonName][jss::Account] = carol.human(); carolSigner[sfBatchSigner.jsonName][jss::SigningPubKey] = ""; carolSigner[sfBatchSigner.jsonName][sfSigners.jsonName] = bobSignerEntry[sfBatchSigner.jsonName][sfSigners.jsonName]; jt2.jv[sfBatchSigners.jsonName][0u] = carolSigner; - env(jt2.jv, ter(temBAD_SIGNATURE)); + env(jt2.jv, Ter(temBAD_SIGNATURE)); env.close(); } } @@ -4691,16 +4692,16 @@ class Batch_test : public beast::unit_test::Suite auto jt = env.jt( batch::outer(alice, seq, batchFee, tfAllOrNothing), - batch::inner(pay(bob, alice, XRP(10)), bobSeq), - batch::inner(pay(carol, alice, XRP(5)), carolSeq), - batch::sig(bob, carol)); + batch::Inner(pay(bob, alice, XRP(10)), bobSeq), + batch::Inner(pay(carol, alice, XRP(5)), carolSeq), + batch::Sig(bob, carol)); auto const s0 = jt.jv[sfBatchSigners.jsonName][0u]; auto const s1 = jt.jv[sfBatchSigners.jsonName][1u]; jt.jv[sfBatchSigners.jsonName][0u] = s1; jt.jv[sfBatchSigners.jsonName][1u] = s0; - env(jt.jv, ter(temBAD_SIGNER)); + env(jt.jv, Ter(temBAD_SIGNER)); env.close(); } @@ -4750,7 +4751,6 @@ public: using namespace test::jtx; auto const sa = testableAmendments(); - testWithFeats(sa - featureBatchV1_1); testWithFeats(sa); } }; diff --git a/src/test/app/LedgerReplay_test.cpp b/src/test/app/LedgerReplay_test.cpp index b6d49b69e5..d24b1d1c09 100644 --- a/src/test/app/LedgerReplay_test.cpp +++ b/src/test/app/LedgerReplay_test.cpp @@ -1,12 +1,15 @@ #include #include +#include #include +#include #include #include #include #include #include #include +#include #include #include @@ -99,7 +102,7 @@ struct LedgerReplay_test : public beast::unit_test::Suite using namespace jtx; - Env env(*this, testable_amendments()); + Env env(*this, testableAmendments()); auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -109,10 +112,9 @@ struct LedgerReplay_test : public beast::unit_test::Suite auto const seq = env.seq(alice); auto const batchFee = batch::calcBatchFee(env, 0, 2); env(batch::outer(alice, seq, batchFee, tfAllOrNothing), - batch::inner(pay(alice, bob, XRP(1)), seq + 1), - batch::inner(pay(alice, bob, XRP(2)), seq + 2), - batch::sig(alice), - ter(tesSUCCESS)); + batch::Inner(pay(alice, bob, XRP(1)), seq + 1), + batch::Inner(pay(alice, bob, XRP(2)), seq + 2), + Ter(tesSUCCESS)); env.close(); LedgerMaster& ledgerMaster = env.app().getLedgerMaster(); @@ -120,7 +122,7 @@ struct LedgerReplay_test : public beast::unit_test::Suite auto const lastClosedParent = ledgerMaster.getLedgerByHash(lastClosed->header().parentHash); auto const replayed = buildLedger( - LedgerReplay(lastClosedParent, lastClosed), tapNONE, env.app(), env.journal); + LedgerReplay(lastClosedParent, lastClosed), TapNone, env.app(), env.journal); BEAST_EXPECT(replayed->header().hash == lastClosed->header().hash); } diff --git a/src/test/ledger/View_test.cpp b/src/test/ledger/View_test.cpp index 1714702c18..264849af0d 100644 --- a/src/test/ledger/View_test.cpp +++ b/src/test/ledger/View_test.cpp @@ -1046,6 +1046,44 @@ class View_test : public beast::unit_test::Suite } } + // Verifies that OpenView::txExists() walks its parent chain so a tx that + // was applied in a previous ledger is still reported as existing from a + // fresh OpenView built over that ledger. This is what lets + // Transactor::checkSeq return tefALREADY for a replay against the closed + // ledger. + void + testTxExistsRecurses() + { + testcase("OpenView::txExists walks parent chain"); + + using namespace jtx; + + Env env(*this); + auto const alice = Account("alice"); + env.fund(XRP(10000), alice); + env.close(); + + // Build the noop tx so we know its txid up-front. + auto const jt = env.jt(noop(alice)); + auto const txid = jt.stx->getTransactionID(); + + // Before submission, no view knows about this tx. + BEAST_EXPECT(!env.current()->txExists(txid)); + BEAST_EXPECT(!env.closed()->txExists(txid)); + + // Submit and close so the tx ends up in the closed ledger but no + // longer in the open view's local txs_. + env(jt.jv); + env.close(); + + BEAST_EXPECT(env.closed()->txExists(txid)); + + // A fresh OpenView built over the closed ledger has an empty txs_ + // map, but must still report the tx as existing via its base. + auto const ov = env.current(); + BEAST_EXPECT(ov->txExists(txid)); + } + void run() override { @@ -1063,6 +1101,7 @@ class View_test : public beast::unit_test::Suite testTransferRate(); testAreCompatible(); testRegressions(); + testTxExistsRecurses(); } }; diff --git a/src/test/rpc/Simulate_test.cpp b/src/test/rpc/Simulate_test.cpp index 1bdd374d84..c67c191164 100644 --- a/src/test/rpc/Simulate_test.cpp +++ b/src/test/rpc/Simulate_test.cpp @@ -421,8 +421,8 @@ class Simulate_test : public beast::unit_test::Suite } { // tfInnerBatchTxn flag on top-level transaction - Json::Value params; - Json::Value tx_json = Json::objectValue; + json::Value params; + json::Value tx_json{json::ValueType::Object}; tx_json[jss::TransactionType] = jss::AccountSet; tx_json[jss::Account] = env.master.human(); tx_json[jss::Flags] = tfInnerBatchTxn; diff --git a/src/xrpld/rpc/handlers/transaction/Simulate.cpp b/src/xrpld/rpc/handlers/transaction/Simulate.cpp index f5c221e54a..70f2af7a53 100644 --- a/src/xrpld/rpc/handlers/transaction/Simulate.cpp +++ b/src/xrpld/rpc/handlers/transaction/Simulate.cpp @@ -359,8 +359,9 @@ doSimulate(RPC::JsonContext& context) // 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."); + return RPC::makeError( + RpcInvalidParams, + "tfInnerBatchTxn flag is not allowed on top-level transactions."); } std::string reason;