diff --git a/.github/workflows/check-pr-commits.yml b/.github/workflows/check-pr-commits.yml index 2697a3a40e..37e15a5648 100644 --- a/.github/workflows/check-pr-commits.yml +++ b/.github/workflows/check-pr-commits.yml @@ -10,4 +10,4 @@ permissions: jobs: check_commits: - uses: XRPLF/actions/.github/workflows/check-pr-commits.yml@481048b78b94ac3343d1292b4ef125a813879f2b + uses: XRPLF/actions/.github/workflows/check-pr-commits.yml@e2c7f400d1e85ae65dad552fd425169fbacca4a3 diff --git a/.github/workflows/check-pr-title.yml b/.github/workflows/check-pr-title.yml index fc03cdf8e1..6d7bdefa08 100644 --- a/.github/workflows/check-pr-title.yml +++ b/.github/workflows/check-pr-title.yml @@ -11,4 +11,4 @@ on: jobs: check_title: if: ${{ github.event.pull_request.draft != true }} - uses: XRPLF/actions/.github/workflows/check-pr-title.yml@e2c7f400d1e85ae65dad552fd425169fbacca4a3 + uses: XRPLF/actions/.github/workflows/check-pr-title.yml@a5d8dd35be543365e90a11358447130c8763871d diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index 5cc99d1804..89255f0e47 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -14,7 +14,7 @@ on: jobs: # Call the workflow in the XRPLF/actions repo that runs the pre-commit hooks. run-hooks: - uses: XRPLF/actions/.github/workflows/pre-commit.yml@e7896f15cc60d0da1a272c77ee5c4026b424f9c7 + uses: XRPLF/actions/.github/workflows/pre-commit.yml@9307df762265e15c745ddcdb38a581c989f7f349 with: runs_on: ubuntu-latest container: '{ "image": "ghcr.io/xrplf/ci/tools-rippled-pre-commit:sha-41ec7c1" }' diff --git a/.github/workflows/publish-docs.yml b/.github/workflows/publish-docs.yml index 637246ec40..bed97cfafa 100644 --- a/.github/workflows/publish-docs.yml +++ b/.github/workflows/publish-docs.yml @@ -47,7 +47,7 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@2bbc2dc1abeec7bfaa886804ab86871ac201764e + uses: XRPLF/actions/prepare-runner@90f11ee655d1687824fb8793db770477d52afbab with: enable_ccache: false diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 920b5b5278..c79b22ac54 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -107,7 +107,7 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@2bbc2dc1abeec7bfaa886804ab86871ac201764e + uses: XRPLF/actions/prepare-runner@90f11ee655d1687824fb8793db770477d52afbab with: enable_ccache: ${{ inputs.ccache_enabled }} diff --git a/.github/workflows/reusable-clang-tidy-files.yml b/.github/workflows/reusable-clang-tidy-files.yml index 81264cf24e..b6c66bc27a 100644 --- a/.github/workflows/reusable-clang-tidy-files.yml +++ b/.github/workflows/reusable-clang-tidy-files.yml @@ -35,7 +35,7 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@2bbc2dc1abeec7bfaa886804ab86871ac201764e + uses: XRPLF/actions/prepare-runner@90f11ee655d1687824fb8793db770477d52afbab with: enable_ccache: false diff --git a/.github/workflows/upload-conan-deps.yml b/.github/workflows/upload-conan-deps.yml index 832e455453..f14efde05b 100644 --- a/.github/workflows/upload-conan-deps.yml +++ b/.github/workflows/upload-conan-deps.yml @@ -70,7 +70,7 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@2bbc2dc1abeec7bfaa886804ab86871ac201764e + uses: XRPLF/actions/prepare-runner@90f11ee655d1687824fb8793db770477d52afbab with: enable_ccache: false diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 22e683cba7..db4a07e41c 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -364,6 +364,13 @@ Transactor::checkFee(PreclaimContext const& ctx, XRPAmount baseFee) auto const balance = (*acctRoot)[sfBalance].xrp(); + // NOTE: Because preclaim evaluates against a static readview, it + // does not reflect fee deductions from other transactions paid by + // the same account within the current ledger. + // As a result, if an account's balance is over-committed across multiple + // transactions, this check may pass optimistically. + // The fee shortfall will be handled by the Transactor::reset mechanism, + // which caps the fee to the remaining actual balance. if (balance < feePaid) { JLOG(ctx.j.trace()) << "Insufficient balance:" << " balance=" << to_string(balance) diff --git a/src/libxrpl/tx/transactors/account/SignerListSet.cpp b/src/libxrpl/tx/transactors/account/SignerListSet.cpp index 03f2ca2084..f4c9840d31 100644 --- a/src/libxrpl/tx/transactors/account/SignerListSet.cpp +++ b/src/libxrpl/tx/transactors/account/SignerListSet.cpp @@ -307,11 +307,7 @@ SignerListSet::replaceSignerList() writeSignersToSLE(signerList, flags); // Add the signer list to the account's directory. - auto const page = - ctx_.view().dirInsert(ownerDirKeylet, signerListKeylet, describeOwnerDir(accountID_)); - - JLOG(j_.trace()) << "Create signer list for account " << toBase58(accountID_) << ": " - << (page ? "success" : "failure"); + << (page ? "success" : "failure"); if (!page) return tecDIR_FULL; // LCOV_EXCL_LINE diff --git a/src/libxrpl/tx/transactors/payment/Payment.cpp b/src/libxrpl/tx/transactors/payment/Payment.cpp index 8d21452829..0fc6477962 100644 --- a/src/libxrpl/tx/transactors/payment/Payment.cpp +++ b/src/libxrpl/tx/transactors/payment/Payment.cpp @@ -555,18 +555,23 @@ Payment::doApply() // This is the total reserve in drops. auto const reserve = view().fees().accountReserve(ownerCount); - // preFeeBalance_ is the balance on the sending account BEFORE the - // fees were charged. We want to make sure we have enough reserve - // to send. Allow final spend to use reserve for fee. - auto const mmm = std::max(reserve, ctx_.tx.getFieldAmount(sfFee).xrp()); + // In a delegated payment, the fee payer is the delegated account, + // not the source account (account_). + bool const accountIsPayer = (ctx_.tx.getFeePayer() == account_); - if (preFeeBalance_ < dstAmount.xrp() + mmm) + // preFeeBalance_ is the balance on the source account (account_) BEFORE the fees + // were charged. If source account is the fee payer, it must also cover the fee. + // The final spend may use the reserve to cover fees. + auto const minRequiredFunds = + accountIsPayer ? std::max(reserve, ctx_.tx.getFieldAmount(sfFee).xrp()) : reserve; + + if (preFeeBalance_ < dstAmount.xrp() + minRequiredFunds) { // Vote no. However the transaction might succeed, if applied in // a different order. JLOG(j_.trace()) << "Delay transaction: Insufficient funds: " << to_string(preFeeBalance_) - << " / " << to_string(dstAmount.xrp() + mmm) << " (" << to_string(reserve) - << ")"; + << " / " << to_string(dstAmount.xrp() + minRequiredFunds) << " (" + << to_string(reserve) << ")"; return tecUNFUNDED_PAYMENT; } diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index 3f6d5e6b47..6c5698a227 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -274,37 +274,42 @@ class Delegate_test : public beast::unit_test::suite testcase("test fee"); using namespace jtx; - Env env(*this); - Account const alice{"alice"}; - Account const bob{"bob"}; - Account const carol{"carol"}; - env.fund(XRP(10000), alice, carol); - env.fund(XRP(1000), bob); - env.close(); + // Common setup: fund alice, bob, carol with 1000 XRP. + auto setup = [&](Env& env) { + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const carol{"carol"}; + env.fund(XRP(1000), alice, bob, carol); + env.close(); + return std::make_tuple(alice, bob, carol); + }; + // No fee deduction for terNO_DELEGATE_PERMISSION. { - auto aliceBalance = env.balance(alice); - auto bobBalance = env.balance(bob); - auto carolBalance = env.balance(carol); + Env env(*this); + auto [alice, bob, carol] = setup(env); - env(pay(alice, carol, XRP(100)), - fee(XRP(2000)), - delegate::as(bob), - ter(terNO_DELEGATE_PERMISSION)); + auto const aliceBalance = env.balance(alice); + auto const bobBalance = env.balance(bob); + auto const carolBalance = env.balance(carol); + + env(pay(alice, carol, XRP(100)), delegate::as(bob), ter(terNO_DELEGATE_PERMISSION)); env.close(); BEAST_EXPECT(env.balance(alice) == aliceBalance); BEAST_EXPECT(env.balance(bob) == bobBalance); BEAST_EXPECT(env.balance(carol) == carolBalance); } - env(delegate::set(alice, bob, {"Payment"})); - env.close(); - + // Delegate pays the fee successfully. { - // Delegate pays the fee - auto aliceBalance = env.balance(alice); - auto bobBalance = env.balance(bob); - auto carolBalance = env.balance(carol); + Env env(*this); + auto [alice, bob, carol] = setup(env); + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + auto const aliceBalance = env.balance(alice); + auto const bobBalance = env.balance(bob); + auto const carolBalance = env.balance(carol); auto const sendAmt = XRP(100); auto const feeAmt = XRP(10); @@ -315,11 +320,16 @@ class Delegate_test : public beast::unit_test::suite BEAST_EXPECT(env.balance(carol) == carolBalance + sendAmt); } + // Bob has insufficient balance to pay the fee, will get terINSUF_FEE_B. { - // insufficient balance to pay fee - auto aliceBalance = env.balance(alice); - auto bobBalance = env.balance(bob); - auto carolBalance = env.balance(carol); + Env env(*this); + auto [alice, bob, carol] = setup(env); + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + auto const aliceBalance = env.balance(alice); + auto const bobBalance = env.balance(bob); + auto const carolBalance = env.balance(carol); env(pay(alice, carol, XRP(100)), fee(XRP(2000)), @@ -331,22 +341,143 @@ class Delegate_test : public beast::unit_test::suite BEAST_EXPECT(env.balance(carol) == carolBalance); } + // The delegated account has enough balance to pay and delegator has enough reserve { - // fee is paid by Delegate - // on context reset (tec error) - auto aliceBalance = env.balance(alice); - auto bobBalance = env.balance(bob); - auto carolBalance = env.balance(carol); - auto const feeAmt = XRP(10); + // Common setup: fund accounts and grant Bob permission to pay on Alice's behalf. + // Alice is funded with exactly (paymentAmount + reserve + baseFee): baseFee covers + // the DelegateSet tx cost, leaving Alice with exactly (paymentAmount + reserve). + // highFee = reserve + baseFee, strictly greater than reserve, so that + // max(reserve, highFee) = highFee — making the direct payment check fail. + auto setup = [&](Env& env) { + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const carol{"carol"}; - env(pay(alice, carol, XRP(20000)), - fee(feeAmt), - delegate::as(bob), - ter(tecUNFUNDED_PAYMENT)); + auto const baseFee = env.current()->fees().base; + auto const reserve = env.current()->fees().accountReserve(1); + auto const paymentAmount = XRP(1); + auto const highFee = reserve + baseFee; + BEAST_EXPECT(highFee > reserve); + + env.fund(paymentAmount + reserve + baseFee, alice); + env.fund(XRP(1000), bob); + env.fund(XRP(1000), carol); + env.close(); + + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + env.require(balance(alice, paymentAmount + reserve)); + + return std::make_tuple(alice, bob, carol, paymentAmount, highFee, reserve); + }; + + // Alice's balance (paymentAmount + reserve) is insufficient to cover both + // the payment and highFee directly. Even though fees are allowed to dip + // below reserve, when Alice pays the fee herself the required funds = + // paymentAmount + max(reserve, highFee) = paymentAmount + highFee + // (since highFee > reserve), which still exceeds her balance. + // tec: highFee is consumed from Alice's balance. + { + Env env(*this); + auto [alice, bob, carol, paymentAmount, highFee, reserve] = setup(env); + auto const aliceBalance = env.balance(alice); + auto const bobBalance = env.balance(bob); + auto const carolBalance = env.balance(carol); + + env(pay(alice, carol, paymentAmount), fee(highFee), ter(tecUNFUNDED_PAYMENT)); + + // tec consumes the fee from Alice; carol and bob are unaffected. + BEAST_EXPECT(env.balance(alice) == aliceBalance - highFee); + BEAST_EXPECT(env.balance(bob) == bobBalance); + BEAST_EXPECT(env.balance(carol) == carolBalance); + } + + // The payment succeeds because the delegated account pays the fee. + // Alice only needs (paymentAmount + reserve). + { + Env env(*this); + auto [alice, bob, carol, paymentAmount, highFee, reserve] = setup(env); + + auto const alicePrePay = env.balance(alice, XRP); + auto const bobPrePay = env.balance(bob, XRP); + auto const carolPrePay = env.balance(carol, XRP); + + env(pay(alice, carol, paymentAmount), delegate::as(bob), fee(highFee)); + env.close(); + + env.require(balance(alice, alicePrePay - paymentAmount)); + env.require(balance(bob, bobPrePay - highFee)); + env.require(balance(carol, carolPrePay + paymentAmount)); + } + } + + // Delegated account can pay the fee even if it dips below reserve. + { + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const carol{"carol"}; + + auto const baseFee = env.current()->fees().base; + auto const baseReserve = env.current()->fees().accountReserve(0); + + env.fund(env.current()->fees().accountReserve(1) + baseFee + XRP(1), alice); + env.fund(baseReserve, bob); + env.fund(XRP(1000), carol); env.close(); - BEAST_EXPECT(env.balance(alice) == aliceBalance); - BEAST_EXPECT(env.balance(bob) == bobBalance - feeAmt); - BEAST_EXPECT(env.balance(carol) == carolBalance); + + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + auto const alicePreTx = env.balance(alice, XRP); + auto const bobPreTx = env.balance(bob, XRP); + + // After paying for this transaction, bob's balance will + // dip below the base reserve + env(pay(alice, carol, XRP(1)), delegate::as(bob)); + env.close(); + + // Bob's balance is now less than the base reserve. + BEAST_EXPECT(env.balance(bob, XRP) < baseReserve); + env.require(balance(bob, bobPreTx - drops(baseFee))); + + // Alice's balance only decreased by the 1.0 XRP she sent. + env.require(balance(alice, alicePreTx - XRP(1))); + } + + // The delegated account has enough balance for the fee, but delegator + // runs into tecUNFUNDED_PAYMENT. + { + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const carol{"carol"}; + + auto const baseFee = env.current()->fees().base; + auto const reserve = env.current()->fees().accountReserve(1); + + // Alice is funded with (reserve + baseFee): after DelegateSet she has + // exactly 'reserve', which is insufficient to send XRP(10) while keeping + // reserve. Bob has plenty to pay the fee. + env.fund(reserve + baseFee, alice); + env.fund(XRP(1000), bob); + env.fund(XRP(1000), carol); + env.close(); + + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + auto const alicePrePay = env.balance(alice, XRP); + auto const bobPrePay = env.balance(bob, XRP); + auto const carolPrePay = env.balance(carol, XRP); + + // Bob pays the fee, but Alice has insufficient balance to send XRP(10). + env(pay(alice, carol, XRP(10)), delegate::as(bob), ter(tecUNFUNDED_PAYMENT)); + + env.require(balance(alice, alicePrePay)); + env.require(balance(bob, bobPrePay - drops(baseFee))); + env.require(balance(carol, carolPrePay)); } } @@ -1238,8 +1369,8 @@ class Delegate_test : public beast::unit_test::suite // test MPTokenIssuanceUnlock and MPTokenIssuanceLock permissions { Env env(*this); - Account alice{"alice"}; - Account bob{"bob"}; + Account const alice{"alice"}; + Account const bob{"bob"}; env.fund(XRP(100000), alice, bob); env.close(); @@ -1285,8 +1416,8 @@ class Delegate_test : public beast::unit_test::suite // test mix of granular and transaction level permission { Env env(*this); - Account alice{"alice"}; - Account bob{"bob"}; + Account const alice{"alice"}; + Account const bob{"bob"}; env.fund(XRP(100000), alice, bob); env.close(); @@ -1332,8 +1463,8 @@ class Delegate_test : public beast::unit_test::suite // tfFullyCanonicalSig won't block delegated transaction { Env env(*this); - Account alice{"alice"}; - Account bob{"bob"}; + Account const alice{"alice"}; + Account const bob{"bob"}; env.fund(XRP(100000), alice, bob); env.close(); @@ -1410,11 +1541,9 @@ class Delegate_test : public beast::unit_test::suite { Env env(*this); - Account const alice{"alice"}; Account const bob{"bob"}; Account const carol{"carol"}; - env.fund(XRP(100000), alice, bob, carol); env.close(); @@ -1448,11 +1577,9 @@ class Delegate_test : public beast::unit_test::suite { Env env(*this); - Account const alice{"alice"}; Account const bob{"bob"}; Account const carol{"carol"}; - env.fund(XRP(100000), alice, bob, carol); env.close(); @@ -1567,8 +1694,8 @@ class Delegate_test : public beast::unit_test::suite Env env(*this, features); - Account alice{"alice"}; - Account bob{"bob"}; + Account const alice{"alice"}; + Account const bob{"bob"}; env.fund(XRP(100000), alice, bob); env.close(); diff --git a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp index 1cfdf44afb..ed50f6e571 100644 --- a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp +++ b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp @@ -98,16 +98,7 @@ LedgerReplayMsgHandler::processProofPathResponse( } // deserialize the header - LedgerHeader info; - try - { - info = deserializeHeader({reply.ledgerheader().data(), reply.ledgerheader().size()}); - } - catch (std::exception const&) - { - JLOG(journal_.debug()) << "Bad message: Cannot deserialize header"; - return false; - } + auto info = deserializeHeader({reply.ledgerheader().data(), reply.ledgerheader().size()}); uint256 const replyHash(reply.ledgerhash()); if (calculateLedgerHash(info) != replyHash) { @@ -201,23 +192,13 @@ LedgerReplayMsgHandler::processReplayDeltaResponse( std::shared_ptr const& msg) { protocol::TMReplayDeltaResponse const& reply = *msg; - if (reply.has_error() || !reply.has_ledgerheader() || !reply.has_ledgerhash() || - reply.ledgerhash().size() != uint256::size()) + if (reply.has_error() || !reply.has_ledgerheader()) { JLOG(journal_.debug()) << "Bad message: Error reply"; return false; } - LedgerHeader info; - try - { - info = deserializeHeader({reply.ledgerheader().data(), reply.ledgerheader().size()}); - } - catch (std::exception const&) - { - JLOG(journal_.debug()) << "Bad message: Cannot deserialize header"; - return false; - } + auto info = deserializeHeader({reply.ledgerheader().data(), reply.ledgerheader().size()}); uint256 const replyHash(reply.ledgerhash()); if (calculateLedgerHash(info) != replyHash) {