feat: Add an invariant to ensure object deletion also deletes its pseudo-account#7445
feat: Add an invariant to ensure object deletion also deletes its pseudo-account#7445Tapanito wants to merge 8 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.
Gave this a review
Two correctness issues: implicit fallthrough drops sle_ on match, and the single-pointer field silently ignores all but the last matching object per transaction — see inline comments.
Review by ReviewBot 🤖
Review by Claude Sonnet 4.6 · Prompt: V15
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7445 +/- ##
=======================================
Coverage 82.0% 82.0%
=======================================
Files 1009 1009
Lines 76782 76815 +33
Branches 8939 8938 -1
=======================================
+ Hits 62975 63007 +32
- Misses 13798 13799 +1
Partials 9 9
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
Tapanito
added
Ready to merge
There was a problem hiding this comment.
Pull request overview
This PR introduces a new transaction invariant intended to ensure certain ledger object types (AMM, Vault, LoanBroker) always reference a valid on-ledger pseudo-account, and extends unit tests to cover the new invariant behavior.
Changes:
- Add a new
ObjectHasPseudoAccountinvariant that checkssfAccountis present and the referenced account root exists (behindfixCleanup3_3_0). - Update invariant wiring (
InvariantCheckstuple) to include the new check. - Adjust invariant unit tests by setting
sfAccountwhere needed and adding dedicated coverage for missing/nonexistent pseudo-accounts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/test/app/Invariants_test.cpp | Updates Vault test setup to include sfAccount and adds a new invariant test for pseudo-account presence/existence. |
| src/libxrpl/tx/invariants/InvariantCheck.cpp | Implements the ObjectHasPseudoAccount invariant logic and logging. |
| include/xrpl/tx/invariants/InvariantCheck.h | Declares the new invariant and adds it to the public InvariantChecks tuple. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Tapanito
changed the title
Change ObjectHasPseudoAccount from checking "object exists → pseudo-account exists" to "object deleted → pseudo-account deleted". Together with AccountRootsDeletedClean (which enforces the reverse: pseudo-account deleted → object deleted), this pair of invariants guarantees that an object and its pseudo-account are always deleted as a unit.
Tapanito
force-pushed
the
tapanito/pseudo-invariants
branch
from
93dfbb5 to
0a8b12c
Compare
|
@gregtatcam , @a1q123456 the Clanker gave me a way to improve this PR. Instead of enforcing that an object pseudo-account always exist, I changed to to enforce that if an object is deleted, so is it's pseudo-account. We already have an invariant that enforces the inverse (deleted pseudo, dangling object). Together, they ensure that there's always a pseudo-account and an object. Could you please re-review the PR? 🙏 |
| XRPAmount{}, | ||
| STTx{ttPAYMENT, [](STObject&) {}}, | ||
| {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, | ||
| {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, |
There was a problem hiding this comment.
Note for reviewers, ValidVault::finalize:290 returns true when !isTesSuccess(ret), so it only fires on the first pass. On the second pass (with tecINVARIANT_FAILED as input), ValidVault passes, but now ObjectHasPseudoAccount also fires because it doesn't gate on the result. This causes tefINVARIANT_FAILED on the second pass.
Tapanito
removed
the
Ready to merge
Overview
Ensures that when a ledger object with an associated pseudo-account (AMM, Vault, LoanBroker) is deleted, its pseudo-account is also deleted.
High Level Overview of Change
The
ObjectHasPseudoAccountinvariant checks: object deleted → pseudo-account deleted.Together with the existing
AccountRootsDeletedCleaninvariant (which checks the reverse: pseudo-account deleted → object deleted), these two invariants guarantee that an object and its pseudo-account are always deleted as a pair.Context of Change
Previously, the only invariant enforcement was one-directional:
AccountRootsDeletedCleanensured that deleting a pseudo-account also deleted the linked object (viagetPseudoAccountFields()). The reverse direction — deleting the object must also delete the pseudo-account — was only enforced by convention in transactor code.This PR closes the gap by adding the complementary check.
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)