fix: Add VaultInvariant check that lossUnrealized is non-negative (FN-32) - #7863
Open
tyalymov wants to merge 1 commit into
Open
fix: Add VaultInvariant check that lossUnrealized is non-negative (FN-32)#7863tyalymov wants to merge 1 commit into
tyalymov wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Tapanito
reviewed
Jul 27, 2026
tyalymov
force-pushed
the
FN-32-vault-invariant-check-loss-unrealized-non-negative
branch
from
July 28, 2026 11:57
0ccf141 to
e729fc5
Compare
Tapanito
reviewed
Jul 28, 2026
Tapanito
reviewed
Jul 28, 2026
Tapanito
left a comment
Tapanito
left a comment
Contributor
There was a problem hiding this comment.
Left a small nit, other than that lgtm.
tyalymov
added
the
Ready to merge
tyalymov
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens the ValidVault::finalize invariant checks by adding a lower-bound validation for sfLossUnrealized (ensuring it cannot be negative when fixCleanup3_4_0 is enabled), and adds a unit test to assert both post- and pre-amendment behavior.
Changes:
- Add an invariant failure when
lossUnrealized < 0underfixCleanup3_4_0. - Add a unit test that forces
lossUnrealized = -1and verifies invariant behavior with and without the amendment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libxrpl/tx/invariants/VaultInvariant.cpp | Adds an amendment-gated lower-bound check rejecting negative lossUnrealized. |
| src/test/app/Invariants_test.cpp | Adds coverage to ensure negative lossUnrealized trips the invariant only when the amendment is enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4 tasks
…non-negative The VaultInvariant validated the upper bound on the vault's sfLossUnrealized field but never the lower bound. A negative lossUnrealized would pass the invariant and cause the withdraw math (assetTotal -= lossUnrealized) to use an effective asset total greater than assetsTotal, letting withdrawers extract more assets than the vault tracks. Add a defense-in-depth check that lossUnrealized must not be negative. Because featureSingleAssetVault is already released, the check is gated behind fixCleanup3_4_0 to avoid introducing a new transaction failure path that could fork the network. The test uses ttLOAN_MANAGE (permitted to change lossUnrealized, isolating the check) plus a paired case with the amendment disabled asserting the transaction still succeeds. Ref: FN-32.
tyalymov
force-pushed
the
FN-32-vault-invariant-check-loss-unrealized-non-negative
branch
from
July 30, 2026 14:49
96174dc to
fce9ff6
Compare
Tapanito
approved these changes
Jul 30, 2026
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.
High Level Overview of Change
ValidVault::finalize(the VaultInvariant) validated the upper bound on the vault'ssfLossUnrealizedfield but never the lower bound. This adds a defense-in-depth checkthat
lossUnrealizedmust be non-negative, plus a unit test.Ref: FN-32.
Context of Change
The invariant already checks
lossUnrealized <= assetsTotal - assetsAvailable,assetsTotal >= 0, andassetsAvailable >= 0, but notlossUnrealized >= 0.A negative
lossUnrealizedwould pass all existing checks. Downstream, the withdrawmath in
VaultHelperscomputesassetTotal -= lossUnrealized; with a negative loss thisinflates the effective asset total above
assetsTotal, which would let withdrawersextract more assets than the vault tracks.
Today the only writers of
sfLossUnrealized(theLoanManageimpair / unimpair / defaultpaths) already guard against underflow, so
lossUnrealizedcannot currently go negative —there is no mainnet exposure. This change is purely defense-in-depth: if a future lending
bug drove the field negative, the invariant would now catch it instead of allowing a
silent over-withdrawal.
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Test Plan
Added a unit test in
Invariants_test.cppthat setslossUnrealizednegative on a vaultand confirms the invariant fails with "loss unrealized must be positive". Verified the test
fails without the fix and passes with it; both test sets are green (
xrpld --unittestandctest --preset conan-debug).