[WIP] Vault dust: solution B' — sfDust on the custody trust line - #7970
Draft
Tapanito wants to merge 14 commits into
Draft
[WIP] Vault dust: solution B' — sfDust on the custody trust line#7970Tapanito wants to merge 14 commits into
Tapanito wants to merge 14 commits into
Conversation
Adds VaultDustProbe.h, the single seam through which the shared test suite reads a Vault's dust reservoir, and VaultRounding_test.cpp, the suite both solution branches are verified against. On this branch kHasDustReservoir is false and readVaultDust always returns zero, so every tier-2 test takes its pre-fix arm. testCharacterizeCurrentRounding harvests exact literals from a real run and demonstrates the leak directly: paying off a loan whose scale is finer than the vault's current scale credits the vault less than the loan's own receivable falls by, with the difference going nowhere.
Adds addAssetsToVault, removeAssetsFromVault, and closeVaultAssets to VaultHelpers, and converts every direct write to sfAssetsTotal / sfAssetsAvailable in the seven call sites (VaultDeposit, VaultWithdraw x2, VaultClawback, LoanSet, LoanPay, LoanManage) to go through them. VaultCreate's literal zero initialization is left alone and commented as the one deliberate exception. This is a pure bookkeeping refactor: each call performs exactly the same field updates the call site did before, in the same order, with no rounding, gating, or transfer logic added. Existing per-site guards (LoanPay's assert, LoanManage's dust-snap-then-check) stay exactly where they are, re-reading the fields the helper wrote.
Fixes the include-order/style findings clang-tidy raised against the two prior commits: VaultHelpers.cpp's new #include lines used quotes instead of angle brackets and were out of order, and VaultRounding_test.cpp had a few unused includes and a missing pair of braces on an if/else.
RepaymentResult carried a baked-in ΔAssetsTotal-based derivation of recognitionDelta and raw that is only valid on this branch, where T += assetsTotalDelta is the only write to sfAssetsTotal during LoanPay. On a solution branch the law is ΔT = r − ΔD (maths doc §4), so that same expression silently yields r − ΔD instead of r, which makes O4 collapse to "ΔD == 0" — true only when there is no dust, false in exactly the case these tests exist to exercise. Both solution branches would have inherited spuriously-failing assertions from a file they are not allowed to edit. RepaymentResult now carries only raw observations (accounting fields, the dust probe, the Loan's own principal, and the real balances of the borrower and every possible fee-leg recipient). Each caller's own if-constexpr arm derives recognitionDelta/raw for itself: the pre-fix arm keeps the branch-local ΔAssetsTotal shortcut (valid only here), the post-fix arm derives raw from real balances alone (raw = -Δborrower - ΔfeeRecipient), which holds regardless of which dust mechanism, if any, is in force. Also adds an O8 (T == A + PO) corroboration to testDustCreatedOnRepayment and to testCharacterizeCurrentRounding's post-fix arm, since O8 needs no probe and no r/raw derivation at all - the testsuite doc now prefers it wherever it applies. Adds the fixture-level d != 0 assertion testsuite doc section 5 item 3 requires. Characterisation literals are unchanged (dust is still exactly 5e-12).
The shared VaultRounding suite asserted the dust oracles behind an if-constexpr capability flag, so it passed on every branch while asserting different things. The post-fix arms were therefore never executed, which is how a recognition-delta derivation valid only pre-fix survived review. Assert the oracles unconditionally instead. Tier 2 now fails on this branch because the dust has nowhere to go, and that failure is the bug's demonstration; each solution branch makes it pass by giving the dust a home. Tier 1 still passes everywhere, so a Tier 1 failure remains a real bug on any branch. Drop kHasDustReservoir; readVaultDust alone is the seam. Add testDustDisappears, which states the whole bug as one identity: AssetsTotal == AssetsAvailable + principal owed, with no probe and no interest/principal decomposition.
Adds sfDust (NUMBER, code 18, SoeDefault, deliberately without kSmdNeedsAsset) to ltRIPPLE_STATE, regenerates protocol code, and fixes a latent codegen gap: LedgerEntry.h.mako never included STNumber.h, which only manifested now because no ltRIPPLE_STATE consumer previously needed a complete STNumber type in the same translation unit.
Adds DustSplit (TokenHelpers.h) and threads an optional DustSplit* through accountSend -> accountSendIOU -> directSendNoLimitIOU -> directSendNoFeeIOU. Passing nullptr (the default, and every existing caller) reproduces today's behaviour exactly. When a split is requested, the receiver-side credit truncates the extended (balance + dust) quantity toward zero at the caller's target scale, so accumulated dust is promoted into sfBalance automatically whenever it clears a whole quantum, and reports the receiver-positive balance/dust deltas back to the caller. Also guards removeEmptyHolding against deleting a line that still carries non-zero sfDust, and the bDelete path inside directSendNoFeeIOU itself, so a debit that zeroes sfBalance never silently destroys dust sitting on the same line.
useVaultDust(vault) is the single eligibility predicate: cash-basis Vault (sfLEVersion == CashBasis) holding an IOU asset. Both are false for a Legacy Vault or an XRP/MPT Vault, which keeps every dust-aware code path skipped for them unconditionally. maybeRenormaliseVaultDust promotes whole quanta of sfDust into sfBalance after a scale-refining removal, moving sfAssetsAvailable and sfAssetsTotal by the same amount so the receivable (AssetsTotal - AssetsAvailable) is unchanged. It is a no-op when dust is not in use, the custody line is absent, or the dust is still sub-quantum at the vault's current scale.
Gated on useVaultDust(vaultSle) && featureLendingProtocolV1_1 (useDust). When off, the arithmetic is byte-identical to before: round down to the Vault's anterior scale, then one accountSendMulti for both the Vault and broker legs. When on: the Vault leg is credited the RAW principal+interest figure (not pre-rounded) through a DustSplit targeting the POSTERIOR scale implied by AssetsTotal after this operation's own recognition update. Both sfAssetsAvailable and sfAssetsTotal then move by the reported balance/dust deltas -- sfAssetsTotal by `assetsTotalDelta - split.dustDelta`, not just `assetsTotalDelta` -- which is what keeps the receivable (AssetsTotal - AssetsAvailable) identical to what it would be with no dust mechanism at all, regardless of the sign of dustDelta. The broker fee leg is sent separately, unaffected by the split. associateAsset(vaultSle, asset) is deliberately skipped under useDust: it snaps a Number field to STAmount's own 16-significant-digit precision (roundToAsset's 2-arg self-precision form), which silently erases the sub-quantum recognition adjustment sfAssetsTotal's 19-digit Number is supposed to be able to carry (common plan §4.2a rule 4). Verified empirically: calling it here reproduced the pre-fix leak bit-for-bit. See the PR description for the full discussion of this finding (plan §6 item 3).
VaultWithdraw's terminal branch folds ALL of sfDust (not just whole quanta -- there is no other shareholder to divide it with) into sfBalance and pays it out with the rest, before closeVaultAssets zeroes both accounting fields, so sfDust ends up exactly zero. VaultWithdraw's non-terminal branch and VaultClawback both call maybeRenormaliseVaultDust after removeAssetsFromVault: a removal refines the Vault's scale, which can strand previously sub-quantum dust at or above one new quantum with no credit around to promote it. LoanManage::defaultLoan calls the same renormaliser after its transfer, for the same reason (its recognition delta is negative there). Did not route defaultCovered through the dust-aware split itself: the existing :196-212 "snap AssetsTotal to AssetsAvailable when the difference looks like dust" workaround already keeps the shared VaultRounding suite's default-path test green without it -- see the PR description for the full characterization.
Reads sfDust off the Vault's IOU custody line and normalizes it to Vault-positive terms, undoing the low/high account sign convention. Returns 0 for absent, Legacy, or integral-asset Vaults.
Covers: sfDust absence/presence and eligibility scoping, both trust-line sign orientations (Vault as low and high account, forced by varying the owner account and asserted explicitly), the credit path producing and bounding dust through a real repayment, ordinary payments never acquiring sfDust (the nullptr path), accountHolds excluding sfDust, and VaultDelete being gated on zero dust. Known gap, documented in place: a re-normalisation test that drove a non-terminal VaultWithdraw after a dust-producing repayment hit the pre-existing "withdrawal and assets outstanding must add up" ValidVault invariant in at least one parameter combination. Removed rather than weakened; see the PR description.
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
1 participant
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.
Context
A Vault's loan repayment can produce a remainder too fine for the Vault's own
STAmountscale to represent. Today that remainder is discarded: the Loan's receivable falls by the full amount while the Vault credits only the representable part, so the books acquire value nobody holds or owes.The shared suite (
src/test/app/lending/VaultRounding_test.cpp, on base branchtapanito/vault-rounding) asserts the corrected behaviour and is RED there by design (8 failures across 6 "Tier 2" dust cases). "Tier 1" cases assert behaviour that is already correct and pass everywhere. Each solution is judged by whether it turns Tier 2 green without regressing Tier 1. Neither branch modifies that test file — it is the frozen shared contract, byte-identical on both.Approach
An
sfDustfield on the Vault's existing custody trust line. The IOU credit primitivedirectSendNoFeeIOUlearns to split a credit into representable balance plus remainder and report the split, opt-in per call via aDustSplit*parameter (nullptr = today's behaviour).Verified
-u Loan: 0 failures.Vault,Invariants: 0 failures.Payment,Offer,AMM,Escrow,Check,Flow,PermissionedDEX): 0 failures across ~157,000 tests.All of the above were re-run after the final O3 fix merge, on the exact commit this PR points at. No known regressions anywhere.
Notable cost
Modifies
directSendNoFeeIOU, which every IOU payment in the ledger traverses. ThebDeleteguard (never delete a trust line holding dust) affects the non-dust path too and deserves review.Diff-size caveat
~30 of the changed files under
include/xrpl/protocol_autogen/are pure generator churn — each only gains an#include <xrpl/protocol/STNumber.h>line because aNUMBER-typed field was added. Real footprint is far smaller than the file count suggests.Ledger footprint
Adds one field to an existing entry type (
ltRIPPLE_STATE); no new ledger objects, no owner-reserve change.