Merge remote-tracking branch 'origin/develop' into tapanito/fix-lending-cross-scale

Address review comments on PR #8002:

- LoanPay.cpp: pre-amendment description incorrectly claimed the vault
  pseudo-account received the raw Number. It received the vaultScale-
  rounded value via accountSendMulti; the real asymmetry is vault-side
  between sfAssetsAvailable (rounded) and sfAssetsTotal (unrounded via
  assetsTotalDelta).
- LoanManage.cpp: replace the associateAsset-based justification for the
  T - A >= totalDefaultAmount guard with the actual invariant: the
  fields loanVaultExposure differences are whole multiples of the same
  immutable 10^sfLoanScale and never outgrow STAmount's 16 digits, so
  the promotion round-trips losslessly. Also drops the misleading
  "vault-associated" descriptor (the underlying fields live on the
  loanSle).
- LoanRounding_test.cpp: the "loan scale ~ -13 vs vaultScale ~ -9" note
  is impossible under computeLoanProperties (which clamps loanScale to
  max(vaultScale, amount.exponent())). Rewrite to describe the actual
  loanScale == vaultScale regime the scenario exercises.

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

@@ -3,6 +3,7 @@
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/Number.h>
 #include <xrpl/beast/utility/Zero.h>
+#include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/View.h>
@@ -169,8 +170,6 @@ LoanManage::defaultLoan(
         return std::min(covered, coverAvailable);
     }();
 
-    auto const vaultDefaultAmount = totalDefaultAmount - defaultCovered;
-
     // Update the Vault object:
 
     // The vault may be at a different scale than the loan. Reduce rounding
@@ -178,7 +177,75 @@ LoanManage::defaultLoan(
     // scale.
     auto const vaultScale = getAssetsTotalScale(vaultSle);
 
+    // Under fixCleanup3_4_0, both vault-side fields are mutated through a
+    // single asset-typed STAmount pair. `amount = STAmount{vaultAsset,
+    // defaultCovered}` is applied to both sfAssetsTotal and
+    // sfAssetsAvailable, and `writeOff = STAmount{vaultAsset,
+    // totalDefaultAmount}` is applied to sfAssetsTotal only. Because the
+    // shared `amount` is normalized to STAmount precision exactly once and
+    // absorbed by both fields symmetrically, sfAssetsAvailable cannot
+    // overshoot sfAssetsTotal from arithmetic alone -- the residual is
+    // exactly `-writeOff` on Total. Pre-amendment, the two fields were
+    // adjusted via values obtained through different Number->STAmount
+    // paths (vaultDefaultAmount rounded down to vaultScale on one side,
+    // defaultCovered kept at the finer loan scale on the other), so the
+    // normalization was asymmetric and a dust-reconciliation snap was
+    // required to paper over the resulting cross-side mismatch -- a snap
+    // that itself minted phantom assets on sfAssetsTotal. See the
+    // sibling change in LoanPay for the same fix on the payment path.
+    //
+    // The Number-precision guard `T - A >= totalDefaultAmount` below is
+    // exactly sufficient: loanVaultExposure returns a difference of
+    // STNumber fields that are whole multiples of the same immutable
+    // `10^sfLoanScale` and never outgrow its 16 digits, so its STAmount
+    // promotion round-trips losslessly with no writeOff slack.
+    bool const useUnifiedAssetArithmetic = view.rules().enabled(fixCleanup3_4_0);
+
+    if (useUnifiedAssetArithmetic)
     {
+        // Prior to realizing the default, the vault must already carry
+        // at least this loan's exposure. A violation here is a corrupt
+        // ledger, not a transaction-input error.
+        //
+        // This guard is strictly stronger than the pre-amendment
+        // `vaultTotalBefore < vaultDefaultAmount` form, but activation is
+        // safe: the ValidVault invariant continuously enforces Total >=
+        // Available (so Total - Available >= 0 on every ledger), and the
+        // pre-amendment cross-scale rounding could only inflate the
+        // difference Total - Available, never deflate it. Any ledger
+        // reaching this point post-activation will therefore already
+        // satisfy the stronger form.
+        Number const vaultTotalBefore = *vaultSle->at(sfAssetsTotal);
+        Number const vaultAvailableBefore = *vaultSle->at(sfAssetsAvailable);
+        if (vaultTotalBefore - vaultAvailableBefore < totalDefaultAmount)
+        {
+            // LCOV_EXCL_START
+            JLOG(j.warn()) << "Vault exposure is less than the loan default amount";
+            return tefBAD_LEDGER;
+            // LCOV_EXCL_STOP
+        }
+
+        // Mutate the two vault fields through a single asset-typed pair
+        // (see the block comment on useUnifiedAssetArithmetic above for
+        // why this is safe):
+        //   (1) write off totalDefaultAmount from Total only;
+        //   (2) add defaultCovered symmetrically to both fields.
+        STAmount const amount{vaultAsset, defaultCovered};
+        STAmount const writeOff{vaultAsset, totalDefaultAmount};
+
+        vaultSle->at(sfAssetsTotal) += amount - writeOff;
+        vaultSle->at(sfAssetsAvailable) += amount;
+
+        XRPL_ASSERT_PARTS(
+            *vaultSle->at(sfAssetsAvailable) <= *vaultSle->at(sfAssetsTotal),
+            "xrpl::LoanManage::defaultLoan",
+            "assets available must not be greater than assets outstanding");
+    }
+    else
+    {
+        // Pre-amendment behavior.
+        auto const vaultDefaultAmount = totalDefaultAmount - defaultCovered;
+
         // Decrease the Total Value of the Vault:
         auto vaultTotalProxy = vaultSle->at(sfAssetsTotal);
         auto vaultAvailableProxy = vaultSle->at(sfAssetsAvailable);
@@ -194,8 +261,8 @@ LoanManage::defaultLoan(
         auto const vaultDefaultRounded = roundToAsset(
             vaultAsset, vaultDefaultAmount, vaultScale, Number::RoundingMode::Downward);
         vaultTotalProxy -= vaultDefaultRounded;
-        // Increase the Asset Available of the Vault by liquidated First-Loss
+        // Increase the Asset Available of the Vault by liquidated
-        // Capital and any unclaimed funds amount:
+        // First-Loss Capital and any unclaimed funds amount:
         vaultAvailableProxy += defaultCovered;
         if (*vaultAvailableProxy > *vaultTotalProxy && !vaultAsset.integral())
         {
@@ -207,8 +274,8 @@ LoanManage::defaultLoan(
                             << "(" << difference.exponent() << ")";
             if (vaultAvailableProxy.value().exponent() - difference.exponent() > 13)
             {
-                // If the difference is dust, bring the total up to match
+                // If the difference is dust, bring the total up to
-                // the available
+                // match the available
                 JLOG(j.debug()) << "Difference between vault assets available and total is "
                                    "dust. Set both to the larger value.";
                 vaultTotalProxy = vaultAvailableProxy;
@@ -223,24 +290,24 @@ LoanManage::defaultLoan(
             return tecINTERNAL;
             // LCOV_EXCL_STOP
         }
-
-        // The loss has been realized
-        if (loanSle->isFlag(lsfLoanImpaired))
-        {
-            auto vaultLossUnrealizedProxy = vaultSle->at(sfLossUnrealized);
-            if (vaultLossUnrealizedProxy < totalDefaultAmount)
-            {
-                // LCOV_EXCL_START
-                JLOG(j.warn()) << "Vault unrealized loss is less than the default amount";
-                return tefBAD_LEDGER;
-                // LCOV_EXCL_STOP
-            }
-            adjustImpreciseNumber(
-                vaultLossUnrealizedProxy, -totalDefaultAmount, vaultAsset, vaultScale);
-        }
-        view.update(vaultSle);
     }
 
+    // The loss has been realized
+    if (loanSle->isFlag(lsfLoanImpaired))
+    {
+        auto vaultLossUnrealizedProxy = vaultSle->at(sfLossUnrealized);
+        if (vaultLossUnrealizedProxy < totalDefaultAmount)
+        {
+            // LCOV_EXCL_START
+            JLOG(j.warn()) << "Vault unrealized loss is less than the default amount";
+            return tefBAD_LEDGER;
+            // LCOV_EXCL_STOP
+        }
+        adjustImpreciseNumber(
+            vaultLossUnrealizedProxy, -totalDefaultAmount, vaultAsset, vaultScale);
+    }
+    view.update(vaultSle);
+
     // Update the LoanBroker object:
 
     {

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

@@ -469,10 +469,28 @@ LoanPay::doApply()
     auto assetsTotalProxy = vaultSle->at(sfAssetsTotal);
 
     auto const totalPaidToVaultRaw = paymentParts->principalPaid + paymentParts->interestPaid;
-    auto const totalPaidToVaultRounded =
+    // Under fixCleanup3_4_0, feed the same raw payment Number into both
-        roundToAsset(asset, totalPaidToVaultRaw, vaultScale, Number::RoundingMode::Downward);
+    // sfAssetsAvailable and the accountSendMulti transfer to the vault
+    // pseudo-account. Both sinks store the value as an asset-typed
+    // STAmount (the ledger field is normalized through STNumber via
+    // associateAsset(*vaultSle, asset) further below, and the pseudo
+    // account trust line stores an STAmount directly), so the IOU
+    // normalization is applied symmetrically and the two balances land on
+    // the same STAmount value -- which is what makes the debug invariant
+    // sfAssetsAvailable == pseudo-account balance hold. Pre-amendment,
+    // sfAssetsAvailable and the pseudo-account both received the
+    // vaultScale-rounded value (accountSendMulti was called with
+    // `totalPaidToVault`, i.e. the rounded amount), but sfAssetsTotal
+    // was mutated with the unrounded `assetsTotalDelta` on the very
+    // next line. That vault-side asymmetry between sfAssetsAvailable
+    // (rounded) and sfAssetsTotal (unrounded) is the counterpart of the
+    // cross-side mismatch `LoanManage::defaultLoan` addresses on the
+    // default path.
+    auto const totalPaidToVault = view.rules().enabled(fixCleanup3_4_0)
+        ? totalPaidToVaultRaw
+        : roundToAsset(asset, totalPaidToVaultRaw, vaultScale, Number::RoundingMode::Downward);
     XRPL_ASSERT_PARTS(
-        !asset.integral() || totalPaidToVaultRaw == totalPaidToVaultRounded,
+        !asset.integral() || totalPaidToVaultRaw == totalPaidToVault,
         "xrpl::LoanPay::doApply",
         "rounding does nothing for integral asset");
     auto const totalPaidToBroker = paymentParts->feePaid;
@@ -518,7 +536,7 @@ LoanPay::doApply()
     }
 #endif
 
-    assetsAvailableProxy += totalPaidToVaultRounded;
+    assetsAvailableProxy += totalPaidToVault;
     assetsTotalProxy += assetsTotalDelta;
 
     XRPL_ASSERT_PARTS(
@@ -527,13 +545,13 @@ LoanPay::doApply()
         "assets available must not be greater than assets outstanding");
 
     JLOG(j_.debug()) << "total paid to vault raw: " << totalPaidToVaultRaw
-                     << ", total paid to vault rounded: " << totalPaidToVaultRounded
+                     << ", total paid to vault: " << totalPaidToVault
                      << ", total paid to broker: " << totalPaidToBroker
                      << ", amount from transaction: " << amount;
 
     // Move funds
     XRPL_ASSERT_PARTS(
-        totalPaidToVaultRounded + totalPaidToBroker <= amount,
+        totalPaidToVault + totalPaidToBroker <= amount,
         "xrpl::LoanPay::doApply",
         "amount is sufficient");
 
@@ -627,7 +645,7 @@ LoanPay::doApply()
     // gate.
     bool const skipPayeeAuth = view.rules().enabled(fixCleanup3_4_0);
 
-    if (!skipPayeeAuth && totalPaidToVaultRounded != beast::kZero)
+    if (!skipPayeeAuth && totalPaidToVault != beast::kZero)
     {
         if (auto const ter = requireAuth(view, asset, vaultPseudoAccount, AuthType::StrongAuth))
             return ter;
@@ -662,7 +680,7 @@ LoanPay::doApply()
             view,
             accountID_,
             asset,
-            {{vaultPseudoAccount, totalPaidToVaultRounded}, {brokerPayee, totalPaidToBroker}},
+            {{vaultPseudoAccount, totalPaidToVault}, {brokerPayee, totalPaidToBroker}},
             j_,
             WaiveTransferFee::Yes))
         return ter;

src/test/app/lending/LoanPay_test.cpp

@@ -1450,7 +1450,7 @@ public:
     {
         runAmendmentIndependent();
         for (auto const& features : jtx::amendmentCombinations(
-                 {fixCleanup3_1_3, fixCleanup3_2_0, featureMPTokensV2}, all_))
+                 {fixCleanup3_1_3, fixCleanup3_2_0, fixCleanup3_4_0, featureMPTokensV2}, all_))
             runAmendmentSensitive(features);
     }
 };

src/test/app/lending/LoanRounding_test.cpp

@@ -38,6 +38,7 @@
 #include <ostream>
 #include <string>
 #include <tuple>
+#include <vector>
 
 namespace xrpl::test {
 
@@ -184,6 +185,233 @@ private:
         }
     }
 
+    // Regression sensor: multi-loan vault with payment history +
+    // impairment exercises the accrual-based accounting on both amendment
+    // branches, then defaults one of the loans. The point is to run the
+    // same drift-inducing scenario under `!fixCleanup3_4_0` and
+    // `fixCleanup3_4_0` and confirm that both branches leave the vault in
+    // the same, invariant-holding state.
+    //
+    // Semantically the two paths do different things:
+    //   * `!fixCleanup3_4_0` in `LoanManage::defaultLoan` computes
+    //     `roundToAsset(totalDefaultAmount - defaultCovered, vaultScale,
+    //     Downward)` and subtracts that from `sfAssetsTotal`, while adding
+    //     the (unrounded) `defaultCovered` to `sfAssetsAvailable`. A
+    //     downstream dust-reconciliation branch pins Total up to Available
+    //     if the arithmetic drifted them apart. Similarly `LoanPay` on
+    //     this branch pre-rounds `totalPaidToVault` down to vaultScale.
+    //   * `fixCleanup3_4_0` reformulates both call sites to feed each
+    //     ledger field through the same asset-typed STAmount, so both
+    //     sides absorb identical IOU/Number-round-trip normalization
+    //     and the two fields cannot diverge from arithmetic alone -- no
+    //     dust snap is needed.
+    //
+    // The scenario intentionally combines: large vault-vs-loan scale gap,
+    // multiple loans with different interest schedules, multiple partial
+    // payments, and impairment of the loan to be defaulted.
+    //
+    // Note: in practice the two branches produce bit-identical output for
+    // legitimate transaction inputs, because `computeLoanProperties`
+    // enforces `loanScale >= vaultScale` (see the `std::max(minimumScale,
+    // amount.exponent())` clamp on the loan-value STAmount exponent).
+    // That constraint makes `roundToAsset(vaultDefaultAmount, vaultScale,
+    // Down)` a no-op for well-formed loans, so `sfAssetsAvailable` cannot
+    // overshoot `sfAssetsTotal` from arithmetic alone. The dust-snap
+    // branch this test guards against is therefore doubly-defensive code
+    // — reachable only from a corrupted ledger state, not a valid
+    // transaction sequence. If either path ever leaves the invariant
+    // violated (or the paths diverge on the stored ledger state) this
+    // test will surface it.
+    void
+    testMultiLoanDefaultDriftFixVsLegacy(FeatureBitset features)
+    {
+        testcase("Multi-loan drift: default is invariant-safe on both branches");
+
+        using namespace jtx;
+        using namespace std::chrono_literals;
+
+        struct RunResult
+        {
+            Number totalBefore;
+            Number availableBefore;
+            Number totalAfter;
+            Number availableAfter;
+            TER defaultTer;
+        };
+
+        auto runScenario = [this](FeatureBitset scenarioFeatures) -> std::optional<RunResult> {
+            using namespace loan;
+            Env env{*this, scenarioFeatures};
+
+            Account const issuer{"issuer"};
+            Account const lender{"lender"};
+            Account const borrower{"borrower"};
+
+            env.fund(XRP(1'000'000'00), issuer, lender, borrower);
+            env.close();
+
+            PrettyAsset const asset = issuer["USD"];
+            env(trust(lender, asset(10'000'000'000)));
+            env(trust(borrower, asset(10'000'000'000)));
+            env(pay(issuer, lender, asset(5'000'000'000)));
+            env(pay(issuer, borrower, asset(5'000'000'000)));
+            env.close();
+
+            // Vault magnitude ~1e7 gives a coarse vaultScale; the loans
+            // below use principals sized so that
+            // `computeLoanProperties(..., minimumScale=vaultScale)` clamps
+            // `loanScale = std::max(vaultScale, amount.exponent())` down
+            // to `vaultScale`. That equal-scale regime is precisely the
+            // one where the pre-amendment paths still round vault-side
+            // mutations asymmetrically (rounded on sfAssetsAvailable and
+            // in the pseudo-account transfer, unrounded on sfAssetsTotal
+            // via assetsTotalDelta / defaultCovered), so the dust-snap
+            // and post-rounding invariant branches this test guards
+            // against get maximum exposure.
+            BrokerParameters const brokerParams{
+                .vaultDeposit = Number{10'000'000},
+                .debtMax = Number{0},
+                .coverRateMin = TenthBips32{1000},
+                .coverRateLiquidation = TenthBips32{2500}};
+
+            auto broker = createVaultAndBroker(env, asset, lender, brokerParams);
+            auto const& vaultKeylet = broker.vaultKeylet();
+
+            auto nextLoanKey = [&]() -> std::optional<Keylet> {
+                auto const brokerSle = env.le(keylet::loanBroker(broker.brokerID));
+                if (!BEAST_EXPECT(brokerSle))
+                    return std::nullopt;
+                auto const seq = brokerSle->at(sfLoanSequence);
+                return keylet::loan(broker.brokerID, SeqProxy::rawSequence(seq));
+            };
+
+            // Three loans with different principals and interest schedules,
+            // so each contributes drift-shaped rounding to the vault.
+            struct LoanSpec
+            {
+                Number principal;
+                std::uint32_t rateBps{};
+                std::uint32_t payTotal{};
+                std::uint32_t payInterval{};
+            };
+            std::array const specs{
+                LoanSpec{
+                    .principal = Number{100},
+                    .rateBps = 1922,
+                    .payTotal = 5816,
+                    .payInterval = 86400 * 6},
+                LoanSpec{
+                    .principal = Number{237},
+                    .rateBps = 1234,
+                    .payTotal = 4321,
+                    .payInterval = 86400 * 5},
+                LoanSpec{
+                    .principal = Number{389},
+                    .rateBps = 977,
+                    .payTotal = 3333,
+                    .payInterval = 86400 * 4},
+            };
+
+            std::vector<Keylet> loans;
+            for (auto const& spec : specs)
+            {
+                auto const keyOpt = nextLoanKey();
+                if (!keyOpt)
+                    return std::nullopt;
+                env(set(borrower, broker.brokerID, spec.principal),
+                    Sig(sfCounterpartySignature, lender),
+                    kInterestRate(TenthBips32{spec.rateBps}),
+                    kPaymentTotal(spec.payTotal),
+                    kPaymentInterval(spec.payInterval),
+                    kGracePeriod(spec.payInterval),
+                    Fee(env.current()->fees().base * 100));
+                env.close();
+                if (env.ter() != tesSUCCESS)
+                    return std::nullopt;
+                loans.push_back(*keyOpt);
+            }
+
+            // Accrue interest for a while, then make one small partial
+            // payment on each loan to force `LoanPay::doApply` to touch
+            // Total/Available with loan-scale-precision deltas.
+            STAmount const smallPay{asset.raw(), Number{1}};
+            auto const payFee = Fee(env.current()->fees().base * 10);
+            env.close(std::chrono::seconds(86400 * 3));
+            for (auto const& loanKey : loans)
+                env(pay(borrower, loanKey.key, smallPay, 0u), payFee);
+            env.close();
+
+            // Accrue more interest, another small payment round.
+            env.close(std::chrono::seconds(86400 * 3));
+            for (auto const& loanKey : loans)
+                env(pay(borrower, loanKey.key, smallPay, 0u), payFee);
+            env.close();
+
+            // Impair the first loan (charges `sfLossUnrealized`), then let
+            // it become defaultable.
+            env(jtx::loan::manage(lender, loans.front().key, tfLoanImpair), payFee);
+            env.close();
+            env.close(std::chrono::seconds(86400 * 40));
+
+            auto const vaultSleBefore = env.le(vaultKeylet);
+            if (!BEAST_EXPECT(vaultSleBefore))
+                return std::nullopt;
+            Number const totalBefore = vaultSleBefore->at(sfAssetsTotal);
+            Number const availableBefore = vaultSleBefore->at(sfAssetsAvailable);
+
+            env(jtx::loan::manage(lender, loans.front().key, tfLoanDefault));
+            auto const defaultTer = env.ter();
+            env.close();
+
+            auto const vaultSleAfter = env.le(vaultKeylet);
+            if (!BEAST_EXPECT(vaultSleAfter))
+                return std::nullopt;
+            Number const totalAfter = vaultSleAfter->at(sfAssetsTotal);
+            Number const availableAfter = vaultSleAfter->at(sfAssetsAvailable);
+
+            log << (env.current()->rules().enabled(fixCleanup3_4_0) ? "[fix=YES] " : "[fix=NO ] ")
+                << "before: T=" << totalBefore << " A=" << availableBefore
+                << " (T-A)=" << (totalBefore - availableBefore)
+                << " | default ter=" << transToken(defaultTer) << " | after: T=" << totalAfter
+                << " A=" << availableAfter << " (T-A)=" << (totalAfter - availableAfter)
+                << std::endl;
+
+            return RunResult{
+                .totalBefore = totalBefore,
+                .availableBefore = availableBefore,
+                .totalAfter = totalAfter,
+                .availableAfter = availableAfter,
+                .defaultTer = defaultTer};
+        };
+
+        auto const legacyOpt = runScenario(features - fixCleanup3_4_0);
+        auto const fixedOpt = runScenario(features | fixCleanup3_4_0);
+        if (!legacyOpt || !fixedOpt)
+        {
+            BEAST_EXPECT(legacyOpt && fixedOpt);
+            return;
+        }
+        auto const& legacy = *legacyOpt;
+        auto const& fixed = *fixedOpt;
+
+        // Invariant must hold on both branches.
+        BEAST_EXPECT(legacy.availableAfter <= legacy.totalAfter);
+        BEAST_EXPECT(fixed.availableAfter <= fixed.totalAfter);
+        // Default must succeed on both branches (the fix removes the
+        // possibility of `tecINTERNAL` from arithmetic drift, and the
+        // legacy path handles this scenario without needing the dust snap).
+        BEAST_EXPECT(legacy.defaultTer == tesSUCCESS);
+        BEAST_EXPECT(fixed.defaultTer == tesSUCCESS);
+        // For legitimate transaction inputs the two branches must land
+        // on the identical post-default vault state (see the note on
+        // `loanScale >= vaultScale` above). If a future change makes the
+        // paths diverge on well-formed loans -- e.g. by altering how
+        // sfAssetsTotal is rounded on one side but not the other -- this
+        // is the assertion that will fire.
+        BEAST_EXPECT(legacy.totalAfter == fixed.totalAfter);
+        BEAST_EXPECT(legacy.availableAfter == fixed.availableAfter);
+    }
+
     void
     testRoundingAllowsUndercoverage(FeatureBitset features)
     {
@@ -1226,6 +1454,7 @@ private:
         testBugVaultWithdrawDustVsAssetsTotal(all_ - fixCleanup3_4_0);
         testBugVaultWithdrawDustVsAssetsTotal(all_);
         testBugInterestDueDeltaCrash();
+        testMultiLoanDefaultDriftFixVsLegacy(all_);
     }
 
     // Tests run under each entry in amendmentCombinations().
@@ -1246,7 +1475,7 @@ public:
     {
         runAmendmentIndependent();
         for (auto const& features : jtx::amendmentCombinations(
-                 {fixCleanup3_1_3, fixCleanup3_2_0, featureMPTokensV2}, all_))
+                 {fixCleanup3_1_3, fixCleanup3_2_0, fixCleanup3_4_0, featureMPTokensV2}, all_))
             runAmendmentSensitive(features);
     }
 };