diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index d8498ffa2f..961bc6e44c 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,7 +15,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. - +XRPL_FIX (PermissionedDomainInvariant, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (BatchInnerSigs, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(LendingProtocol, Supported::yes, VoteBehavior::DefaultNo) diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index b3fa027753..7558c951b0 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -36,6 +36,12 @@ class Invariants_test : public beast::unit_test::suite // changes that will cause the check to fail. using Precheck = std::function; + static FeatureBitset + defaultAmendments() + { + return xrpl::test::jtx::testable_amendments() | featureInvariantsV1_1 | featureSingleAssetVault; + } + /** Run a specific test case to put the ledger into a state that will be * detected by an invariant. Simulates the actions of a transaction that * would violate an invariant. @@ -62,10 +68,23 @@ class Invariants_test : public beast::unit_test::suite std::initializer_list ters = {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, Preclose const& preclose = {}, TxAccount setTxAccount = TxAccount::None) + { + return doInvariantCheck( + test::jtx::Env(*this, defaultAmendments()), expect_logs, precheck, fee, tx, ters, preclose, setTxAccount); + } + + void + doInvariantCheck( + test::jtx::Env&& env, + std::vector const& expect_logs, + Precheck const& precheck, + XRPAmount fee = XRPAmount{}, + STTx tx = STTx{ttACCOUNT_SET, [](STObject&) {}}, + std::initializer_list ters = {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + Preclose const& preclose = {}, + TxAccount setTxAccount = TxAccount::None) { using namespace test::jtx; - FeatureBitset amendments = testable_amendments() | featureInvariantsV1_1 | featureSingleAssetVault; - Env env{*this, amendments}; Account const A1{"A1"}; Account const A2{"A2"}; @@ -74,11 +93,28 @@ class Invariants_test : public beast::unit_test::suite BEAST_EXPECT(preclose(A1, A2, env)); env.close(); + if (setTxAccount != TxAccount::None) + tx.setAccountID(sfAccount, setTxAccount == TxAccount::A1 ? A1.id() : A2.id()); + + return doInvariantCheck(std::move(env), A1, A2, expect_logs, precheck, fee, tx, ters); + } + + void + doInvariantCheck( + test::jtx::Env&& env, + test::jtx::Account const& A1, + test::jtx::Account const& A2, + std::vector const& expect_logs, + Precheck const& precheck, + XRPAmount fee = XRPAmount{}, + STTx tx = STTx{ttACCOUNT_SET, [](STObject&) {}}, + std::initializer_list ters = {tecINVARIANT_FAILED, tefINVARIANT_FAILED}) + { + using namespace test::jtx; + OpenView ov{*env.current()}; test::StreamSink sink{beast::severities::kWarning}; beast::Journal jlog{sink}; - if (setTxAccount != TxAccount::None) - tx.setAccountID(sfAccount, setTxAccount == TxAccount::A1 ? A1.id() : A2.id()); ApplyContext ac{env.app(), ov, tx, tesSUCCESS, env.current()->fees().base, tapNONE, jlog}; BEAST_EXPECT(precheck(A1, A2, ac)); @@ -91,19 +127,21 @@ class Invariants_test : public beast::unit_test::suite for (TER const& terExpect : ters) { terActual = ac.checkInvariants(terActual, fee); - BEAST_EXPECT(terExpect == terActual); + BEAST_EXPECTS(terExpect == terActual, std::to_string(TERtoInt(terActual))); auto const messages = sink.messages().str(); - BEAST_EXPECT( - messages.starts_with("Invariant failed:") || messages.starts_with("Transaction caused an exception")); + + if (terActual != tesSUCCESS) + { + BEAST_EXPECTS( + messages.starts_with("Invariant failed:") || + messages.starts_with("Transaction caused an exception"), + messages); + } + // std::cerr << messages << '\n'; for (auto const& m : expect_logs) { - if (messages.find(m) == std::string::npos) - { - // uncomment if you want to log the invariant failure - // std::cerr << " --> " << m << std::endl; - fail(); - } + BEAST_EXPECTS(messages.find(m) != std::string::npos, m); } } } @@ -1119,86 +1157,80 @@ class Invariants_test : public beast::unit_test::suite }); } - void + static std::shared_ptr createPermissionedDomain( ApplyContext& ac, - std::shared_ptr& sle, test::jtx::Account const& A1, - test::jtx::Account const& A2) + test::jtx::Account const& A2, + std::uint32_t numCreds = 2, + std::uint32_t seq = 10) { - sle->setAccountID(sfOwner, A1); - sle->setFieldU32(sfSequence, 10); + Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), seq); + auto sle = std::make_shared(pdKeylet); - STArray credentials(sfAcceptedCredentials, 2); - for (std::size_t n = 0; n < 2; ++n) + sle->setAccountID(sfOwner, A1); + sle->setFieldU32(sfSequence, seq); + + if (numCreds) { - auto cred = STObject::makeInnerObject(sfCredential); - cred.setAccountID(sfIssuer, A2); - auto credType = "cred_type" + std::to_string(n); - cred.setFieldVL(sfCredentialType, Slice(credType.c_str(), credType.size())); - credentials.push_back(std::move(cred)); + // This array is sorted naturally, but if you willing to change this + // behavior don't forget to use credentials::makeSorted + STArray credentials(sfAcceptedCredentials, numCreds); + for (std::size_t n = 0; n < numCreds; ++n) + { + auto cred = STObject::makeInnerObject(sfCredential); + cred.setAccountID(sfIssuer, A2); + auto credType = "cred_type" + std::to_string(n); + cred.setFieldVL(sfCredentialType, Slice(credType.c_str(), credType.size())); + credentials.push_back(std::move(cred)); + } + sle->setFieldArray(sfAcceptedCredentials, credentials); } - sle->setFieldArray(sfAcceptedCredentials, credentials); + ac.view().insert(sle); + return sle; }; void - testPermissionedDomainInvariants() + testPermissionedDomainInvariants(FeatureBitset features) { using namespace test::jtx; - testcase << "PermissionedDomain"; - doInvariantCheck( - {{"permissioned domain with no rules."}}, - [](Account const& A1, Account const&, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - slePd->setAccountID(sfOwner, A1); - slePd->setFieldU32(sfSequence, 10); + bool const fixPDEnabled = features[fixPermissionedDomainInvariant]; + std::initializer_list badTers = {tecINVARIANT_FAILED, tecINVARIANT_FAILED}; + std::initializer_list failTers = {tecINVARIANT_FAILED, tefINVARIANT_FAILED}; - ac.view().insert(slePd); - return true; + testcase << "PermissionedDomain" + std::string(fixPDEnabled ? " fix" : ""); + + doInvariantCheck( + Env(*this, features), + {{"permissioned domain with no rules."}}, + [](Account const& A1, Account const& A2, ApplyContext& ac) { + return createPermissionedDomain(ac, A1, A2, 0).get(); }, XRPAmount{}, - STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, + fixPDEnabled ? failTers : badTers); testcase << "PermissionedDomain 2"; auto constexpr tooBig = maxPermissionedDomainCredentialsArraySize + 1; doInvariantCheck( + Env(*this, features), {{"permissioned domain bad credentials size " + std::to_string(tooBig)}}, [](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - slePd->setAccountID(sfOwner, A1); - slePd->setFieldU32(sfSequence, 10); - - STArray credentials(sfAcceptedCredentials, tooBig); - for (std::size_t n = 0; n < tooBig; ++n) - { - auto cred = STObject::makeInnerObject(sfCredential); - cred.setAccountID(sfIssuer, A2); - auto credType = std::string("cred_type") + std::to_string(n); - cred.setFieldVL(sfCredentialType, Slice(credType.c_str(), credType.size())); - credentials.push_back(std::move(cred)); - } - slePd->setFieldArray(sfAcceptedCredentials, credentials); - ac.view().insert(slePd); - return true; + return !!createPermissionedDomain(ac, A1, A2, tooBig); }, XRPAmount{}, STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + fixPDEnabled ? failTers : badTers); testcase << "PermissionedDomain 3"; doInvariantCheck( + Env(*this, features), {{"permissioned domain credentials aren't sorted"}}, [](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - slePd->setAccountID(sfOwner, A1); - slePd->setFieldU32(sfSequence, 10); + auto slePd = createPermissionedDomain(ac, A1, A2, 0); STArray credentials(sfAcceptedCredentials, 2); for (std::size_t n = 0; n < 2; ++n) @@ -1210,21 +1242,19 @@ class Invariants_test : public beast::unit_test::suite credentials.push_back(std::move(cred)); } slePd->setFieldArray(sfAcceptedCredentials, credentials); - ac.view().insert(slePd); + ac.view().update(slePd); return true; }, XRPAmount{}, STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + fixPDEnabled ? failTers : badTers); testcase << "PermissionedDomain 4"; doInvariantCheck( + Env(*this, features), {{"permissioned domain credentials aren't unique"}}, [](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - slePd->setAccountID(sfOwner, A1); - slePd->setFieldU32(sfSequence, 10); + auto slePd = createPermissionedDomain(ac, A1, A2, 0); STArray credentials(sfAcceptedCredentials, 2); for (std::size_t n = 0; n < 2; ++n) @@ -1235,22 +1265,20 @@ class Invariants_test : public beast::unit_test::suite credentials.push_back(std::move(cred)); } slePd->setFieldArray(sfAcceptedCredentials, credentials); - ac.view().insert(slePd); + ac.view().update(slePd); return true; }, XRPAmount{}, - STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, + fixPDEnabled ? failTers : badTers); testcase << "PermissionedDomain Set 1"; doInvariantCheck( + Env(*this, features), {{"permissioned domain with no rules."}}, [&](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - // create PD - createPermissionedDomain(ac, slePd, A1, A2); + auto slePd = createPermissionedDomain(ac, A1, A2); // update PD with empty rules { @@ -1262,18 +1290,16 @@ class Invariants_test : public beast::unit_test::suite return true; }, XRPAmount{}, - STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, + fixPDEnabled ? failTers : badTers); testcase << "PermissionedDomain Set 2"; doInvariantCheck( + Env(*this, features), {{"permissioned domain bad credentials size " + std::to_string(tooBig)}}, [&](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - // create PD - createPermissionedDomain(ac, slePd, A1, A2); + auto slePd = createPermissionedDomain(ac, A1, A2); // update PD { @@ -1295,18 +1321,16 @@ class Invariants_test : public beast::unit_test::suite return true; }, XRPAmount{}, - STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, + fixPDEnabled ? failTers : badTers); testcase << "PermissionedDomain Set 3"; doInvariantCheck( + Env(*this, features), {{"permissioned domain credentials aren't sorted"}}, [&](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - // create PD - createPermissionedDomain(ac, slePd, A1, A2); + auto slePd = createPermissionedDomain(ac, A1, A2); // update PD { @@ -1327,18 +1351,16 @@ class Invariants_test : public beast::unit_test::suite return true; }, XRPAmount{}, - STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, + fixPDEnabled ? failTers : badTers); testcase << "PermissionedDomain Set 4"; doInvariantCheck( + Env(*this, features), {{"permissioned domain credentials aren't unique"}}, [&](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - // create PD - createPermissionedDomain(ac, slePd, A1, A2); + auto slePd = createPermissionedDomain(ac, A1, A2); // update PD { @@ -1357,8 +1379,155 @@ class Invariants_test : public beast::unit_test::suite return true; }, XRPAmount{}, - STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, + fixPDEnabled ? failTers : badTers); + + std::initializer_list goodTers = {tesSUCCESS, tesSUCCESS}; + + std::vector badMoreThan1{{"transaction affected more than 1 permissioned domain entry."}}; + std::vector emptyV; + std::vector badNoDomains{{"no domain objects affected by"}}; + std::vector badNotDeleted{{"domain object modified, but not deleted by "}}; + std::vector badDeleted{{"domain object deleted by"}}; + std::vector badTx{{"domain object(s) affected by an unauthorized transaction."}}; + + { + testcase << "PermissionedDomain set 2 domains "; + doInvariantCheck( + Env(*this, features), + fixPDEnabled ? badMoreThan1 : emptyV, + [](Account const& A1, Account const& A2, ApplyContext& ac) { + createPermissionedDomain(ac, A1, A2); + createPermissionedDomain(ac, A1, A2, 2, 11); + return true; + }, + XRPAmount{}, + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, + fixPDEnabled ? failTers : goodTers); + } + + { + testcase << "PermissionedDomain del 2 domains"; + + Env env1(*this, features); + + Account const A1{"A1"}; + Account const A2{"A2"}; + env1.fund(XRP(1000), A1, A2); + env1.close(); + + [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2); + [[maybe_unused]] auto [seq2, pd2] = createPermissionedDomainEnv(env1, A1, A2); + env1.close(); + + doInvariantCheck( + std::move(env1), + A1, + A2, + fixPDEnabled ? badMoreThan1 : emptyV, + [&pd1, &pd2](Account const&, Account const&, ApplyContext& ac) { + auto sle1 = ac.view().peek({ltPERMISSIONED_DOMAIN, pd1}); + auto sle2 = ac.view().peek({ltPERMISSIONED_DOMAIN, pd2}); + ac.view().erase(sle1); + ac.view().erase(sle2); + return true; + }, + XRPAmount{}, + STTx{ttPERMISSIONED_DOMAIN_DELETE, [](STObject&) {}}, + fixPDEnabled ? failTers : goodTers); + } + + { + testcase << "PermissionedDomain set 0 domains "; + doInvariantCheck( + Env(*this, features), + fixPDEnabled ? badNoDomains : emptyV, + [](Account const&, Account const&, ApplyContext&) { return true; }, + XRPAmount{}, + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, + fixPDEnabled ? badTers : goodTers); + } + + { + testcase << "PermissionedDomain del 0 domains"; + + Env env1(*this, features); + + Account const A1{"A1"}; + Account const A2{"A2"}; + env1.fund(XRP(1000), A1, A2); + env1.close(); + + [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2); + [[maybe_unused]] auto [seq2, pd2] = createPermissionedDomainEnv(env1, A1, A2); + env1.close(); + + doInvariantCheck( + Env(*this, features), + A1, + A2, + fixPDEnabled ? badNoDomains : emptyV, + [](Account const&, Account const&, ApplyContext&) { return true; }, + XRPAmount{}, + STTx{ttPERMISSIONED_DOMAIN_DELETE, [](STObject&) {}}, + fixPDEnabled ? badTers : goodTers); + } + + { + testcase << "PermissionedDomain set, delete domain"; + + Env env1(*this, features); + + Account const A1{"A1"}; + Account const A2{"A2"}; + env1.fund(XRP(1000), A1, A2); + env1.close(); + + [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2); + env1.close(); + + doInvariantCheck( + std::move(env1), + A1, + A2, + fixPDEnabled ? badDeleted : emptyV, + [&pd1](Account const&, Account const&, ApplyContext& ac) { + auto sle1 = ac.view().peek({ltPERMISSIONED_DOMAIN, pd1}); + ac.view().erase(sle1); + return true; + }, + XRPAmount{}, + STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}}, + fixPDEnabled ? failTers : goodTers); + } + + { + testcase << "PermissionedDomain del, create domain "; + doInvariantCheck( + Env(*this, features), + fixPDEnabled ? badNotDeleted : emptyV, + [](Account const& A1, Account const& A2, ApplyContext& ac) { + createPermissionedDomain(ac, A1, A2); + return true; + }, + XRPAmount{}, + STTx{ttPERMISSIONED_DOMAIN_DELETE, [](STObject&) {}}, + fixPDEnabled ? failTers : goodTers); + } + + { + testcase << "PermissionedDomain invalid tx"; + + doInvariantCheck( + fixPDEnabled ? badTx : emptyV, + [&](Account const& A1, Account const& A2, ApplyContext& ac) { + createPermissionedDomain(ac, A1, A2); + return true; + }, + XRPAmount{}, + STTx{ttPAYMENT, [](STObject&) {}}, + failTers); + } } void @@ -1478,13 +1647,43 @@ class Invariants_test : public beast::unit_test::suite }); } - void - testPermissionedDEX() + static std::pair + createPermissionedDomainEnv( + test::jtx::Env& env, + test::jtx::Account const& A1, + test::jtx::Account const& A2, + std::uint32_t numCreds = 2) { using namespace test::jtx; - testcase << "PermissionedDEX"; + + pdomain::Credentials credentials; + + for (std::size_t n = 0; n < numCreds; ++n) + { + auto credType = "cred_type" + std::to_string(n); + credentials.push_back({A2, credType}); + } + + std::uint32_t const seq = env.seq(A1); + env(pdomain::setTx(A1, credentials)); + uint256 key = pdomain::getNewDomain(env.meta()); + + // std::cout << "PD, acc: " << A1.id() << ", seq: " << seq << ", k: " << + // key << std::endl; + return {seq, key}; + } + + void + testPermissionedDEX(FeatureBitset features) + { + using namespace test::jtx; + + bool const fixPDEnabled = features[fixPermissionedDomainInvariant]; + + testcase << "PermissionedDEX" + std::string(fixPDEnabled ? " fix" : ""); doInvariantCheck( + Env(*this, features), {{"domain doesn't exist"}}, [](Account const& A1, Account const&, ApplyContext& ac) { Keylet const offerKey = keylet::offer(A1.id(), 10); @@ -1511,12 +1710,9 @@ class Invariants_test : public beast::unit_test::suite // missing domain ID in offer object doInvariantCheck( + Env(*this, features), {{"hybrid offer is malformed"}}, [&](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - createPermissionedDomain(ac, slePd, A1, A2); - Keylet const offerKey = keylet::offer(A2.id(), 10); auto sleOffer = std::make_shared(offerKey); sleOffer->setAccountID(sfAccount, A2); @@ -1531,116 +1727,154 @@ class Invariants_test : public beast::unit_test::suite return true; }, XRPAmount{}, - STTx{ttOFFER_CREATE, [&](STObject& tx) {}}, + STTx{ttOFFER_CREATE, [&](STObject&) {}}, {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); // more than one entry in sfAdditionalBooks - doInvariantCheck( - {{"hybrid offer is malformed"}}, - [&](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - createPermissionedDomain(ac, slePd, A1, A2); + { + Env env1(*this, features); - Keylet const offerKey = keylet::offer(A2.id(), 10); - auto sleOffer = std::make_shared(offerKey); - sleOffer->setAccountID(sfAccount, A2); - sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10)); - sleOffer->setFieldAmount(sfTakerGets, XRP(1)); - sleOffer->setFlag(lsfHybrid); - sleOffer->setFieldH256(sfDomainID, pdKeylet.key); + Account const A1{"A1"}; + Account const A2{"A2"}; + env1.fund(XRP(1000), A1, A2); + env1.close(); - STArray bookArr; - bookArr.push_back(STObject::makeInnerObject(sfBook)); - bookArr.push_back(STObject::makeInnerObject(sfBook)); - sleOffer->setFieldArray(sfAdditionalBooks, bookArr); - ac.view().insert(sleOffer); - return true; - }, - XRPAmount{}, - STTx{ttOFFER_CREATE, [&](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2); + env1.close(); + + doInvariantCheck( + std::move(env1), + A1, + A2, + {{"hybrid offer is malformed"}}, + [&pd1](Account const& A1, Account const& A2, ApplyContext& ac) { + Keylet const offerKey = keylet::offer(A2.id(), 10); + auto sleOffer = std::make_shared(offerKey); + sleOffer->setAccountID(sfAccount, A2); + sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10)); + sleOffer->setFieldAmount(sfTakerGets, XRP(1)); + sleOffer->setFlag(lsfHybrid); + sleOffer->setFieldH256(sfDomainID, pd1); + + STArray bookArr; + bookArr.push_back(STObject::makeInnerObject(sfBook)); + bookArr.push_back(STObject::makeInnerObject(sfBook)); + sleOffer->setFieldArray(sfAdditionalBooks, bookArr); + ac.view().insert(sleOffer); + return true; + }, + XRPAmount{}, + STTx{ttOFFER_CREATE, [&](STObject&) {}}, + {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + } // hybrid offer missing sfAdditionalBooks - doInvariantCheck( - {{"hybrid offer is malformed"}}, - [&](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - createPermissionedDomain(ac, slePd, A1, A2); + { + Env env1(*this, features); - Keylet const offerKey = keylet::offer(A2.id(), 10); - auto sleOffer = std::make_shared(offerKey); - sleOffer->setAccountID(sfAccount, A2); - sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10)); - sleOffer->setFieldAmount(sfTakerGets, XRP(1)); - sleOffer->setFlag(lsfHybrid); - sleOffer->setFieldH256(sfDomainID, pdKeylet.key); - ac.view().insert(sleOffer); - return true; - }, - XRPAmount{}, - STTx{ttOFFER_CREATE, [&](STObject& tx) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + Account const A1{"A1"}; + Account const A2{"A2"}; + env1.fund(XRP(1000), A1, A2); + env1.close(); - doInvariantCheck( - {{"transaction consumed wrong domains"}}, - [&](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - createPermissionedDomain(ac, slePd, A1, A2); + [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2); + env1.close(); - Keylet const badDomainKeylet = keylet::permissionedDomain(A1.id(), 20); - auto sleBadPd = std::make_shared(badDomainKeylet); - createPermissionedDomain(ac, sleBadPd, A1, A2); + doInvariantCheck( + std::move(env1), + A1, + A2, + {{"hybrid offer is malformed"}}, + [&pd1](Account const& A1, Account const& A2, ApplyContext& ac) { + Keylet const offerKey = keylet::offer(A2.id(), 10); + auto sleOffer = std::make_shared(offerKey); + sleOffer->setAccountID(sfAccount, A2); + sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10)); + sleOffer->setFieldAmount(sfTakerGets, XRP(1)); + sleOffer->setFlag(lsfHybrid); + sleOffer->setFieldH256(sfDomainID, pd1); + ac.view().insert(sleOffer); + return true; + }, + XRPAmount{}, + STTx{ttOFFER_CREATE, [&](STObject&) {}}, + {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + } - Keylet const offerKey = keylet::offer(A2.id(), 10); - auto sleOffer = std::make_shared(offerKey); - sleOffer->setAccountID(sfAccount, A2); - sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10)); - sleOffer->setFieldAmount(sfTakerGets, XRP(1)); - sleOffer->setFieldH256(sfDomainID, pdKeylet.key); - ac.view().insert(sleOffer); - return true; - }, - XRPAmount{}, - STTx{ - ttOFFER_CREATE, - [&](STObject& tx) { - Account const A1{"A1"}; - Keylet const badDomainKey = keylet::permissionedDomain(A1.id(), 20); - tx.setFieldH256(sfDomainID, badDomainKey.key); - tx.setFieldAmount(sfTakerPays, A1["USD"](10)); - tx.setFieldAmount(sfTakerGets, XRP(1)); - }}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + { + Env env1(*this, features); - doInvariantCheck( - {{"domain transaction affected regular offers"}}, - [&](Account const& A1, Account const& A2, ApplyContext& ac) { - Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10); - auto slePd = std::make_shared(pdKeylet); - createPermissionedDomain(ac, slePd, A1, A2); + Account const A1{"A1"}; + Account const A2{"A2"}; + env1.fund(XRP(1000), A1, A2); + env1.close(); - Keylet const offerKey = keylet::offer(A2.id(), 10); - auto sleOffer = std::make_shared(offerKey); - sleOffer->setAccountID(sfAccount, A2); - sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10)); - sleOffer->setFieldAmount(sfTakerGets, XRP(1)); - ac.view().insert(sleOffer); - return true; - }, - XRPAmount{}, - STTx{ - ttOFFER_CREATE, - [&](STObject& tx) { - Account const A1{"A1"}; - Keylet const domainKey = keylet::permissionedDomain(A1.id(), 10); - tx.setFieldH256(sfDomainID, domainKey.key); - tx.setFieldAmount(sfTakerPays, A1["USD"](10)); - tx.setFieldAmount(sfTakerGets, XRP(1)); - }}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2); + [[maybe_unused]] auto [seq2, pd2] = createPermissionedDomainEnv(env1, A1, A2); + env1.close(); + + doInvariantCheck( + std::move(env1), + A1, + A2, + {{"transaction consumed wrong domains"}}, + [&pd1](Account const& A1, Account const& A2, ApplyContext& ac) { + Keylet const offerKey = keylet::offer(A2.id(), 10); + auto sleOffer = std::make_shared(offerKey); + sleOffer->setAccountID(sfAccount, A2); + sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10)); + sleOffer->setFieldAmount(sfTakerGets, XRP(1)); + sleOffer->setFieldH256(sfDomainID, pd1); + ac.view().insert(sleOffer); + return true; + }, + XRPAmount{}, + STTx{ + ttOFFER_CREATE, + [&pd2, &A1](STObject& tx) { + tx.setFieldH256(sfDomainID, pd2); + tx.setFieldAmount(sfTakerPays, A1["USD"](10)); + tx.setFieldAmount(sfTakerGets, XRP(1)); + }}, + {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + } + + { + Env env1(*this, features); + + Account const A1{"A1"}; + Account const A2{"A2"}; + env1.fund(XRP(1000), A1, A2); + env1.close(); + + [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2); + env1.close(); + + doInvariantCheck( + std::move(env1), + A1, + A2, + {{"domain transaction affected regular offers"}}, + [&](Account const& A1, Account const& A2, ApplyContext& ac) { + Keylet const offerKey = keylet::offer(A2.id(), 10); + auto sleOffer = std::make_shared(offerKey); + sleOffer->setAccountID(sfAccount, A2); + sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10)); + sleOffer->setFieldAmount(sfTakerGets, XRP(1)); + ac.view().insert(sleOffer); + return true; + }, + XRPAmount{}, + STTx{ + ttOFFER_CREATE, + [&](STObject& tx) { + Account const A1{"A1"}; + tx.setFieldH256(sfDomainID, pd1); + tx.setFieldAmount(sfTakerPays, A1["USD"](10)); + tx.setFieldAmount(sfTakerGets, XRP(1)); + }}, + {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); + } } Keylet @@ -3490,8 +3724,10 @@ public: testNoZeroEscrow(); testValidNewAccountRoot(); testNFTokenPageInvariants(); - testPermissionedDomainInvariants(); - testPermissionedDEX(); + testPermissionedDomainInvariants(defaultAmendments() | fixPermissionedDomainInvariant); + testPermissionedDomainInvariants(defaultAmendments() - fixPermissionedDomainInvariant); + testPermissionedDEX(defaultAmendments() | fixPermissionedDomainInvariant); + testPermissionedDEX(defaultAmendments() - fixPermissionedDomainInvariant); testNoModifiedUnmodifiableFields(); testValidPseudoAccounts(); testValidLoanBroker(); diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index 1a1fe9a9eb..8485202144 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -38,13 +38,17 @@ class PermissionedDomains_test : public beast::unit_test::suite testable_amendments() // | featurePermissionedDomains | featureCredentials}; + FeatureBitset withFix_{ + testable_amendments() // + | featurePermissionedDomains | featureCredentials}; + // Verify that each tx type can execute if the feature is enabled. void - testEnabled() + testEnabled(FeatureBitset features) { testcase("Enabled"); Account const alice("alice"); - Env env(*this, withFeature_); + Env env(*this, features); env.fund(XRP(1000), alice); pdomain::Credentials credentials{{alice, "first credential"}}; env(pdomain::setTx(alice, credentials)); @@ -237,10 +241,10 @@ class PermissionedDomains_test : public beast::unit_test::suite // Test PermissionedDomainSet void - testSet() + testSet(FeatureBitset features) { testcase("Set"); - Env env(*this, withFeature_); + Env env(*this, features); env.set_parse_failure_expected(true); int const accNum = 12; @@ -395,10 +399,10 @@ class PermissionedDomains_test : public beast::unit_test::suite // Test PermissionedDomainDelete void - testDelete() + testDelete(FeatureBitset features) { testcase("Delete"); - Env env(*this, withFeature_); + Env env(*this, features); Account const alice("alice"); env.fund(XRP(1000), alice); @@ -448,14 +452,14 @@ class PermissionedDomains_test : public beast::unit_test::suite } void - testAccountReserve() + testAccountReserve(FeatureBitset features) { // Verify that the reserve behaves as expected for creating. testcase("Account Reserve"); using namespace test::jtx; - Env env(*this, withFeature_); + Env env(*this, features); Account const alice("alice"); // Fund alice enough to exist, but not enough to meet @@ -500,12 +504,16 @@ public: void run() override { - testEnabled(); + testEnabled(withFeature_); + testEnabled(withFix_); testCredentialsDisabled(); testDisabled(); - testSet(); - testDelete(); - testAccountReserve(); + testSet(withFeature_); + testSet(withFix_); + testDelete(withFeature_); + testDelete(withFix_); + testAccountReserve(withFeature_); + testAccountReserve(withFix_); } }; diff --git a/src/test/jtx/impl/balance.cpp b/src/test/jtx/impl/balance.cpp index 9d6f5c6124..1ebeeed437 100644 --- a/src/test/jtx/impl/balance.cpp +++ b/src/test/jtx/impl/balance.cpp @@ -4,6 +4,10 @@ namespace xrpl { namespace test { namespace jtx { +#define TEST_EXPECT(cond) env.test.expect(cond, __FILE__, __LINE__) +#define TEST_EXPECTS(cond, reason) \ + ((cond) ? (env.test.pass(), true) : (env.test.fail((reason), __FILE__, __LINE__), false)) + void doBalance(Env& env, AccountID const& account, bool none, STAmount const& value, Issue const& issue) { @@ -12,11 +16,13 @@ doBalance(Env& env, AccountID const& account, bool none, STAmount const& value, auto const sle = env.le(keylet::account(account)); if (none) { - env.test.expect(!sle); + TEST_EXPECT(!sle); } - else if (env.test.expect(sle)) + else if (TEST_EXPECT(sle)) { - env.test.expect(sle->getFieldAmount(sfBalance) == value); + TEST_EXPECTS( + sle->getFieldAmount(sfBalance) == value, + sle->getFieldAmount(sfBalance).getText() + " / " + value.getText()); } } else @@ -24,15 +30,15 @@ doBalance(Env& env, AccountID const& account, bool none, STAmount const& value, auto const sle = env.le(keylet::line(account, issue)); if (none) { - env.test.expect(!sle); + TEST_EXPECT(!sle); } - else if (env.test.expect(sle)) + else if (TEST_EXPECT(sle)) { auto amount = sle->getFieldAmount(sfBalance); amount.setIssuer(issue.account); if (account > issue.account) amount.negate(); - env.test.expect(amount == value); + TEST_EXPECTS(amount == value, amount.getText()); } } } @@ -43,12 +49,12 @@ doBalance(Env& env, AccountID const& account, bool none, STAmount const& value, auto const sle = env.le(keylet::mptoken(mptIssue.getMptID(), account)); if (none) { - env.test.expect(!sle); + TEST_EXPECT(!sle); } - else if (env.test.expect(sle)) + else if (TEST_EXPECT(sle)) { STAmount const amount{mptIssue, sle->getFieldU64(sfMPTAmount)}; - env.test.expect(amount == value); + TEST_EXPECT(amount == value); } } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index fe47449c36..24a37270ce 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -1507,7 +1507,7 @@ ValidMPTIssuance::finalize( void ValidPermissionedDomain::visitEntry( - bool, + bool isDel, std::shared_ptr const& before, std::shared_ptr const& after) { @@ -1516,39 +1516,29 @@ ValidPermissionedDomain::visitEntry( if (after && after->getType() != ltPERMISSIONED_DOMAIN) return; - auto check = [](SleStatus& sleStatus, std::shared_ptr const& sle) { + auto check = [isDel](std::vector& sleStatus, std::shared_ptr const& sle) { auto const& credentials = sle->getFieldArray(sfAcceptedCredentials); - sleStatus.credentialsSize_ = credentials.size(); auto const sorted = credentials::makeSorted(credentials); - sleStatus.isUnique_ = !sorted.empty(); + + SleStatus ss{credentials.size(), false, !sorted.empty(), isDel}; // If array have duplicates then all the other checks are invalid - sleStatus.isSorted_ = false; - - if (sleStatus.isUnique_) + if (ss.isUnique_) { unsigned i = 0; for (auto const& cred : sorted) { auto const& credTx = credentials[i++]; - sleStatus.isSorted_ = (cred.first == credTx[sfIssuer]) && (cred.second == credTx[sfCredentialType]); - if (!sleStatus.isSorted_) + ss.isSorted_ = (cred.first == credTx[sfIssuer]) && (cred.second == credTx[sfCredentialType]); + if (!ss.isSorted_) break; } } + sleStatus.emplace_back(std::move(ss)); }; - if (before) - { - sleStatus_[0] = SleStatus(); - check(*sleStatus_[0], after); - } - if (after) - { - sleStatus_[1] = SleStatus(); - check(*sleStatus_[1], after); - } + check(sleStatus_, after); } bool @@ -1559,9 +1549,6 @@ ValidPermissionedDomain::finalize( ReadView const& view, beast::Journal const& j) { - if (tx.getTxnType() != ttPERMISSIONED_DOMAIN_SET || result != tesSUCCESS) - return true; - auto check = [](SleStatus const& sleStatus, beast::Journal const& j) { if (!sleStatus.credentialsSize_) { @@ -1595,7 +1582,76 @@ ValidPermissionedDomain::finalize( return true; }; - return (sleStatus_[0] ? check(*sleStatus_[0], j) : true) && (sleStatus_[1] ? check(*sleStatus_[1], j) : true); + if (view.rules().enabled(fixPermissionedDomainInvariant)) + { + // No permissioned domains should be affected if the transaction failed + if (result != tesSUCCESS) + // If nothing changed, all is good. If there were changes, that's + // bad. + return sleStatus_.empty(); + + if (sleStatus_.size() > 1) + { + JLOG(j.fatal()) << "Invariant failed: transaction affected more " + "than 1 permissioned domain entry."; + return false; + } + + switch (tx.getTxnType()) + { + case ttPERMISSIONED_DOMAIN_SET: { + if (sleStatus_.empty()) + { + JLOG(j.fatal()) << "Invariant failed: no domain objects affected by " + "PermissionedDomainSet"; + return false; + } + + auto const& sleStatus = sleStatus_[0]; + if (sleStatus.isDelete_) + { + JLOG(j.fatal()) << "Invariant failed: domain object " + "deleted by PermissionedDomainSet"; + return false; + } + return check(sleStatus, j); + } + case ttPERMISSIONED_DOMAIN_DELETE: { + if (sleStatus_.empty()) + { + JLOG(j.fatal()) << "Invariant failed: no domain objects affected by " + "PermissionedDomainDelete"; + return false; + } + + if (!sleStatus_[0].isDelete_) + { + JLOG(j.fatal()) << "Invariant failed: domain object " + "modified, but not deleted by " + "PermissionedDomainDelete"; + return false; + } + return true; + } + default: { + if (!sleStatus_.empty()) + { + JLOG(j.fatal()) << "Invariant failed: " << sleStatus_.size() + << " domain object(s) affected by an " + "unauthorized transaction. " + << tx.getTxnType(); + return false; + } + return true; + } + } + } + else + { + if (tx.getTxnType() != ttPERMISSIONED_DOMAIN_SET || result != tesSUCCESS || sleStatus_.empty()) + return true; + return check(sleStatus_[0], j); + } } //------------------------------------------------------------------------------ diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index 9a20e372ba..7e9a62d9e2 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/InvariantCheck.h @@ -449,9 +449,11 @@ class ValidPermissionedDomain struct SleStatus { std::size_t credentialsSize_{0}; - bool isSorted_ = false, isUnique_ = false; + bool isSorted_ = false; + bool isUnique_ = false; + bool isDelete_ = false; }; - std::optional sleStatus_[2]; + std::vector sleStatus_; public: void