perf: Only run invariants on the relevant ledger entry types#7440
perf: Only run invariants on the relevant ledger entry types#7440mvadari wants to merge 11 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7440 +/- ##
=======================================
Coverage 82.0% 82.0%
=======================================
Files 1009 1009
Lines 76782 76818 +36
Branches 8940 8940
=======================================
+ Hits 62978 63014 +36
Misses 13795 13795
Partials 9 9
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| static_assert( | ||
| allInvariantChecksAreRouted(std::make_index_sequence<std::tuple_size_v<InvariantChecks>>{}), | ||
| "Every invariant check must be routed exactly once."); |
There was a problem hiding this comment.
Your proposal associates a set of invariants with each ledger entry type. E.g. run ValidVault when ACCOUNT_ROOT, MPToken, RippleState etc. were modified. Basically, you're proposing a central coordinator to dispatch calls to visitEntry only for specific objects.
From architectural perspective:
The design you're proposing would work great if the invariants we have were specifically associated with the validity of a particular ledger entry. However, that's not the case at the moment. Invariants today are holistic, verifying that specific protocol rules are not broken, which requires verifying a state of bunch of different ledger entries. I think such split of invariants is fragile, the design makes it very easy to omit an invariant when an object changes. Perhaps a hyperbolised example, but an engineer might not consider that VaultDeposit may affect an IOU/MPToken pair, or `MPToken/MPToken pair.
Every transaction modifies an ACCOUNT_ROOT on account of fees. So the case ACCOUNT_ROOT: will always run, taking away from the benefits.
From performance perspective:
How does this affect performance? Can you run a local experiment to compare your proposal v.s. what we have today?
There was a problem hiding this comment.
The obvious rebuttal of my "manual registration is fragile" argument is, "don't make it manual". We can have each invariant declare the set of ledger entry types it inspects, e.g. static constexprt list. From this list we could build a dispatch table at compile time.
I think this approach still fails for a few reasons, we still depend on the same judgement call from engineers, declaring types that they think that matter. Second, there are invariants that deliberately must look at all modified ledger entries, e.g. ValidAmounts, so in the end we'll still iterate through all modfied ledger entries. So even if we introduce a marginal reduction in call counts to visitEntry, we introduce the risk of not calling invariants when they have to be. Trading performance for safety isn't a trade-off worth making. Invariants are the last line of defense against state corruption, they should be impossible to omit.
There was a problem hiding this comment.
Right now we're roughly doing this:
for tx in transaction: # loop 1
for sle in tx.sles_modified: # loop 2
for invariant in invariants: # loop 3
invariant.run(sle)This change doesn't affect loop 1 or loop 2 (we can't get around running invariants on every tx and on every SLE modified in every tx, but it does simplify how many invariants run in loop 3, based on the SLE type.
I'll see if Claude/Codex can whip up an approximate performance harness, I'm not very experienced in that.
There was a problem hiding this comment.
Sure, it does optimize that, and I pointed that out above. What I'm saying is that this selective check is not worth the risks introduced. Invariants are a last line of defense, we should be making it harder to skip invariants, not easier.
There was a problem hiding this comment.
There's a static assert check to ensure that every invariant is called at least once. Beyond that, it's equivalent to the check in many invariants that say if sle->getType() == ltENTRY || ...
There was a problem hiding this comment.
Sure, but the assertion does not handle a case when an invariant should've been called, but wasn't.
There was a problem hiding this comment.
Would it help if the list of ledger entry types that are supposed to be covered are in InvariantCheck.cpp instead of ApplyContext.cpp, so that it's alongside where the invariants are specified and is clearer which ledger entries they map to? Then it's basically the same as the existing getType checks, just a few lines of code higher up.
Edit: just made this change, lmk what you think.
There was a problem hiding this comment.
These are the performance results (harness is pushed):
| Scenario | # objects | develop | this branch | Delta |
|---|---|---|---|---|
| account root updates | 1 | 18,211ns | 17,004ns | 6.6% faster |
| trust line updates | 1 | 30,728ns | 30,404ns | 1.1% faster |
| offer creates | 1 | 12,070ns | 10,171ns | 15.7% faster |
| all ledger entry types | 1 | 231,158ns | 184,900ns | 20.0% faster |
| account root updates | 16 | 187,734ns | 160,869ns | 14.3% faster |
| trust line updates | 16 | 372,997ns | 371,117ns | 0.5% faster |
| offer creates | 16 | 76,305ns | 49,578ns | 35.0% faster |
| all ledger entry types | 16 | 4,151,360ns | 3,262,792ns | 21.4% faster |
| account root updates | 64 | 757,699ns | 648,849ns | 14.4% faster |
| trust line updates | 64 | 1,567,445ns | 1,535,748ns | 2.0% faster |
| offer creates | 64 | 278,215ns | 173,460ns | 37.7% faster |
| all ledger entry types | 64 | 18,380,415ns | 14,895,225ns | 19.0% faster |
Suite runtime: 61.5s on develop to 52.5s on latest feature branch, about 14.6% faster overall.
Note: this is just timing the invariant checks, not the full transaction logic
| XRPBalanceChecks::visitEntry(bool, SLE::const_ref before, SLE::const_ref after) | ||
| { | ||
| XRPL_ASSERT( | ||
| (!before || before->getType() == ltACCOUNT_ROOT) && |
There was a problem hiding this comment.
This gives me an idea, how about, instead of assertion per type, have something along the lines of:
XRPL_ASSERT(!before || kRelevantLedgerEntryTypes.contains(before->getType(), [...])
There was a problem hiding this comment.
Isn't that tautological?
mvadari
removed
the
DraftRunCI
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR optimizes invariant checking by routing each invariant’s visitEntry calls only to ledger entry types that the invariant actually inspects, reducing unnecessary per-entry work during transaction application.
Changes:
- Add a compile-time “relevant ledger entry types” declaration (
kRelevantLedgerEntryTypes) to each invariant checker and enforce its presence viastatic_assert. - Update
ApplyContext::checkInvariantsHelperto dispatch invariants via per-type routing instead of calling every invariant for every entry. - Add a manual performance benchmark test (
InvariantPerf_test) to measure invariant overhead across representative ledger entry mutations.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/app/InvariantPerf_test.cpp | Adds a manual benchmark to time invariant checking across multiple change patterns and ledger entry types. |
| src/libxrpl/tx/ApplyContext.cpp | Implements visit routing: all-entry invariants always run; type-specific invariants run only for relevant ledger entry types. |
| include/xrpl/tx/invariants/InvariantEntryTypes.h | Introduces VisitLedgerEntryTypes/VisitAllLedgerEntryTypes/VisitNoLedgerEntryTypes helpers for routing declarations. |
| include/xrpl/tx/invariants/InvariantCheck.h | Adds kRelevantLedgerEntryTypes declarations for core invariants and enforces routing coverage via compile-time checks in ApplyContext. |
| include/xrpl/tx/invariants/.h (AMM, Directory, Freeze, Loan, LoanBroker, MPT, NFT, Permissioned, Vault) | Declares each invariant’s relevant ledger entry types for routing. |
| src/libxrpl/tx/invariants/InvariantCheck.cpp | Adds type assertions and adapts several invariants to rely on routed entry types. |
| src/libxrpl/tx/invariants/{Directory,Loan,NFT,PermissionedDEX,PermissionedDomain}Invariant.cpp | Adds type assertions (XRPL_ASSERT) consistent with routed dispatch expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (auto const type = entryTypeForMappedInvariants(before, after)) | ||
| { | ||
| auto const iter = std::find_if( | ||
| kLedgerTypeInvariantVisitors.cbegin(), | ||
| kLedgerTypeInvariantVisitors.cend(), | ||
| [type](auto const& visitor) { return visitor.first == *type; }); | ||
| if (iter != kLedgerTypeInvariantVisitors.cend()) | ||
| iter->second(checkers, isDelete, before, after); | ||
| } |
| #pragma once | ||
|
|
||
| #include <xrpl/protocol/LedgerFormats.h> | ||
|
|
||
| namespace xrpl { |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
High Level Overview of Change
This PR refactors the invariant code to only run each invariant on the relevant ledger entry types, instead of all of them (as currently happens). For example, the
AccountRootsNotDeletedinvariant only runs onAccountRootledger entries that are modified in a tx, and not all ledger entries (which are then error'd out of quickly).This PR also adds a performance harness for testing invariant performance, which is used below.
Other than performance, this PR is a pure refactor and involves no functionality changes. Tests are not modified at all.
Note: the actual source code changes are only a couple hundred lines, most of the diff is the performance harness (818 lines).
Performance Gains
Suite runtime: 61.5s on
developto 52.5s on this branch, about 14.6% faster overall.Context of Change
Improve performance of invariants
API Impact
N/A