Merge branch 'develop'

Resolve conflicts with develop's typedef/using rename (#8177) and its new
amendments. Rename SLE::const_ref to SLE::ConstRef in
DepositPreauth::visitInvariantEntry, since develop removed the old alias.

include/xrpl/protocol/detail/features.macro

@@ -15,6 +15,7 @@
 // Add new amendments to the top of this list.
 // Keep it sorted in reverse chronological order.
 
+XRPL_FIX    (DepositPreauthSelf,          Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(Cosign,                      Supported::No,  VoteBehavior::DefaultNo)
 XRPL_FEATURE(SmartEscrow,                 Supported::No,  VoteBehavior::DefaultNo)
 XRPL_FEATURE(LendingProtocolV1_2,         Supported::No,  VoteBehavior::DefaultNo)

include/xrpl/tx/transactors/payment/DepositPreauth.h

@@ -11,6 +11,8 @@
 #include <xrpl/tx/ApplyContext.h>
 #include <xrpl/tx/Transactor.h>
 
+#include <cstdint>
+
 namespace xrpl {
 
 class DepositPreauth : public Transactor
@@ -48,6 +50,27 @@ public:
     // Interface used by AccountDelete
     static TER
     removeFromLedger(ApplyView& view, UInt256 const& delIndex, beast::Journal j);
+
+private:
+    /**
+     * Number of DepositPreauth grants this transaction inserted.
+     */
+    std::uint32_t grantsCreated_{0};
+
+    /**
+     * Number of DepositPreauth grants this transaction erased.
+     */
+    std::uint32_t grantsRemoved_{0};
+
+    /**
+     * Set when a touched grant's owner is not the submitting account.
+     */
+    bool grantOwnerMismatch_{false};
+
+    /**
+     * Set when a created grant's credential array is not canonically sorted.
+     */
+    bool grantCredsNotCanonical_{false};
 };
 
 }  // namespace xrpl

src/libxrpl/tx/transactors/payment/DepositPreauth.cpp

@@ -12,6 +12,7 @@
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/Keylet.h>
+#include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STArray.h>
@@ -72,8 +73,10 @@ DepositPreauth::preflight(PreflightContext const& ctx)
             return temINVALID_ACCOUNT_ID;
         }
 
-        // An account may not preauthorize itself.
-        if (optAuth && (target == ctx.tx[sfAccount]))
+        // An account may not preauthorize itself. The Unauthorize direction is
+        // amendment-gated because older ledgers rejected it later, in preclaim.
+        bool const selfTarget = (target == ctx.tx[sfAccount]);
+        if ((optAuth || (optUnauth && ctx.rules.enabled(fixDepositPreauthSelf))) && selfTarget)
         {
             JLOG(ctx.j.trace()) << "Malformed transaction: Attempting to DepositPreauth self.";
             return temCANNOT_PREAUTH_SELF;
@@ -133,9 +136,11 @@ DepositPreauth::preclaim(PreclaimContext const& ctx)
             auto const& issuer = o[sfIssuer];
             if (!ctx.view.exists(keylet::account(issuer)))
                 return tecNO_ISSUER;
+            // Duplicates are bad user input, not node corruption. Preflight
+            // already rejects them with the same code, so this stays unreachable.
             auto [it, ins] = sorted.emplace(issuer, o[sfCredentialType]);
             if (!ins)
-                return tefINTERNAL;  // LCOV_EXCL_LINE
+                return temMALFORMED;  // LCOV_EXCL_LINE
         }
 
         // Verify that the Preauth entry they asked to add is not already
@@ -145,11 +150,16 @@ DepositPreauth::preclaim(PreclaimContext const& ctx)
     }
     else if (ctx.tx.isFieldPresent(sfUnauthorizeCredentials))
     {
+        auto const sorted = credentials::makeSorted(ctx.tx.getFieldArray(sfUnauthorizeCredentials));
+
+        // makeSorted returns an empty set to report duplicates, and an empty set
+        // would otherwise build the keylet of the empty credential set. Preflight
+        // already rejects duplicates, so this stays unreachable.
+        if (sorted.empty())
+            return temMALFORMED;  // LCOV_EXCL_LINE
+
         // Verify that the Preauth entry is in the ledger.
-        if (!ctx.view.exists(
-                keylet::depositPreauth(
-                    account,
-                    credentials::makeSorted(ctx.tx.getFieldArray(sfUnauthorizeCredentials)))))
+        if (!ctx.view.exists(keylet::depositPreauth(account, sorted)))
             return tecNO_ENTRY;
     }
     return tesSUCCESS;
@@ -300,10 +310,48 @@ DepositPreauth::removeFromLedger(ApplyView& view, UInt256 const& preauthIndex, b
     return tesSUCCESS;
 }
 
-void
-DepositPreauth::visitInvariantEntry(bool, SLE::ConstRef, SLE::ConstRef)
+namespace {
+
+// True when the array is strictly ascending by issuer then credential type,
+// which is what makeSorted produces and so also means duplicate-free.
+bool
+credentialsCanonical(STArray const& credentials)
 {
-    // No transaction-specific invariants yet (future work).
+    std::optional<std::pair<AccountID, Slice>> prev;
+    for (auto const& credential : credentials)
+    {
+        std::pair<AccountID, Slice> const current{
+            credential[sfIssuer], credential[sfCredentialType]};
+        if (prev && !(*prev < current))
+            return false;
+        prev = current;
+    }
+    return true;
+}
+
+}  // namespace
+
+void
+DepositPreauth::visitInvariantEntry(bool isDelete, SLE::ConstRef before, SLE::ConstRef after)
+{
+    // On deletion `after` holds the erased entry, so only isDelete is reliable.
+    auto const& sle = after ? after : before;
+    if (!sle || sle->getType() != ltDEPOSIT_PREAUTH)
+        return;
+
+    // An existing grant was modified rather than created or removed.
+    if (before && !isDelete)
+        return;
+
+    std::uint32_t& counter = isDelete ? grantsRemoved_ : grantsCreated_;
+    ++counter;
+
+    if (sle->getAccountID(sfAccount) != ctx_.tx[sfAccount])
+        grantOwnerMismatch_ = true;
+
+    if (!isDelete && sle->isFieldPresent(sfAuthorizeCredentials) &&
+        !credentialsCanonical(sle->getFieldArray(sfAuthorizeCredentials)))
+        grantCredsNotCanonical_ = true;
 }
 
 bool
@@ -311,10 +359,38 @@ DepositPreauth::finalizeInvariants(
     STTx const&,
     TER,
     XRPAmount,
-    ReadView const&,
-    beast::Journal const&)
+    ReadView const& view,
+    beast::Journal const& j)
 {
-    // No transaction-specific invariants yet (future work).
+    if (!view.rules().enabled(fixDepositPreauthSelf))
+        return true;
+
+    // LCOV_EXCL_START
+    // A violation means the transactor wrote a grant it should not have, so
+    // these branches are unreachable unless one of the paths above is broken.
+    if (grantOwnerMismatch_)
+    {
+        JLOG(j.fatal()) << "DepositPreauth invariant: grant owner is not the "
+                           "submitting account.";
+        return false;
+    }
+
+    // A failed transaction touches no grant, so only an excess is a violation.
+    if (grantsCreated_ + grantsRemoved_ > 1)
+    {
+        JLOG(j.fatal()) << "DepositPreauth invariant: " << grantsCreated_ + grantsRemoved_
+                        << " grants changed in one transaction.";
+        return false;
+    }
+
+    if (grantCredsNotCanonical_)
+    {
+        JLOG(j.fatal()) << "DepositPreauth invariant: credential array is not "
+                           "canonically sorted.";
+        return false;
+    }
+    // LCOV_EXCL_STOP
+
     return true;
 }
 

src/test/app/DepositAuth_test.cpp

@@ -17,6 +17,8 @@
 #include <test/jtx/require.h>
 #include <test/jtx/sendmax.h>
 #include <test/jtx/seq.h>
+#include <test/jtx/sig.h>
+#include <test/jtx/sponsor.h>
 #include <test/jtx/ter.h>
 #include <test/jtx/ticket.h>
 #include <test/jtx/trust.h>
@@ -502,6 +504,27 @@ struct DepositPreauth_test : public beast::unit_test::Suite
         env(deposit::auth(alice, alice), Ter(temCANNOT_PREAUTH_SELF));
         env.close();
 
+        // alice unauthorizes herself.  Before fixDepositPreauthSelf this
+        // slipped past preflight and was charged a fee by preclaim.
+        {
+            auto const baseFee = env.current()->fees().base;
+            PrettyAmount const aliceXrpBalance{env.balance(alice, XRP)};
+            if (features[fixDepositPreauthSelf])
+            {
+                env(deposit::unauth(alice, alice), Ter(temCANNOT_PREAUTH_SELF));
+                env.close();
+                // Preflight rejects the transaction, so alice keeps her fee.
+                BEAST_EXPECT(env.balance(alice, XRP) == aliceXrpBalance);
+            }
+            else
+            {
+                env(deposit::unauth(alice, alice), Ter(tecNO_ENTRY));
+                env.close();
+                // Reaching preclaim costs alice the transaction fee.
+                BEAST_EXPECT(env.balance(alice, XRP) == aliceXrpBalance - drops(baseFee));
+            }
+        }
+
         // alice authorizes an unfunded account.
         env(deposit::auth(alice, carol), Ter(tecNO_TARGET));
         env.close();
@@ -1476,6 +1499,110 @@ struct DepositPreauth_test : public beast::unit_test::Suite
         }
     }
 
+    void
+    testUnauthCredentialsDuplicates(FeatureBitset features)
+    {
+        testcase("Unauthorize credentials duplicates");
+
+        using namespace jtx;
+
+        Account const stock{"stock"};
+        Account const issuer{"issuer"};
+
+        Env env(*this, features);
+        env.fund(XRP(5000), stock, issuer);
+        env.close();
+
+        // A duplicated issuer and credential type pair is rejected by preflight,
+        // so it never reaches the keylet built from makeSorted.  This asserts
+        // the exact code rather than merely that the transaction failed.
+        std::vector<deposit::AuthorizeCredentials> const duplicated = {
+            {.issuer = issuer, .credType = "a"}, {.issuer = issuer, .credType = "a"}};
+
+        env(deposit::unauthCredentials(stock, duplicated), Ter(temMALFORMED));
+        env.close();
+
+        // A well-formed unauthorize for a grant that was never created still
+        // reports the missing entry, confirming the new emptiness check did not
+        // swallow the normal path.
+        std::vector<deposit::AuthorizeCredentials> const single = {
+            {.issuer = issuer, .credType = "b"}};
+
+        env(deposit::unauthCredentials(stock, single), Ter(tecNO_ENTRY));
+        env.close();
+    }
+
+    void
+    testInvariants(FeatureBitset features)
+    {
+        testcase("Invariants");
+
+        using namespace jtx;
+
+        Account const alice{"alice"};
+        Account const becky{"becky"};
+        Account const sponsor{"sponsor"};
+        Account const issuer{"issuer"};
+
+        Env env(*this, features);
+        env.fund(XRP(5000), alice, becky, sponsor, issuer);
+        env.close();
+
+        // Normal create and remove must satisfy the invariants, so a passing
+        // result here is what proves the hooks do not misfire.
+        env.require(Owners(alice, 0));
+        env(deposit::auth(alice, becky));
+        env.close();
+        env.require(Owners(alice, 1));
+
+        env(deposit::unauth(alice, becky));
+        env.close();
+        env.require(Owners(alice, 0));
+
+        // A credential grant writes a canonically sorted array; the invariant
+        // checks that ordering, so creating one exercises it.
+        std::vector<deposit::AuthorizeCredentials> const creds = {
+            {.issuer = issuer, .credType = "b"}, {.issuer = issuer, .credType = "a"}};
+
+        env(deposit::authCredentials(alice, creds));
+        env.close();
+        env.require(Owners(alice, 1));
+
+        env(deposit::unauthCredentials(alice, creds));
+        env.close();
+        env.require(Owners(alice, 0));
+
+        // A reserve-sponsored grant puts the owner-count delta on the sponsor
+        // instead of the grant owner, so the invariants assert nothing about
+        // owner counts.
+        if (features[featureSponsor])
+        {
+            BEAST_EXPECT(ownerCount(env, alice) == 0);
+            BEAST_EXPECT(sponsoredOwnerCount(env, alice) == 0);
+            BEAST_EXPECT(ownerCount(env, sponsor) == 0);
+            BEAST_EXPECT(sponsoringOwnerCount(env, sponsor) == 0);
+
+            env(deposit::auth(alice, becky),
+                sponsor::As(sponsor, spfSponsorReserve),
+                Sig(sfSponsorSignature, sponsor));
+            env.close();
+
+            // Alice owns the grant, but the sponsor carries its reserve.
+            BEAST_EXPECT(ownerCount(env, alice) == 1);
+            BEAST_EXPECT(sponsoredOwnerCount(env, alice) == 1);
+            BEAST_EXPECT(ownerCount(env, sponsor) == 0);
+            BEAST_EXPECT(sponsoringOwnerCount(env, sponsor) == 1);
+
+            // Removal finds the sponsor on the object and undoes both sides.
+            env(deposit::unauth(alice, becky));
+            env.close();
+            BEAST_EXPECT(ownerCount(env, alice) == 0);
+            BEAST_EXPECT(sponsoredOwnerCount(env, alice) == 0);
+            BEAST_EXPECT(ownerCount(env, sponsor) == 0);
+            BEAST_EXPECT(sponsoringOwnerCount(env, sponsor) == 0);
+        }
+    }
+
     void
     run() override
     {
@@ -1483,6 +1610,7 @@ struct DepositPreauth_test : public beast::unit_test::Suite
         auto const supported{jtx::testableAmendments()};
         testInvalid(supported);
         testInvalid(supported - fixCleanup3_3_0);
+        testInvalid(supported - fixDepositPreauthSelf);
         testPayment(supported - featureCredentials);
         testPayment(supported);
         testCredentialsPayment();
@@ -1490,6 +1618,10 @@ struct DepositPreauth_test : public beast::unit_test::Suite
         testCredentialsCreation();
         testExpiredCreds();
         testSortingCredentials();
+        testUnauthCredentialsDuplicates(supported);
+        testUnauthCredentialsDuplicates(supported - fixDepositPreauthSelf);
+        testInvariants(supported);
+        testInvariants(supported - fixDepositPreauthSelf);
     }
 };