From 9cf2c0c3aee03a02fd8b5639fb6b88496c2daba2 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 20 May 2026 15:23:09 -0400 Subject: [PATCH] more fixes --- include/xrpl/protocol/detail/features.macro | 1 + src/libxrpl/tx/invariants/InvariantCheck.cpp | 39 ++++++++------------ src/test/app/Invariants_test.cpp | 8 ++-- 3 files changed, 20 insertions(+), 28 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index fd62b74d59..64a7ea3e93 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,6 +15,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. +XRPL_FIX (ConstantInvariant, Supported::No, VoteBehavior::DefaultNo) XRPL_FIX (Cleanup3_2_0, Supported::No, VoteBehavior::DefaultNo) XRPL_FEATURE(MPTokensV2, Supported::No, VoteBehavior::DefaultNo) XRPL_FIX (Cleanup3_1_3, Supported::Yes, VoteBehavior::DefaultYes) diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 3113942352..1e328390a1 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -984,8 +984,8 @@ NoModifiedUnmodifiableFields::visitEntry( } // Check whether any constant (or unannotated) fields in the given template -// have been modified between before and after. Recurses into STObject and -// STArray fields using InnerObjectFormats. +// have been modified between before and after. +// TODO(future): recurse into STObject and STArray fields using InnerObjectFormats static bool hasConstantFieldChanged(STObject const& before, STObject const& after, SOTemplate const& tmpl) { @@ -1001,21 +1001,10 @@ hasConstantFieldChanged(STObject const& before, STObject const& after, SOTemplat if (constant == SoeImmutable) { - if (elem.style() == SoeOptional) - { - // Optional constant fields may be added or removed, - // but their value must not change once present. - if (bPresent && aPresent && *bField != *aField) - return true; - } - else - { - // Required and default constant fields must not - // change at all — including transitions between - // default (not-present) and explicit values. - if (bPresent != aPresent || (bPresent && aPresent && *bField != *aField)) - return true; - } + // Field must not change at all — including transitions between + // default (not-present) and explicit values. + if (bPresent != aPresent || (bPresent && aPresent && *bField != *aField)) + return true; } // SoeMutable fields may change freely — no recursion // into inner objects/arrays is needed because the parent @@ -1038,7 +1027,7 @@ NoModifiedUnmodifiableFields::finalize( return beforeField != afterField || (afterField && before->at(field) != after->at(field)); }; - bool const useTemplate = view.rules().enabled(featureInvariantsV1_1); + bool const useTemplate = view.rules().enabled(fixConstantInvariant); for (auto const& slePair : changedEntries_) { @@ -1057,12 +1046,12 @@ NoModifiedUnmodifiableFields::finalize( // that is not reliably present via peekAtPField, so we // check it explicitly. if (!bad) - bad = kFIELD_CHANGED(before, after, sfLedgerIndex); + bad = kFieldChanged(before, after, sfLedgerIndex); } // Old hardcoded check bool badOld = false; - [[maybe_unused]] bool enforceOld = false; + [[maybe_unused]] bool enforceOld = view.rules().enabled(featureLendingProtocol); switch (type) { case ltLOAN_BROKER: @@ -1071,7 +1060,6 @@ NoModifiedUnmodifiableFields::finalize( * amendment status, allowing for detection and logging of * potential issues even when the amendment is disabled. */ - enforce = view.rules().enabled(featureLendingProtocol); bad = kFieldChanged(before, after, sfLedgerEntryType) || kFieldChanged(before, after, sfLedgerIndex) || kFieldChanged(before, after, sfSequence) || @@ -1090,7 +1078,6 @@ NoModifiedUnmodifiableFields::finalize( * amendment status, allowing for detection and logging of * potential issues even when the amendment is disabled. */ - enforce = view.rules().enabled(featureLendingProtocol); bad = kFieldChanged(before, after, sfLedgerEntryType) || kFieldChanged(before, after, sfLedgerIndex) || kFieldChanged(before, after, sfSequence) || @@ -1122,7 +1109,6 @@ NoModifiedUnmodifiableFields::finalize( * all transactions are affected because that's when it * was added. */ - enforce = view.rules().enabled(featureLendingProtocol); bad = kFieldChanged(before, after, sfLedgerEntryType) || kFieldChanged(before, after, sfLedgerIndex); } @@ -1143,7 +1129,12 @@ NoModifiedUnmodifiableFields::finalize( { JLOG(j.fatal()) << "Invariant failed: changed an unchangeable field for " << tx.getTransactionID(); - if (!useTemplate && enforceOld) + // If the new check passes when the old check fails, it's a bug in the new check. + XRPL_ASSERT( + !useTemplate, + "xrpl::NoModifiedUnmodifiableFields::finalize : new invariant version " + "passes when old version fails"); + if (enforceOld) return false; } } diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 3743e787ed..5a46d796cf 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -2154,7 +2154,7 @@ class Invariants_test : public beast::unit_test::Suite {tesSUCCESS, tesSUCCESS}); } - // Without featureInvariantsV1_1, old hardcoded path should + // Without fixConstantInvariant, old hardcoded path should // still catch sfLedgerEntryType/sfLedgerIndex changes { auto const mods = std::to_array>({ @@ -2165,7 +2165,7 @@ class Invariants_test : public beast::unit_test::Suite for (auto const& mod : mods) { doInvariantCheck( - Env(*this, defaultAmendments() - featureInvariantsV1_1), + Env(*this, defaultAmendments() - fixConstantInvariant), {{"changed an unchangeable field"}}, [&](Account const& a1, Account const&, ApplyContext& ac) { auto sle = ac.view().peek(keylet::account(a1.id())); @@ -2178,12 +2178,12 @@ class Invariants_test : public beast::unit_test::Suite } } - // Without featureInvariantsV1_1, modifying a SoeImmutable field + // Without fixConstantInvariant, modifying a SoeImmutable field // that is NOT sfLedgerEntryType/sfLedgerIndex on a non-loan // type should NOT fail (old code doesn't check it) { doInvariantCheck( - Env(*this, defaultAmendments() - featureInvariantsV1_1), + Env(*this, defaultAmendments() - fixConstantInvariant), {}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { auto sle = ac.view().peek(keylet::account(a1.id()));