diff --git a/include/xrpl/ledger/helpers/AccountRootHelpers.h b/include/xrpl/ledger/helpers/AccountRootHelpers.h index 13b22becc2..c9d4406c32 100644 --- a/include/xrpl/ledger/helpers/AccountRootHelpers.h +++ b/include/xrpl/ledger/helpers/AccountRootHelpers.h @@ -62,8 +62,8 @@ struct Adjustment * @param view The ledger view to read from * @param sle The ledger entry for the account * @param j Journal for logging - * @param ownerCountAdj Adjustment to the owner count (default: 0) - * @param accountCountAdj Adjustment to the account count (default: 0) + * @param adj Adjustment to the owner/account count (default: 0/0). Positive to add, negative to + * subtract. * @return The account reserve amount in drops */ [[nodiscard]] XRPAmount @@ -74,8 +74,8 @@ accountReserve(ReadView const& view, SLE::const_ref sle, beast::Journal j, Adjus * @param view The ledger view to read from * @param id The account ID * @param j Journal for logging - * @param ownerCountAdj Adjustment to the owner count (default: 0) - * @param accountCountAdj Adjustment to the account count (default: 0) + * @param adj Adjustment to the owner/account count (default: 0/0). Positive to add, negative to + * subtract. * @return The account reserve amount in drops */ [[nodiscard]] inline XRPAmount @@ -91,8 +91,8 @@ accountReserve(ReadView const& view, AccountID const& id, beast::Journal j, Adju * @param accSle The account's ledger entry * @param accBalance The account's balance * @param sponsorSle The sponsor's ledger entry (if applicable) - * @param ownerCountAdj Adjustment to the owner count - * @param accountCountAdj Adjustment to the account count (default: 0) + * @param adj Adjustment to the owner/account count (default: 0/0). Positive to add, negative to + * subtract. * @param j Journal for logging (default: null sink) * @return Transaction result code */ @@ -103,7 +103,7 @@ checkReserve( STAmount const& accBalance, SLE::const_ref sponsorSle, Adjustment adj, - beast::Journal j = beast::Journal{beast::Journal::getNullSink()}); + beast::Journal j); /** Return number of the objects which reserve is covered by the account(sle) (so called "owner * count"). Actual owner count can be adjusted by delta in ownerCountAdj. diff --git a/include/xrpl/ledger/helpers/EscrowHelpers.h b/include/xrpl/ledger/helpers/EscrowHelpers.h index b4093d121b..92b2b15b9d 100644 --- a/include/xrpl/ledger/helpers/EscrowHelpers.h +++ b/include/xrpl/ledger/helpers/EscrowHelpers.h @@ -78,8 +78,10 @@ escrowUnlockApplyHelper( { JLOG(journal.trace()) << "Trust line does not exist. " "Insufficient reserve to create line."; - - return tecNO_LINE_INSUF_RESERVE; + // checkReserve can return tecINSUFFICIENT_RESERVE or tecINTERNAL + if (ret == tecINSUFFICIENT_RESERVE) + return tecNO_LINE_INSUF_RESERVE; + return ret; } Currency const currency = issue.currency; @@ -214,8 +216,6 @@ escrowUnlockApplyHelper( // update owner count. increaseOwnerCount(ctx.view, sleDest, *sponsorSle, 1, journal); - auto mptSle = ctx.view.peek(mptKeylet); - addSponsorToLedgerEntry(mptSle, *sponsorSle); } if (!ctx.view.exists(mptKeylet) && !receiverIssuer) diff --git a/include/xrpl/protocol/TxFlags.h b/include/xrpl/protocol/TxFlags.h index a6f3978f96..1d4f33a39d 100644 --- a/include/xrpl/protocol/TxFlags.h +++ b/include/xrpl/protocol/TxFlags.h @@ -142,7 +142,7 @@ inline constexpr FlagValue tfUniversalMask = ~tfUniversal; TF_FLAG(tfMPTCanTrade, lsfMPTCanTrade) \ TF_FLAG(tfMPTCanTransfer, lsfMPTCanTransfer) \ TF_FLAG(tfMPTCanClawback, lsfMPTCanClawback) \ - TF_FLAG(tfMPTCanHoldConfidentialBalance, lsfMPTCanHoldConfidentialBalance), \ + TF_FLAG(tfMPTCanHoldConfidentialBalance, lsfMPTCanHoldConfidentialBalance), \ MASK_ADJ(0)) \ \ TRANSACTION(MPTokenAuthorize, \ @@ -222,14 +222,14 @@ inline constexpr FlagValue tfUniversalMask = ~tfUniversal; TF_FLAG(tfSponsorshipSetRequireSignForFee, 0x00010000) \ TF_FLAG(tfSponsorshipClearRequireSignForFee, 0x00020000) \ TF_FLAG(tfSponsorshipSetRequireSignForReserve, 0x00040000) \ - TF_FLAG(tfSponsorshipClearRequireSignForReserve, 0x00080000) \ + TF_FLAG(tfSponsorshipClearRequireSignForReserve, 0x00080000) \ TF_FLAG(tfDeleteObject, 0x00100000), \ MASK_ADJ(0)) \ \ TRANSACTION(SponsorshipTransfer, \ - TF_FLAG(tfSponsorshipEnd, 0x00000001) \ - TF_FLAG(tfSponsorshipCreate, 0x00000002) \ - TF_FLAG(tfSponsorshipReassign, 0x00000004), \ + TF_FLAG(tfSponsorshipEnd, 0x00010000) \ + TF_FLAG(tfSponsorshipCreate, 0x00020000) \ + TF_FLAG(tfSponsorshipReassign, 0x00040000), \ MASK_ADJ(0)) // clang-format on diff --git a/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp b/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp index 6ef4c47482..1e8de4ad7f 100644 --- a/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp @@ -97,7 +97,7 @@ accountCountImpl(SLE::const_ref sle, std::int32_t accountCountAdj, beast::Journa std::int64_t totalAccountCount{currentAccountCount + accountCountAdj}; if (totalAccountCount > std::numeric_limits::max()) { - JLOG(j.error()) << "Reserve count exceeds max!"; + JLOG(j.fatal()) << "Reserve count exceeds max!"; totalAccountCount = std::numeric_limits::max(); } else if (totalAccountCount < 0) @@ -124,8 +124,7 @@ ownerCount(SLE::const_ref sle, beast::Journal j, std::int32_t ownerCountAdj) XRPL_ASSERT( currentOwnerCount >= sponsoredOwnerCount, - "xrpl::ownerCount : OwnerCount must be greater than or equal to " - "SponsoredOwnerCount"); + "xrpl::ownerCount : OwnerCount must be greater than or equal to SponsoredOwnerCount"); std::int64_t deltaCount = static_cast(ownerCountAdj) - sponsoredOwnerCount + sponsoringOwnerCount; @@ -331,15 +330,7 @@ decreaseOwnerCountForObject( XRPAmount accountReserve(ReadView const& view, SLE::const_ref sle, beast::Journal j, Adjustment adj) { - if (!sle) - { - Throw("xrpl::accountReserve : valid sle"); // LCOV_EXCL_LINE - } - - if (sle->getType() != ltACCOUNT_ROOT) - { - Throw("xrpl::accountReserve : valid sle type"); // LCOV_EXCL_LINE - } + XRPL_ASSERT(sle && sle->getType() == ltACCOUNT_ROOT, "xrpl::accountReserve : valid sle"); std::uint32_t const currentOwnerCount = ownerCount(sle, j, adj.ownerCountDelta); std::uint32_t const currentAccountCount = accountCountImpl(sle, adj.accountCountDelta, j); @@ -356,6 +347,9 @@ checkReserve( Adjustment adj, beast::Journal j) { + // TODO: swap to assert after fixCleanup3_2_0 is retired + if (!accSle || accSle->getType() != ltACCOUNT_ROOT) + return tefINTERNAL; // LCOV_EXCL_LINE if (sponsorSle) { auto const sle = ctx.view.read( diff --git a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp index 56716f3bac..159acc4692 100644 --- a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp +++ b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp @@ -679,7 +679,12 @@ addEmptyHolding( if (auto const ret = checkReserve(ctx, sleDst, priorBalance, sponsorSle, {.ownerCountDelta = 1}, journal); !isTesSuccess(ret)) - return tecNO_LINE_INSUF_RESERVE; + { + // checkReserve can return tecINSUFFICIENT_RESERVE or tecINTERNAL + if (ret == tecINSUFFICIENT_RESERVE) + return tecNO_LINE_INSUF_RESERVE; + return ret; + } return trustCreate( ctx.view, diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index fbc17d13f6..9a461735b7 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -416,6 +416,10 @@ Transactor::checkSponsor(ReadView const& view, STTx const& tx) auto const hasSponsorSignature = tx.isFieldPresent(sfSponsorSignature); + // Skip Sponsorship existence checks if the sponsor has signed the transaction - this + // transaction is valid regardless of the Sponsorship object. + // The use of the Sponsorship object is properly handled in + // getFeePayer/checkReserve/increaseOwnerCount/decreaseOwnerCount. if (hasSponsorSignature) return tesSUCCESS; @@ -636,7 +640,14 @@ Transactor::payFee() return tefINTERNAL; // LCOV_EXCL_LINE } - if (feePaid > balance) + // Only sponsor fee-payers reject here on insufficient funds. For an + // ordinary account, the fee falls through and is capped by reset(), which + // caps to the account's balance. That capping is wrong for sponsors: a + // co-signed sponsor would be charged into its own reserve, and a prefunded + // sponsorship's fee amount should be rejected rather than partially spent. + if (feePaid > balance && + (feePayer.type == FeePayerType::SponsorPreFunded || + feePayer.type == FeePayerType::SponsorCoSigned)) { if ((balance > beast::kZero) && !view().open()) return tecINSUFF_FEE; diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 1ad3868cc1..9144eab935 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -138,9 +138,13 @@ XRPNotCreated::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref a { switch (before->getType()) { - case ltACCOUNT_ROOT: + case ltACCOUNT_ROOT: { + XRPL_ASSERT( + isXRP((*before)[sfBalance]), + "XRPNotCreated::visitEntry : AccountRoot.Balance is XRP"); drops_ -= (*before)[sfBalance].xrp().drops(); break; + } case ltPAYCHAN: drops_ -= ((*before)[sfAmount] - (*before)[sfBalance]).xrp().drops(); break; @@ -150,7 +154,12 @@ XRPNotCreated::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref a break; case ltSPONSORSHIP: if (before->isFieldPresent(sfFeeAmount)) + { + XRPL_ASSERT( + isXRP((*before)[sfFeeAmount]), + "XRPNotCreated::visitEntry : Sponsorship.FeeAmount is XRP"); drops_ -= (*before)[sfFeeAmount].xrp().drops(); + } break; default: break; diff --git a/src/libxrpl/tx/transactors/token/TrustSet.cpp b/src/libxrpl/tx/transactors/token/TrustSet.cpp index 8cffeb47ed..115b6b0f82 100644 --- a/src/libxrpl/tx/transactors/token/TrustSet.cpp +++ b/src/libxrpl/tx/transactors/token/TrustSet.cpp @@ -527,9 +527,6 @@ TrustSet::doApply() { SLE::pointer const lowSponsor = getSponsor(uLowAccountID); - // should be checked PreFunded Sponsor before increaseOwnerCount() - // For PreFunded sponsors, we need to check if there are sufficient reserves before - // calling increaseOwnerCount(). if (auto const ret = checkReserve( ctx_.getApplyViewContext(), sleLowAccount, @@ -538,7 +535,12 @@ TrustSet::doApply() {.ownerCountDelta = 1}, j_); lowSponsor && !isTesSuccess(ret)) - return tecINSUF_RESERVE_LINE; + { + // checkReserve can return tecINSUFFICIENT_RESERVE or tecINTERNAL + if (ret == tecINSUFFICIENT_RESERVE) + return tecINSUF_RESERVE_LINE; + return ret; + } // Set reserve for low account. increaseOwnerCount(view(), sleLowAccount, lowSponsor, 1, viewJ); diff --git a/src/test/app/EscrowToken_test.cpp b/src/test/app/EscrowToken_test.cpp index 666ee498bd..4015f5ddc8 100644 --- a/src/test/app/EscrowToken_test.cpp +++ b/src/test/app/EscrowToken_test.cpp @@ -937,7 +937,7 @@ struct EscrowToken_test : public beast::unit_test::Suite auto const expectedResult = env.current()->rules().enabled(fixCleanup3_2_0) ? Ter(tesSUCCESS) - : Ter(tefEXCEPTION); + : Ter(tefINTERNAL); env(escrow::cancel(alice, alice, seq), Fee(baseFee), expectedResult); env.close(); diff --git a/src/test/app/Sponsor_test.cpp b/src/test/app/Sponsor_test.cpp index 5de0567d21..27e90d52f7 100644 --- a/src/test/app/Sponsor_test.cpp +++ b/src/test/app/Sponsor_test.cpp @@ -3027,76 +3027,6 @@ public: } } - void - testDID(bool cosigning) - { - testcase("DID"); - using namespace test::jtx; - Account const alice("alice"); - Account const sponsor("sponsor"); - Account const sponsor2("sponsor2"); - - { - Env env{*this, testableAmendments()}; - env.fund(XRP(1000000), alice, sponsor, sponsor2); - env.close(); - - // DIDSet - testEachSponsorship( - env, - cosigning, - sponsor, - alice, - 1, - 1, - tecINSUFFICIENT_RESERVE, - [&](Env& env, auto const& submit) { submit(did::set(alice), did::Uri("uri")); }); - - // transfer sponsor - auto const keylet = keylet::did(alice); - if (cosigning) - { - env(sponsor::transfer(alice, tfSponsorshipReassign, keylet.key), - sponsor::As(sponsor2, spfSponsorReserve), - Sig(sfSponsorSignature, sponsor2)); - env.close(); - } - else - { - env(sponsor::set_reserve(sponsor2, 0, 1), sponsor::SponseeAcc(alice)); - env.close(); - env(sponsor::transfer(alice, tfSponsorshipReassign, keylet.key), - sponsor::As(sponsor2, spfSponsorReserve)); - env.close(); - } - - BEAST_EXPECT(ownerCount(env, alice) == 1); - BEAST_EXPECT(sponsoredOwnerCount(env, alice) == 1); - BEAST_EXPECT(sponsoringOwnerCount(env, sponsor) == 0); - BEAST_EXPECT(sponsoringOwnerCount(env, sponsor2) == 1); - if (!cosigning) - { - auto const sponsorshipSle = env.le(keylet::sponsorship(sponsor2, alice)); - BEAST_EXPECT(sponsorshipSle); - if (sponsorshipSle) - { - BEAST_EXPECT( - !sponsorshipSle->isFieldPresent(sfRemainingOwnerCount) || - sponsorshipSle->getFieldU32(sfRemainingOwnerCount) == 0); - } - } - - // DIDDelete - env(did::del(alice)); - env.close(); - - BEAST_EXPECT(ownerCount(env, alice) == 0); - BEAST_EXPECT(sponsoredOwnerCount(env, alice) == 0); - BEAST_EXPECT(sponsoringOwnerCount(env, sponsor) == 0); - BEAST_EXPECT(sponsoringOwnerCount(env, sponsor2) == 0); - } - } - void testEscrow(bool cosigning) {