Conversation
The ValidConfidentialMPToken invariant folded an erased MPToken's pre-transaction public balance into the confidential-state check, which is gated on the issuance's ConfidentialOutstandingAmount. Any transaction that drains an MPToken and erases it in the same doApply -- an AMMWithdraw of the whole pool, a LoanBrokerDelete returning cover -- therefore failed with tecINVARIANT_FAILED as soon as any unrelated holder of the same issuance held a confidential balance. Those pseudo-account MPTokens carry no confidential state of their own, so the COA gate never should have applied to them. Split the two rules. The public balance is now read from the erase-time snapshot rather than the pre-transaction one, and is checked independently of the issuance's confidential state. The ciphertext check keeps the COA gate unchanged. The new behaviour is gated on fixCleanup3_5_0; on non-amended ledgers the pre-transaction balance is still folded into the COA gate, preserving current consensus behaviour. Adds regression tests for the LoanBrokerDelete and AMMWithdraw paths, each covering both amendment states and including a negative control where no unrelated holder has converted.
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.
🔵 Needs a closer look
It changes consensus-critical invariant enforcement paths (including amendment-gated behavior), so it warrants careful human validation beyond automated review.
Pull request overview
This PR fixes ValidConfidentialMPToken invariant behavior so MPToken deletion is no longer rejected due to an unrelated holder’s confidential balance when the MPToken is legitimately drained to zero and erased within the same transaction, while preserving pre-amendment consensus behavior and gating the corrected rule on fixCleanup3_5_0.
Changes:
- Split “deleted with public balance” from the issuance-wide COA-gated confidential-state checks, and (post-
fixCleanup3_5_0) evaluate public balance at erase-time (aftersnapshot). - Preserve pre-amendment behavior by reconstructing the legacy “deletedWithEncrypted OR pre-transaction balance” logic when
fixCleanup3_5_0is not enabled. - Add regression tests covering
LoanBrokerDeleteand fullAMMWithdrawfailure modes pre-amendment (with COA set by an unrelated holder) and success post-amendment.
File summaries
| File | Description |
|---|---|
src/libxrpl/tx/invariants/MPTInvariant.cpp |
Records pre-/erase-time balances on MPToken erase and adjusts finalize logic to decouple public-balance deletion from COA-gated confidential checks under fixCleanup3_5_0. |
include/xrpl/tx/invariants/MPTInvariant.h |
Updates invariant documentation and extends tracking state to include before/after balance flags for erased MPTokens. |
src/test/app/lending/LoanBroker_test.cpp |
Adds a regression test demonstrating pre-amendment invariant failure and post-amendment success for LoanBrokerDelete with unrelated COA. |
src/test/app/ConfidentialTransferExtended_test.cpp |
Adds a regression test demonstrating the same pre-/post-amendment behavior for full AMMWithdraw deleting an AMM pseudo-account MPToken. |
Review details
Suppressed comments (2)
src/test/app/lending/LoanBroker_test.cpp:1812
- This
env.close()is redundant:MPTTester::set()closes the ledger automatically whenclose=true(the default). Removing the extra close reduces test runtime without affecting sequencing or ledger state.
mptt.generateKeyPair(issuer);
mptt.set({.account = issuer, .issuerPubKey = mptt.getPubKey(issuer)});
env.close();
src/test/app/lending/LoanBroker_test.cpp:1857
- This
env.close()is redundant:MPTTester::convert()closes the ledger automatically when constructed with the defaultclose=true. Keeping both adds an extra ledger close inside the loop.
mptt.generateKeyPair(carol);
mptt.convert({.account = carol, .amt = 1, .holderPubKey = mptt.getPubKey(carol)});
env.close();
}
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| changes_[id].deletedWithBalanceBefore = before->getFieldU64(sfMPTAmount) > 0; | ||
| if (after) | ||
| changes_[id].deletedWithBalanceAfter = after->getFieldU64(sfMPTAmount) > 0; |
There was a problem hiding this comment.
Resolved incidentally in 5cda80f — the offending line is gone (this file was refactored per thread #4, moving the check into ValidMPTIssuance::finalize). The new location in ValidMPTIssuance::finalize guards sleHolding->getType() == ltMPTOKEN before reading sfMPTAmount, since deletedHoldings_ mixes ltMPTOKEN and ltRIPPLE_STATE.
| mptAlice.generateKeyPair(carol); | ||
| mptAlice.convert( | ||
| {.account = carol, .amt = 1, .holderPubKey = mptAlice.getPubKey(carol)}); | ||
| env.close(); |
| mptt.authorize({.account = alice}); | ||
| mptt.authorize({.account = carol}); | ||
| env.close(); | ||
| env(pay(issuer, alice, mpt(100'000))); | ||
| env(pay(issuer, carol, mpt(100))); | ||
| env.close(); |
There was a problem hiding this comment.
Removed all three in 5cda80f — one after the paired mptt.authorize calls, one after mptt.set({.issuerPubKey = ...}), and one after mptt.convert(...). The env.close() calls that follow raw env(pay(...)) were kept, since pay does not close.
Add braces around the nested loop body (readability-braces-around-statements) and include <optional> for std::nullopt (misc-include-cleaner).
changes_ is keyed by MPTokenIssuanceID, so every holder of an issuance shares one Changes entry. deletedWithBalanceBefore/After were written by unconditional assignment, so when one transaction erased two MPTokens of the same issuance, whichever was visited last decided the flag: an empty sibling visited after a funded one reset it to false and the erased balance went unreported. Only ever set the flags, matching how deletedWithEncrypted has always been recorded. Adds an invariant test erasing a funded MPToken alongside an empty one. Because visit order follows ledger key order, the test funds each holder in turn so that one run always places the empty sibling last; that run failed before this change.
| // own terms: it destroys value and has nothing to do with the | ||
| // issuance's confidential state. Checked before the issuance lookup | ||
| // because it needs no issuance, and judged on the erase-time balance. | ||
| if (cleanupEnabled && checks.deletedWithBalanceAfter) |
There was a problem hiding this comment.
I think it is cleaner to move this and the related logic into ValidMPTIssuance. It reads as a general per-MPToken erase-time public balance check rather than something specific to confidential state, and ValidMPTIssuance already has deletedHoldings_ capturing the erase-time snapshot. The other confidential related checks stay in ValidConfidentialMPToken.
(ValidMPTIssuance today covers both issuance-object rules and several per-holder MPToken rules despite its name — historic naming debt）
There was a problem hiding this comment.
Done in 5cda80f. Moved the erase-time public balance check into ValidMPTIssuance::finalize, iterating the pre-existing deletedHoldings_ vector and gated on fixCleanup3_5_0. Confidential-specific checks stay in ValidConfidentialMPToken. Also dropped the redundant deletedWithBalanceAfter flag (deletedHoldings_ already carries the erase-time snapshot) and extended the visitEntry capture gate to fixCleanup3_2_0 || fixCleanup3_5_0 so the new check has data even when the earlier amendment is off. Added two InvariantsMPT cases that pin each half of the amendment gate — falsification confirmed each fails when its gate is broken.
Move the erased-MPToken sfMPTAmount == 0 check from ValidConfidentialMPToken into ValidMPTIssuance, which already snapshots every erased MPToken per-holder in deletedHoldings_. The rule has no dependence on confidential state and is now judged per-holder rather than per-issuance (requested by @yinyiqian1 on XRPLF#8209). Also: - Drop the redundant deletedWithBalanceAfter flag; the erase-time snapshot lives in deletedHoldings_ directly. - Extend the visitEntry MPToken erase-capture gate to fixCleanup3_2_0 || fixCleanup3_5_0 so the new check has data even when the earlier cleanup amendment is off. - Add InvariantsMPT cases pinning both halves of the amendment gate: one with fixCleanup3_2_0 disabled to exercise the capture gate, and one with fixCleanup3_5_0 disabled to pin the finalize gate. - Remove four redundant env.close() calls after MPTTester methods (which close the ledger themselves) in ConfidentialTransferExtended_test.cpp and LoanBroker_test.cpp.
| return false; | ||
| } | ||
|
|
||
| // Erasing an MPToken that still holds a public balance is wrong on its |
There was a problem hiding this comment.
Would you mind simplifying the comment:
// Deleting an MPToken with a non-zero MPTAmount is rejected.
There was a problem hiding this comment.
Done in 07494e8 — replaced with your one-liner verbatim.
| // vault-pseudo holding-deletion rule does not apply, so both are | ||
| // gated on fix320Enabled. The MPToken half of deletedHoldings_ also | ||
| // feeds the fixCleanup3_5_0 erase-time public balance check in | ||
| // finalize(), so it is captured whenever either amendment is on. |
There was a problem hiding this comment.
Can we keep the existing comments unchanged and only add the new logic? Since we've added the comment in finalize, keeping the current comment as-is on develop should be fine.
There was a problem hiding this comment.
Restored in 07494e8, with one deviation I want to flag.
Develop's text is back byte-for-byte except its last sentence, which now reads "Skip both blocks when the amendment is off so we avoid wasted work on the hot path, except where noted for fixCleanup3_5_0 below." Without that qualifier the sentence is false: the MPToken half of the deletedHoldings_ capture now also runs when fixCleanup3_5_0 is on and fixCleanup3_2_0 is off
| * MPTokens and RippleStates deleted during apply. finalize() checks each | ||
| * holder's AccountRoot to detect vault pseudo-account holdings deleted | ||
| * outside VaultDelete. All these checks are gated on fixCleanup3_2_0. | ||
| * MPTokens and RippleStates deleted during apply. Under fixCleanup3_2_0, |
There was a problem hiding this comment.
Can we keep the existing comments unchanged and only add the new logic?
/**
* MPTokens and RippleStates deleted during apply. finalize() checks each
* holder's AccountRoot to detect vault pseudo-account holdings deleted
* outside VaultDelete. All these checks are gated on fixCleanup3_2_0.
*
* Under fixCleanup3_5_0, finalize() also rejects any MPToken erased with
* a non-zero sfMPTAmount.
*/
There was a problem hiding this comment.
Done in 07494e8 — restored verbatim, your text character for character.
| // same transaction share this entry. Only ever set these, never | ||
| // clear them, or an empty sibling visited later would mask a | ||
| // funded MPToken. | ||
| if (before->getFieldU64(sfMPTAmount) > 0) |
There was a problem hiding this comment.
This line is currently required to preserve consensus safety, as we performed this check prior to fixCleanup3_5_0. Gating it with if (!rules.enabled(fixCleanup3_5_0)) makes the retirement for this legacy bug clear, indicating that it is intended for removal in a future cleanup.
There was a problem hiding this comment.
Done in 07494e8 — the capture is now gated on !isFeatureEnabled(fixCleanup3_5_0) and labelled as retired.
| { | ||
| bool const hasPublicBalance = before->getFieldU64(sfMPTAmount) > 0; | ||
| bool const hasEncryptedFields = before->isFieldPresent(sfConfidentialBalanceSpending) || | ||
| // changes_ is keyed by issuance, so sibling holders erased by the |
There was a problem hiding this comment.
those comments indicate the behavior does not diverge after the fixCleanup3_5_0 is enabled, but sounds not very clear.
Can we specify sibling-masking and consensus safety clearly here?
There was a problem hiding this comment.
Done in 07494e8 — split into two paragraphs, one per concern.
StephEdelman
left a comment
StephEdelman
left a comment
There was a problem hiding this comment.
This PR modifies the behavior of a synthetic, issuer‑defined ledger object (MPToken) that does not represent native value, does not behave like XRP, and does not participate in the substrate‑level economic layer of the XRPL. Because MPToken is not a real asset, the PR does not correct economic behavior — it adjusts protocol mechanics around a synthetic object.
yinyiqian1 asked for the legacy erase-time public balance capture in ValidConfidentialMPToken to be gated on the amendment, so its retirement is explicit and a future cleanup can drop it outright. The surrounding comments now state the sibling-masking and consensus-safety rationale separately, and the comments this PR had rewritten only in passing are back to their develop wording.
High Level Overview of Change
The
ValidConfidentialMPTokeninvariant folded an erased MPToken's pre-transaction public balance into the confidential-state check, which is gated on the issuance'ssfConfidentialOutstandingAmount(COA). Any transaction that drains an MPToken and erases it within the samedoApplytherefore failed withtecINVARIANT_FAILEDas soon as any unrelated holder of the same issuance held a confidential balance.This splits the public-balance rule out of the COA gate, gated on
fixCleanup3_5_0.Context of Change
The invariant recorded a single flag from two unrelated conditions:
Two problems compound here:
beforeis the MPToken at the start of the transaction, sohasPublicBalanceis true for every legitimate drain-then-erase. Onlyafter— whichApplyStateTabledoes supply onAction::Erase— can express "erased while still holding a balance".Because COA is issuance-wide, a single unrelated holder converting one unit was enough to block deletion of MPTokens with no confidential state at all. Affected paths include
AMMWithdrawof an entire pool andLoanBrokerDeletereturning cover — both operate on pseudo-account MPTokens, which can never hold ciphertext fields (a pseudo-account cannot signConfidentialMPTConvert, andConfidentialMPTSend::preclaimrejects any destination that has not already converted).Changes
ValidMPTIssuance::finalize, gated onfixCleanup3_5_0: any MPToken erased with a non-zerosfMPTAmountis rejected. It iterates the pre-existingdeletedHoldings_vector, which holds the erase-time SLE per holder, so a funded MPToken is caught even when erased alongside empty siblings of the same issuance. No issuance lookup is needed, because the rule does not depend on one.visitEntrycapturesdeletedHoldings_whenever eitherfixCleanup3_2_0orfixCleanup3_5_0is on, since the two amendments are independent.ValidConfidentialMPTokenkeeps only the ciphertext rules, with its COA gate unchanged.deletedWithBalanceBeforeflag reproduces the legacy behaviour and is captured only whilefixCleanup3_5_0is off, marking it as retired and slated for removal in a future cleanup. Pre-amendment,finalizerecombinesdeletedWithEncrypted || deletedWithBalanceBefore, which is exactly the old flag — current consensus behaviour is preserved.Type of Change
API Impact
None.
Test Plan
Two regression tests, each looping over
carolConverts(whether an unrelated third party has set COA) andwithFix(amendment state):xrpl.tx.LoanBroker—LoanBrokerDeleteof a broker whose vault holds MPT cover.xrpl.app.ConfidentialTransferExtended—AMMWithdrawof an entire XRP/MPT pool.Both assert
tecINVARIANT_FAILEDpre-amendment with COA set, andtesSUCCESSboth post-amendment and in the negative control where nobody has converted.xrpl.app.InvariantsMPTadds two cases pinning each half of the amendment gate independently: one withfixCleanup3_2_0off andfixCleanup3_5_0on (the only configuration in which the second half of thedeletedHoldings_capture gate is load-bearing), and one withfixCleanup3_5_0off.Removing the non-amended reconstruction term alone turns the pre-amendment assertions from
tecINVARIANT_FAILEDintotesSUCCESS, confirming the amendment gate is load-bearing rather than defensive.Note: In the pre-amendment ( before the activation of
fixCleanup3_5_0amendment) version, both of these transactions have straight-forward workarounds. In the case ofAMMWithdraw, LP-holders can always withdraw all but dust amounts from the AMM-pool. Similarly, the unit-test pertaining toLoanBrokerDeleteshows that two separate transactions that disambiguate the transfer of Vault-Cover and the deletion of the LoanBroker will resolve this issue, at the cost of one additional transaction fees.