fix: Eliminate cross-scale arithmetic in Loan default and payment paths - #8002
fix: Eliminate cross-scale arithmetic in Loan default and payment paths#8002Tapanito wants to merge 10 commits into
Conversation
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the cross-scale arithmetic fix in LoanManage::defaultLoan and LoanPay::doApply. Traced the new unified-scale math in defaultLoan: given the pre-mutation guard Total - Available >= totalDefaultAmount, the update Total += defaultCovered - totalDefaultAmount and Available += defaultCovered algebraically preserves Available <= Total (new Total-Available = old Total-Available - totalDefaultAmount >= 0), so the invariant the fix claims to guarantee does check out. The LoanPay.cpp change is a straightforward rename/removal of the pre-round-down step, gated correctly behind the amendment, and the pre-amendment branches are preserved verbatim in both files. Test coverage (amendmentCombinations additions plus the new multi-loan drift regression test) looks appropriately targeted. No confident correctness or security issues found in the added lines; one point worth a quick sanity check from the author below.
b7e1de5 to
5577672
Compare
There was a problem hiding this comment.
Well-structured, well-tested change that gates a cleaner vault-arithmetic path behind fixCleanup3_4_0 and preserves the legacy path byte-for-byte. The core algebra in LoanManage::defaultLoan (Total -= writeOff, Available += amount, using the same STAmount amount on both sides so it cancels exactly) is sound regardless of rounding direction for defaultCovered. The one thing I couldn't fully convince myself of from the diff alone is whether the independent STAmount conversion of totalDefaultAmount (writeOff) could ever round upward past what the raw-Number pre-check verified, and symmetrically whether dropping the vaultScale downward-rounding in LoanPay could ever trip the assetsAvailable <= assetsTotal assert for non-integral IOUs. Both are flagged below as confirmation questions rather than asserted bugs, since the new regression test (testMultiLoanDefaultDriftFixVsLegacy) and the loanScale>=vaultScale clamp argument in the description make actual violation unlikely in practice.
Tapanito
changed the title
5577672 to
de75528
Compare
There was a problem hiding this comment.
The vault‑side accounting unification in LoanManage::defaultLoan and LoanPay::doApply is symmetric with the pre‑amendment paths (arithmetic checked term-by-term) and is exercised by new tests (testMultiLoanDefaultDriftFixVsLegacy plus expanded amendmentCombinations) that assert both branches converge on identical, invariant-safe state. I could not find a clear correctness bug in the added (+) lines themselves; the design is well-reasoned and the legacy code path is preserved verbatim for existing ledgers. The two items below are confirmation questions rather than confirmed bugs, worth a quick sanity check before merge given this touches amendment-activation safety on live ledger state.
de75528 to
14786c1
Compare
Gated behind fixCleanup3_4_0. Pre-amendment, `defaultLoan` mutated
`sfAssetsTotal` and `sfAssetsAvailable` with values at different scales --
`vaultDefaultAmount` rounded down to `vaultScale`, but `defaultCovered`
kept at the finer loan scale. That asymmetry could leave
`sfAssetsAvailable > sfAssetsTotal` from the arithmetic alone, and was
patched by a dust-reconciliation branch that snapped `Total` up to
`Available`. That snap effectively minted phantom assets on `Total`
(see `LoanRounding_test::testDustManipulation`).
Under the fix, the vault-side update is composed from two well-defined
STAmount operations at the vault asset's own scale:
(1) Write-off: `sfAssetsTotal -= totalDefaultAmount` -- the defaulted
loan's exposure leaves the vault.
(2) Symmetric cash inflow: `defaultCovered` returns from first-loss
capital. Apply the same STAmount to both `sfAssetsTotal` and
`sfAssetsAvailable`, preserving the gap.
Because both deltas are STAmounts of the same asset, no cross-scale
rounding step is needed and `Available > Total` cannot occur from
arithmetic. The dust-branch snap and its downstream `tecINTERNAL`
guard are unreachable under the fix and only run on the pre-amendment
path.
The pre-mutation sanity check on the fix path tightens to the correct
invariant (`sfAssetsTotal - sfAssetsAvailable >= totalDefaultAmount`);
the pre-amendment path retains its original `vaultTotalProxy <
vaultDefaultAmount` check for byte-for-byte compatibility with existing
ledgers.
`LoanRounding_test` now runs `testDustManipulation` (and the other
amendment-sensitive rounding tests) under both branches by adding
`fixCleanup3_4_0` to the `amendmentCombinations` matrix. Both branches
land the same post-default equality for the specific test setup; the
fix path reaches it by clean arithmetic rather than a snap.
Gated behind fixCleanup3_4_0. Pre-amendment, the payment path used the same cross-scale pattern that `LoanManage::defaultLoan` addressed: `totalPaidToVault` was rounded down to `vaultScale` before being applied to `sfAssetsAvailable`, while `assetsTotalDelta` (`valueChange` for accrual, `interestPaid` for cash-basis) was applied at the finer loan scale. The `sfAssetsAvailable <= sfAssetsTotal` invariant was only defended by an assertion after the fact. Under the fix, drop the `roundToAsset(..., vaultScale, Downward)` step and apply the raw `totalPaidToVault` to both the ledger update and the cash transfer via `accountSendMulti`. The pseudo-account balance and `sfAssetsAvailable` therefore land on the same STAmount value without an intermediate scale reduction. For integral assets this is a no-op (the removed rounding was already a no-op there). For non-integral IOUs, the vault receives the full raw payment (up to 1 vaultScale ULP more than pre-amendment) and the existing `tecPRECISION_LOSS` / `tecINTERNAL` post-checks are no longer needed as safety nets against an arithmetic-induced Available > Total. `LoanPay_test` now runs the amendment-sensitive tests under both branches by adding `fixCleanup3_4_0` to the `amendmentCombinations` matrix, matching the sibling change to `LoanRounding_test`.
Adds testMultiLoanDefaultDriftFixVsLegacy which runs the same drift-inducing scenario (3 loans, different scales, partial payments, impairment, then default) under both !fixCleanup3_4_0 and fixCleanup3_4_0 and confirms both paths leave the vault invariant-safe. Empirically the two branches produce identical post-default state because computeLoanProperties clamps loanScale >= vaultScale (via std::max(minimumScale, amount.exponent())), so the pre-amendment roundToAsset(vaultDefaultAmount, vaultScale, Down) is a no-op for well-formed loans. The dust-reconciliation branch this test guards against is doubly-defensive code — reachable only from a corrupted ledger, not a valid transaction sequence. The test acts as a regression sensor: any future change that breaks the invariant on either branch (or lets them diverge) will surface here.
14786c1 to
c12e473
Compare
There was a problem hiding this comment.
This is a well-scoped, thoughtfully documented amendment-gated fix. The pre-amendment paths are preserved byte-for-byte and the new unified-arithmetic paths are covered by a dedicated cross-branch regression test. I couldn't find a clear, provable bug in the changed lines — the one thing worth a second pair of eyes is a subtle rounding-direction question in the new defaultLoan guard, flagged below as a question rather than an assertion.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
This is a careful, well-documented amendment-gated change that unifies the STAmount normalization path for vault Total/Available mutations in defaultLoan and doApply, removing two defensive workarounds (the dust snap and the pre-rounding step). The pre-amendment paths are preserved byte-for-byte and a new cross-branch regression test asserts bit-identical output. The core arithmetic checks out under manual verification, but two spots are worth a second look before merging since they touch the exact invariant the amendment is designed to guarantee.
There was a problem hiding this comment.
This is a well-reasoned, heavily-commented arithmetic refactor in the vault accounting paths (LoanManage::defaultLoan, LoanPay::doApply), gated behind fixCleanup3_4_0, with matching pre-amendment paths preserved verbatim and a dedicated regression test (testMultiLoanDefaultDriftFixVsLegacy) asserting bit-identical results between the legacy and fixed branches. I traced the new unified-STAmount arithmetic in defaultLoan (amount - writeOff applied to sfAssetsTotal, amount applied to sfAssetsAvailable) against the pre-amendment semantics (Total -= vaultDefaultAmount, Available += defaultCovered) and they match algebraically. The tightened pre-mutation invariant check (Total - Available >= totalDefaultAmount) is correctly implemented and justified by the ValidVault invariant. The LoanPay.cpp change symmetrically removes the vaultScale rounding on totalPaidToVault under the amendment, consistent with the stated design. I did not find a concrete, high-confidence bug in the added (+) lines; the one subtlety worth a human sanity-check (whether combining amount-writeOff as a single STAmount subtraction before adding to sfAssetsTotal re-normalizes differently than applying amount and writeOff independently) is already covered by the bit-identical regression test the author added, so I'm not flagging it as an issue absent evidence it fails.
There was a problem hiding this comment.
This PR reformulates the vault-side accounting in LoanManage::defaultLoan and LoanPay::doApply to route sfAssetsTotal/sfAssetsAvailable mutations through a single asset-typed STAmount, gated behind fixCleanup3_4_0, with the legacy path preserved verbatim for existing ledgers. The arithmetic and invariant reasoning in both production files checks out under careful tracing (sign conventions, guard tightening, and the moved loss-realization block are all consistent), and the new regression test cross-checks both amendment branches. The one issue worth fixing is a factual inaccuracy in a newly added explanatory comment in LoanPay.cpp that misdescribes the pre-amendment behavior.
There was a problem hiding this comment.
This MR reformulates LoanManage::defaultLoan and LoanPay::doApply to push sfAssetsTotal/sfAssetsAvailable mutations through symmetric STAmount normalization under fixCleanup3_4_0, replacing asymmetric cross-scale Number/STAmount round-trips. I traced the algebra: the new Total += amount - writeOff / Available += amount formulation reduces to the same net deltas as the pre-amendment Total -= vaultDefaultAmount / Available += defaultCovered path, so the arithmetic is consistent. The pre-amendment branch is preserved verbatim in the else-arm, the tightened pre-mutation guard (T - A >= totalDefaultAmount) is strictly stronger but justified by the standing ValidVault invariant (Total >= Available always holds, and pre-amendment rounding could only inflate the gap, never shrink it), and the loss-realization/view.update logic was correctly hoisted to run unconditionally for both branches. The LoanPay.cpp change symmetrically drops the vaultScale rounding on the fix path and threads the same raw value into both the ledger field and the trust-line transfer amount, matching the stated intent. New regression test (testMultiLoanDefaultDriftFixVsLegacy) exercises a realistic multi-loan/partial-payment/impairment scenario under both amendment states and asserts bit-identical outcomes, which is solid coverage for exactly the kind of drift this fix targets. I did not find any lines added in this diff that introduce a clear correctness, security, or resource-management bug worth flagging with confidence — the change is well-reasoned, the math checks out algebraically, and the amendment gating/compatibility story is sound.
gregtatcam
left a comment
gregtatcam
left a comment
There was a problem hiding this comment.
👍 LGTM
Consider two comment update suggestions.
There was a problem hiding this comment.
Pull request overview
This PR fixes cross-scale arithmetic drift in vault accounting during loan default (LoanManage::defaultLoan) and loan payments (LoanPay::doApply) by gating new symmetric, asset-typed normalization behavior behind the fixCleanup3_4_0 amendment. It also extends the rounding/invariant test matrix to exercise both pre- and post-amendment behaviors.
Changes:
- Update
LoanPay::doApplyto apply a single payment value symmetrically to both the vault ledger field and the pseudo-account transfer underfixCleanup3_4_0. - Update
LoanManage::defaultLoanto mutatesfAssetsTotal/sfAssetsAvailablevia a shared asset-typed STAmount normalization path underfixCleanup3_4_0, keeping the legacy behavior unchanged when the amendment is disabled. - Extend lending tests so amendment-combination runs include
fixCleanup3_4_0, and add a regression scenario comparing legacy vs fixed outcomes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/test/app/lending/LoanRounding_test.cpp | Adds a regression scenario and includes fixCleanup3_4_0 in amendment combinations. |
| src/test/app/lending/LoanPay_test.cpp | Includes fixCleanup3_4_0 in amendment combinations for payment-path tests. |
| src/libxrpl/tx/transactors/lending/LoanPay.cpp | Gates removal of pre-rounding so the vault field and trust-line transfer normalize symmetrically under fixCleanup3_4_0. |
| src/libxrpl/tx/transactors/lending/LoanManage.cpp | Gates unified STAmount-based arithmetic in default handling under fixCleanup3_4_0, preserving legacy behavior otherwise. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
There was a problem hiding this comment.
This PR reformulates the vault-side accounting in LoanManage::defaultLoan and LoanPay::doApply to route sfAssetsTotal/sfAssetsAvailable mutations through symmetric STAmount conversions, gated behind fixCleanup3_4_0. I manually re-derived the arithmetic: Total_new - Available_new = (Total - Available) - totalDefaultAmount, which confirms the new pre-mutation guard (T - A >= totalDefaultAmount) is exactly necessary and sufficient to preserve the Available <= Total invariant after the mutation — the sign/derivation is correct, not a bug. The LoanPay change (dropping the vaultScale round-down on totalPaidToVault) is a straightforward removal of an asymmetric rounding step, consistently threaded through the ledger update, the assertion, the logging, and the accountSendMulti call. Both changes are extensively self-documented in-code with the exact reasoning for amendment-activation safety, and the new regression test cross-checks bit-identical outcomes between the legacy and fixed branches for well-formed inputs. I did not find a correctness, security, or resource-management issue in the added lines that I'm confident about; the change is well-scoped, amendment-gated, and consistent with the existing codebase's rounding/invariant conventions.
There was a problem hiding this comment.
This is a well-reasoned, heavily-documented refactor that reformulates vault-side accounting in LoanManage::defaultLoan and LoanPay::doApply to use symmetric STAmount normalization under fixCleanup3_4_0, replacing asymmetric Number->STAmount round-trips. I traced the arithmetic in both call sites against the pre-amendment behavior they replace (Total -= vaultDefaultAmount / Available += defaultCovered becomes Total += amount - writeOff / Available += amount, where amount and writeOff are both STAmount-typed) and it checks out mathematically. The new pre-mutation guard in defaultLoan is a strictly stronger, and per the ValidVault invariant safe, replacement for the removed dust-snap logic. The unconditional debug-assert pattern used for the new invariant check is consistent with the existing convention already present in LoanPay.cpp (which asserts rather than hard-fails on the same invariant). The test additions exercise both amendment branches directly against each other and assert bit-identical results, which is a strong regression sensor for exactly the drift this MR removes. I did not find any high-confidence correctness, security, or resource-management issues in the added lines; the change appears internally consistent with its extensive description and existing codebase conventions.
There was a problem hiding this comment.
The diff reformulates LoanManage::defaultLoan and LoanPay::doApply to mutate sfAssetsTotal/sfAssetsAvailable through a single asset-typed STAmount under fixCleanup3_4_0, matching the PR description closely. I traced the arithmetic (writeOff/amount decomposition, the tightened pre-mutation guard, and the raw-vs-rounded totalPaidToVault change in LoanPay) against the pre-amendment behavior and confirmed the new paths are algebraically equivalent to the old ones (Total_new = Total - totalDefaultAmount + defaultCovered, Available_new = Available + defaultCovered), with the amendment gating and dead-code preservation done correctly. The view.update(vaultSle) call, previously nested only inside the pre-amendment scope, is now correctly hoisted to run unconditionally after both branches with no change in call frequency. The one subtlety — whether STAmount-normalizing writeOff from totalDefaultAmount could round up past the Number-precision guard and violate the available<=total invariant — is explicitly addressed and justified in the added code comment (loanScale clamp bounds the difference losslessly to 16 significant digits), and I found no counter-evidence, so I'm not flagging it. New tests appropriately extend amendmentCombinations and add a dedicated legacy-vs-fixed drift regression test. No confident bugs, security issues, or correctness problems found in the added lines.
Tapanito
added
Ready to merge
Stack
This PR is the base of the Vault dust-custody stack:
Landing #7983 (and everything downstream) is not safe without this fix: the vault mutations centralised in
VaultHelpersassume the symmetric-STAmount arithmetic introduced here, and the dust-custody design layered on top (#8016 → #8018) relies onsfAssetsTotal/sfAssetsAvailablebeing reconcilable through a single normalised STAmount pair rather than the pre-amendment defensive dust snap.Summary
Fix cross-scale arithmetic in the vault-side accounting for
LoanManage::defaultLoanandLoanPay::doApply, gated behindfixCleanup3_4_0. Both call sites previously mutatedsfAssetsTotalandsfAssetsAvailablewith values obtained through different Number->STAmount paths (one vaultScale-rounded, the other kept at loan-scale precision), and papered over the resulting drift with defensive branches (a dust-reconciliation snap indefaultLoanthat pinnedTotalup toAvailable, and a pre-round-down inLoanPaycombined with a post-application invariant assertion).Under the amendment both sites are reformulated 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 -- the defensive branches become unreachable. The mechanism is symmetric STAmount normalization, not "scale unification": STAmount construction normalizes the underlying Number to STAmount precision (16 significant digits for IOU), and by applying the same STAmount to both
sfAssetsTotalandsfAssetsAvailable, both fields absorb an identical residual.Changes
LoanManage::defaultLoantotalDefaultAmountfromsfAssetsTotalviawriteOff = STAmount{vaultAsset, totalDefaultAmount}, (2) applyamount = STAmount{vaultAsset, defaultCovered}symmetrically to bothsfAssetsTotalandsfAssetsAvailable. Because the sharedamountis normalized to STAmount precision exactly once and absorbed by both fields,sfAssetsAvailablecannot overshootsfAssetsTotalfrom arithmetic alone.sfAssetsTotal - sfAssetsAvailable >= totalDefaultAmount). Amendment activation is safe:ValidVaultenforcesTotal >= Availablecontinuously, and pre-amendment cross-scale rounding could only inflateTotal - Available, never deflate it.tecINTERNALguard) for byte-for-byte compatibility with existing ledgers.LoanPay::doApplyroundToAsset(..., vaultScale, Downward)step ontotalPaidToVault; applies the raw value to both the ledger update and the cash transfer. The ledger field is normalized through STNumber viaassociateAsset(*vaultSle, asset)and the trust-line balance is normalized as STAmount byaccountSendMulti, so the two sinks converge on the same STAmount value.LoanRounding_test/LoanPay_test:fixCleanup3_4_0added toamendmentCombinationsso the amendment-sensitive rounding tests run under both branches.LoanRounding_test::testMultiLoanDefaultDriftFixVsLegacynew: exercises a multi-loan + partial-payment + impairment lifecycle under both branches, asserts the vault invariant holds on both, and asserts the two branches land on bit-identicalsfAssetsTotal/sfAssetsAvailablefor legitimate transaction inputs (per the loanScale >= vaultScale clamp).Amendment gating
Behind
fixCleanup3_4_0(already declared,Supported::Yes/VoteBehavior::DefaultNo). Existing ledgers hit the pre-amendment path unchanged; new ledgers under the amendment take the clean arithmetic.Notes on the dust snap
Empirically the pre-amendment dust-reconciliation branch appears unreachable through public transactions:
computeLoanPropertiesclampsloanScale >= vaultScale(viastd::max(minimumScale, amount.exponent())), soroundToAsset(vaultDefaultAmount, vaultScale, Downward)is a no-op for well-formed loans. The removal is therefore a code-quality improvement -- the fix formalizes what the loan-scale clamp already guarantees, so the vault mutations no longer rely on a defensive snap to preserve the invariant.