refactor: Add transaction-specific invariant checking - #6551
Conversation
Introduce a two-phase visitor pattern (visitInvariantEntry / finalizeInvariants) on Transactor so individual transaction types can define their own post-condition checks. These run before the existing protocol-wide invariants and short-circuit on failure to avoid misleading secondary errors. - Add pure virtual visitInvariantEntry and finalizeInvariants to Transactor - Implement checkTransactionInvariants to drive the visitor loop - Extract checkInvariants to orchestrate transaction-specific then protocol-wide checks with reset-and-retry on failure - Move failInvariantCheck from private to public in ApplyContext
ee845b1 to
40ee1e1
Compare
Tapanito
changed the title
Tapanito
changed the title
…into tapanito/transaction-invariant
Rename class qualifiers in visitInvariantEntry and finalizeInvariants definitions to match their actual transactor classes (e.g., DeleteAccount → AccountDelete, CancelCheck → CheckCancel).
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Always run both invariant checks instead of short-circuiting on transaction invariant failure. Return the most severe failure code (tef > tec). Also switch logger from j_ to ctx_.journal.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6551 +/- ##
=========================================
+ Coverage 82.5% 82.6% +0.1%
=========================================
Files 1010 1010
Lines 79246 79566 +320
Branches 7529 7537 +8
=========================================
+ Hits 65389 65696 +307
- Misses 13857 13870 +13
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
/ai-review |
There was a problem hiding this comment.
The new per-transaction invariant framework ships with every finalizeInvariants as a no-op stub, makeTransactor exposed in a public header with no lifetime contract, a misleading doc comment in Transactor.h, no short-circuit on invariant failure, and a missing obligation-type audit in AccountDelete for new feature types. See inline comments.
Review by Claude Opus 4.6 · Prompt: V12
godexsoft
left a comment
godexsoft
left a comment
There was a problem hiding this comment.
Leaving a few questions and nits. Overall this is a good change 👍
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 137 out of 137 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
Addresses a review comment: the soft BEAST_EXPECT assertion would not abort on failure, risking a null pointer dereference in the subsequent checkInvariants call. Use an early-return guard instead.
Add direct includes for std::shared_ptr, SLE, STTx, and XRPAmount in transactor translation units that override visitInvariantEntry or finalizeInvariants. Previously these types were pulled in transitively, which misc-include-cleaner flags as errors under the project's clang-tidy configuration.
…on-invariant # Conflicts: # src/test/app/Invariants_test.cpp
Callers must use isDelete rather than after == nullptr to detect deletions; after is non-null for erased SLEs as supplied by the apply logic.
The fatal log emitted when finalizeInvariants returns false previously lacked identifying information, making postmortems difficult. Include the full transaction JSON, matching the global invariant checker.
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
Use container::empty() and container::contains() instead of size() == 0 and find() != end(), matching the readability-container-size-empty and readability-container-contains checks.
Add direct includes for std::exception, std::distance, beast::Journal, xrpl::ReadView, xrpl::Serializer, xrpl::SerialIter, xrpl::Number, and xrpl::roundToAsset across files that use them. Also suppress modernize-use-ranges on a std::is_sorted call where the ranges version does not compile because SignerEntry is not std::totally_ordered.
Serializer.h, STAmount.h, Number.h, and <iterator> are only used inside XRPL_ASSERT or #ifdef DEBUG blocks that compile to nothing in release builds. Without the pragma, include-cleaner flags them as unused in release CI while flagging them as missing in debug builds.
checkTransactionInvariants only returns tecINVARIANT_FAILED or the input result, never tefINVARIANT_FAILED. Only protocol invariants can escalate to tef, so the txResult comparison was dead code.
| // Protocol invariants second (broader). These check properties that must hold regardless of | ||
| // transaction type. | ||
| auto const protoResult = ctx_.checkInvariants(result, fee); | ||
|
|
||
| // Fail if either check failed. tef (fatal) takes priority over tec. | ||
| if (protoResult == tefINVARIANT_FAILED) | ||
| return tefINVARIANT_FAILED; |
There was a problem hiding this comment.
You first check the tx invariants, but if the proto invariants result in a fatal failure then the tx invariants were checked needlessly.
I'd recommend to rewrite this as follows:
auto const protoResult = ctx_.checkInvariants(result, fee);
if (protoResult == tefINVARIANT_FAILED || protoResult == tecINVARIANT_FAILED)
return protoResult;
auto const txResult = checkTransactionInvariants(result, fee);
if (txResult == tecINVARIANT_FAILED)
return txResult;
return result;
btw is protoResult guaranteed to only return either tefINVARIANT_FAILED, tecINVARIANT_FAILED or success, and is txResult guaranteed to only return tecINVARIANT_FAILED or success? If not, then you're ignoring other failures.
There was a problem hiding this comment.
While I agree with the idea, I tried this approach already. It does not work.
The main problem, is that invariant tests are written with the idea that multiple invariant failures can appear at the same time. By skipping transaction invariants after global invariants (or global invariants after transaction invariants) we break a lot of invariant tests.
Since we are touching sensitive files, I am very reluctant to change unit-tests that serve as our mechanism of verification that nothing broke.
There was a problem hiding this comment.
Understood. We may consider doing this later if this optimization has meaningful impact. With extensive testing and running the change on a node for a while, we should be confident that we did it right.
Introduces a mechanism for individual transaction types to define their own
post-condition invariants, complementing the existing protocol-wide invariant
checks. Both transaction-specific and protocol-wide invariants always run; the
worst failure code is returned (
tef>tec).visitInvariantEntry/finalizeInvariantsvirtual methods onTransactor(two-phase visitor pattern matching protocol invariants)checkTransactionInvariantsto drive the visitor loop over modifiedledger entries
checkInvariantsonTransactorto orchestrate transaction-specificthen protocol-wide checks
makeTransactorfactory inapplyStepsto construct concretetransactors from an
ApplyContextat runtimeInvariants_test::doInvariantCheckto run both invariant layersDesign decisions
visitInvariantEntry/finalizeInvariantsare pure virtual: Forcesevery transactor to explicitly acknowledge transaction-specific invariants.
This makes it difficult to add a new transaction type without considering
what invariants it should enforce. No-op overrides are provided for all
existing transactors as a starting point.
makeTransactorinapplySteps.h/cpp: Natural companion toinvoke_apply; avoids duplicating thetransactions.macroplumbing intest code.
Transactor::~Transactormade public: Required bystd::unique_ptr<Transactor>returned frommakeTransactor; consistentwith public constructors on all subclasses.
Amendment considerations
This refactoring does not require an amendment — there are no functional
changes to any invariant logic. The existing protocol invariants continue to
run exactly as before, and the new transaction-specific invariant methods are
all no-ops. Moving an invariant from the protocol layer to a transaction-specific
override also does not require an amendment, as the check itself remains
functionally identical; only its call site changes.
Note for reviewers
Important changes are in the following files:
This PR will be followed by multiple other PRs to refactor invariants themselves:
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)