fix more issues

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<std::runtime_error>("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:

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},
 }))

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;
         }

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);