diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index 8f341de27e..34449d416f 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/include/xrpl/protocol/detail/ledger_entries.macro @@ -87,8 +87,8 @@ LEDGER_ENTRY(ltNEGATIVE_UNL, 0x004e, NegativeUNL, nunl, ({ \sa keylet::nftpage_min, keylet::nftpage_max, keylet::nftpage */ LEDGER_ENTRY(ltNFTOKEN_PAGE, 0x0050, NFTokenPage, nft_page, ({ - {sfPreviousPageMin, soeOPTIONAL, soeCONSTANT}, - {sfNextPageMin, soeOPTIONAL, soeCONSTANT}, + {sfPreviousPageMin, soeOPTIONAL, soeNOTCONSTANT}, + {sfNextPageMin, soeOPTIONAL, soeNOTCONSTANT}, {sfNFTokens, soeREQUIRED, soeNOTCONSTANT}, {sfPreviousTxnID, soeREQUIRED, soeNOTCONSTANT}, {sfPreviousTxnLgrSeq, soeREQUIRED, soeNOTCONSTANT}, @@ -106,8 +106,8 @@ LEDGER_ENTRY(ltSIGNER_LIST, 0x0053, SignerList, signer_list, ({ {sfSignerQuorum, soeREQUIRED, soeNOTCONSTANT}, {sfSignerEntries, soeREQUIRED, soeNOTCONSTANT}, {sfSignerListID, soeREQUIRED, soeCONSTANT}, - {sfPreviousTxnID, soeREQUIRED, soeCONSTANT}, - {sfPreviousTxnLgrSeq, soeREQUIRED, soeCONSTANT}, + {sfPreviousTxnID, soeREQUIRED, soeNOTCONSTANT}, + {sfPreviousTxnLgrSeq, soeREQUIRED, soeNOTCONSTANT}, })) /** A ledger object which describes a ticket. @@ -261,7 +261,7 @@ LEDGER_ENTRY(ltXCHAIN_OWNED_CLAIM_ID, 0x0071, XChainOwnedClaimID, xchain_owned_c {sfXChainBridge, soeREQUIRED, soeCONSTANT}, {sfXChainClaimID, soeREQUIRED, soeCONSTANT}, {sfOtherChainSource, soeREQUIRED, soeCONSTANT}, - {sfXChainClaimAttestations, soeREQUIRED, soeCONSTANT}, + {sfXChainClaimAttestations, soeREQUIRED, soeNOTCONSTANT}, {sfSignatureReward, soeREQUIRED, soeCONSTANT}, {sfOwnerNode, soeREQUIRED, soeCONSTANT}, {sfPreviousTxnID, soeREQUIRED, soeNOTCONSTANT}, @@ -316,7 +316,7 @@ LEDGER_ENTRY(ltXCHAIN_OWNED_CREATE_ACCOUNT_CLAIM_ID, 0x0074, XChainOwnedCreateAc {sfAccount, soeREQUIRED, soeCONSTANT}, {sfXChainBridge, soeREQUIRED, soeCONSTANT}, {sfXChainAccountCreateCount, soeREQUIRED, soeCONSTANT}, - {sfXChainCreateAccountAttestations, soeREQUIRED, soeCONSTANT}, + {sfXChainCreateAccountAttestations, soeREQUIRED, soeNOTCONSTANT}, {sfOwnerNode, soeREQUIRED, soeCONSTANT}, {sfPreviousTxnID, soeREQUIRED, soeNOTCONSTANT}, {sfPreviousTxnLgrSeq, soeREQUIRED, soeNOTCONSTANT}, @@ -352,7 +352,7 @@ LEDGER_ENTRY(ltPAYCHAN, 0x0078, PayChannel, payment_channel, ({ {sfAccount, soeREQUIRED, soeCONSTANT}, {sfDestination, soeREQUIRED, soeCONSTANT}, {sfSequence, soeOPTIONAL, soeCONSTANT}, - {sfAmount, soeREQUIRED, soeCONSTANT}, + {sfAmount, soeREQUIRED, soeNOTCONSTANT}, {sfBalance, soeREQUIRED, soeNOTCONSTANT}, {sfPublicKey, soeREQUIRED, soeCONSTANT}, {sfSettleDelay, soeREQUIRED, soeCONSTANT}, @@ -361,8 +361,8 @@ LEDGER_ENTRY(ltPAYCHAN, 0x0078, PayChannel, payment_channel, ({ {sfSourceTag, soeOPTIONAL, soeCONSTANT}, {sfDestinationTag, soeOPTIONAL, soeCONSTANT}, {sfOwnerNode, soeREQUIRED, soeCONSTANT}, - {sfPreviousTxnID, soeREQUIRED, soeCONSTANT}, - {sfPreviousTxnLgrSeq, soeREQUIRED, soeCONSTANT}, + {sfPreviousTxnID, soeREQUIRED, soeNOTCONSTANT}, + {sfPreviousTxnLgrSeq, soeREQUIRED, soeNOTCONSTANT}, {sfDestinationNode, soeOPTIONAL, soeCONSTANT}, })) diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 827c72974a..ea36869fb7 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -963,53 +963,29 @@ hasConstantFieldChanged(STObject const& before, STObject const& after, SOTemplat bool const bPresent = bField && bField->getSType() != STI_NOTPRESENT; bool const aPresent = aField && aField->getSType() != STI_NOTPRESENT; - if (constant == soeCONSTANT || constant == soeCONSTANTINVALID) + XRPL_ASSERT( + constant != soeCONSTANTINVALID, "xrpl::hasConstantFieldChanged : constant is invalid"); + if (constant == soeCONSTANT) { - // The field must not change at all. - if (bPresent != aPresent || (aPresent && *bField != *aField)) - return true; - } - else if (constant == soeNOTCONSTANT) - { - // The field itself may change, but if it is an STObject or - // STArray we still need to recurse and check the inner - // fields against the inner object template. - if (!bPresent || !aPresent) - continue; - - if (sf.fieldType == STI_OBJECT) + if (elem.style() == soeOPTIONAL) { - auto const* innerTmpl = - InnerObjectFormats::getInstance().findSOTemplateBySField(sf); - if (innerTmpl) - { - auto const& bObj = before.getFieldObject(sf); - auto const& aObj = after.getFieldObject(sf); - if (hasConstantFieldChanged(bObj, aObj, *innerTmpl)) - return true; - } + // Optional constant fields may be added or removed, + // but their value must not change once present. + if (bPresent && aPresent && *bField != *aField) + return true; } - else if (sf.fieldType == STI_ARRAY) + else { - auto const& bArr = before.getFieldArray(sf); - auto const& aArr = after.getFieldArray(sf); - auto const n = std::min(bArr.size(), aArr.size()); - for (std::size_t i = 0; i < n; ++i) - { - auto const& bElem = bArr[i]; - auto const& aElem = aArr[i]; - // Each element of the array is an STObject whose - // template is determined by its SField name. - auto const* innerTmpl = - InnerObjectFormats::getInstance().findSOTemplateBySField(bElem.getFName()); - if (innerTmpl) - { - if (hasConstantFieldChanged(bElem, aElem, *innerTmpl)) - return true; - } - } + // 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; } } + // soeNOTCONSTANT fields may change freely — no recursion + // into inner objects/arrays is needed because the parent + // field explicitly allows changes to its entire contents. } return false; } @@ -1042,6 +1018,12 @@ NoModifiedUnmodifiableFields::finalize( auto const* format = LedgerFormats::getInstance().findByType(type); if (format) bad = hasConstantFieldChanged(*before, *after, format->getSOTemplate()); + + // sfLedgerIndex is a non-serialized (discardable) field + // that is not reliably present via peekAtPField, so we + // check it explicitly. + if (!bad) + bad = fieldChanged(before, after, sfLedgerIndex); } // Old hardcoded check @@ -1092,10 +1074,6 @@ NoModifiedUnmodifiableFields::finalize( fieldChanged(before, after, sfLedgerIndex); } - XRPL_ASSERT( - !bad || useTemplate, - "xrpl::NoModifiedUnmodifiableFields::finalize : no bad " - "changes or enforce invariant (template)"); if (bad) { JLOG(j.fatal()) << "Invariant failed: changed an unchangeable field for " diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 5153e9d71e..cfd766a42e 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -2108,10 +2108,12 @@ class Invariants_test : public beast::unit_test::suite // TODO: Loan Object + // Template-based checks: common constant fields on AccountRoot { auto const mods = std::to_array>({ [](SLE::pointer& sle) { sle->at(sfLedgerEntryType) += 1; }, [](SLE::pointer& sle) { sle->at(sfLedgerIndex) = uint256(1u); }, + [](SLE::pointer& sle) { sle->at(sfAccount) = Account("other").id(); }, }); for (auto const& mod : mods) @@ -2128,6 +2130,69 @@ class Invariants_test : public beast::unit_test::suite }); } } + + // Template-based checks: soeNOTCONSTANT field + // (sfPreviousTxnID) on AccountRoot should NOT fail when + // modified — no invariant checks this field. + { + doInvariantCheck( + {}, + [&](Account const& A1, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + sle->at(sfPreviousTxnID) = uint256(42u); + ac.view().update(sle); + return true; + }, + XRPAmount{}, + STTx{ttACCOUNT_SET, [](STObject&) {}}, + {tesSUCCESS, tesSUCCESS}); + } + + // Without featureInvariantsV1_1, old hardcoded path should + // still catch sfLedgerEntryType/sfLedgerIndex changes + { + auto const mods = std::to_array>({ + [](SLE::pointer& sle) { sle->at(sfLedgerEntryType) += 1; }, + [](SLE::pointer& sle) { sle->at(sfLedgerIndex) = uint256(1u); }, + }); + + for (auto const& mod : mods) + { + doInvariantCheck( + Env(*this, defaultAmendments() - featureInvariantsV1_1), + {{"changed an unchangeable field"}}, + [&](Account const& A1, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + mod(sle); + ac.view().update(sle); + return true; + }); + } + } + + // Without featureInvariantsV1_1, modifying a soeCONSTANT 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), + {}, + [&](Account const& A1, Account const& A2, ApplyContext& ac) { + auto sle = ac.view().peek(keylet::account(A1.id())); + if (!sle) + return false; + sle->at(sfAccount) = A2.id(); + ac.view().update(sle); + return true; + }, + XRPAmount{}, + STTx{ttACCOUNT_SET, [](STObject&) {}}, + {tesSUCCESS, tesSUCCESS}); + } } void @@ -2924,7 +2989,7 @@ class Invariants_test : public beast::unit_test::suite TxAccount::A2); doInvariantCheck( - {"violation of vault immutable data"}, + {"violation of vault immutable data", "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); @@ -2936,11 +3001,11 @@ class Invariants_test : public beast::unit_test::suite }, XRPAmount{}, STTx{ttVAULT_SET, [](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, precloseXrp); doInvariantCheck( - {"violation of vault immutable data"}, + {"violation of vault immutable data", "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); @@ -2952,11 +3017,11 @@ class Invariants_test : public beast::unit_test::suite }, XRPAmount{}, STTx{ttVAULT_SET, [](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, precloseXrp); doInvariantCheck( - {"violation of vault immutable data"}, + {"violation of vault immutable data", "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); @@ -2968,7 +3033,7 @@ class Invariants_test : public beast::unit_test::suite }, XRPAmount{}, STTx{ttVAULT_SET, [](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, precloseXrp); doInvariantCheck( @@ -3233,7 +3298,8 @@ class Invariants_test : public beast::unit_test::suite {"create operation must not have updated a vault", "shares issuer and vault pseudo-account must be the same", "shares issuer must be a pseudo-account", - "shares issuer pseudo-account must point back to the vault"}, + "shares issuer pseudo-account must point back to the vault", + "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); @@ -3249,7 +3315,7 @@ class Invariants_test : public beast::unit_test::suite }, XRPAmount{}, STTx{ttVAULT_CREATE, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, [&](Account const& A1, Account const& A2, Env& env) { Vault const vault{env}; auto [tx, keylet] = vault.create({.owner = A1, .asset = xrpIssue()});