From 3cccbdf9662e6dd072808a362b3fc5953034e045 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Fri, 3 Apr 2026 15:03:31 +0200 Subject: [PATCH] fix: unify invariant overwrite tests into single parameterized helper Consolidate the regression tests and legacy tests into a single testInvariantOverwrite(FeatureBitset) that tests both pre- and post-amendment behavior for all three affected invariants. --- src/test/app/Invariants_test.cpp | 142 +++++++++---------------------- 1 file changed, 42 insertions(+), 100 deletions(-) diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 2864b885ea..4a4385e291 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -500,26 +500,6 @@ class Invariants_test : public beast::unit_test::suite ac.view().insert(sleNew); return true; }); - - // Regression: two trust lines where the first is bad (XRP) and the - // second is valid. Plain assignment would overwrite the earlier true. - doInvariantCheck( - {{"an XRP trust line was created"}}, - [](Account const& A1, Account const& A2, ApplyContext& ac) { - // First: bad XRP trust line - auto const sleBad = - std::make_shared(keylet::line(A1, A2, xrpIssue().currency)); - ac.view().insert(sleBad); - - // Second: valid USD trust line — must NOT clear the flag - Account const A3{"A3"}; - auto const sleGood = - std::make_shared(keylet::line(A1, A3, A1["USD"].currency)); - sleGood->setFieldAmount(sfLowLimit, A1["USD"](0)); - sleGood->setFieldAmount(sfHighLimit, A1["USD"](0)); - ac.view().insert(sleGood); - return true; - }); } void @@ -598,31 +578,6 @@ class Invariants_test : public beast::unit_test::suite ac.view().insert(sleNew); return true; }); - - // Regression: two trust lines where the first has deep freeze without - // freeze, and the second is valid. Plain assignment would overwrite - // the earlier true. - doInvariantCheck( - {{"a trust line with deep freeze flag without normal freeze was " - "created"}}, - [](Account const& A1, Account const& A2, ApplyContext& ac) { - // First: bad — lowDeepFreeze without lowFreeze - auto const sleBad = std::make_shared(keylet::line(A1, A2, A1["USD"].currency)); - sleBad->setFieldAmount(sfLowLimit, A1["USD"](0)); - sleBad->setFieldAmount(sfHighLimit, A1["USD"](0)); - sleBad->setFieldU32(sfFlags, lsfLowDeepFreeze); - ac.view().insert(sleBad); - - // Second: valid — no deep freeze flags at all - Account const A3{"A3"}; - auto const sleGood = - std::make_shared(keylet::line(A1, A3, A1["EUR"].currency)); - sleGood->setFieldAmount(sfLowLimit, A1["EUR"](0)); - sleGood->setFieldAmount(sfHighLimit, A1["EUR"](0)); - sleGood->setFieldU32(sfFlags, 0u); - ac.view().insert(sleGood); - return true; - }); } void @@ -978,25 +933,6 @@ class Invariants_test : public beast::unit_test::suite return true; }); - // Regression: MPT OutstandingAmount exceeds max, but locked <= - // outstanding. Plain assignment would overwrite earlier bad_ = true. - doInvariantCheck( - {{"escrow specifies invalid amount"}}, - [](Account const& A1, Account const&, ApplyContext& ac) { - auto const sle = ac.view().peek(keylet::account(A1.id())); - if (!sle) - return false; - - MPTIssue const mpt{MPTIssue{makeMptID(1, AccountID(0x4985601))}}; - auto sleNew = std::make_shared(keylet::mptIssuance(mpt.getMptID())); - // outstanding exceeds maxMPTokenAmount -> checkAmount sets bad_ - sleNew->setFieldU64(sfOutstandingAmount, maxMPTokenAmount + 1); - // locked is valid and <= outstanding -> must NOT clear bad_ - sleNew->setFieldU64(sfLockedAmount, 10); - ac.view().insert(sleNew); - return true; - }); - // MPT MPTAmount < 0 doInvariantCheck( {{"escrow specifies invalid amount"}}, @@ -3868,31 +3804,35 @@ class Invariants_test : public beast::unit_test::suite precloseMpt); } - // Verify that without fixSecurity3_1_3, the overwrite bug is - // preserved: a valid entry after a bad entry overwrites the flag, - // causing the invariant to incorrectly pass. + // Test the invariant overwrite fix for both pre- and post-amendment + // behavior. With the fix enabled, |= accumulates violations across + // entries so a later valid entry cannot clear an earlier violation. + // Without the fix, = assignment means the last-visited entry wins. void - testInvariantOverwriteLegacy() + testInvariantOverwrite(FeatureBitset features) { using namespace test::jtx; - auto const features = defaultAmendments() - fixSecurity3_1_3; + bool const fixEnabled = features[fixSecurity3_1_3]; + std::initializer_list const failTers = {tecINVARIANT_FAILED, tefINVARIANT_FAILED}; + std::initializer_list const passTers = {tesSUCCESS, tesSUCCESS}; - // NoXRPTrustLines: a trust line with sfLowLimit set to xrpIssue - // (bad), followed by a valid trust line visited last (higher key). - // Legacy = assignment overwrites the flag from true to false. - // The invariant checks sfLowLimit/sfHighLimit issue, not the - // keylet currency, so we use non-XRP keylet currencies to - // control the hash ordering. - testcase << "legacy overwrite: NoXRPTrustLines"; + // Regression: bad XRP trust line followed by a valid trust line. + // With the fix, the invariant catches the violation. + // Without the fix, the valid entry overwrites the flag to false. + // We use non-XRP keylet currencies so we can control the hash + // ordering (the invariant checks sfLowLimit/sfHighLimit issue, + // not the keylet currency). + testcase << "overwrite: NoXRPTrustLines" + std::string(fixEnabled ? " fix" : ""); doInvariantCheck( Env(*this, features), - {}, + fixEnabled ? std::vector{{"an XRP trust line was created"}} + : std::vector{}, [](Account const& A1, Account const& A2, ApplyContext& ac) { Account const A3{"A3"}; - // Compute ordering once, then assign "bad" (XRP issue) - // to the lower-sorting key and "good" (proper IOU limits) - // to the higher-sorting key. + // Compute ordering: assign "bad" (XRP issue) to the + // lower-sorting key and "good" to the higher-sorting key, + // so that the good entry is visited last. char const* const c1 = "AAA"; char const* const c2 = "ZZZ"; auto const k1 = keylet::line(A1, A2, A1[c1].currency); @@ -3919,15 +3859,15 @@ class Invariants_test : public beast::unit_test::suite }, XRPAmount{}, STTx{ttACCOUNT_SET, [](STObject&) {}}, - {tesSUCCESS, tesSUCCESS}); + fixEnabled ? failTers : passTers); - // NoDeepFreezeTrustLinesWithoutFreeze: bad deep-freeze trust line - // and a valid trust line. Legacy = assignment means the last-visited - // entry determines the flag. We ensure good sorts after bad. - testcase << "legacy overwrite: NoDeepFreezeTrustLinesWithoutFreeze"; + // Regression: bad deep-freeze trust line followed by a valid one. + testcase << "overwrite: NoDeepFreeze" + std::string(fixEnabled ? " fix" : ""); doInvariantCheck( Env(*this, features), - {}, + fixEnabled ? std::vector{{"a trust line with deep freeze flag without " + "normal freeze was created"}} + : std::vector{}, [](Account const& A1, Account const& A2, ApplyContext& ac) { Account const A3{"A3"}; @@ -3958,34 +3898,35 @@ class Invariants_test : public beast::unit_test::suite }, XRPAmount{}, STTx{ttACCOUNT_SET, [](STObject&) {}}, - {tesSUCCESS, tesSUCCESS}); + fixEnabled ? failTers : passTers); - // NoZeroEscrow: MPT outstanding exceeds max, but locked <= outstanding. - // Legacy = assignment overwrites bad_ = true with the result of - // outstanding < locked (which is false), so NoZeroEscrow passes. - // ValidMPTIssuance still fires ("a MPT issuance was created"), so - // the overall result is tecINVARIANT_FAILED, but we verify that - // the NoZeroEscrow message is absent (it would be present with the - // fix enabled). - testcase << "legacy overwrite: NoZeroEscrow MPT"; + // Regression: MPT OutstandingAmount exceeds max, but locked <= + // outstanding. Plain assignment would overwrite bad_ = true. + // With the fix, NoZeroEscrow catches it. + // Without the fix, NoZeroEscrow passes but ValidMPTIssuance + // still fires ("a MPT issuance was created"). + testcase << "overwrite: NoZeroEscrow MPT" + std::string(fixEnabled ? " fix" : ""); doInvariantCheck( Env(*this, features), - {{"a MPT issuance was created"}}, - [&](Account const& A1, Account const&, ApplyContext& ac) { + fixEnabled ? std::vector{{"escrow specifies invalid amount"}} + : std::vector{{"a MPT issuance was created"}}, + [](Account const& A1, Account const&, ApplyContext& ac) { auto const sle = ac.view().peek(keylet::account(A1.id())); - if (!BEAST_EXPECT(sle)) + if (!sle) return false; MPTIssue const mpt{MPTIssue{makeMptID(1, AccountID(0x4985601))}}; auto sleNew = std::make_shared(keylet::mptIssuance(mpt.getMptID())); + // outstanding exceeds maxMPTokenAmount -> checkAmount sets bad_ sleNew->setFieldU64(sfOutstandingAmount, maxMPTokenAmount + 1); + // locked is valid and <= outstanding -> must NOT clear bad_ sleNew->setFieldU64(sfLockedAmount, 10); ac.view().insert(sleNew); return true; }, XRPAmount{}, STTx{ttACCOUNT_SET, [](STObject&) {}}, - {tecINVARIANT_FAILED, tefINVARIANT_FAILED}); + fixEnabled ? failTers : failTers); } public: @@ -4013,7 +3954,8 @@ public: testValidPseudoAccounts(); testValidLoanBroker(); testVault(); - testInvariantOverwriteLegacy(); + testInvariantOverwrite(defaultAmendments()); + testInvariantOverwrite(defaultAmendments() - fixSecurity3_1_3); } };