add tests, fix issues

This commit is contained in:
Mayukha Vadari
2026-04-27 08:52:52 -04:00
parent b46ca16f7d
commit cf33298abd
3 changed files with 106 additions and 62 deletions

View File

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

View File

@@ -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 "

View File

@@ -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<std::function<void(SLE::pointer&)>>({
[](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<std::function<void(SLE::pointer&)>>({
[](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()});