feat: Add invariant checks to ensure only fields that are expected to be modified are modified#7029
feat: Add invariant checks to ensure only fields that are expected to be modified are modified#7029mvadari wants to merge 29 commits into
Conversation
… to be modified are modified
![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 generalizes the NoModifiedUnmodifiableFields invariant to use ledger-entry SOTemplate metadata (via a new SOEConstant annotation) so invariant enforcement can be driven by ledger format definitions instead of hardcoded per-ledger-type checks.
Changes:
- Add
SOEConstantmetadata toSOElement/SOTemplateand annotate ledger-entry and inner-object format fields as constant vs mutable. - Update
NoModifiedUnmodifiableFieldsto perform a template-driven “constant fields unchanged” check (gated byfeatureInvariantsV1_1for enforcement). - Extend/adjust invariant unit tests to cover template-based behavior and updated failure expectations.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/app/Invariants_test.cpp | Adds new coverage for template-based constant/not-constant behavior and updates expected invariant outcomes/messages. |
| src/libxrpl/tx/invariants/InvariantCheck.cpp | Implements the template-based constant-field invariant check alongside the legacy hardcoded path. |
| src/libxrpl/protocol/LedgerFormats.cpp | Annotates common ledger fields (sfLedgerIndex, sfLedgerEntryType, sfFlags) with constant/mutable metadata. |
| src/libxrpl/protocol/InnerObjectFormats.cpp | Adds constant/mutable annotations to inner-object format templates. |
| include/xrpl/protocol/detail/ledger_entries.macro | Annotates ledger entry field templates with soeCONSTANT / soeNOTCONSTANT. |
| include/xrpl/protocol/detail/features.macro | Removes an outdated comment line (no behavioral change). |
| include/xrpl/protocol/SOTemplate.h | Introduces SOEConstant and extends SOElement constructors/accessors to carry mutability metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7029 +/- ##
=========================================
- Coverage 82.0% 81.9% -0.0%
=========================================
Files 1007 1007
Lines 76873 76913 +40
Branches 8973 9002 +29
=========================================
+ Hits 63012 63013 +1
- Misses 13852 13891 +39
Partials 9 9
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
mvadari
changed the title
Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mvadari
force-pushed
the
mvadari/constant-field-invariant
branch
from
885c88c to
7de9560
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // Optional constant fields may be added or removed, | ||
| // but their value must not change once present. | ||
| if (bPresent && aPresent && *bField != *aField) | ||
| return true; |
There was a problem hiding this comment.
Can they, though? I would think that if an object is created without field X being set, it should stay unset forever. And the other way around, too.
Are there any cases where we expect a constant optional field to be set or cleared? If so, maybe we need another option - soeCONSTANTVALUE or something. Depending on how deep this goes, maybe soeCONSTANTVALUESETTABLE, soeCONSTANTVALUECLEARABLE, soeCONSTANTVALUEONLY.
There was a problem hiding this comment.
We have just this use-case in the SAV: #7077. Where a field is either set at creation or it is not, it can never be modified in any other way.
There was a problem hiding this comment.
Should I ignore whether or not the field is optional, then, and just always assume? I've made that change for now
There was a problem hiding this comment.
I added an SoeImmutableSetOnce field (e.g. sfFirstNFTokenSequence)
| } | ||
| // soeNOTCONSTANT fields may change freely — no recursion | ||
| // into inner objects/arrays is needed because the parent | ||
| // field explicitly allows changes to its entire contents. |
There was a problem hiding this comment.
Would it be possible to have a mutable parent field holding constant subfields?
Specifically, I'm thinking of SignerList.SignerEntries, which is mutable, but the SignerEntry inner object can't let the Account field change. That seems to require recursion. (Sorry.)
There was a problem hiding this comment.
I explored that at first but it got complicated quickly, so I left it out for now (we can revisit later)
Tapanito
left a comment
Tapanito
left a comment
There was a problem hiding this comment.
The ledger_entries.macro and InnerObjectFormats annotations need to be audited very carefully.
We might not have unit-tests that are mutating some mutable fields, thus marking field that are technically mutable as immutable, would cause the invariant to fire.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…ri/constant-field-invariant
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
I did a couple of audits on this logic to ensure that it was good. |
|
@mvadari given that this invariant iterates through all fields of each SLE, what are the performance implications of this PR? |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…ri/constant-field-invariant
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
| auto const* bField = before.peekAtPField(sf); | ||
| auto const* aField = after.peekAtPField(sf); | ||
| bool const bPresent = (bField != nullptr) && bField->getSType() != STI_NOTPRESENT; | ||
| bool const aPresent = (aField != nullptr) && aField->getSType() != STI_NOTPRESENT; |
There was a problem hiding this comment.
What do you think about creating these as std::optional? So you don't need the separate *Present flags?
// It'll probably be easier with a helper
auto getField = [](STObject const& obj, auto const& sf)
-> std::optional<STBase const*>
{
auto const* field = obj.peekAfPField(sf);
if (field == nullptr || field->getSType() == STI_NOTPRESENT)
return std::nullopt;
return field;
};
auto const bField = getField(before, sf);
auto const aField = getField(after, sf);
| // LedgerStateFix repairs ledger invariants, so it may need to modify | ||
| // fields that are otherwise immutable. | ||
| if (tx.getTxnType() == ttLEDGER_STATE_FIX) | ||
| return true; |
There was a problem hiding this comment.
Any examples, or is this future-proofing?
| return false; | ||
| } | ||
|
|
||
| continue; |
There was a problem hiding this comment.
Suggestion: Add a comment
// If the template was used, the field-by-field check is redundant, so skip it.
continue;
|
|
||
| // Template-based checks: SoeImmutableSetOnce field on Oracle. | ||
| { | ||
| Keylet oracleKeylet = keylet::amendments(); |
There was a problem hiding this comment.
You could initialize oracleKeylet with bogus values
Keylet oracleKeylet{ltAny, beast::kZero};
(Edit: Because I was confused at first, and having a clearly bogus value will avoid that.)
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
High Level Overview of Change
This PR updates
NoModifiedUnmodifiableFieldsto be more general and handle all ledger entries. To make this better-documented, it also updatesledger_entries.macro(andSOTemplate) to take an optionalSOEConstantparameter, which determines if the field is allowed to be updated or not.This change is amendment-gated, with a fix amendment (this can be changed to a feature amendment if needed).
Context of Change
Improving invariants
API Impact
N/A