Merge remote-tracking branch 'origin/develop' into a1q123456/split-loan-set-and-loan-accept-implementation

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

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<std::logic_error>("Request for child node ID of " + to_string(*this));
 
-    if (id_ != (id_ & depthMask(depth_)))
+    if (!isPrefixOf(id_))
         Throw<std::logic_error>("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<SHAMapNodeID>
 deserializeSHAMapNodeID(void const* data, std::size_t size)
 {

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())

src/libxrpl/tx/transactors/lending/LoanPay.cpp

@@ -2,6 +2,7 @@
 
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/Number.h>
+#include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/utility/Zero.h>
 #include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/json/to_string.h>
@@ -9,6 +10,8 @@
 #include <xrpl/ledger/View.h>
 #include <xrpl/ledger/helpers/LendingHelpers.h>
 #include <xrpl/ledger/helpers/TokenHelpers.h>
+#include <xrpl/protocol/AccountID.h>
+#include <xrpl/protocol/Asset.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/LedgerFormats.h>
@@ -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.
         //

src/test/app/lending/LoanPay_test.cpp

@@ -5,6 +5,7 @@
 #include <test/jtx/amount.h>
 #include <test/jtx/fee.h>
 #include <test/jtx/jtx_json.h>
+#include <test/jtx/noop.h>
 #include <test/jtx/pay.h>
 #include <test/jtx/ter.h>
 #include <test/jtx/trust.h>
@@ -13,6 +14,7 @@
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/beast/utility/Zero.h>
 #include <xrpl/json/json_value.h>
+#include <xrpl/ledger/helpers/LendingHelpers.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/Issue.h>
@@ -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);

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;
     }