diff --git a/include/xrpl/shamap/SHAMapNodeID.h b/include/xrpl/shamap/SHAMapNodeID.h index fcd5a4d00e..f35ba2d2a7 100644 --- a/include/xrpl/shamap/SHAMapNodeID.h +++ b/include/xrpl/shamap/SHAMapNodeID.h @@ -55,6 +55,20 @@ public: [[nodiscard]] SHAMapNodeID getChildNodeID(unsigned int branch) const; + /** + * Test whether this node ID lies on the path to the given leaf key + * + * A node at depth d identifies the tree path spelled by the first d + * nibbles of its key, so any leaf beneath it must agree on that prefix. + * A node ID that fails this test names a different subtree than the one + * it was built for. + * + * @param key the key of a leaf below this node + * @return whether this node ID is a prefix of the leaf key + */ + [[nodiscard]] bool + isPrefixOf(uint256 const& key) const; + /** * Create a SHAMapNodeID of a node with the depth of the node and * the key of a leaf diff --git a/src/libxrpl/shamap/SHAMapNodeID.cpp b/src/libxrpl/shamap/SHAMapNodeID.cpp index ecde22a63d..8fd7afe8fc 100644 --- a/src/libxrpl/shamap/SHAMapNodeID.cpp +++ b/src/libxrpl/shamap/SHAMapNodeID.cpp @@ -46,8 +46,7 @@ SHAMapNodeID::SHAMapNodeID(unsigned int depth, uint256 const& hash) : id_(hash), XRPL_ASSERT( depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::SHAMapNodeID : maximum depth input"); XRPL_ASSERT( - id_ == (id_ & depthMask(depth)), - "xrpl::SHAMapNodeID::SHAMapNodeID : hash and depth inputs do match"); + isPrefixOf(id_), "xrpl::SHAMapNodeID::SHAMapNodeID : hash and depth inputs do match"); } std::string @@ -79,7 +78,7 @@ SHAMapNodeID::getChildNodeID(unsigned int branch) const if (depth_ >= SHAMap::kLeafDepth) Throw("Request for child node ID of " + to_string(*this)); - if (id_ != (id_ & depthMask(depth_))) + if (!isPrefixOf(id_)) Throw("Incorrect mask for " + to_string(*this)); SHAMapNodeID node{depth_ + 1, id_}; @@ -87,6 +86,12 @@ SHAMapNodeID::getChildNodeID(unsigned int branch) const return node; } +bool +SHAMapNodeID::isPrefixOf(uint256 const& key) const +{ + return (key & depthMask(depth_)) == id_; +} + [[nodiscard]] std::optional deserializeSHAMapNodeID(void const* data, std::size_t size) { diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index e6948ec3ac..a12e524a5f 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -555,10 +555,9 @@ SHAMap::addKnownNode( { XRPL_ASSERT(!nodeID.isRoot(), "xrpl::SHAMap::addKnownNode : valid node"); XRPL_ASSERT(treeNode, "xrpl::SHAMap::addKnownNode : non-null tree node"); - XRPL_ASSERT( - !treeNode->isLeaf() || - SHAMapNodeID::createID(nodeID.getDepth(), leafKey(*treeNode)).getNodeID() == - nodeID.getNodeID(), + XRPL_ASSERT_IF( + treeNode->isLeaf(), + nodeID.isPrefixOf(leafKey(*treeNode)), "xrpl::SHAMap::addKnownNode : leaf position consistent with node ID"); if (!isSynching()) diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index c9ab9d81b9..ad7cec460f 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -9,6 +10,8 @@ #include #include #include +#include +#include #include #include #include @@ -33,6 +36,34 @@ namespace xrpl { +namespace { +// Returns the account's true, unclamped balance in `asset`, for use only in +// fund-conservation checks. accountHolds(..., SpendableHandling::FullBalance) +// cannot be used for this: for XRP it always defers to xrpLiquid, which +// subtracts the account's reserve, so a payee sitting below its own reserve +// would appear to receive nothing even though its raw ledger balance grew. +// That mismatch is exactly what a conservation check must not see. +STAmount +conservationBalance(ReadView const& view, AccountID const& id, Asset const& asset, beast::Journal j) +{ + if (isXRP(asset)) + { + auto const sle = view.read(keylet::account(id)); + if (!sle) + return STAmount{asset}; // LCOV_EXCL_LINE + return view.balanceHookIOU(id, xrpAccount(), sle->getFieldAmount(sfBalance)); + } + return accountHolds( + view, + id, + asset, + FreezeHandling::IgnoreFreeze, + AuthHandling::IgnoreAuth, + j, + SpendableHandling::FullBalance); +} +} // namespace + bool LoanPay::checkExtraFeatures(PreflightContext const& ctx) { @@ -592,34 +623,13 @@ LoanPay::doApply() } // These three values are used to check that funds are conserved after the transfers - auto const accountBalanceBefore = accountHolds( - view, - accountID_, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + auto const accountBalanceBefore = conservationBalance(view, accountID_, asset, j_); auto const vaultBalanceBefore = accountID_ == vaultPseudoAccount ? STAmount{asset, 0} - : accountHolds( - view, - vaultPseudoAccount, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + : conservationBalance(view, vaultPseudoAccount, asset, j_); auto const brokerBalanceBefore = accountID_ == brokerPayee ? STAmount{asset, 0} - : accountHolds( - view, - brokerPayee, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + : conservationBalance(view, brokerPayee, asset, j_); if (totalPaidToVaultRounded != beast::kZero) { @@ -675,33 +685,13 @@ LoanPay::doApply() #endif // Check that funds are conserved - auto const accountBalanceAfter = accountHolds( - view, - accountID_, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + auto const accountBalanceAfter = conservationBalance(view, accountID_, asset, j_); auto const vaultBalanceAfter = accountID_ == vaultPseudoAccount ? STAmount{asset, 0} - : accountHolds( - view, - vaultPseudoAccount, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); - auto const brokerBalanceAfter = accountID_ == brokerPayee ? STAmount{asset, 0} - : accountHolds( - view, - brokerPayee, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + : conservationBalance(view, vaultPseudoAccount, asset, j_); + auto const brokerBalanceAfter = accountID_ == brokerPayee + ? STAmount{asset, 0} + : conservationBalance(view, brokerPayee, asset, j_); auto const balanceScale = [&]() { // Find a reasonable scale to use for the balance comparisons. // diff --git a/src/test/app/lending/LoanPay_test.cpp b/src/test/app/lending/LoanPay_test.cpp index 0229d522d5..97ca67743a 100644 --- a/src/test/app/lending/LoanPay_test.cpp +++ b/src/test/app/lending/LoanPay_test.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -13,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -728,6 +730,110 @@ private: } } + void + testLoanPayFundsConservedPayeeBelowReserve(FeatureBitset features) + { + // Regression test: LoanPay::doApply's fund-conservation check used to + // read XRP balances via accountHolds(..., SpendableHandling:: + // FullBalance), which for XRP always defers to xrpLiquid (balance + // minus reserve, clamped at zero). When the broker fee landed on a + // payee sitting below its own reserve, that payee's clamped balance + // stayed zero and the fee vanished from the conservation sum, + // tripping "funds are conserved (with rounding)". + testcase("LoanPay funds conserved: broker fee payee below reserve"); + + using namespace jtx; + + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + // Broker defaults match the fuzz workload: ManagementFeeRate = 100 + // tenth-bips. The service fee guarantees feePaid > 0 on the first + // regular payment. + BrokerParameters const brokerParams; + Number const serviceFeeValue{2}; + LoanParameters const loanParams{ + .account = borrower, + .counter = lender, + .principalRequest = 1000, + .serviceFee = serviceFeeValue, + .interest = TenthBips32{percentageToTenthBips(12)}, + .payTotal = 12, + .payInterval = 3600}; + + auto const loanOpt = + createLoan(env, AssetType::XRP, brokerParams, loanParams, issuer, lender, borrower); + if (BEAST_EXPECT(loanOpt); !loanOpt.has_value()) + return; + auto const& [broker, loanKeylet, brokerPseudo] = *loanOpt; + + auto const vaultPseudo = [&]() { + auto const vaultSle = env.le(keylet::vault(broker.vaultID)); + if (!BEAST_EXPECT(vaultSle)) + return AccountID{}; + return vaultSle->at(sfAccount); + }(); + + // Raw AccountRoot balance, matching LoanPay::doApply's conservation + // check (not the reserve-clamped accountHolds()/xrpLiquid() value). + auto rawBalance = [&](AccountID const& id) -> STAmount { + auto const sle = env.le(keylet::account(id)); + if (!BEAST_EXPECT(sle)) + return STAmount{}; + return sle->getFieldAmount(sfBalance); + }; + auto lenderReserve = [&] { + return env.current()->fees().accountReserve(ownerCount(env, lender), 1); + }; + + STAmount const baseFee{env.current()->fees().base}; + + // Park the lender (broker owner, fee payee) exactly at its reserve, + // then burn part of the reserve with an oversized transaction fee. + // Fees are exempt from the reserve check, so the balance ends up + // below the reserve. + env(pay(lender, issuer, rawBalance(lender.id()) - lenderReserve() - baseFee)); + env(noop(lender), Fee(XRP(100))); + env.close(); + BEAST_EXPECT(env.balance(lender) < lenderReserve()); + + // First regular payment, exactly the amount due. + auto const state = getCurrentState(env, broker, loanKeylet); + STAmount const serviceFee = broker.asset(serviceFeeValue); + STAmount const roundedPeriodicPayment{ + broker.asset, + roundPeriodicPayment(broker.asset, state.periodicPayment, state.loanScale)}; + STAmount const totalDue = roundToScale( + roundedPeriodicPayment + serviceFee, state.loanScale, Number::RoundingMode::Upward); + + auto const borrowerBefore = rawBalance(borrower.id()); + auto const vaultBefore = rawBalance(vaultPseudo); + auto const lenderBefore = rawBalance(lender.id()); + + // Before the fix, this aborted inside LoanPay::doApply on + // XRPL_ASSERT_PARTS(goodRounding, "xrpl::LoanPay::doApply", "funds + // are conserved (with rounding)"). + env(loan::pay(borrower, loanKeylet.key, totalDue)); + env.close(); + + auto const borrowerAfter = rawBalance(borrower.id()); + auto const vaultAfter = rawBalance(vaultPseudo); + auto const lenderAfter = rawBalance(lender.id()); + + // The broker fee reached the lender's AccountRoot, even though the + // lender's balance remains below its reserve. + BEAST_EXPECT(lenderAfter > lenderBefore); + BEAST_EXPECT(lenderAfter < lenderReserve()); + + // Total funds conserved across the payer, vault, and fee payee. + BEAST_EXPECT( + borrowerBefore - baseFee + vaultBefore + lenderBefore == + borrowerAfter + vaultAfter + lenderAfter); + } + void runAmendmentIndependent() { @@ -741,6 +847,7 @@ private: #if LOAN_TODO testLoanPayLateFullPaymentBypassesPenalties(features); #endif + testLoanPayFundsConservedPayeeBelowReserve(features); testOverpaymentManagementFee(features); testDosLoanPay(features); testLoanNextPaymentDueDateOverflow(features); diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp index abd669d446..230c802022 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -75,11 +75,10 @@ getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& if (treeNode.isLeaf()) { auto const key = leafKey(treeNode); - auto const expectedID = SHAMapNodeID::createID(nodeID->getDepth(), key); SOMETIMES( - nodeID->getNodeID() != expectedID.getNodeID(), + !nodeID->isPrefixOf(key), "xrpl::getSHAMapNodeID : legacy leaf ID inconsistent with key"); - if (nodeID->getNodeID() != expectedID.getNodeID()) + if (!nodeID->isPrefixOf(key)) return std::nullopt; }