fix: Update invariant checks for Permissioned Domains (#6134)

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)

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<bool(test::jtx::Account const& a, test::jtx::Account const& b, ApplyContext& ac)>;
 
+    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<TER> 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<std::string> const& expect_logs,
+        Precheck const& precheck,
+        XRPAmount fee = XRPAmount{},
+        STTx tx = STTx{ttACCOUNT_SET, [](STObject&) {}},
+        std::initializer_list<TER> 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<std::string> const& expect_logs,
+        Precheck const& precheck,
+        XRPAmount fee = XRPAmount{},
+        STTx tx = STTx{ttACCOUNT_SET, [](STObject&) {}},
+        std::initializer_list<TER> 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)
+                BEAST_EXPECTS(messages.find(m) != std::string::npos, m);
-                {
-                    // uncomment if you want to log the invariant failure
-                    // std::cerr << "   --> " << m << std::endl;
-                    fail();
-                }
             }
         }
     }
@@ -1119,86 +1157,80 @@ class Invariants_test : public beast::unit_test::suite
             });
     }
 
-    void
+    static std::shared_ptr<SLE>
     createPermissionedDomain(
         ApplyContext& ac,
-        std::shared_ptr<SLE>& 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);
+        Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), seq);
-        sle->setFieldU32(sfSequence, 10);
+        auto sle = std::make_shared<SLE>(pdKeylet);
 
-        STArray credentials(sfAcceptedCredentials, 2);
+        sle->setAccountID(sfOwner, A1);
-        for (std::size_t n = 0; n < 2; ++n)
+        sle->setFieldU32(sfSequence, seq);
+
+        if (numCreds)
         {
-            auto cred = STObject::makeInnerObject(sfCredential);
+            // This array is sorted naturally, but if you willing to change this
-            cred.setAccountID(sfIssuer, A2);
+            // behavior don't forget to use credentials::makeSorted
-            auto credType = "cred_type" + std::to_string(n);
+            STArray credentials(sfAcceptedCredentials, numCreds);
-            cred.setFieldVL(sfCredentialType, Slice(credType.c_str(), credType.size()));
+            for (std::size_t n = 0; n < numCreds; ++n)
-            credentials.push_back(std::move(cred));
+            {
+                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";
+        bool const fixPDEnabled = features[fixPermissionedDomainInvariant];
-        doInvariantCheck(
+        std::initializer_list<TER> badTers = {tecINVARIANT_FAILED, tecINVARIANT_FAILED};
-            {{"permissioned domain with no rules."}},
+        std::initializer_list<TER> failTers = {tecINVARIANT_FAILED, tefINVARIANT_FAILED};
-            [](Account const& A1, Account const&, ApplyContext& ac) {
-                Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10);
-                auto slePd = std::make_shared<SLE>(pdKeylet);
-                slePd->setAccountID(sfOwner, A1);
-                slePd->setFieldU32(sfSequence, 10);
 
-                ac.view().insert(slePd);
+        testcase << "PermissionedDomain" + std::string(fixPDEnabled ? " fix" : "");
-                return true;
+
+        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) {}},
+            STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}},
-            {tecINVARIANT_FAILED, tecINVARIANT_FAILED});
+            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);
+                return !!createPermissionedDomain(ac, A1, A2, tooBig);
-                auto slePd = std::make_shared<SLE>(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;
             },
             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 = createPermissionedDomain(ac, A1, A2, 0);
-                auto slePd = std::make_shared<SLE>(pdKeylet);
-                slePd->setAccountID(sfOwner, A1);
-                slePd->setFieldU32(sfSequence, 10);
 
                 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 = createPermissionedDomain(ac, A1, A2, 0);
-                auto slePd = std::make_shared<SLE>(pdKeylet);
-                slePd->setAccountID(sfOwner, A1);
-                slePd->setFieldU32(sfSequence, 10);
 
                 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) {}},
+            STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}},
-            {tecINVARIANT_FAILED, tecINVARIANT_FAILED});
+            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<SLE>(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) {}},
+            STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}},
-            {tecINVARIANT_FAILED, tecINVARIANT_FAILED});
+            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<SLE>(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) {}},
+            STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}},
-            {tecINVARIANT_FAILED, tecINVARIANT_FAILED});
+            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<SLE>(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) {}},
+            STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}},
-            {tecINVARIANT_FAILED, tecINVARIANT_FAILED});
+            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<SLE>(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) {}},
+            STTx{ttPERMISSIONED_DOMAIN_SET, [](STObject&) {}},
-            {tecINVARIANT_FAILED, tecINVARIANT_FAILED});
+            fixPDEnabled ? failTers : badTers);
+
+        std::initializer_list<TER> goodTers = {tesSUCCESS, tesSUCCESS};
+
+        std::vector<std::string> badMoreThan1{{"transaction affected more than 1 permissioned domain entry."}};
+        std::vector<std::string> emptyV;
+        std::vector<std::string> badNoDomains{{"no domain objects affected by"}};
+        std::vector<std::string> badNotDeleted{{"domain object modified, but not deleted by "}};
+        std::vector<std::string> badDeleted{{"domain object deleted by"}};
+        std::vector<std::string> 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
+    static std::pair<std::uint32_t, uint256>
-    testPermissionedDEX()
+    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<SLE>(pdKeylet);
-                createPermissionedDomain(ac, slePd, A1, A2);
-
                 Keylet const offerKey = keylet::offer(A2.id(), 10);
                 auto sleOffer = std::make_shared<SLE>(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"}},
+            Env env1(*this, features);
-            [&](Account const& A1, Account const& A2, ApplyContext& ac) {
-                Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10);
-                auto slePd = std::make_shared<SLE>(pdKeylet);
-                createPermissionedDomain(ac, slePd, A1, A2);
 
-                Keylet const offerKey = keylet::offer(A2.id(), 10);
+            Account const A1{"A1"};
-                auto sleOffer = std::make_shared<SLE>(offerKey);
+            Account const A2{"A2"};
-                sleOffer->setAccountID(sfAccount, A2);
+            env1.fund(XRP(1000), A1, A2);
-                sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10));
+            env1.close();
-                sleOffer->setFieldAmount(sfTakerGets, XRP(1));
-                sleOffer->setFlag(lsfHybrid);
-                sleOffer->setFieldH256(sfDomainID, pdKeylet.key);
 
-                STArray bookArr;
+            [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2);
-                bookArr.push_back(STObject::makeInnerObject(sfBook));
+            env1.close();
-                bookArr.push_back(STObject::makeInnerObject(sfBook));
+
-                sleOffer->setFieldArray(sfAdditionalBooks, bookArr);
+            doInvariantCheck(
-                ac.view().insert(sleOffer);
+                std::move(env1),
-                return true;
+                A1,
-            },
+                A2,
-            XRPAmount{},
+                {{"hybrid offer is malformed"}},
-            STTx{ttOFFER_CREATE, [&](STObject& tx) {}},
+                [&pd1](Account const& A1, Account const& A2, ApplyContext& ac) {
-            {tecINVARIANT_FAILED, tecINVARIANT_FAILED});
+                    Keylet const offerKey = keylet::offer(A2.id(), 10);
+                    auto sleOffer = std::make_shared<SLE>(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"}},
+            Env env1(*this, features);
-            [&](Account const& A1, Account const& A2, ApplyContext& ac) {
-                Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10);
-                auto slePd = std::make_shared<SLE>(pdKeylet);
-                createPermissionedDomain(ac, slePd, A1, A2);
 
-                Keylet const offerKey = keylet::offer(A2.id(), 10);
+            Account const A1{"A1"};
-                auto sleOffer = std::make_shared<SLE>(offerKey);
+            Account const A2{"A2"};
-                sleOffer->setAccountID(sfAccount, A2);
+            env1.fund(XRP(1000), A1, A2);
-                sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10));
+            env1.close();
-                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});
 
-        doInvariantCheck(
+            [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2);
-            {{"transaction consumed wrong domains"}},
+            env1.close();
-            [&](Account const& A1, Account const& A2, ApplyContext& ac) {
-                Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10);
-                auto slePd = std::make_shared<SLE>(pdKeylet);
-                createPermissionedDomain(ac, slePd, A1, A2);
 
-                Keylet const badDomainKeylet = keylet::permissionedDomain(A1.id(), 20);
+            doInvariantCheck(
-                auto sleBadPd = std::make_shared<SLE>(badDomainKeylet);
+                std::move(env1),
-                createPermissionedDomain(ac, sleBadPd, A1, A2);
+                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<SLE>(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<SLE>(offerKey);
+            Env env1(*this, features);
-                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});
 
-        doInvariantCheck(
+            Account const A1{"A1"};
-            {{"domain transaction affected regular offers"}},
+            Account const A2{"A2"};
-            [&](Account const& A1, Account const& A2, ApplyContext& ac) {
+            env1.fund(XRP(1000), A1, A2);
-                Keylet const pdKeylet = keylet::permissionedDomain(A1.id(), 10);
+            env1.close();
-                auto slePd = std::make_shared<SLE>(pdKeylet);
-                createPermissionedDomain(ac, slePd, A1, A2);
 
-                Keylet const offerKey = keylet::offer(A2.id(), 10);
+            [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2);
-                auto sleOffer = std::make_shared<SLE>(offerKey);
+            [[maybe_unused]] auto [seq2, pd2] = createPermissionedDomainEnv(env1, A1, A2);
-                sleOffer->setAccountID(sfAccount, A2);
+            env1.close();
-                sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10));
+
-                sleOffer->setFieldAmount(sfTakerGets, XRP(1));
+            doInvariantCheck(
-                ac.view().insert(sleOffer);
+                std::move(env1),
-                return true;
+                A1,
-            },
+                A2,
-            XRPAmount{},
+                {{"transaction consumed wrong domains"}},
-            STTx{
+                [&pd1](Account const& A1, Account const& A2, ApplyContext& ac) {
-                ttOFFER_CREATE,
+                    Keylet const offerKey = keylet::offer(A2.id(), 10);
-                [&](STObject& tx) {
+                    auto sleOffer = std::make_shared<SLE>(offerKey);
-                    Account const A1{"A1"};
+                    sleOffer->setAccountID(sfAccount, A2);
-                    Keylet const domainKey = keylet::permissionedDomain(A1.id(), 10);
+                    sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10));
-                    tx.setFieldH256(sfDomainID, domainKey.key);
+                    sleOffer->setFieldAmount(sfTakerGets, XRP(1));
-                    tx.setFieldAmount(sfTakerPays, A1["USD"](10));
+                    sleOffer->setFieldH256(sfDomainID, pd1);
-                    tx.setFieldAmount(sfTakerGets, XRP(1));
+                    ac.view().insert(sleOffer);
-                }},
+                    return true;
-            {tecINVARIANT_FAILED, tecINVARIANT_FAILED});
+                },
+                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<SLE>(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();
+        testPermissionedDomainInvariants(defaultAmendments() | fixPermissionedDomainInvariant);
-        testPermissionedDEX();
+        testPermissionedDomainInvariants(defaultAmendments() - fixPermissionedDomainInvariant);
+        testPermissionedDEX(defaultAmendments() | fixPermissionedDomainInvariant);
+        testPermissionedDEX(defaultAmendments() - fixPermissionedDomainInvariant);
         testNoModifiedUnmodifiableFields();
         testValidPseudoAccounts();
         testValidLoanBroker();

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();
+        testSet(withFeature_);
-        testDelete();
+        testSet(withFix_);
-        testAccountReserve();
+        testDelete(withFeature_);
+        testDelete(withFix_);
+        testAccountReserve(withFeature_);
+        testAccountReserve(withFix_);
     }
 };
 

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

src/xrpld/app/tx/detail/InvariantCheck.cpp

@@ -1507,7 +1507,7 @@ ValidMPTIssuance::finalize(
 
 void
 ValidPermissionedDomain::visitEntry(
-    bool,
+    bool isDel,
     std::shared_ptr<SLE const> const& before,
     std::shared_ptr<SLE const> const& after)
 {
@@ -1516,39 +1516,29 @@ ValidPermissionedDomain::visitEntry(
     if (after && after->getType() != ltPERMISSIONED_DOMAIN)
         return;
 
-    auto check = [](SleStatus& sleStatus, std::shared_ptr<SLE const> const& sle) {
+    auto check = [isDel](std::vector<SleStatus>& sleStatus, std::shared_ptr<SLE const> 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 (ss.isUnique_)
-
-        if (sleStatus.isUnique_)
         {
             unsigned i = 0;
             for (auto const& cred : sorted)
             {
                 auto const& credTx = credentials[i++];
-                sleStatus.isSorted_ = (cred.first == credTx[sfIssuer]) && (cred.second == credTx[sfCredentialType]);
+                ss.isSorted_ = (cred.first == credTx[sfIssuer]) && (cred.second == credTx[sfCredentialType]);
-                if (!sleStatus.isSorted_)
+                if (!ss.isSorted_)
                     break;
             }
         }
+        sleStatus.emplace_back(std::move(ss));
     };
 
-    if (before)
-    {
-        sleStatus_[0] = SleStatus();
-        check(*sleStatus_[0], after);
-    }
-
     if (after)
-    {
+        check(sleStatus_, after);
-        sleStatus_[1] = SleStatus();
-        check(*sleStatus_[1], 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);
+    }
 }
 
 //------------------------------------------------------------------------------

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> sleStatus_[2];
+    std::vector<SleStatus> sleStatus_;
 
 public:
     void