fix: Store Delegate object in delegating and authorized account directories for proper deletion (#6681)

API-CHANGELOG.md

@@ -28,6 +28,8 @@ This section contains changes targeting a future version.
 
 ### Additions
 
+- `ledger_entry`, `account_objects`: The `Delegate` ledger entry now includes an optional `DestinationNode` field, which stores the index into the authorized account's owner directory. This field is present on entries created after bidirectional directory tracking was introduced and may appear in RPC responses for those entries. ([#6681](https://github.com/XRPLF/rippled/pull/6681))
+
 - `server_definitions`: Added the following new sections to the response ([#6321](https://github.com/XRPLF/rippled/pull/6321)):
   - `TRANSACTION_FORMATS`: Describes the fields and their optionality for each transaction type, including common fields shared across all transactions.
   - `LEDGER_ENTRY_FORMATS`: Describes the fields and their optionality for each ledger entry type, including common fields shared across all ledger entries.

include/xrpl/protocol/detail/ledger_entries.macro

@@ -466,6 +466,7 @@ LEDGER_ENTRY(ltDELEGATE, 0x0083, Delegate, delegate, ({
     {sfAuthorize,            soeREQUIRED},
     {sfPermissions,          soeREQUIRED},
     {sfOwnerNode,            soeREQUIRED},
+    {sfDestinationNode,      soeOPTIONAL},
     {sfPreviousTxnID,        soeREQUIRED},
     {sfPreviousTxnLgrSeq,    soeREQUIRED},
 }))

include/xrpl/protocol_autogen/ledger_entries/Delegate.h

@@ -90,6 +90,30 @@ public:
         return this->sle_->at(sfOwnerNode);
     }
 
+    /**
+     * @brief Get sfDestinationNode (soeOPTIONAL)
+     * @return The field value, or std::nullopt if not present.
+     */
+    [[nodiscard]]
+    protocol_autogen::Optional<SF_UINT64::type::value_type>
+    getDestinationNode() const
+    {
+        if (hasDestinationNode())
+            return this->sle_->at(sfDestinationNode);
+        return std::nullopt;
+    }
+
+    /**
+     * @brief Check if sfDestinationNode is present.
+     * @return True if the field is present, false otherwise.
+     */
+    [[nodiscard]]
+    bool
+    hasDestinationNode() const
+    {
+        return this->sle_->isFieldPresent(sfDestinationNode);
+    }
+
     /**
      * @brief Get sfPreviousTxnID (soeREQUIRED)
      * @return The field value.
@@ -203,6 +227,17 @@ public:
         return *this;
     }
 
+    /**
+     * @brief Set sfDestinationNode (soeOPTIONAL)
+     * @return Reference to this builder for method chaining.
+     */
+    DelegateBuilder&
+    setDestinationNode(std::decay_t<typename SF_UINT64::type::value_type> const& value)
+    {
+        object_[sfDestinationNode] = value;
+        return *this;
+    }
+
     /**
      * @brief Set sfPreviousTxnID (soeREQUIRED)
      * @return Reference to this builder for method chaining.

include/xrpl/tx/transactors/delegate/DelegateSet.h

@@ -38,11 +38,7 @@ public:
 
     // Interface used by AccountDelete
     static TER
-    deleteDelegate(
-        ApplyView& view,
-        std::shared_ptr<SLE> const& sle,
-        AccountID const& account,
-        beast::Journal j);
+    deleteDelegate(ApplyView& view, std::shared_ptr<SLE> const& sle, beast::Journal j);
 };
 
 }  // namespace xrpl

src/libxrpl/tx/transactors/account/AccountDelete.cpp

@@ -179,12 +179,12 @@ TER
 removeDelegateFromLedger(
     ServiceRegistry&,
     ApplyView& view,
-    AccountID const& account,
-    uint256 const& delIndex,
+    AccountID const&,
+    uint256 const&,
     std::shared_ptr<SLE> const& sleDel,
     beast::Journal j)
 {
-    return DelegateSet::deleteDelegate(view, sleDel, account, j);
+    return DelegateSet::deleteDelegate(view, sleDel, j);
 }
 
 // Return nullptr if the LedgerEntryType represents an obligation that can't

src/libxrpl/tx/transactors/delegate/DelegateSet.cpp

@@ -5,7 +5,6 @@
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
-#include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
@@ -83,7 +82,7 @@ DelegateSet::doApply()
         if (permissions.empty())
         {
             // if permissions array is empty, delete the ledger object.
-            return deleteDelegate(view(), sle, account_, j_);
+            return deleteDelegate(view(), sle, j_);
         }
 
         sle->setFieldArray(sfPermissions, permissions);
@@ -106,6 +105,8 @@ DelegateSet::doApply()
     sle->setAccountID(sfAuthorize, authAccount);
 
     sle->setFieldArray(sfPermissions, permissions);
+
+    // Add to delegating account's owner directory
     auto const page =
         ctx_.view().dirInsert(keylet::ownerDir(account_), delegateKey, describeOwnerDir(account_));
 
@@ -113,6 +114,17 @@ DelegateSet::doApply()
         return tecDIR_FULL;  // LCOV_EXCL_LINE
 
     (*sle)[sfOwnerNode] = *page;
+
+    // Add to authorized account's owner directory so the object can be found
+    // and cleaned up when the authorized account is deleted.
+    auto const destPage = ctx_.view().dirInsert(
+        keylet::ownerDir(authAccount), delegateKey, describeOwnerDir(authAccount));
+
+    if (!destPage)
+        return tecDIR_FULL;  // LCOV_EXCL_LINE
+
+    (*sle)[sfDestinationNode] = *destPage;
+
     ctx_.view().insert(sle);
     adjustOwnerCount(ctx_.view(), sleOwner, 1, ctx_.journal);
 
@@ -120,16 +132,16 @@ DelegateSet::doApply()
 }
 
 TER
-DelegateSet::deleteDelegate(
-    ApplyView& view,
-    std::shared_ptr<SLE> const& sle,
-    AccountID const& account,
-    beast::Journal j)
+DelegateSet::deleteDelegate(ApplyView& view, std::shared_ptr<SLE> const& sle, beast::Journal j)
 {
     if (!sle)
         return tecINTERNAL;  // LCOV_EXCL_LINE
 
-    if (!view.dirRemove(keylet::ownerDir(account), (*sle)[sfOwnerNode], sle->key(), false))
+    auto const delegator = (*sle)[sfAccount];
+    auto const delegatee = (*sle)[sfAuthorize];
+
+    // Remove from delegating account's owner directory
+    if (!view.dirRemove(keylet::ownerDir(delegator), (*sle)[sfOwnerNode], sle->key(), false))
     {
         // LCOV_EXCL_START
         JLOG(j.fatal()) << "Unable to delete Delegate from owner.";
@@ -137,7 +149,20 @@ DelegateSet::deleteDelegate(
         // LCOV_EXCL_STOP
     }
 
-    auto const sleOwner = view.peek(keylet::account(account));
+    // Remove from authorized account's owner directory, if present
+    if (auto const optPage = (*sle)[~sfDestinationNode])
+    {
+        if (!view.dirRemove(keylet::ownerDir(delegatee), *optPage, sle->key(), false))
+        {
+            // LCOV_EXCL_START
+            JLOG(j.fatal()) << "Unable to delete Delegate from authorized account.";
+            return tefBAD_LEDGER;
+            // LCOV_EXCL_STOP
+        }
+    }
+
+    // Only the delegating account's owner count was incremented on creation
+    auto const sleOwner = view.peek(keylet::account(delegator));
     if (!sleOwner)
         return tecINTERNAL;  // LCOV_EXCL_LINE
 

src/test/app/Delegate_test.cpp

@@ -28,6 +28,7 @@
 #include <xrpl/basics/strHex.h>
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/json/json_value.h>
+#include <xrpl/ledger/Dir.h>
 #include <xrpl/ledger/helpers/DelegateHelpers.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
@@ -41,6 +42,7 @@
 #include <xrpl/protocol/TxFlags.h>
 #include <xrpl/protocol/jss.h>
 
+#include <algorithm>
 #include <cstddef>
 #include <cstdint>
 #include <memory>
@@ -584,32 +586,179 @@ class Delegate_test : public beast::unit_test::suite
         testcase("test deleting account");
         using namespace jtx;
 
-        Env env(*this);
-        Account const alice{"alice"};
-        Account const bob{"bob"};
-        env.fund(XRP(100000), alice, bob);
-        env.close();
-
-        env(delegate::set(alice, bob, {"Payment"}));
-        env.close();
-        BEAST_EXPECT(env.closed()->exists(keylet::delegate(alice.id(), bob.id())));
-
-        for (std::uint32_t i = 0; i < 256; ++i)
+        // Delegator (alice) deletes account: Delegate object is cleaned up from
+        // both alice's and bob's owner directories.
+        {
+            Env env(*this);
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+            Account const carol{"carol"};
+            env.fund(XRP(100000), alice, bob, carol);
             env.close();
 
-        auto const aliceBalance = env.balance(alice);
-        auto const bobBalance = env.balance(bob);
+            env(delegate::set(alice, bob, {"Payment"}));
+            env.close();
 
-        // alice deletes account, this will remove the Delegate object
-        auto const deleteFee = drops(env.current()->fees().increment);
-        env(acctdelete(alice, bob), fee(deleteFee));
-        env.close();
+            auto const delegateKey = keylet::delegate(alice.id(), bob.id());
+            BEAST_EXPECT(env.closed()->exists(delegateKey));
 
-        BEAST_EXPECT(!env.closed()->exists(keylet::account(alice.id())));
-        BEAST_EXPECT(!env.closed()->exists(keylet::ownerDir(alice.id())));
-        BEAST_EXPECT(env.balance(bob) == bobBalance + aliceBalance - deleteFee);
+            auto hasKey = [](xrpl::Dir const& dir, uint256 const& key) {
+                return std::any_of(  // NOLINT(modernize-use-ranges)
+                    dir.begin(), dir.end(), [&](auto const& sle) { return sle->key() == key; });
+            };
 
-        BEAST_EXPECT(!env.closed()->exists(keylet::delegate(alice.id(), bob.id())));
+            // Delegate object should appear in both alice's and bob's directories
+            BEAST_EXPECT(
+                hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(alice.id())), delegateKey.key));
+            BEAST_EXPECT(
+                hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(bob.id())), delegateKey.key));
+
+            for (std::uint32_t i = 0; i < 256; ++i)
+                env.close();
+
+            auto const aliceBalance = env.balance(alice);
+            auto const carolBalance = env.balance(carol);
+
+            // alice deletes account, this will remove the Delegate object from
+            // both alice's and bob's owner directories
+            auto const deleteFee = drops(env.current()->fees().increment);
+            env(acctdelete(alice, carol), fee(deleteFee));
+            env.close();
+
+            BEAST_EXPECT(!env.closed()->exists(keylet::account(alice.id())));
+            BEAST_EXPECT(!env.closed()->exists(keylet::ownerDir(alice.id())));
+            BEAST_EXPECT(!env.closed()->exists(delegateKey));
+            // bob's directory should no longer reference the Delegate object
+            BEAST_EXPECT(
+                !hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(bob.id())), delegateKey.key));
+            BEAST_EXPECT(env.balance(carol) == carolBalance + aliceBalance - deleteFee);
+        }
+
+        // Delegatee (bob) deletes account: Delegate object is cleaned up from
+        // both alice's and bob's owner directories, freeing alice's reserve so
+        // she can subsequently delete her own account.
+        {
+            Env env(*this);
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+            Account const carol{"carol"};
+            env.fund(XRP(100000), alice, bob, carol);
+            env.close();
+
+            env(delegate::set(alice, bob, {"Payment"}));
+            env.close();
+
+            auto const delegateKey = keylet::delegate(alice.id(), bob.id());
+            BEAST_EXPECT(env.closed()->exists(delegateKey));
+
+            auto hasKey = [](xrpl::Dir const& dir, uint256 const& key) {
+                return std::any_of(  // NOLINT(modernize-use-ranges)
+                    dir.begin(), dir.end(), [&](auto const& sle) { return sle->key() == key; });
+            };
+
+            BEAST_EXPECT(
+                hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(alice.id())), delegateKey.key));
+            BEAST_EXPECT(
+                hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(bob.id())), delegateKey.key));
+
+            // The Delegate entry counts against alice's ownerCount.
+            auto const sleAlice = env.closed()->read(keylet::account(alice.id()));
+            BEAST_EXPECT(sleAlice);
+            BEAST_EXPECT(sleAlice->getFieldU32(sfOwnerCount) == 1);
+
+            for (std::uint32_t i = 0; i < 256; ++i)
+                env.close();
+
+            auto const bobBalance = env.balance(bob);
+            auto const carolBalance = env.balance(carol);
+
+            // bob (the authorized/delegatee account) deletes his account.
+            // This must clean up the Delegate object from both alice's and
+            // bob's owner directories so alice's delegation does not survive
+            // a potential account resurrection.
+            auto const deleteFee = drops(env.current()->fees().increment);
+            env(acctdelete(bob, carol), fee(deleteFee));
+            env.close();
+
+            BEAST_EXPECT(!env.closed()->exists(keylet::account(bob.id())));
+            BEAST_EXPECT(!env.closed()->exists(keylet::ownerDir(bob.id())));
+            BEAST_EXPECT(!env.closed()->exists(delegateKey));
+            // alice's directory should no longer reference the Delegate object
+            BEAST_EXPECT(
+                !hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(alice.id())), delegateKey.key));
+            BEAST_EXPECT(env.balance(carol) == carolBalance + bobBalance - deleteFee);
+
+            // alice's ownerCount is now 0; she can delete her own account.
+            auto const sleAlice2 = env.closed()->read(keylet::account(alice.id()));
+            BEAST_EXPECT(sleAlice2);
+            BEAST_EXPECT(sleAlice2->getFieldU32(sfOwnerCount) == 0);
+
+            auto const aliceDeleteFee = drops(env.current()->fees().increment);
+            env(acctdelete(alice, carol), fee(aliceDeleteFee));
+            env.close();
+
+            BEAST_EXPECT(!env.closed()->exists(keylet::account(alice.id())));
+        }
+
+        // Multiple delegators -> same delegatee: when the delegatee (bob)
+        // deletes his account, ALL Delegate objects (from alice and carol)
+        // must be cleaned up from every delegator's directory.
+        {
+            Env env(*this);
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+            Account const carol{"carol"};
+            Account const dave{"dave"};
+            env.fund(XRP(100000), alice, bob, carol, dave);
+            env.close();
+
+            // Both alice and carol delegate to bob
+            env(delegate::set(alice, bob, {"Payment"}));
+            env(delegate::set(carol, bob, {"EscrowCreate"}));
+            env.close();
+
+            auto const aliceBobKey = keylet::delegate(alice.id(), bob.id());
+            auto const carolBobKey = keylet::delegate(carol.id(), bob.id());
+
+            auto hasKey = [](xrpl::Dir const& dir, uint256 const& key) {
+                return std::any_of(  // NOLINT(modernize-use-ranges)
+                    dir.begin(), dir.end(), [&](auto const& sle) { return sle->key() == key; });
+            };
+
+            // Both Delegate objects exist and are in bob's directory
+            BEAST_EXPECT(env.closed()->exists(aliceBobKey));
+            BEAST_EXPECT(env.closed()->exists(carolBobKey));
+            BEAST_EXPECT(
+                hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(bob.id())), aliceBobKey.key));
+            BEAST_EXPECT(
+                hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(bob.id())), carolBobKey.key));
+
+            for (std::uint32_t i = 0; i < 256; ++i)
+                env.close();
+
+            auto const bobBalance = env.balance(bob);
+            auto const daveBalance = env.balance(dave);
+
+            auto const deleteFee = drops(env.current()->fees().increment);
+            env(acctdelete(bob, dave), fee(deleteFee));
+            env.close();
+
+            // bob's account and directory are gone
+            BEAST_EXPECT(!env.closed()->exists(keylet::account(bob.id())));
+            BEAST_EXPECT(!env.closed()->exists(keylet::ownerDir(bob.id())));
+
+            // Both Delegate objects are erased
+            BEAST_EXPECT(!env.closed()->exists(aliceBobKey));
+            BEAST_EXPECT(!env.closed()->exists(carolBobKey));
+
+            // alice's and carol's directories no longer reference the objects
+            BEAST_EXPECT(
+                !hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(alice.id())), aliceBobKey.key));
+            BEAST_EXPECT(
+                !hasKey(xrpl::Dir(*env.closed(), keylet::ownerDir(carol.id())), carolBobKey.key));
+
+            BEAST_EXPECT(env.balance(dave) == daveBalance + bobBalance - deleteFee);
+        }
     }
 
     void

src/tests/libxrpl/protocol_autogen/ledger_entries/DelegateTests.cpp

@@ -24,6 +24,7 @@ TEST(DelegateTests, BuilderSettersRoundTrip)
     auto const authorizeValue = canonical_ACCOUNT();
     auto const permissionsValue = canonical_ARRAY();
     auto const ownerNodeValue = canonical_UINT64();
+    auto const destinationNodeValue = canonical_UINT64();
     auto const previousTxnIDValue = canonical_UINT256();
     auto const previousTxnLgrSeqValue = canonical_UINT32();
 
@@ -36,6 +37,7 @@ TEST(DelegateTests, BuilderSettersRoundTrip)
         previousTxnLgrSeqValue
     };
 
+    builder.setDestinationNode(destinationNodeValue);
 
     builder.setLedgerIndex(index);
     builder.setFlags(0x1u);
@@ -82,6 +84,14 @@ TEST(DelegateTests, BuilderSettersRoundTrip)
         expectEqualField(expected, actual, "sfPreviousTxnLgrSeq");
     }
 
+    {
+        auto const& expected = destinationNodeValue;
+        auto const actualOpt = entry.getDestinationNode();
+        ASSERT_TRUE(actualOpt.has_value());
+        expectEqualField(expected, *actualOpt, "sfDestinationNode");
+        EXPECT_TRUE(entry.hasDestinationNode());
+    }
+
     EXPECT_TRUE(entry.hasLedgerIndex());
     auto const ledgerIndex = entry.getLedgerIndex();
     ASSERT_TRUE(ledgerIndex.has_value());
@@ -99,6 +109,7 @@ TEST(DelegateTests, BuilderFromSleRoundTrip)
     auto const authorizeValue = canonical_ACCOUNT();
     auto const permissionsValue = canonical_ARRAY();
     auto const ownerNodeValue = canonical_UINT64();
+    auto const destinationNodeValue = canonical_UINT64();
     auto const previousTxnIDValue = canonical_UINT256();
     auto const previousTxnLgrSeqValue = canonical_UINT32();
 
@@ -108,6 +119,7 @@ TEST(DelegateTests, BuilderFromSleRoundTrip)
     sle->at(sfAuthorize) = authorizeValue;
     sle->setFieldArray(sfPermissions, permissionsValue);
     sle->at(sfOwnerNode) = ownerNodeValue;
+    sle->at(sfDestinationNode) = destinationNodeValue;
     sle->at(sfPreviousTxnID) = previousTxnIDValue;
     sle->at(sfPreviousTxnLgrSeq) = previousTxnLgrSeqValue;
 
@@ -180,6 +192,19 @@ TEST(DelegateTests, BuilderFromSleRoundTrip)
         expectEqualField(expected, fromBuilder, "sfPreviousTxnLgrSeq");
     }
 
+    {
+        auto const& expected = destinationNodeValue;
+
+        auto const fromSleOpt = entryFromSle.getDestinationNode();
+        auto const fromBuilderOpt = entryFromBuilder.getDestinationNode();
+
+        ASSERT_TRUE(fromSleOpt.has_value());
+        ASSERT_TRUE(fromBuilderOpt.has_value());
+
+        expectEqualField(expected, *fromSleOpt, "sfDestinationNode");
+        expectEqualField(expected, *fromBuilderOpt, "sfDestinationNode");
+    }
+
     EXPECT_EQ(entryFromSle.getKey(), index);
     EXPECT_EQ(entryFromBuilder.getKey(), index);
 }
@@ -220,4 +245,31 @@ TEST(DelegateTests, BuilderThrowsOnWrongEntryType)
     EXPECT_THROW(DelegateBuilder{wrongEntry.getSle()}, std::runtime_error);
 }
 
+// 5) Build with only required fields and verify optional fields return nullopt.
+TEST(DelegateTests, OptionalFieldsReturnNullopt)
+{
+    uint256 const index{3u};
+
+    auto const accountValue = canonical_ACCOUNT();
+    auto const authorizeValue = canonical_ACCOUNT();
+    auto const permissionsValue = canonical_ARRAY();
+    auto const ownerNodeValue = canonical_UINT64();
+    auto const previousTxnIDValue = canonical_UINT256();
+    auto const previousTxnLgrSeqValue = canonical_UINT32();
+
+    DelegateBuilder builder{
+        accountValue,
+        authorizeValue,
+        permissionsValue,
+        ownerNodeValue,
+        previousTxnIDValue,
+        previousTxnLgrSeqValue
+    };
+
+    auto const entry = builder.build(index);
+
+    // Verify optional fields are not present
+    EXPECT_FALSE(entry.hasDestinationNode());
+    EXPECT_FALSE(entry.getDestinationNode().has_value());
+}
 }