Allow NFT to be burned when number of offers is greater than 500 (#4346)

* Allow offers to be removable
* Delete sell offers first

Signed-off-by: Shawn Xie <shawnxie920@gmail.com>

src/ripple/app/tx/impl/NFTokenBurn.cpp

@@ -77,9 +77,14 @@ NFTokenBurn::preclaim(PreclaimContext const& ctx)
         }
     }
 
-    // If there are too many offers, then burning the token would produce too
-    // much metadata.  Disallow burning a token with too many offers.
-    return nft::notTooManyOffers(ctx.view, ctx.tx[sfNFTokenID]);
+    if (!ctx.view.rules().enabled(fixUnburnableNFToken))
+    {
+        // If there are too many offers, then burning the token would produce
+        // too much metadata.  Disallow burning a token with too many offers.
+        return nft::notTooManyOffers(ctx.view, ctx.tx[sfNFTokenID]);
+    }
+
+    return tesSUCCESS;
 }
 
 TER
@@ -104,9 +109,38 @@ NFTokenBurn::doApply()
         view().update(issuer);
     }
 
-    // Optimized deletion of all offers.
-    nft::removeAllTokenOffers(view(), keylet::nft_sells(ctx_.tx[sfNFTokenID]));
-    nft::removeAllTokenOffers(view(), keylet::nft_buys(ctx_.tx[sfNFTokenID]));
+    if (ctx_.view().rules().enabled(fixUnburnableNFToken))
+    {
+        // Delete up to 500 offers in total.
+        // Because the number of sell offers is likely to be less than
+        // the number of buy offers, we prioritize the deletion of sell
+        // offers in order to clean up sell offer directory
+        std::size_t const deletedSellOffers = nft::removeTokenOffersWithLimit(
+            view(),
+            keylet::nft_sells(ctx_.tx[sfNFTokenID]),
+            maxDeletableTokenOfferEntries);
+
+        if (maxDeletableTokenOfferEntries > deletedSellOffers)
+        {
+            nft::removeTokenOffersWithLimit(
+                view(),
+                keylet::nft_buys(ctx_.tx[sfNFTokenID]),
+                maxDeletableTokenOfferEntries - deletedSellOffers);
+        }
+    }
+    else
+    {
+        // Deletion of all offers.
+        nft::removeTokenOffersWithLimit(
+            view(),
+            keylet::nft_sells(ctx_.tx[sfNFTokenID]),
+            std::numeric_limits<int>::max());
+
+        nft::removeTokenOffersWithLimit(
+            view(),
+            keylet::nft_buys(ctx_.tx[sfNFTokenID]),
+            std::numeric_limits<int>::max());
+    }
 
     return tesSUCCESS;
 }

src/ripple/app/tx/impl/details/NFTokenUtils.cpp

@@ -520,34 +520,55 @@ findTokenAndPage(
     }
     return std::nullopt;
 }
-void
-removeAllTokenOffers(ApplyView& view, Keylet const& directory)
+
+std::size_t
+removeTokenOffersWithLimit(
+    ApplyView& view,
+    Keylet const& directory,
+    std::size_t maxDeletableOffers)
 {
-    view.dirDelete(directory, [&view](uint256 const& id) {
-        auto offer = view.peek(Keylet{ltNFTOKEN_OFFER, id});
+    if (maxDeletableOffers == 0)
+        return 0;
 
-        if (!offer)
-            Throw<std::runtime_error>(
-                "Offer " + to_string(id) + " not found in ledger!");
+    std::optional<std::uint64_t> pageIndex{0};
+    std::size_t deletedOffersCount = 0;
 
-        auto const owner = (*offer)[sfOwner];
+    do
+    {
+        auto const page = view.peek(keylet::page(directory, *pageIndex));
+        if (!page)
+            break;
 
-        if (!view.dirRemove(
-                keylet::ownerDir(owner),
-                (*offer)[sfOwnerNode],
-                offer->key(),
-                false))
-            Throw<std::runtime_error>(
-                "Offer " + to_string(id) + " not found in owner directory!");
+        // We get the index of the next page in case the current
+        // page is deleted after all of its entries have been removed
+        pageIndex = (*page)[~sfIndexNext];
 
-        adjustOwnerCount(
-            view,
-            view.peek(keylet::account(owner)),
-            -1,
-            beast::Journal{beast::Journal::getNullSink()});
+        auto offerIndexes = page->getFieldV256(sfIndexes);
 
-        view.erase(offer);
-    });
+        // We reverse-iterate the offer directory page to delete all entries.
+        // Deleting an entry in a NFTokenOffer directory page won't cause
+        // entries from other pages to move to the current, so, it is safe to
+        // delete entries one by one in the page. It is required to iterate
+        // backwards to handle iterator invalidation for vector, as we are
+        // deleting during iteration.
+        for (int i = offerIndexes.size() - 1; i >= 0; --i)
+        {
+            if (auto const offer = view.peek(keylet::nftoffer(offerIndexes[i])))
+            {
+                if (deleteTokenOffer(view, offer))
+                    ++deletedOffersCount;
+                else
+                    Throw<std::runtime_error>(
+                        "Offer " + to_string(offerIndexes[i]) +
+                        " cannot be deleted!");
+            }
+
+            if (maxDeletableOffers == deletedOffersCount)
+                break;
+        }
+    } while (pageIndex.value_or(0) && maxDeletableOffers != deletedOffersCount);
+
+    return deletedOffersCount;
 }
 
 TER

src/ripple/app/tx/impl/details/NFTokenUtils.h

@@ -53,9 +53,13 @@ constexpr std::uint16_t const flagOnlyXRP = 0x0002;
 constexpr std::uint16_t const flagCreateTrustLines = 0x0004;
 constexpr std::uint16_t const flagTransferable = 0x0008;
 
-/** Deletes all offers from the specified token offer directory. */
-void
-removeAllTokenOffers(ApplyView& view, Keylet const& directory);
+/** Delete up to a specified number of offers from the specified token offer
+ * directory. */
+std::size_t
+removeTokenOffersWithLimit(
+    ApplyView& view,
+    Keylet const& directory,
+    std::size_t maxDeletableOffers);
 
 /** Returns tesSUCCESS if NFToken has few enough offers that it can be burned */
 TER

src/test/app/NFTokenBurn_test.cpp

@@ -49,6 +49,37 @@ class NFTokenBurn_test : public beast::unit_test::suite
         return nfts[jss::result][jss::account_nfts].size();
     };
 
+    // Helper function that returns new nft id for an account and create
+    // specified number of sell offers
+    uint256
+    createNftAndOffers(
+        test::jtx::Env& env,
+        test::jtx::Account const& owner,
+        std::vector<uint256>& offerIndexes,
+        size_t const tokenCancelCount)
+    {
+        using namespace test::jtx;
+        uint256 const nftokenID =
+            token::getNextID(env, owner, 0, tfTransferable);
+        env(token::mint(owner, 0),
+            token::uri(std::string(maxTokenURILength, 'u')),
+            txflags(tfTransferable));
+        env.close();
+
+        offerIndexes.reserve(tokenCancelCount);
+
+        for (uint32_t i = 0; i < tokenCancelCount; ++i)
+        {
+            // Create sell offer
+            offerIndexes.push_back(keylet::nftoffer(owner, env.seq(owner)).key);
+            env(token::createOffer(owner, nftokenID, drops(1)),
+                txflags(tfSellNFToken));
+            env.close();
+        }
+
+        return nftokenID;
+    };
+
     void
     testBurnRandom(FeatureBitset features)
     {
@@ -492,94 +523,251 @@ class NFTokenBurn_test : public beast::unit_test::suite
 
         using namespace test::jtx;
 
-        Env env{*this, features};
-
-        Account const alice("alice");
-        Account const becky("becky");
-        env.fund(XRP(1000), alice, becky);
-        env.close();
-
-        // We structure the test to try and maximize the metadata produced.
-        // This verifies that we don't create too much metadata during a
-        // maximal burn operation.
-        //
-        // 1. alice mints an nft with a full-sized URI.
-        // 2. We create 1000 new accounts, each of which creates an offer for
-        //    alice's nft.
-        // 3. becky creates one more offer for alice's NFT
-        // 4. Attempt to burn the nft which fails because there are too
-        //    many offers.
-        // 5. Cancel becky's offer and the nft should become burnable.
-        uint256 const nftokenID =
-            token::getNextID(env, alice, 0, tfTransferable);
-        env(token::mint(alice, 0),
-            token::uri(std::string(maxTokenURILength, 'u')),
-            txflags(tfTransferable));
-        env.close();
-
-        std::vector<uint256> offerIndexes;
-        offerIndexes.reserve(maxTokenOfferCancelCount);
-        for (uint32_t i = 0; i < maxTokenOfferCancelCount; ++i)
+        // Test what happens if a NFT is unburnable when there are
+        // more than 500 offers, before fixUnburnableNFToken goes live
+        if (!features[fixUnburnableNFToken])
         {
-            Account const acct(std::string("acct") + std::to_string(i));
-            env.fund(XRP(1000), acct);
+            Env env{*this, features};
+
+            Account const alice("alice");
+            Account const becky("becky");
+            env.fund(XRP(1000), alice, becky);
             env.close();
 
-            offerIndexes.push_back(keylet::nftoffer(acct, env.seq(acct)).key);
-            env(token::createOffer(acct, nftokenID, drops(1)),
+            // We structure the test to try and maximize the metadata produced.
+            // This verifies that we don't create too much metadata during a
+            // maximal burn operation.
+            //
+            // 1. alice mints an nft with a full-sized URI.
+            // 2. We create 500 new accounts, each of which creates an offer
+            //    for alice's nft.
+            // 3. becky creates one more offer for alice's NFT
+            // 4. Attempt to burn the nft which fails because there are too
+            //    many offers.
+            // 5. Cancel becky's offer and the nft should become burnable.
+            uint256 const nftokenID =
+                token::getNextID(env, alice, 0, tfTransferable);
+            env(token::mint(alice, 0),
+                token::uri(std::string(maxTokenURILength, 'u')),
+                txflags(tfTransferable));
+            env.close();
+
+            std::vector<uint256> offerIndexes;
+            offerIndexes.reserve(maxTokenOfferCancelCount);
+            for (std::uint32_t i = 0; i < maxTokenOfferCancelCount; ++i)
+            {
+                Account const acct(std::string("acct") + std::to_string(i));
+                env.fund(XRP(1000), acct);
+                env.close();
+
+                offerIndexes.push_back(
+                    keylet::nftoffer(acct, env.seq(acct)).key);
+                env(token::createOffer(acct, nftokenID, drops(1)),
+                    token::owner(alice));
+                env.close();
+            }
+
+            // Verify all offers are present in the ledger.
+            for (uint256 const& offerIndex : offerIndexes)
+            {
+                BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex)));
+            }
+
+            // Create one too many offers.
+            uint256 const beckyOfferIndex =
+                keylet::nftoffer(becky, env.seq(becky)).key;
+            env(token::createOffer(becky, nftokenID, drops(1)),
+                token::owner(alice));
+
+            // Attempt to burn the nft which should fail.
+            env(token::burn(alice, nftokenID), ter(tefTOO_BIG));
+
+            // Close enough ledgers that the burn transaction is no longer
+            // retried.
+            for (int i = 0; i < 10; ++i)
+                env.close();
+
+            // Cancel becky's offer, but alice adds a sell offer.  The token
+            // should still not be burnable.
+            env(token::cancelOffer(becky, {beckyOfferIndex}));
+            env.close();
+
+            uint256 const aliceOfferIndex =
+                keylet::nftoffer(alice, env.seq(alice)).key;
+            env(token::createOffer(alice, nftokenID, drops(1)),
+                txflags(tfSellNFToken));
+            env.close();
+
+            env(token::burn(alice, nftokenID), ter(tefTOO_BIG));
+            env.close();
+
+            // Cancel alice's sell offer.  Now the token should be burnable.
+            env(token::cancelOffer(alice, {aliceOfferIndex}));
+            env.close();
+
+            env(token::burn(alice, nftokenID));
+            env.close();
+
+            // Burning the token should remove all the offers from the ledger.
+            for (uint256 const& offerIndex : offerIndexes)
+            {
+                BEAST_EXPECT(!env.le(keylet::nftoffer(offerIndex)));
+            }
+
+            // Both alice and becky should have ownerCounts of zero.
+            BEAST_EXPECT(ownerCount(env, alice) == 0);
+            BEAST_EXPECT(ownerCount(env, becky) == 0);
+        }
+
+        // Test that up to 499 buy/sell offers will be removed when NFT is
+        // burned after fixUnburnableNFToken is enabled. This is to test that we
+        // can successfully remove all offers if the number of offers is less
+        // than 500.
+        if (features[fixUnburnableNFToken])
+        {
+            Env env{*this, features};
+
+            Account const alice("alice");
+            Account const becky("becky");
+            env.fund(XRP(100000), alice, becky);
+            env.close();
+
+            // alice creates 498 sell offers and becky creates 1 buy offers.
+            // When the token is burned, 498 sell offers and 1 buy offer are
+            // removed. In total, 499 offers are removed
+            std::vector<uint256> offerIndexes;
+            auto const nftokenID = createNftAndOffers(
+                env, alice, offerIndexes, maxDeletableTokenOfferEntries - 2);
+
+            // Verify all sell offers are present in the ledger.
+            for (uint256 const& offerIndex : offerIndexes)
+            {
+                BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex)));
+            }
+
+            // Becky creates a buy offer
+            uint256 const beckyOfferIndex =
+                keylet::nftoffer(becky, env.seq(becky)).key;
+            env(token::createOffer(becky, nftokenID, drops(1)),
                 token::owner(alice));
             env.close();
-        }
 
-        // Verify all offers are present in the ledger.
-        for (uint256 const& offerIndex : offerIndexes)
-        {
-            BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex)));
-        }
-
-        // Create one too many offers.
-        uint256 const beckyOfferIndex =
-            keylet::nftoffer(becky, env.seq(becky)).key;
-        env(token::createOffer(becky, nftokenID, drops(1)),
-            token::owner(alice));
-
-        // Attempt to burn the nft which should fail.
-        env(token::burn(alice, nftokenID), ter(tefTOO_BIG));
-
-        // Close enough ledgers that the burn transaction is no longer retried.
-        for (int i = 0; i < 10; ++i)
+            // Burn the token
+            env(token::burn(alice, nftokenID));
             env.close();
 
-        // Cancel becky's offer, but alice adds a sell offer.  The token
-        // should still not be burnable.
-        env(token::cancelOffer(becky, {beckyOfferIndex}));
-        env.close();
+            // Burning the token should remove all 498 sell offers
+            // that alice created
+            for (uint256 const& offerIndex : offerIndexes)
+            {
+                BEAST_EXPECT(!env.le(keylet::nftoffer(offerIndex)));
+            }
 
-        uint256 const aliceOfferIndex =
-            keylet::nftoffer(alice, env.seq(alice)).key;
-        env(token::createOffer(alice, nftokenID, drops(1)),
-            txflags(tfSellNFToken));
-        env.close();
+            // Burning the token should also remove the one buy offer
+            // that becky created
+            BEAST_EXPECT(!env.le(keylet::nftoffer(beckyOfferIndex)));
 
-        env(token::burn(alice, nftokenID), ter(tefTOO_BIG));
-        env.close();
-
-        // Cancel alice's sell offer.  Now the token should be burnable.
-        env(token::cancelOffer(alice, {aliceOfferIndex}));
-        env.close();
-
-        env(token::burn(alice, nftokenID));
-        env.close();
-
-        // Burning the token should remove all the offers from the ledger.
-        for (uint256 const& offerIndex : offerIndexes)
-        {
-            BEAST_EXPECT(!env.le(keylet::nftoffer(offerIndex)));
+            // alice and becky should have ownerCounts of zero
+            BEAST_EXPECT(ownerCount(env, alice) == 0);
+            BEAST_EXPECT(ownerCount(env, becky) == 0);
         }
 
-        // Both alice and becky should have ownerCounts of zero.
-        BEAST_EXPECT(ownerCount(env, alice) == 0);
-        BEAST_EXPECT(ownerCount(env, becky) == 0);
+        // Test that up to 500 buy offers are removed when NFT is burned
+        // after fixUnburnableNFToken is enabled
+        if (features[fixUnburnableNFToken])
+        {
+            Env env{*this, features};
+
+            Account const alice("alice");
+            Account const becky("becky");
+            env.fund(XRP(100000), alice, becky);
+            env.close();
+
+            // alice creates 501 sell offers for the token
+            // After we burn the token, 500 of the sell offers should be
+            // removed, and one is left over
+            std::vector<uint256> offerIndexes;
+            auto const nftokenID = createNftAndOffers(
+                env, alice, offerIndexes, maxDeletableTokenOfferEntries + 1);
+
+            // Verify all sell offers are present in the ledger.
+            for (uint256 const& offerIndex : offerIndexes)
+            {
+                BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex)));
+            }
+
+            // Burn the token
+            env(token::burn(alice, nftokenID));
+            env.close();
+
+            uint32_t offerDeletedCount = 0;
+            // Count the number of sell offers that have been deleted
+            for (uint256 const& offerIndex : offerIndexes)
+            {
+                if (!env.le(keylet::nftoffer(offerIndex)))
+                    offerDeletedCount++;
+            }
+
+            BEAST_EXPECT(offerIndexes.size() == maxTokenOfferCancelCount + 1);
+
+            // 500 sell offers should be removed
+            BEAST_EXPECT(offerDeletedCount == maxTokenOfferCancelCount);
+
+            // alice should have ownerCounts of one for the orphaned sell offer
+            BEAST_EXPECT(ownerCount(env, alice) == 1);
+        }
+
+        // Test that up to 500 buy/sell offers are removed when NFT is burned
+        // after fixUnburnableNFToken is enabled
+        if (features[fixUnburnableNFToken])
+        {
+            Env env{*this, features};
+
+            Account const alice("alice");
+            Account const becky("becky");
+            env.fund(XRP(100000), alice, becky);
+            env.close();
+
+            // alice creates 499 sell offers and becky creates 2 buy offers.
+            // When the token is burned, 499 sell offers and 1 buy offer
+            // are removed.
+            // In total, 500 offers are removed
+            std::vector<uint256> offerIndexes;
+            auto const nftokenID = createNftAndOffers(
+                env, alice, offerIndexes, maxDeletableTokenOfferEntries - 1);
+
+            // Verify all sell offers are present in the ledger.
+            for (uint256 const& offerIndex : offerIndexes)
+            {
+                BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex)));
+            }
+
+            // becky creates 2 buy offers
+            env(token::createOffer(becky, nftokenID, drops(1)),
+                token::owner(alice));
+            env.close();
+            env(token::createOffer(becky, nftokenID, drops(1)),
+                token::owner(alice));
+            env.close();
+
+            // Burn the token
+            env(token::burn(alice, nftokenID));
+            env.close();
+
+            // Burning the token should remove all 499 sell offers from the
+            // ledger.
+            for (uint256 const& offerIndex : offerIndexes)
+            {
+                BEAST_EXPECT(!env.le(keylet::nftoffer(offerIndex)));
+            }
+
+            // alice should have ownerCount of zero because all her
+            // sell offers have been deleted
+            BEAST_EXPECT(ownerCount(env, alice) == 0);
+
+            // becky has ownerCount of one due to an orphaned buy offer
+            BEAST_EXPECT(ownerCount(env, becky) == 1);
+        }
     }
 
     void
@@ -598,7 +786,8 @@ public:
         FeatureBitset const all{supported_amendments()};
         FeatureBitset const fixNFTDir{fixNFTokenDirV1};
 
-        testWithFeats(all - fixNFTDir);
+        testWithFeats(all - fixUnburnableNFToken - fixNFTDir);
+        testWithFeats(all - fixUnburnableNFToken);
         testWithFeats(all);
     }
 };