From 58c2c82a30322ca3b0fd390620ab41926308da83 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Thu, 5 Jun 2025 14:54:45 +0200 Subject: [PATCH 1/6] fix: Amendment-guard `TokenEscrow` preclaim and expand tests (#5473) This change amendment-guards the preclaim for `TokenEscrow`, as well as expands tests to increase code coverage. --- src/libxrpl/protocol/STAmount.cpp | 7 +- src/test/app/EscrowToken_test.cpp | 227 +++++++++++++++--- src/test/app/Escrow_test.cpp | 6 + src/test/ledger/Invariants_test.cpp | 147 ++++++++++++ src/xrpld/app/tx/detail/Escrow.cpp | 87 ++++--- src/xrpld/app/tx/detail/InvariantCheck.cpp | 41 ++-- .../app/tx/detail/MPTokenIssuanceDestroy.cpp | 2 +- src/xrpld/ledger/detail/View.cpp | 95 ++++---- 8 files changed, 467 insertions(+), 145 deletions(-) diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 845ad6481a0..0c722448853 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -581,7 +581,10 @@ canAdd(STAmount const& a, STAmount const& b) return true; } + // LCOV_EXCL_START + UNREACHABLE("STAmount::canAdd : unexpected STAmount type"); return false; + // LCOV_EXCL_STOP } /** @@ -653,8 +656,10 @@ canSubtract(STAmount const& a, STAmount const& b) return false; return true; } - + // LCOV_EXCL_START + UNREACHABLE("STAmount::canSubtract : unexpected STAmount type"); return false; + // LCOV_EXCL_STOP } void diff --git a/src/test/app/EscrowToken_test.cpp b/src/test/app/EscrowToken_test.cpp index da9610f0c3e..6ba8c48c93c 100644 --- a/src/test/app/EscrowToken_test.cpp +++ b/src/test/app/EscrowToken_test.cpp @@ -21,9 +21,11 @@ #include #include +#include #include #include +#include #include #include @@ -56,27 +58,39 @@ struct EscrowToken_test : public beast::unit_test::suite return 0; } - void - issuerIOUEscrowed( + jtx::PrettyAmount + issuerBalance( jtx::Env& env, jtx::Account const& account, - Currency const& currency, - int const& outstanding, - int const& locked) + Issue const& issue) { Json::Value params; params[jss::account] = account.human(); auto jrr = env.rpc("json", "gateway_balances", to_string(params)); auto const result = jrr[jss::result]; - auto const actualOutstanding = - result[jss::obligations][to_string(currency)]; - BEAST_EXPECT(actualOutstanding == to_string(outstanding)); - if (locked != 0) - { - auto const actualEscrowed = - result[jss::locked][to_string(currency)]; - BEAST_EXPECT(actualEscrowed == to_string(locked)); - } + auto const obligations = + result[jss::obligations][to_string(issue.currency)]; + if (obligations.isNull()) + return {STAmount(issue, 0), account.name()}; + STAmount const amount = amountFromString(issue, obligations.asString()); + return {amount, account.name()}; + } + + jtx::PrettyAmount + issuerEscrowed( + jtx::Env& env, + jtx::Account const& account, + Issue const& issue) + { + Json::Value params; + params[jss::account] = account.human(); + auto jrr = env.rpc("json", "gateway_balances", to_string(params)); + auto const result = jrr[jss::result]; + auto const locked = result[jss::locked][to_string(issue.currency)]; + if (locked.isNull()) + return {STAmount(issue, 0), account.name()}; + STAmount const amount = amountFromString(issue, locked.asString()); + return {amount, account.name()}; } void @@ -136,6 +150,37 @@ struct EscrowToken_test : public beast::unit_test::suite env(escrow::cancel(bob, alice, seq2), finishResult); env.close(); } + + for (bool const withTokenEscrow : {false, true}) + { + auto const amend = + withTokenEscrow ? features : features - featureTokenEscrow; + Env env{*this, amend}; + auto const baseFee = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const gw = Account{"gateway"}; + auto const USD = gw["USD"]; + env.fund(XRP(5000), alice, bob, gw); + env(fset(gw, asfAllowTrustLineLocking)); + env.close(); + env.trust(USD(10'000), alice, bob); + env.close(); + env(pay(gw, alice, USD(5000))); + env(pay(gw, bob, USD(5000))); + env.close(); + + auto const seq1 = env.seq(alice); + env(escrow::finish(bob, alice, seq1), + escrow::condition(escrow::cb1), + escrow::fulfillment(escrow::fb1), + fee(baseFee * 150), + ter(tecNO_TARGET)); + env.close(); + + env(escrow::cancel(bob, alice, seq1), ter(tecNO_TARGET)); + env.close(); + } } void @@ -865,34 +910,76 @@ struct EscrowToken_test : public beast::unit_test::suite env.close(); env.trust(USD(10'000), alice, bob); env.close(); - env(pay(gw, alice, USD(5000))); - env(pay(gw, bob, USD(5000))); + env(pay(gw, alice, USD(5'000))); + env(pay(gw, bob, USD(5'000))); env.close(); + auto const outstandingUSD = USD(10'000); + + // Create & Finish Escrow auto const seq1 = env.seq(alice); - env(escrow::create(alice, bob, USD(1'000)), - escrow::condition(escrow::cb1), - escrow::finish_time(env.now() + 1s), - fee(baseFee * 150), - ter(tesSUCCESS)); - env.close(); - env(escrow::finish(bob, alice, seq1), - escrow::condition(escrow::cb1), - escrow::fulfillment(escrow::fb1), - fee(baseFee * 150), - ter(tesSUCCESS)); - env.close(); + { + auto const preAliceUSD = env.balance(alice, USD); + auto const preBobUSD = env.balance(bob, USD); + env(escrow::create(alice, bob, USD(1'000)), + escrow::condition(escrow::cb1), + escrow::finish_time(env.now() + 1s), + fee(baseFee * 150), + ter(tesSUCCESS)); + env.close(); + BEAST_EXPECT(env.balance(alice, USD) == preAliceUSD - USD(1'000)); + BEAST_EXPECT(env.balance(bob, USD) == preBobUSD); + BEAST_EXPECT( + issuerBalance(env, gw, USD) == outstandingUSD - USD(1'000)); + BEAST_EXPECT(issuerEscrowed(env, gw, USD) == USD(1'000)); + } + { + auto const preAliceUSD = env.balance(alice, USD); + auto const preBobUSD = env.balance(bob, USD); + env(escrow::finish(bob, alice, seq1), + escrow::condition(escrow::cb1), + escrow::fulfillment(escrow::fb1), + fee(baseFee * 150), + ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT(env.balance(alice, USD) == preAliceUSD); + BEAST_EXPECT(env.balance(bob, USD) == preBobUSD + USD(1'000)); + BEAST_EXPECT(issuerBalance(env, gw, USD) == outstandingUSD); + BEAST_EXPECT(issuerEscrowed(env, gw, USD) == USD(0)); + } + + // Create & Cancel Escrow auto const seq2 = env.seq(alice); - env(escrow::create(alice, bob, USD(1'000)), - escrow::condition(escrow::cb2), - escrow::finish_time(env.now() + 1s), - escrow::cancel_time(env.now() + 2s), - fee(baseFee * 150), - ter(tesSUCCESS)); - env.close(); - env(escrow::cancel(bob, alice, seq2), ter(tesSUCCESS)); - env.close(); + { + auto const preAliceUSD = env.balance(alice, USD); + auto const preBobUSD = env.balance(bob, USD); + env(escrow::create(alice, bob, USD(1'000)), + escrow::condition(escrow::cb2), + escrow::finish_time(env.now() + 1s), + escrow::cancel_time(env.now() + 2s), + fee(baseFee * 150), + ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT(env.balance(alice, USD) == preAliceUSD - USD(1'000)); + BEAST_EXPECT(env.balance(bob, USD) == preBobUSD); + BEAST_EXPECT( + issuerBalance(env, gw, USD) == outstandingUSD - USD(1'000)); + BEAST_EXPECT(issuerEscrowed(env, gw, USD) == USD(1'000)); + } + { + auto const preAliceUSD = env.balance(alice, USD); + auto const preBobUSD = env.balance(bob, USD); + env(escrow::cancel(bob, alice, seq2), ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT(env.balance(alice, USD) == preAliceUSD + USD(1'000)); + BEAST_EXPECT(env.balance(bob, USD) == preBobUSD); + BEAST_EXPECT(issuerBalance(env, gw, USD) == outstandingUSD); + BEAST_EXPECT(issuerEscrowed(env, gw, USD) == USD(0)); + } } void @@ -2430,7 +2517,6 @@ struct EscrowToken_test : public beast::unit_test::suite mptGw.authorize({.account = alice}); mptGw.authorize({.account = bob}); auto const MPT = mptGw["MPT"]; - env(pay(gw, alice, MPT(10))); env(pay(gw, bob, MPT(10))); env.close(); @@ -2521,6 +2607,39 @@ struct EscrowToken_test : public beast::unit_test::suite env.close(); } + // tecOBJECT_NOT_FOUND: MPT issuance does not exist + { + Env env{*this, features}; + auto const baseFee = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + env.fund(XRP(10'000), alice, bob); + env.close(); + + auto const seq1 = env.seq(alice); + env.app().openLedger().modify( + [&](OpenView& view, beast::Journal j) { + Sandbox sb(&view, tapNONE); + auto sleNew = + std::make_shared(keylet::escrow(alice, seq1)); + MPTIssue const mpt{ + MPTIssue{makeMptID(1, AccountID(0x4985601))}}; + STAmount amt(mpt, 10); + sleNew->setAccountID(sfDestination, bob); + sleNew->setFieldAmount(sfAmount, amt); + sb.insert(sleNew); + sb.apply(view); + return true; + }); + + env(escrow::finish(bob, alice, seq1), + escrow::condition(escrow::cb1), + escrow::fulfillment(escrow::fb1), + fee(baseFee * 150), + ter(tecOBJECT_NOT_FOUND)); + env.close(); + } + // tecLOCKED: issuer has locked the dest { Env env{*this, features}; @@ -2726,6 +2845,36 @@ struct EscrowToken_test : public beast::unit_test::suite env(escrow::cancel(bob, alice, seq1), ter(tecNO_AUTH)); env.close(); } + + // tecOBJECT_NOT_FOUND: MPT issuance does not exist + { + Env env{*this, features}; + auto const baseFee = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + env.fund(XRP(10'000), alice, bob); + + auto const seq1 = env.seq(alice); + env.app().openLedger().modify( + [&](OpenView& view, beast::Journal j) { + Sandbox sb(&view, tapNONE); + auto sleNew = + std::make_shared(keylet::escrow(alice, seq1)); + MPTIssue const mpt{ + MPTIssue{makeMptID(1, AccountID(0x4985601))}}; + STAmount amt(mpt, 10); + sleNew->setAccountID(sfDestination, bob); + sleNew->setFieldAmount(sfAmount, amt); + sb.insert(sleNew); + sb.apply(view); + return true; + }); + + env(escrow::cancel(bob, alice, seq1), + fee(baseFee), + ter(tecOBJECT_NOT_FOUND)); + env.close(); + } } void @@ -3603,12 +3752,14 @@ struct EscrowToken_test : public beast::unit_test::suite fee(baseFee * 150)); env.close(); + env(pay(alice, gw, MPT(10'000)), ter(tecPATH_PARTIAL)); env(pay(alice, gw, MPT(9'990))); env(pay(bob, gw, MPT(10'000))); BEAST_EXPECT(env.balance(alice, MPT) == MPT(0)); BEAST_EXPECT(mptEscrowed(env, alice, MPT) == 10); BEAST_EXPECT(env.balance(bob, MPT) == MPT(0)); BEAST_EXPECT(mptEscrowed(env, bob, MPT) == 0); + BEAST_EXPECT(env.balance(gw, MPT) == MPT(10)); mptGw.authorize({.account = bob, .flags = tfMPTUnauthorize}); mptGw.destroy( {.id = mptGw.issuanceID(), diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index aa86ad338e3..21ef70a86e3 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -369,6 +369,12 @@ struct Escrow_test : public beast::unit_test::suite env.fund(XRP(5000), "alice", "bob", "gw"); env.close(); + // temINVALID_FLAG + env(escrow::create("alice", "bob", XRP(1000)), + escrow::finish_time(env.now() + 5s), + txflags(tfPassive), + ter(temINVALID_FLAG)); + // Finish time is in the past env(escrow::create("alice", "bob", XRP(1000)), escrow::finish_time(env.now() - 5s), diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index ebd2235cc96..6178b413d52 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -762,6 +762,153 @@ class Invariants_test : public beast::unit_test::suite ac.view().insert(sleNew); return true; }); + + // IOU < 0 + doInvariantCheck( + {{"escrow specifies invalid amount"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + // escrow with too-little iou + auto const sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + auto sleNew = std::make_shared( + keylet::escrow(A1, (*sle)[sfSequence] + 2)); + + Issue const usd{ + Currency(0x5553440000000000), AccountID(0x4985601)}; + STAmount amt(usd, -1); + sleNew->setFieldAmount(sfAmount, amt); + ac.view().insert(sleNew); + return true; + }); + + // IOU bad currency + doInvariantCheck( + {{"escrow specifies invalid amount"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + // escrow with bad iou currency + auto const sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + auto sleNew = std::make_shared( + keylet::escrow(A1, (*sle)[sfSequence] + 2)); + + Issue const bad{badCurrency(), AccountID(0x4985601)}; + STAmount amt(bad, 1); + sleNew->setFieldAmount(sfAmount, amt); + ac.view().insert(sleNew); + return true; + }); + + // MPT < 0 + doInvariantCheck( + {{"escrow specifies invalid amount"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + // escrow with too-little mpt + auto const sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + auto sleNew = std::make_shared( + keylet::escrow(A1, (*sle)[sfSequence] + 2)); + + MPTIssue const mpt{ + MPTIssue{makeMptID(1, AccountID(0x4985601))}}; + STAmount amt(mpt, -1); + sleNew->setFieldAmount(sfAmount, amt); + ac.view().insert(sleNew); + return true; + }); + + // MPT OutstandingAmount < 0 + doInvariantCheck( + {{"escrow specifies invalid amount"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + // mpissuance outstanding is negative + auto const sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + + MPTIssue const mpt{ + MPTIssue{makeMptID(1, AccountID(0x4985601))}}; + auto sleNew = + std::make_shared(keylet::mptIssuance(mpt.getMptID())); + sleNew->setFieldU64(sfOutstandingAmount, -1); + ac.view().insert(sleNew); + return true; + }); + + // MPT LockedAmount < 0 + doInvariantCheck( + {{"escrow specifies invalid amount"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + // mpissuance locked is less than locked + auto const sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + + MPTIssue const mpt{ + MPTIssue{makeMptID(1, AccountID(0x4985601))}}; + auto sleNew = + std::make_shared(keylet::mptIssuance(mpt.getMptID())); + sleNew->setFieldU64(sfLockedAmount, -1); + ac.view().insert(sleNew); + return true; + }); + + // MPT OutstandingAmount < LockedAmount + doInvariantCheck( + {{"escrow specifies invalid amount"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + // mpissuance outstanding is less than locked + auto const sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + + MPTIssue const mpt{ + MPTIssue{makeMptID(1, AccountID(0x4985601))}}; + auto sleNew = + std::make_shared(keylet::mptIssuance(mpt.getMptID())); + sleNew->setFieldU64(sfOutstandingAmount, 1); + sleNew->setFieldU64(sfLockedAmount, 10); + ac.view().insert(sleNew); + return true; + }); + + // MPT MPTAmount < 0 + doInvariantCheck( + {{"escrow specifies invalid amount"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + // mptoken amount is negative + auto const sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + + MPTIssue const mpt{ + MPTIssue{makeMptID(1, AccountID(0x4985601))}}; + auto sleNew = + std::make_shared(keylet::mptoken(mpt.getMptID(), A1)); + sleNew->setFieldU64(sfMPTAmount, -1); + ac.view().insert(sleNew); + return true; + }); + + // MPT LockedAmount < 0 + doInvariantCheck( + {{"escrow specifies invalid amount"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { + // mptoken locked amount is negative + auto const sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + + MPTIssue const mpt{ + MPTIssue{makeMptID(1, AccountID(0x4985601))}}; + auto sleNew = + std::make_shared(keylet::mptoken(mpt.getMptID(), A1)); + sleNew->setFieldU64(sfLockedAmount, -1); + ac.view().insert(sleNew); + return true; + }); } void diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index f7840650f7d..75080da9a5c 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -595,7 +595,7 @@ EscrowCreate::doApply() }, amount.asset().value()); !isTesSuccess(ret)) - return ret; + return ret; // LCOV_EXCL_LINE } // increment owner count @@ -766,26 +766,26 @@ EscrowFinish::preclaim(PreclaimContext const& ctx) return err; } - auto const k = keylet::escrow(ctx.tx[sfOwner], ctx.tx[sfOfferSequence]); - auto const slep = ctx.view.read(k); - if (!slep) - return tecNO_TARGET; - - AccountID const dest = (*slep)[sfDestination]; - STAmount const amount = (*slep)[sfAmount]; - - if (!isXRP(amount)) + if (ctx.view.rules().enabled(featureTokenEscrow)) { - if (!ctx.view.rules().enabled(featureTokenEscrow)) - return temDISABLED; // LCOV_EXCL_LINE + auto const k = keylet::escrow(ctx.tx[sfOwner], ctx.tx[sfOfferSequence]); + auto const slep = ctx.view.read(k); + if (!slep) + return tecNO_TARGET; - if (auto const ret = std::visit( - [&](T const&) { - return escrowFinishPreclaimHelper(ctx, dest, amount); - }, - amount.asset().value()); - !isTesSuccess(ret)) - return ret; + AccountID const dest = (*slep)[sfDestination]; + STAmount const amount = (*slep)[sfAmount]; + + if (!isXRP(amount)) + { + if (auto const ret = std::visit( + [&](T const&) { + return escrowFinishPreclaimHelper(ctx, dest, amount); + }, + amount.asset().value()); + !isTesSuccess(ret)) + return ret; + } } return tesSUCCESS; } @@ -1015,7 +1015,12 @@ EscrowFinish::doApply() auto const k = keylet::escrow(ctx_.tx[sfOwner], ctx_.tx[sfOfferSequence]); auto const slep = ctx_.view().peek(k); if (!slep) + { + if (ctx_.view().rules().enabled(featureTokenEscrow)) + return tecINTERNAL; // LCOV_EXCL_LINE + return tecNO_TARGET; + } // If a cancel time is present, a finish operation should only succeed prior // to that time. fix1571 corrects a logic error in the check that would make @@ -1245,7 +1250,7 @@ escrowCancelPreclaimHelper( keylet::mptIssuance(amount.get().getMptID()); auto const sleIssuance = ctx.view.read(issuanceKey); if (!sleIssuance) - return tecOBJECT_NOT_FOUND; // LCOV_EXCL_LINE + return tecOBJECT_NOT_FOUND; // If the issuer has requireAuth set, check if the account is // authorized @@ -1261,26 +1266,27 @@ escrowCancelPreclaimHelper( TER EscrowCancel::preclaim(PreclaimContext const& ctx) { - auto const k = keylet::escrow(ctx.tx[sfOwner], ctx.tx[sfOfferSequence]); - auto const slep = ctx.view.read(k); - if (!slep) - return tecNO_TARGET; - - AccountID const account = (*slep)[sfAccount]; - STAmount const amount = (*slep)[sfAmount]; - - if (!isXRP(amount)) + if (ctx.view.rules().enabled(featureTokenEscrow)) { - if (!ctx.view.rules().enabled(featureTokenEscrow)) - return temDISABLED; // LCOV_EXCL_LINE + auto const k = keylet::escrow(ctx.tx[sfOwner], ctx.tx[sfOfferSequence]); + auto const slep = ctx.view.read(k); + if (!slep) + return tecNO_TARGET; - if (auto const ret = std::visit( - [&](T const&) { - return escrowCancelPreclaimHelper(ctx, account, amount); - }, - amount.asset().value()); - !isTesSuccess(ret)) - return ret; + AccountID const account = (*slep)[sfAccount]; + STAmount const amount = (*slep)[sfAmount]; + + if (!isXRP(amount)) + { + if (auto const ret = std::visit( + [&](T const&) { + return escrowCancelPreclaimHelper( + ctx, account, amount); + }, + amount.asset().value()); + !isTesSuccess(ret)) + return ret; + } } return tesSUCCESS; } @@ -1291,7 +1297,12 @@ EscrowCancel::doApply() auto const k = keylet::escrow(ctx_.tx[sfOwner], ctx_.tx[sfOfferSequence]); auto const slep = ctx_.view().peek(k); if (!slep) + { + if (ctx_.view().rules().enabled(featureTokenEscrow)) + return tecINTERNAL; // LCOV_EXCL_LINE + return tecNO_TARGET; + } if (ctx_.view().rules().enabled(fix1571)) { diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 31b8fe3cc1a..d93378d3cde 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -271,26 +271,6 @@ NoZeroEscrow::visitEntry( std::shared_ptr const& after) { auto isBad = [](STAmount const& amount) { - // IOU case - if (amount.holds()) - { - if (amount <= beast::zero) - return true; - - if (badCurrency() == amount.getCurrency()) - return true; - } - - // MPT case - if (amount.holds()) - { - if (amount <= beast::zero) - return true; - - if (amount.mpt() > MPTAmount{maxMPTokenAmount}) - return true; - } - // XRP case if (amount.native()) { @@ -300,7 +280,28 @@ NoZeroEscrow::visitEntry( if (amount.xrp() >= INITIAL_XRP) return true; } + else + { + // IOU case + if (amount.holds()) + { + if (amount <= beast::zero) + return true; + + if (badCurrency() == amount.getCurrency()) + return true; + } + + // MPT case + if (amount.holds()) + { + if (amount <= beast::zero) + return true; + if (amount.mpt() > MPTAmount{maxMPTokenAmount}) + return true; // LCOV_EXCL_LINE + } + } return false; }; diff --git a/src/xrpld/app/tx/detail/MPTokenIssuanceDestroy.cpp b/src/xrpld/app/tx/detail/MPTokenIssuanceDestroy.cpp index a2e1f33b948..e2b87dbd791 100644 --- a/src/xrpld/app/tx/detail/MPTokenIssuanceDestroy.cpp +++ b/src/xrpld/app/tx/detail/MPTokenIssuanceDestroy.cpp @@ -59,7 +59,7 @@ MPTokenIssuanceDestroy::preclaim(PreclaimContext const& ctx) return tecHAS_OBLIGATIONS; if ((*sleMPT)[~sfLockedAmount].value_or(0) != 0) - return tecHAS_OBLIGATIONS; + return tecHAS_OBLIGATIONS; // LCOV_EXCL_LINE return tesSUCCESS; } diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index d3161dccae7..cb958190145 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -2747,18 +2747,18 @@ rippleLockEscrowMPT( auto const mptID = keylet::mptIssuance(mptIssue.getMptID()); auto sleIssuance = view.peek(mptID); if (!sleIssuance) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleLockEscrowMPT: MPT issuance not found for " << mptIssue.getMptID(); - return tecOBJECT_NOT_FOUND; // LCOV_EXCL_LINE - } + return tecOBJECT_NOT_FOUND; + } // LCOV_EXCL_STOP if (amount.getIssuer() == sender) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleLockEscrowMPT: sender is the issuer, cannot lock MPTs."; - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP // 1. Decrease the MPT Holder MPTAmount // 2. Increase the MPT Holder EscrowedAmount @@ -2766,23 +2766,23 @@ rippleLockEscrowMPT( auto const mptokenID = keylet::mptoken(mptID.key, sender); auto sle = view.peek(mptokenID); if (!sle) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleLockEscrowMPT: MPToken not found for " << sender; - return tecOBJECT_NOT_FOUND; // LCOV_EXCL_LINE - } + return tecOBJECT_NOT_FOUND; + } // LCOV_EXCL_STOP auto const amt = sle->getFieldU64(sfMPTAmount); auto const pay = amount.mpt().value(); // Underflow check for subtraction if (!canSubtract(STAmount(mptIssue, amt), STAmount(mptIssue, pay))) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleLockEscrowMPT: insufficient MPTAmount for " << to_string(sender) << ": " << amt << " < " << pay; - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP (*sle)[sfMPTAmount] = amt - pay; @@ -2790,12 +2790,12 @@ rippleLockEscrowMPT( uint64_t const locked = (*sle)[~sfLockedAmount].value_or(0); if (!canAdd(STAmount(mptIssue, locked), STAmount(mptIssue, pay))) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleLockEscrowMPT: overflow on locked amount for " << to_string(sender) << ": " << locked << " + " << pay; - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP if (sle->isFieldPresent(sfLockedAmount)) (*sle)[sfLockedAmount] += pay; @@ -2815,13 +2815,13 @@ rippleLockEscrowMPT( // Overflow check for addition if (!canAdd( STAmount(mptIssue, issuanceEscrowed), STAmount(mptIssue, pay))) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleLockEscrowMPT: overflow on issuance " "locked amount for " << mptIssue.getMptID() << ": " << issuanceEscrowed << " + " << pay; - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP if (sleIssuance->isFieldPresent(sfLockedAmount)) (*sleIssuance)[sfLockedAmount] += pay; @@ -2846,21 +2846,21 @@ rippleUnlockEscrowMPT( auto const mptID = keylet::mptIssuance(mptIssue.getMptID()); auto sleIssuance = view.peek(mptID); if (!sleIssuance) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: MPT issuance not found for " << mptIssue.getMptID(); - return tecOBJECT_NOT_FOUND; // LCOV_EXCL_LINE - } + return tecOBJECT_NOT_FOUND; + } // LCOV_EXCL_STOP // Decrease the Issuance EscrowedAmount { if (!sleIssuance->isFieldPresent(sfLockedAmount)) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: no locked amount in issuance for " << mptIssue.getMptID(); - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP auto const locked = sleIssuance->getFieldU64(sfLockedAmount); auto const redeem = amount.mpt().value(); @@ -2868,12 +2868,12 @@ rippleUnlockEscrowMPT( // Underflow check for subtraction if (!canSubtract( STAmount(mptIssue, locked), STAmount(mptIssue, redeem))) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: insufficient locked amount for " << mptIssue.getMptID() << ": " << locked << " < " << redeem; - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP auto const newLocked = locked - redeem; if (newLocked == 0) @@ -2889,23 +2889,23 @@ rippleUnlockEscrowMPT( auto const mptokenID = keylet::mptoken(mptID.key, receiver); auto sle = view.peek(mptokenID); if (!sle) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: MPToken not found for " << receiver; return tecOBJECT_NOT_FOUND; // LCOV_EXCL_LINE - } + } // LCOV_EXCL_STOP auto current = sle->getFieldU64(sfMPTAmount); auto delta = amount.mpt().value(); // Overflow check for addition if (!canAdd(STAmount(mptIssue, current), STAmount(mptIssue, delta))) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: overflow on MPTAmount for " << to_string(receiver) << ": " << current << " + " << delta; - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP (*sle)[sfMPTAmount] += delta; view.update(sle); @@ -2919,55 +2919,56 @@ rippleUnlockEscrowMPT( // Underflow check for subtraction if (!canSubtract( STAmount(mptIssue, outstanding), STAmount(mptIssue, redeem))) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: insufficient outstanding amount for " << mptIssue.getMptID() << ": " << outstanding << " < " << redeem; - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP sleIssuance->setFieldU64(sfOutstandingAmount, outstanding - redeem); view.update(sleIssuance); } if (issuer == sender) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: sender is the issuer, " "cannot unlock MPTs."; - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP else { // Decrease the MPT Holder EscrowedAmount auto const mptokenID = keylet::mptoken(mptID.key, sender); auto sle = view.peek(mptokenID); if (!sle) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: MPToken not found for " << sender; - return tecOBJECT_NOT_FOUND; // LCOV_EXCL_LINE - } + return tecOBJECT_NOT_FOUND; + } // LCOV_EXCL_STOP if (!sle->isFieldPresent(sfLockedAmount)) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: no locked amount in MPToken for " << to_string(sender); - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP auto const locked = sle->getFieldU64(sfLockedAmount); auto const delta = amount.mpt().value(); // Underflow check for subtraction + // LCOV_EXCL_START if (!canSubtract(STAmount(mptIssue, locked), STAmount(mptIssue, delta))) - { + { // LCOV_EXCL_START JLOG(j.error()) << "rippleUnlockEscrowMPT: insufficient locked amount for " << to_string(sender) << ": " << locked << " < " << delta; - return tecINTERNAL; // LCOV_EXCL_LINE - } + return tecINTERNAL; + } // LCOV_EXCL_STOP auto const newLocked = locked - delta; if (newLocked == 0) From 8bf4a5cbff4392fcca9990d931ba528085b9a22a Mon Sep 17 00:00:00 2001 From: Vlad <129996061+vvysokikh1@users.noreply.github.com> Date: Thu, 5 Jun 2025 14:37:30 +0100 Subject: [PATCH 2/6] chore: Remove external project build cores division (#5475) The CMake statements that make it seem as if the number of cores used to build external project dependencies is halved don't actually do anything. This change removes these statements. --- cmake/RippledSanity.cmake | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/cmake/RippledSanity.cmake b/cmake/RippledSanity.cmake index 3dd5fb782fd..28ce8541352 100644 --- a/cmake/RippledSanity.cmake +++ b/cmake/RippledSanity.cmake @@ -2,16 +2,6 @@ convenience variables and sanity checks #]===================================================================] -include(ProcessorCount) - -if (NOT ep_procs) - ProcessorCount(ep_procs) - if (ep_procs GREATER 1) - # never use more than half of cores for EP builds - math (EXPR ep_procs "${ep_procs} / 2") - message (STATUS "Using ${ep_procs} cores for ExternalProject builds.") - endif () -endif () get_property(is_multiconfig GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG) set (CMAKE_CONFIGURATION_TYPES "Debug;Release" CACHE STRING "" FORCE) From d494bf45b2ba646a7283746f94306b08eaf3f08c Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 6 Jun 2025 12:01:02 -0400 Subject: [PATCH 3/6] refactor: Collapse some split log messages into one (#5347) Multi-line log messages are hard to work with. Writing these handful of related messages as one message should make the log a tiny bit easier to manage. --- src/xrpld/app/ledger/detail/InboundLedger.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index c1eed3a9f32..eafa939506a 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -502,15 +502,17 @@ InboundLedger::trigger(std::shared_ptr const& peer, TriggerReason reason) if (auto stream = journal_.debug()) { - stream << "Trigger acquiring ledger " << hash_; + std::stringstream ss; + ss << "Trigger acquiring ledger " << hash_; if (peer) - stream << " from " << peer; + ss << " from " << peer; if (complete_ || failed_) - stream << "complete=" << complete_ << " failed=" << failed_; + ss << " complete=" << complete_ << " failed=" << failed_; else - stream << "header=" << mHaveHeader << " tx=" << mHaveTransactions - << " as=" << mHaveState; + ss << " header=" << mHaveHeader << " tx=" << mHaveTransactions + << " as=" << mHaveState; + stream << ss.str(); } if (!mHaveHeader) From 35a40a8e6236110bfbaa96d9a891728b4bae8a4e Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Tue, 10 Jun 2025 14:47:27 +0800 Subject: [PATCH 4/6] fix: Improve multi-sign usage of `simulate` (#5479) This change allows users to submit simulate requests from a multi-sign account without needing to specify the accounts that are doing the multi-signing, and fixes an error with simulate that allowed double-"signed" (both single-sign and multi-sign public keys are provided) transactions. --- src/test/rpc/Simulate_test.cpp | 92 ++++++++++++++++++++++++-- src/xrpld/app/tx/detail/Transactor.cpp | 38 +++++++---- 2 files changed, 112 insertions(+), 18 deletions(-) diff --git a/src/test/rpc/Simulate_test.cpp b/src/test/rpc/Simulate_test.cpp index a4360ccc8b3..5b3c0d23721 100644 --- a/src/test/rpc/Simulate_test.cpp +++ b/src/test/rpc/Simulate_test.cpp @@ -720,7 +720,11 @@ class Simulate_test : public beast::unit_test::suite Json::Value const& tx) { auto result = resp[jss::result]; checkBasicReturnValidity( - result, tx, env.seq(alice), env.current()->fees().base * 2); + result, + tx, + env.seq(alice), + tx.isMember(jss::Signers) ? env.current()->fees().base * 2 + : env.current()->fees().base); BEAST_EXPECT(result[jss::engine_result] == "tesSUCCESS"); BEAST_EXPECT(result[jss::engine_result_code] == 0); @@ -762,6 +766,10 @@ class Simulate_test : public beast::unit_test::suite tx[jss::Account] = alice.human(); tx[jss::TransactionType] = jss::AccountSet; tx[sfDomain] = newDomain; + + // test with autofill + testTx(env, tx, validateOutput, false); + tx[sfSigners] = Json::arrayValue; { Json::Value signer; @@ -771,7 +779,7 @@ class Simulate_test : public beast::unit_test::suite tx[sfSigners].append(signerOuter); } - // test with autofill + // test with just signer accounts testTx(env, tx, validateOutput, false); tx[sfSigningPubKey] = ""; @@ -780,8 +788,7 @@ class Simulate_test : public beast::unit_test::suite // transaction requires a non-base fee tx[sfFee] = (env.current()->fees().base * 2).jsonClipped().asString(); - tx[sfSigners][0u][sfSigner][jss::SigningPubKey] = - strHex(becky.pk().slice()); + tx[sfSigners][0u][sfSigner][jss::SigningPubKey] = ""; tx[sfSigners][0u][sfSigner][jss::TxnSignature] = ""; // test without autofill @@ -830,11 +837,12 @@ class Simulate_test : public beast::unit_test::suite tx[jss::Account] = env.master.human(); tx[jss::TransactionType] = jss::AccountSet; tx[sfDomain] = newDomain; + // master key is disabled, so this is invalid + tx[jss::SigningPubKey] = strHex(env.master.pk().slice()); // test with autofill testTx(env, tx, testSimulation); - tx[sfSigningPubKey] = ""; tx[sfTxnSignature] = ""; tx[sfSequence] = env.seq(env.master); tx[sfFee] = env.current()->fees().base.jsonClipped().asString(); @@ -844,6 +852,79 @@ class Simulate_test : public beast::unit_test::suite } } + void + testInvalidSingleAndMultiSigningTransaction() + { + testcase( + "Transaction with both single-signing SigningPubKey and " + "multi-signing Signers"); + + using namespace jtx; + Env env(*this); + static auto const newDomain = "123ABC"; + Account const alice("alice"); + Account const becky("becky"); + Account const carol("carol"); + env.fund(XRP(10000), alice); + env.close(); + + // set up valid multisign + env(signers(alice, 1, {{becky, 1}, {carol, 1}})); + env.close(); + + { + std::function const& + testSimulation = [&](Json::Value const& resp, + Json::Value const& tx) { + auto result = resp[jss::result]; + checkBasicReturnValidity( + result, + tx, + env.seq(env.master), + env.current()->fees().base * 2); + + BEAST_EXPECT(result[jss::engine_result] == "temINVALID"); + BEAST_EXPECT(result[jss::engine_result_code] == -277); + BEAST_EXPECT( + result[jss::engine_result_message] == + "The transaction is ill-formed."); + + BEAST_EXPECT( + !result.isMember(jss::meta) && + !result.isMember(jss::meta_blob)); + }; + + Json::Value tx; + + tx[jss::Account] = env.master.human(); + tx[jss::TransactionType] = jss::AccountSet; + tx[sfDomain] = newDomain; + // master key is disabled, so this is invalid + tx[jss::SigningPubKey] = strHex(env.master.pk().slice()); + tx[sfSigners] = Json::arrayValue; + { + Json::Value signer; + signer[jss::Account] = becky.human(); + Json::Value signerOuter; + signerOuter[sfSigner] = signer; + tx[sfSigners].append(signerOuter); + } + + // test with autofill + testTx(env, tx, testSimulation, false); + + tx[sfTxnSignature] = ""; + tx[sfSequence] = env.seq(env.master); + tx[sfFee] = env.current()->fees().base.jsonClipped().asString(); + tx[sfSigners][0u][sfSigner][jss::SigningPubKey] = + strHex(becky.pk().slice()); + tx[sfSigners][0u][sfSigner][jss::TxnSignature] = ""; + + // test without autofill + testTx(env, tx, testSimulation); + } + } + void testMultisignedBadPubKey() { @@ -1117,6 +1198,7 @@ class Simulate_test : public beast::unit_test::suite testTransactionTecFailure(); testSuccessfulTransactionMultisigned(); testTransactionSigningFailure(); + testInvalidSingleAndMultiSigningTransaction(); testMultisignedBadPubKey(); testDeleteExpiredCredentials(); testSuccessfulTransactionNetworkID(); diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 5f4cba5cf88..0db04848426 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -184,6 +184,12 @@ preflight2(PreflightContext const& ctx) return temINVALID; // LCOV_EXCL_LINE } } + + if (!ctx.tx.getSigningPubKey().empty()) + { + // trying to single-sign _and_ multi-sign a transaction + return temINVALID; + } return tesSUCCESS; } @@ -297,9 +303,9 @@ Transactor::checkFee(PreclaimContext const& ctx, XRPAmount baseFee) if (balance < feePaid) { - JLOG(ctx.j.trace()) << "Insufficient balance:" - << " balance=" << to_string(balance) - << " paid=" << to_string(feePaid); + JLOG(ctx.j.trace()) + << "Insufficient balance:" << " balance=" << to_string(balance) + << " paid=" << to_string(feePaid); if ((balance > beast::zero) && !ctx.view.open()) { @@ -571,13 +577,13 @@ Transactor::apply() NotTEC Transactor::checkSign(PreclaimContext const& ctx) { + auto const pkSigner = ctx.tx.getSigningPubKey(); // Ignore signature check on batch inner transactions if (ctx.tx.isFlag(tfInnerBatchTxn) && ctx.view.rules().enabled(featureBatch)) { // Defensive Check: These values are also checked in Batch::preflight - if (ctx.tx.isFieldPresent(sfTxnSignature) || - !ctx.tx.getSigningPubKey().empty() || + if (ctx.tx.isFieldPresent(sfTxnSignature) || !pkSigner.empty() || ctx.tx.isFieldPresent(sfSigners)) { return temINVALID_FLAG; // LCOV_EXCL_LINE @@ -585,25 +591,30 @@ Transactor::checkSign(PreclaimContext const& ctx) return tesSUCCESS; } + if ((ctx.flags & tapDRY_RUN) && pkSigner.empty() && + !ctx.tx.isFieldPresent(sfSigners)) + { + // simulate: skip signature validation when neither SigningPubKey nor + // Signers are provided + return tesSUCCESS; + } + auto const idAccount = ctx.tx[~sfDelegate].value_or(ctx.tx[sfAccount]); // If the pk is empty and not simulate or simulate and signers, // then we must be multi-signing. - if ((ctx.flags & tapDRY_RUN && ctx.tx.isFieldPresent(sfSigners)) || - (!(ctx.flags & tapDRY_RUN) && ctx.tx.getSigningPubKey().empty())) + if (ctx.tx.isFieldPresent(sfSigners)) { STArray const& txSigners(ctx.tx.getFieldArray(sfSigners)); return checkMultiSign(ctx.view, idAccount, txSigners, ctx.flags, ctx.j); } // Check Single Sign - auto const pkSigner = ctx.tx.getSigningPubKey(); - // This ternary is only needed to handle `simulate` XRPL_ASSERT( - (ctx.flags & tapDRY_RUN) || !pkSigner.empty(), + !pkSigner.empty(), "ripple::Transactor::checkSingleSign : non-empty signer or simulation"); - if (!(ctx.flags & tapDRY_RUN) && !publicKeyType(makeSlice(pkSigner))) + if (!publicKeyType(makeSlice(pkSigner))) { JLOG(ctx.j.trace()) << "checkSingleSign: signing public key type is unknown"; @@ -798,14 +809,15 @@ Transactor::checkMultiSign( // public key. auto const spk = txSigner.getFieldVL(sfSigningPubKey); - if (!(flags & tapDRY_RUN) && !publicKeyType(makeSlice(spk))) + // spk being non-empty in non-simulate is checked in + // STTx::checkMultiSign + if (!spk.empty() && !publicKeyType(makeSlice(spk))) { JLOG(j.trace()) << "checkMultiSign: signing public key type is unknown"; return tefBAD_SIGNATURE; } - // This ternary is only needed to handle `simulate` XRPL_ASSERT( (flags & tapDRY_RUN) || !spk.empty(), "ripple::Transactor::checkMultiSign : non-empty signer or " From ea17abb92a261da2cbf2bbcdbd6231a29f1fa9db Mon Sep 17 00:00:00 2001 From: yinyiqian1 Date: Wed, 11 Jun 2025 01:21:24 -0400 Subject: [PATCH 5/6] fix: Ensure delegate tests do not silently fail with batch (#5476) The tests that ensure `tfInnerBatchTxn` won't block delegated transactions silently fail in `Delegate_test.cpp`. This change removes these cases from that file and adds them to `Batch_test.cpp` instead where they do not silently fail, because there the batch delegate results are explicitly checked. Moving them to that file further avoids refactoring many helper functions. --- src/test/app/Batch_test.cpp | 141 +++++++++++++++++++++++++++++++++ src/test/app/Delegate_test.cpp | 124 ----------------------------- 2 files changed, 141 insertions(+), 124 deletions(-) diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 6874a42c9e2..0866bca2ef0 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -3765,6 +3765,8 @@ class Batch_test : public beast::unit_test::suite } // delegated non atomic inner (AccountSet) + // this also makes sure tfInnerBatchTxn won't block delegated AccountSet + // with granular permission { test::jtx::Env env{*this, envconfig()}; @@ -3810,6 +3812,145 @@ class Batch_test : public beast::unit_test::suite BEAST_EXPECT(env.balance(alice) == preAlice - XRP(2) - batchFee); BEAST_EXPECT(env.balance(bob) == preBob + XRP(2)); } + + // delegated non atomic inner (MPTokenIssuanceSet) + // this also makes sure tfInnerBatchTxn won't block delegated + // MPTokenIssuanceSet with granular permission + { + test::jtx::Env env{*this, envconfig()}; + Account alice{"alice"}; + Account bob{"bob"}; + env.fund(XRP(100000), alice, bob); + env.close(); + + auto const mptID = makeMptID(env.seq(alice), alice); + MPTTester mpt(env, alice, {.fund = false}); + env.close(); + mpt.create({.flags = tfMPTCanLock}); + env.close(); + + // alice gives granular permission to bob of MPTokenIssuanceLock + env(delegate::set( + alice, bob, {"MPTokenIssuanceLock", "MPTokenIssuanceUnlock"})); + env.close(); + + auto const seq = env.seq(alice); + auto const batchFee = batch::calcBatchFee(env, 0, 2); + + Json::Value jv1; + jv1[sfTransactionType] = jss::MPTokenIssuanceSet; + jv1[sfAccount] = alice.human(); + jv1[sfDelegate] = bob.human(); + jv1[sfSequence] = seq + 1; + jv1[sfMPTokenIssuanceID] = to_string(mptID); + jv1[sfFlags] = tfMPTLock; + + Json::Value jv2; + jv2[sfTransactionType] = jss::MPTokenIssuanceSet; + jv2[sfAccount] = alice.human(); + jv2[sfDelegate] = bob.human(); + jv2[sfSequence] = seq + 2; + jv2[sfMPTokenIssuanceID] = to_string(mptID); + jv2[sfFlags] = tfMPTUnlock; + + auto const [txIDs, batchID] = submitBatch( + env, + tesSUCCESS, + batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::inner(jv1, seq + 1), + batch::inner(jv2, seq + 2)); + env.close(); + + std::vector testCases = { + {0, "Batch", "tesSUCCESS", batchID, std::nullopt}, + {1, "MPTokenIssuanceSet", "tesSUCCESS", txIDs[0], batchID}, + {2, "MPTokenIssuanceSet", "tesSUCCESS", txIDs[1], batchID}, + }; + validateClosedLedger(env, testCases); + } + + // delegated non atomic inner (TrustSet) + // this also makes sure tfInnerBatchTxn won't block delegated TrustSet + // with granular permission + { + test::jtx::Env env{*this, envconfig()}; + Account gw{"gw"}; + Account alice{"alice"}; + Account bob{"bob"}; + env.fund(XRP(10000), gw, alice, bob); + env(fset(gw, asfRequireAuth)); + env.close(); + env(trust(alice, gw["USD"](50))); + env.close(); + + env(delegate::set( + gw, bob, {"TrustlineAuthorize", "TrustlineFreeze"})); + env.close(); + + auto const seq = env.seq(gw); + auto const batchFee = batch::calcBatchFee(env, 0, 2); + + auto jv1 = trust(gw, gw["USD"](0), alice, tfSetfAuth); + jv1[sfDelegate] = bob.human(); + auto jv2 = trust(gw, gw["USD"](0), alice, tfSetFreeze); + jv2[sfDelegate] = bob.human(); + + auto const [txIDs, batchID] = submitBatch( + env, + tesSUCCESS, + batch::outer(gw, seq, batchFee, tfAllOrNothing), + batch::inner(jv1, seq + 1), + batch::inner(jv2, seq + 2)); + env.close(); + + std::vector testCases = { + {0, "Batch", "tesSUCCESS", batchID, std::nullopt}, + {1, "TrustSet", "tesSUCCESS", txIDs[0], batchID}, + {2, "TrustSet", "tesSUCCESS", txIDs[1], batchID}, + }; + validateClosedLedger(env, testCases); + } + + // inner transaction not authorized by the delegating account. + { + test::jtx::Env env{*this, envconfig()}; + Account gw{"gw"}; + Account alice{"alice"}; + Account bob{"bob"}; + env.fund(XRP(10000), gw, alice, bob); + env(fset(gw, asfRequireAuth)); + env.close(); + env(trust(alice, gw["USD"](50))); + env.close(); + + env(delegate::set( + gw, bob, {"TrustlineAuthorize", "TrustlineFreeze"})); + env.close(); + + auto const seq = env.seq(gw); + auto const batchFee = batch::calcBatchFee(env, 0, 2); + + auto jv1 = trust(gw, gw["USD"](0), alice, tfSetFreeze); + jv1[sfDelegate] = bob.human(); + auto jv2 = trust(gw, gw["USD"](0), alice, tfClearFreeze); + jv2[sfDelegate] = bob.human(); + + auto const [txIDs, batchID] = submitBatch( + env, + tesSUCCESS, + batch::outer(gw, seq, batchFee, tfIndependent), + batch::inner(jv1, seq + 1), + // tecNO_DELEGATE_PERMISSION: not authorized to clear freeze + batch::inner(jv2, seq + 2)); + env.close(); + + std::vector testCases = { + {0, "Batch", "tesSUCCESS", batchID, std::nullopt}, + {1, "TrustSet", "tesSUCCESS", txIDs[0], batchID}, + {2, "TrustSet", "tecNO_DELEGATE_PERMISSION", txIDs[1], batchID}, + }; + validateClosedLedger(env, testCases); + } } void diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index dc3264d777c..ca13e4f4cdc 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -913,37 +913,6 @@ class Delegate_test : public beast::unit_test::suite gw, gw["USD"](0), alice, tfSetfAuth | tfFullyCanonicalSig), delegate::as(bob)); } - - // tfInnerBatchTxn won't block delegated transaction - { - Env env(*this); - Account gw{"gw"}; - Account alice{"alice"}; - Account bob{"bob"}; - env.fund(XRP(10000), gw, alice, bob); - env(fset(gw, asfRequireAuth)); - env.close(); - env(trust(alice, gw["USD"](50))); - env.close(); - - env(delegate::set( - gw, bob, {"TrustlineAuthorize", "TrustlineFreeze"})); - env.close(); - - auto const seq = env.seq(gw); - auto const batchFee = batch::calcBatchFee(env, 0, 2); - auto jv1 = trust(gw, gw["USD"](0), alice, tfSetfAuth); - jv1[sfDelegate] = bob.human(); - auto jv2 = trust(gw, gw["USD"](0), alice, tfSetFreeze); - jv2[sfDelegate] = bob.human(); - - // batch::inner will set tfInnerBatchTxn, this should not - // block delegated transaction - env(batch::outer(gw, seq, batchFee, tfAllOrNothing), - batch::inner(jv1, seq + 1), - batch::inner(jv2, seq + 2)); - env.close(); - } } void @@ -1234,53 +1203,6 @@ class Delegate_test : public beast::unit_test::suite env(jt); BEAST_EXPECT((*env.le(alice))[sfDomain] == makeSlice(domain)); } - - // tfInnerBatchTxn won't block delegated transaction - { - Env env(*this); - Account alice{"alice"}; - Account bob{"bob"}; - env.fund(XRP(10000), alice, bob); - env.close(); - - env(delegate::set( - alice, bob, {"AccountDomainSet", "AccountEmailHashSet"})); - env.close(); - - auto const seq = env.seq(alice); - auto const batchFee = batch::calcBatchFee(env, 0, 3); - - auto jv1 = noop(alice); - std::string const domain1 = "example1.com"; - jv1[sfDomain] = strHex(domain1); - jv1[sfDelegate] = bob.human(); - jv1[sfSequence] = seq + 1; - - auto jv2 = noop(alice); - std::string const domain2 = "example2.com"; - jv2[sfDomain] = strHex(domain2); - jv2[sfDelegate] = bob.human(); - jv2[sfSequence] = seq + 2; - - // bob set domain back and add email hash for alice - auto jv3 = noop(alice); - std::string const mh("5F31A79367DC3137FADA860C05742EE6"); - jv3[sfDomain] = strHex(domain1); - jv3[sfEmailHash] = mh; - jv3[sfDelegate] = bob.human(); - jv3[sfSequence] = seq + 3; - - // batch::inner will set tfInnerBatchTxn, this should not - // block delegated transaction - env(batch::outer(alice, seq, batchFee, tfAllOrNothing), - batch::inner(jv1, seq + 1), - batch::inner(jv2, seq + 2), - batch::inner(jv3, seq + 3)); - env.close(); - - BEAST_EXPECT((*env.le(alice))[sfDomain] == makeSlice(domain1)); - BEAST_EXPECT(to_string((*env.le(alice))[sfEmailHash]) == mh); - } } void @@ -1411,52 +1333,6 @@ class Delegate_test : public beast::unit_test::suite .flags = tfMPTLock | tfFullyCanonicalSig, .delegate = bob}); } - - // tfInnerBatchTxn won't block delegated transaction - { - Env env(*this); - Account alice{"alice"}; - Account bob{"bob"}; - env.fund(XRP(100000), alice, bob); - env.close(); - - auto const mptID = makeMptID(env.seq(alice), alice); - MPTTester mpt(env, alice, {.fund = false}); - env.close(); - mpt.create({.flags = tfMPTCanLock}); - env.close(); - - // alice gives granular permission to bob of MPTokenIssuanceLock - env(delegate::set( - alice, bob, {"MPTokenIssuanceLock", "MPTokenIssuanceUnlock"})); - env.close(); - - auto const seq = env.seq(alice); - auto const batchFee = batch::calcBatchFee(env, 0, 2); - - Json::Value jv1; - jv1[sfTransactionType] = jss::MPTokenIssuanceSet; - jv1[sfAccount] = alice.human(); - jv1[sfDelegate] = bob.human(); - jv1[sfSequence] = seq + 1; - jv1[sfMPTokenIssuanceID] = to_string(mptID); - jv1[sfFlags] = tfMPTLock; - - Json::Value jv2; - jv2[sfTransactionType] = jss::MPTokenIssuanceSet; - jv2[sfAccount] = alice.human(); - jv2[sfDelegate] = bob.human(); - jv2[sfSequence] = seq + 2; - jv2[sfMPTokenIssuanceID] = to_string(mptID); - jv2[sfFlags] = tfMPTUnlock; - - // batch::inner will set tfInnerBatchTxn, this should not - // block delegated transaction - env(batch::outer(alice, seq, batchFee, tfAllOrNothing), - batch::inner(jv1, seq + 1), - batch::inner(jv2, seq + 2)); - env.close(); - } } void From edb4f0342c65bd739fee60b74566f3e771134c6c Mon Sep 17 00:00:00 2001 From: Michael Legleux Date: Wed, 11 Jun 2025 17:10:45 -0700 Subject: [PATCH 6/6] Set version to 2.5.0-rc2 --- src/libxrpl/protocol/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxrpl/protocol/BuildInfo.cpp b/src/libxrpl/protocol/BuildInfo.cpp index b9b583046ee..24485d1c16f 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -36,7 +36,7 @@ namespace BuildInfo { // and follow the format described at http://semver.org/ //------------------------------------------------------------------------------ // clang-format off -char const* const versionString = "2.5.0-rc1" +char const* const versionString = "2.5.0-rc2" // clang-format on #if defined(DEBUG) || defined(SANITIZER)