diff --git a/include/xrpl/protocol/SOTemplate.h b/include/xrpl/protocol/SOTemplate.h index d208c8699b..952ad346eb 100644 --- a/include/xrpl/protocol/SOTemplate.h +++ b/include/xrpl/protocol/SOTemplate.h @@ -30,6 +30,7 @@ enum SOETxMPTIssue { SoeMptNone, SoeMptSupported, SoeMptNotSupported }; enum SOEConstant { SoeImmutable, SoeMutable, + SoeImmutableSetOnce, }; //------------------------------------------------------------------------------ @@ -54,6 +55,14 @@ private: nm += ": '" + fieldName.getName() + "'"; Throw("SField (" + nm + ") in SOElement must be useful."); } + + XRPL_ASSERT( + constant_ != SoeImmutableSetOnce || style_ == SoeOptional, + "xrpl::SOElement::init : set-once fields must be optional"); + XRPL_ASSERT( + constant_ != SoeMutable || style_ == SoeRequired || style_ == SoeOptional || + style_ == SoeDefault, + "xrpl::SOElement::init : mutable fields must have a valid style"); } public: diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index 2075aaa143..a36bcb017d 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/include/xrpl/protocol/detail/ledger_entries.macro @@ -146,7 +146,7 @@ LEDGER_ENTRY(ltACCOUNT_ROOT, 0x0061, AccountRoot, account, ({ {sfNFTokenMinter, SoeOptional, SoeMutable}, {sfMintedNFTokens, SoeDefault, SoeMutable}, {sfBurnedNFTokens, SoeDefault, SoeMutable}, - {sfFirstNFTokenSequence, SoeOptional, SoeImmutable}, + {sfFirstNFTokenSequence, SoeOptional, SoeImmutableSetOnce}, {sfAMMID, SoeOptional, SoeImmutable}, // pseudo-account designator {sfVaultID, SoeOptional, SoeImmutable}, // pseudo-account designator {sfLoanBrokerID, SoeOptional, SoeImmutable}, // pseudo-account designator @@ -280,10 +280,10 @@ LEDGER_ENTRY(ltRIPPLE_STATE, 0x0072, RippleState, state, ({ {sfHighLimit, SoeRequired, SoeMutable}, {sfPreviousTxnID, SoeRequired, SoeMutable}, {sfPreviousTxnLgrSeq, SoeRequired, SoeMutable}, - {sfLowNode, SoeOptional, SoeMutable}, + {sfLowNode, SoeOptional, SoeImmutable}, {sfLowQualityIn, SoeOptional, SoeMutable}, {sfLowQualityOut, SoeOptional, SoeMutable}, - {sfHighNode, SoeOptional, SoeMutable}, + {sfHighNode, SoeOptional, SoeImmutable}, {sfHighQualityIn, SoeOptional, SoeMutable}, {sfHighQualityOut, SoeOptional, SoeMutable}, })) diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 1e328390a1..b56e5ca3d6 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -1001,11 +1001,22 @@ hasConstantFieldChanged(STObject const& before, STObject const& after, SOTemplat if (constant == SoeImmutable) { - // Field must not change at all — including transitions between + // Field must not change at all, including transitions between // default (not-present) and explicit values. if (bPresent != aPresent || (bPresent && aPresent && *bField != *aField)) return true; } + else if (constant == SoeImmutableSetOnce) + { + XRPL_ASSERT( + elem.style() == SoeOptional, + "xrpl::hasConstantFieldChanged : set-once fields must be optional"); + + // Field may be set once, but cannot be removed or changed after + // it is present. + if (bPresent && (!aPresent || *bField != *aField)) + return true; + } // SoeMutable fields may change freely — no recursion // into inner objects/arrays is needed because the parent // field explicitly allows changes to its entire contents. @@ -1035,9 +1046,14 @@ NoModifiedUnmodifiableFields::finalize( auto const& after = slePair.second; auto const type = after->getType(); - // New template-based check - bool bad = false; + // New template-based check. This is a superset of the old hardcoded + // path below: the common template includes sfLedgerEntryType, the + // explicit check below covers sfLedgerIndex, and the loan/loan-broker + // fields from the old switch are marked immutable in their ledger + // templates. + if (useTemplate) { + bool bad = false; auto const* format = LedgerFormats::getInstance().findByType(type); if (format != nullptr) bad = hasConstantFieldChanged(*before, *after, format->getSOTemplate()); @@ -1047,11 +1063,20 @@ NoModifiedUnmodifiableFields::finalize( // check it explicitly. if (!bad) bad = kFieldChanged(before, after, sfLedgerIndex); + + if (bad) + { + JLOG(j.fatal()) << "Invariant failed: changed an unchangeable field for " + << tx.getTransactionID(); + return false; + } + + continue; } // Old hardcoded check bool badOld = false; - [[maybe_unused]] bool enforceOld = view.rules().enabled(featureLendingProtocol); + bool const enforceOld = view.rules().enabled(featureLendingProtocol); switch (type) { case ltLOAN_BROKER: @@ -1060,7 +1085,7 @@ NoModifiedUnmodifiableFields::finalize( * amendment status, allowing for detection and logging of * potential issues even when the amendment is disabled. */ - bad = kFieldChanged(before, after, sfLedgerEntryType) || + badOld = kFieldChanged(before, after, sfLedgerEntryType) || kFieldChanged(before, after, sfLedgerIndex) || kFieldChanged(before, after, sfSequence) || kFieldChanged(before, after, sfOwnerNode) || @@ -1078,9 +1103,9 @@ NoModifiedUnmodifiableFields::finalize( * amendment status, allowing for detection and logging of * potential issues even when the amendment is disabled. */ - bad = kFieldChanged(before, after, sfLedgerEntryType) || + badOld = kFieldChanged(before, after, sfLedgerEntryType) || kFieldChanged(before, after, sfLedgerIndex) || - kFieldChanged(before, after, sfSequence) || + kFieldChanged(before, after, sfLoanSequence) || kFieldChanged(before, after, sfOwnerNode) || kFieldChanged(before, after, sfLoanBrokerNode) || kFieldChanged(before, after, sfLoanBrokerID) || @@ -1109,18 +1134,10 @@ NoModifiedUnmodifiableFields::finalize( * all transactions are affected because that's when it * was added. */ - bad = kFieldChanged(before, after, sfLedgerEntryType) || + badOld = kFieldChanged(before, after, sfLedgerEntryType) || kFieldChanged(before, after, sfLedgerIndex); } - if (bad) - { - JLOG(j.fatal()) << "Invariant failed: changed an unchangeable field for " - << tx.getTransactionID(); - if (useTemplate) - return false; - } - XRPL_ASSERT( !badOld || enforceOld, "xrpl::NoModifiedUnmodifiableFields::finalize : no bad " @@ -1129,11 +1146,6 @@ NoModifiedUnmodifiableFields::finalize( { JLOG(j.fatal()) << "Invariant failed: changed an unchangeable field for " << tx.getTransactionID(); - // 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 5a46d796cf..f9631ae9b5 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -3125,7 +3125,7 @@ class Invariants_test : public beast::unit_test::Suite TxAccount::A2); doInvariantCheck( - {"updated shares must not exceed maximum"}, + {"updated shares must not exceed maximum", "changed an unchangeable field"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { auto const keylet = keylet::vault(a1.id(), ac.view().seq()); auto sleVault = ac.view().peek(keylet); @@ -3141,7 +3141,7 @@ class Invariants_test : public beast::unit_test::Suite }, XRPAmount{}, STTx{ttVAULT_DEPOSIT, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, precloseXrp, TxAccount::A2);