refactor: Validate visitEntry SLE nullability via InvariantEntry - #8133
Open
Tapanito wants to merge 3 commits into
Open
refactor: Validate visitEntry SLE nullability via InvariantEntry#8133Tapanito wants to merge 3 commits into
Tapanito wants to merge 3 commits into
Conversation
Throw if after is null or a deletion lacks before, then drop the duplicate checker-level guards that assumed those pointers might be missing.
Own before/after as shared_ptrs so a malformed create/delete cannot reach checkers, and throw std::logic_error so the runner can fail the transaction in all builds.
…tEntry Give checkers a direct InvariantEntry include and take SLE pointers by value so clang-tidy -Werror matches CI.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Clean, well-executed mechanical refactor. InvariantEntry correctly centralizes the after-never-null / delete-implies-before contract validation, and all ~20 checker classes plus the runner/test call sites were updated consistently to match the new signature. I traced each call site where the previous defensive after && / before && null-guards were dropped (AMMInvariant, DirectoryInvariant, InvariantCheck.cpp's many checkers, MPTInvariant, NFTInvariant, LoanBrokerInvariant, PermissionedDEXInvariant/PermissionedDomainInvariant, VaultInvariant) and confirmed each is now safe because InvariantEntry's constructor enforces after != nullptr unconditionally and before != nullptr whenever isDelete is true, so no null-dereference regressions were introduced. The one place where a return-value check on update() was dropped (ValidMPTBalanceChanges::visitEntry in MPTInvariant.cpp) is behaviorally a no-op since it was the last statement in the function both before and after the change. No security, correctness, or resource-management issues found in the changed lines.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the transaction-invariant visitation API to enforce a single, validated contract for ledger-entry changes via a new InvariantEntry wrapper (notably: after is never null, and deletions must have before). This consolidates nullability validation, removes per-checker defensive guards, and ensures contract breaks fail closed via exception handling in the invariant runner.
Changes:
- Introduces
InvariantEntryand updatesTxInvariantCheck/invariant checkers to acceptInvariantEntry const&instead of(isDelete, before, after). - Updates invariant implementations to rely on the validated contract (
entry.after()non-null) and removes now-redundant null/defensive paths. - Updates and adds unit tests to reflect the new visitation API and validate
InvariantEntrycontract enforcement.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/app/invariants/InvariantsPermissioned_test.cpp | Updates invariant visitation tests to use InvariantEntry and adjusts coverage around delete semantics. |
| src/test/app/invariants/InvariantsMisc_test.cpp | Adds InvariantEntry validation tests and updates test invariants to new visitEntry signature. |
| src/test/app/invariants/InvariantsAMM_test.cpp | Updates AMM invariant tests to construct validated InvariantEntry for deletions. |
| src/libxrpl/tx/invariants/VaultInvariant.cpp | Migrates ValidVault::visitEntry to InvariantEntry and removes after null assertions/guards. |
| src/libxrpl/tx/invariants/SponsorshipInvariant.cpp | Migrates sponsorship invariants to InvariantEntry and simplifies null checks. |
| src/libxrpl/tx/invariants/PermissionedDomainInvariant.cpp | Migrates permissioned-domain invariant to InvariantEntry and assumes non-null after. |
| src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp | Migrates permissioned-DEX invariant to InvariantEntry and removes defensive after==null path. |
| src/libxrpl/tx/invariants/NFTInvariant.cpp | Migrates NFT invariants to InvariantEntry and removes after null guards. |
| src/libxrpl/tx/invariants/MPTInvariant.cpp | Migrates MPT invariants to InvariantEntry and simplifies conditional flows based on non-null after. |
| src/libxrpl/tx/invariants/LoanInvariant.cpp | Migrates loan invariant to InvariantEntry. |
| src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp | Migrates loan-broker invariant to InvariantEntry and refactors after-type dispatch. |
| src/libxrpl/tx/invariants/InvariantRunner.cpp | Centralizes contract validation by constructing InvariantEntry in the runner and passing to all checkers. |
| src/libxrpl/tx/invariants/InvariantCheck.cpp | Updates core invariant checks to accept InvariantEntry and removes unreachable/null-after code. |
| src/libxrpl/tx/invariants/FreezeInvariant.cpp | Migrates freeze invariant to InvariantEntry and removes after assertions/guards. |
| src/libxrpl/tx/invariants/DirectoryInvariant.cpp | Migrates directory invariant to InvariantEntry and simplifies deletion/after handling. |
| src/libxrpl/tx/invariants/AMMInvariant.cpp | Migrates AMM invariant to InvariantEntry and simplifies after-driven type logic. |
| include/xrpl/tx/Transactor.h | Updates Transactor’s TxInvariantCheck implementation to accept InvariantEntry. |
| include/xrpl/tx/invariants/VaultInvariant.h | Updates vault invariant header to new visitEntry(InvariantEntry const&) signature. |
| include/xrpl/tx/invariants/SponsorshipInvariant.h | Updates sponsorship invariant headers to new visitEntry(InvariantEntry const&) signatures. |
| include/xrpl/tx/invariants/PermissionedDomainInvariant.h | Updates permissioned-domain invariant header to new signature. |
| include/xrpl/tx/invariants/PermissionedDEXInvariant.h | Updates permissioned-DEX invariant header to new signature. |
| include/xrpl/tx/invariants/NFTInvariant.h | Updates NFT invariant headers to new signatures. |
| include/xrpl/tx/invariants/MPTInvariant.h | Updates MPT invariant headers/docs to use InvariantEntry. |
| include/xrpl/tx/invariants/LoanInvariant.h | Updates loan invariant header to new signature. |
| include/xrpl/tx/invariants/LoanBrokerInvariant.h | Updates loan-broker invariant header to new signature. |
| include/xrpl/tx/invariants/InvariantRunner.h | Updates TxInvariantCheck interface to take InvariantEntry. |
| include/xrpl/tx/invariants/InvariantEntry.h | Adds the new validated InvariantEntry type and enforces visitEntry contract via exceptions. |
| include/xrpl/tx/invariants/InvariantCheck.h | Updates invariant checker prototype and concrete check declarations to use InvariantEntry. |
| include/xrpl/tx/invariants/FreezeInvariant.h | Updates freeze invariant header to new signature. |
| include/xrpl/tx/invariants/DirectoryInvariant.h | Updates directory invariant header to new signature. |
| include/xrpl/tx/invariants/AMMInvariant.h | Updates AMM invariant header to new signature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
72
to
79
| /** | ||
| * @brief called for each ledger entry in the current transaction. | ||
| * | ||
| * @param isDelete true if the SLE is being deleted. | ||
| * @param before ledger entry before modification by the transaction. `before` will be null if | ||
| * the entry is new. | ||
| * @param after ledger entry after modification by the transaction. Always non-null. When | ||
| * deleting, `after` may differ from `before`. Whether that is important is up to the | ||
| * individual invariant check. | ||
| * @param entry validated, non-owning view of the modified ledger entry. | ||
| * | ||
| * @note `after` IS NEVER NULL. `isDelete` is the only correct way to check for deletions. | ||
| * Do not make logic or branching decisions on whether on `after` is set, because it will | ||
| * always be set. Treat a null `after` as a programming error (with XRPL_ASSERT). An | ||
| * invariant MAY check for null defensively, if it makes more sense, but an assertion is | ||
| * preferred for new invariants. | ||
| * @note `entry.after()` IS NEVER NULL. `entry.isDelete()` is the only | ||
| * correct way to check for deletions. | ||
| */ |
Comment on lines
66
to
70
| /** | ||
| * @brief Called for each ledger entry modified by the transaction. | ||
| * | ||
| * @param isDelete true if the SLE is being deleted. | ||
| * @param before the entry's state before the transaction (nullptr for | ||
| * newly created entries). | ||
| * @param after the entry's state after the transaction. For deletions | ||
| * this is the SLE being erased; use @p isDelete rather than | ||
| * a null @p after to detect deletions. @p after is | ||
| * never null. | ||
| * @param entry a validated, non-owning view of the modified entry. | ||
| */ |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
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.
Summary
visitEntrycontract in one place:afteris never null, and a deletion always hasbefore. Checkers take a validatedInvariantEntryinstead of three loose arguments.InvariantEntryowns theshared_ptrs and throwsstd::logic_erroron a contract break so the existing runnertry/catchcan fail the transaction in all builds (tecINVARIANT_FAILED/tefINVARIANT_FAILED), rather than usingXRPL_ASSERTwhich is not testable and not fail-closed in release.rawEraseof a key that is not in the parent view now fails invariants instead of being silently skipped; that path is a transactor bug, so failing the transaction is the correct behavior.Follow-up (not in this PR): migrate
Transactor::visitInvariantEntryoverrides toInvariantEntry const&.Test plan
InvariantsMisc,InvariantsAMM,InvariantsPermissioned,InvariantsVault,LoanInvariants— 0 failures./xrpld -u --unittest-jobs 10— 247 suites, 0 failures