fix: Pin overpayment principal reduction to exact on-grid value#7360
Open
Tapanito wants to merge 2 commits into
Open
fix: Pin overpayment principal reduction to exact on-grid value#7360Tapanito wants to merge 2 commits into
Tapanito wants to merge 2 commits into
Conversation
doOverpayment asserted that trackedPrincipalDelta exactly equals the reduction in stored principal after re-amortization. The assertion was failing because tryOverpayment re-derived the new principal from the new periodic payment via a (P * factor) / factor round-trip, which is not exact in Number arithmetic. A positive residual, combined with Upward rounding, pushed the stored principal one scale-unit high, making the principal drop by delta - 1 unit rather than delta. Under fixCleanup3_2_0, pin the new principal to the exact on-grid reduction (roundedOldState.principalOutstanding - trackedPrincipalDelta) instead. Both operands are already at loan scale, so the subtraction is exact and requires no further rounding. Interest is re-derived via constructLoanState so the V = P + I + M invariant still holds. The three assertions in doOverpayment that encode this invariant are also gated behind the same amendment, since they only hold once the fix ensures the exact reduction. Adds testBugOverpaymentPrincipalChange to reproduce the core-dump scenario (principal=100, rate=1, n=3, interval=60, one period + residual overpayment of 0.049999998) and testLoanSetNearZeroInterestRateSucceeds to verify scheduled payments cover principal at InterestRate=1.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Tapanito
force-pushed
the
tapanito/lending-disable-assertions
branch
from
1fc141f to
c737fe2
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a debug-assertion abort in doOverpayment ("principal change agrees") caused by a (P*factor)/factor round-trip in tryOverpayment that left the re-amortized principal one scale-unit above the exact on-grid target. Under fixCleanup3_2_0, the new principal is now pinned to roundedOldState.principalOutstanding - trackedPrincipalDelta and the three related invariant assertions are gated on the same amendment. Two regression tests are added.
Changes:
- Pin re-amortized principal to exact on-grid value in
tryOverpaymentunderfixCleanup3_2_0, deriving interest viaconstructLoanState. - Gate three invariant assertions in
doOverpayment(principal change agrees,interest paid agrees,principal payment matches) behindfixCleanup3_2_0. - Add
testBugOverpaymentPrincipalChangeandtestLoanSetNearZeroInterestRateSucceedsregression tests. - Unrelated: deletes
cfg/xrpld-example.cfgandcfg/validators-example.txt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libxrpl/ledger/helpers/LendingHelpers.cpp | Pin principal in tryOverpayment and amendment-gate three doOverpayment invariants. |
| src/test/app/Loan_test.cpp | Add regression tests for the overpayment crash and near-zero-rate LoanSet. |
| cfg/xrpld-example.cfg | Deletes example config that is still referenced by install/packaging — appears out of scope. |
| cfg/validators-example.txt | Deletes example validators file still referenced by install/packaging — appears out of scope. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7360 +/- ##
=========================================
- Coverage 82.4% 82.4% -0.0%
=========================================
Files 1011 1011
Lines 76477 76483 +6
Branches 7322 7314 -8
=========================================
Hits 63005 63005
- Misses 13472 13478 +6
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
a1q123456
requested changes
May 30, 2026
| // pushes the stored principal one scale-unit high and breaks the | ||
| // "principal change agrees" check. Pin the principal to the exact value and | ||
| // re-derive interest so the V = P + I + M invariant still holds. | ||
| if (rules.enabled(fixCleanup3_2_0)) |
Contributor
There was a problem hiding this comment.
If we're going to rewrite the variable anyway, it'll be nice avoiding calling computeTheoreticalLoanState when fixCleanup3_2_0 is enabled.
| } | ||
|
|
||
| // Gated together with the principal-change check above (same rationale). | ||
| if (rules.enabled(fixCleanup3_2_0)) |
Contributor
There was a problem hiding this comment.
This if can be merged with the if block above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
doOverpayment asserted that trackedPrincipalDelta exactly equals the reduction in stored principal after re-amortization. The assertion was failing because tryOverpayment re-derived the new principal from the new periodic payment via a (P * factor) / factor round-trip, which is not exact in Number arithmetic. A positive residual, combined with Upward rounding, pushed the stored principal one scale-unit high, making the principal drop by delta - 1 unit rather than delta.
Under fixCleanup3_2_0, pin the new principal to the exact on-grid reduction (roundedOldState.principalOutstanding - trackedPrincipalDelta) instead. Both operands are already at loan scale, so the subtraction is exact and requires no further rounding. Interest is re-derived via constructLoanState so the V = P + I + M invariant still holds. The three assertions in doOverpayment that encode this invariant are also gated behind the same amendment, since they only hold once the fix ensures the exact reduction.
Adds testBugOverpaymentPrincipalChange to reproduce the core-dump scenario (principal=100, rate=1, n=3, interval=60, one period + residual overpayment of 0.049999998) and
testLoanSetNearZeroInterestRateSucceeds to verify scheduled payments cover principal at InterestRate=1.
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)