Introduce fixNFTokenDirV1 amendment:

o Fixes an off-by-one when determining which NFTokenPage an
  NFToken belongs on.
o Improves handling of packed sets of 32 NFTs with
  identical low 96-bits.
o Fixes marker handling by the account_nfts RPC command.
o Tightens constraints of NFTokenPage invariant checks.

Adds unit tests to exercise the fixed cases as well as tests
for previously untested functionality.

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

@@ -18,6 +18,8 @@
 //==============================================================================
 
 #include <ripple/app/tx/impl/InvariantCheck.h>
+
+#include <ripple/app/tx/impl/details/NFTokenUtils.h>
 #include <ripple/basics/FeeUnits.h>
 #include <ripple/basics/Log.h>
 #include <ripple/ledger/ReadView.h>
@@ -493,23 +495,27 @@ ValidNewAccountRoot::finalize(
 
 void
 ValidNFTokenPage::visitEntry(
-    bool,
+    bool isDelete,
     std::shared_ptr<SLE const> const& before,
     std::shared_ptr<SLE const> const& after)
 {
     static constexpr uint256 const& pageBits = nft::pageMask;
     static constexpr uint256 const accountBits = ~pageBits;
 
-    auto check = [this](std::shared_ptr<SLE const> const& sle) {
-        auto const account = sle->key() & accountBits;
-        auto const limit = sle->key() & pageBits;
+    auto check = [this, isDelete](std::shared_ptr<SLE const> const& sle) {
+        uint256 const account = sle->key() & accountBits;
+        uint256 const hiLimit = sle->key() & pageBits;
+        std::optional<uint256> const prev = (*sle)[~sfPreviousPageMin];
 
-        if (auto const prev = (*sle)[~sfPreviousPageMin])
+        // Make sure that any page links...
+        //  1. Are properly associated with the owning account and
+        //  2. The page is correctly ordered between links.
+        if (prev)
         {
             if (account != (*prev & accountBits))
                 badLink_ = true;
 
-            if (limit <= (*prev & pageBits))
+            if (hiLimit <= (*prev & pageBits))
                 badLink_ = true;
         }
 
@@ -518,17 +524,42 @@ ValidNFTokenPage::visitEntry(
             if (account != (*next & accountBits))
                 badLink_ = true;
 
-            if (limit >= (*next & pageBits))
+            if (hiLimit >= (*next & pageBits))
                 badLink_ = true;
         }
 
-        for (auto const& obj : sle->getFieldArray(sfNFTokens))
         {
-            if ((obj[sfNFTokenID] & pageBits) >= limit)
-                badEntry_ = true;
-
-            if (auto uri = obj[~sfURI]; uri && uri->empty())
-                badURI_ = true;
+            auto const& nftokens = sle->getFieldArray(sfNFTokens);
+
+            // An NFTokenPage should never contain too many tokens or be empty.
+            if (std::size_t const nftokenCount = nftokens.size();
+                (!isDelete && nftokenCount == 0) ||
+                nftokenCount > dirMaxTokensPerPage)
+                invalidSize_ = true;
+
+            // If prev is valid, use it to establish a lower bound for
+            // page entries.  If prev is not valid the lower bound is zero.
+            uint256 const loLimit =
+                prev ? *prev & pageBits : uint256(beast::zero);
+
+            // Also verify that all NFTokenIDs in the page are sorted.
+            uint256 loCmp = loLimit;
+            for (auto const& obj : nftokens)
+            {
+                uint256 const tokenID = obj[sfNFTokenID];
+                if (!nft::compareTokens(loCmp, tokenID))
+                    badSort_ = true;
+                loCmp = tokenID;
+
+                // None of the NFTs on this page should belong on lower or
+                // higher pages.
+                if (uint256 const tokenPageBits = tokenID & pageBits;
+                    tokenPageBits < loLimit || tokenPageBits >= hiLimit)
+                    badEntry_ = true;
+
+                if (auto uri = obj[~sfURI]; uri && uri->empty())
+                    badURI_ = true;
+            }
         }
     };
 
@@ -559,12 +590,24 @@ ValidNFTokenPage::finalize(
         return false;
     }
 
+    if (badSort_)
+    {
+        JLOG(j.fatal()) << "Invariant failed: NFTs on page are not sorted.";
+        return false;
+    }
+
     if (badURI_)
     {
         JLOG(j.fatal()) << "Invariant failed: NFT contains empty URI.";
         return false;
     }
 
+    if (invalidSize_)
+    {
+        JLOG(j.fatal()) << "Invariant failed: NFT page has invalid size.";
+        return false;
+    }
+
     return true;
 }
 

src/ripple/app/tx/impl/InvariantCheck.h

@@ -320,9 +320,11 @@ class ValidNewAccountRoot
 
 class ValidNFTokenPage
 {
-    bool badLink_ = false;
     bool badEntry_ = false;
+    bool badLink_ = false;
+    bool badSort_ = false;
     bool badURI_ = false;
+    bool invalidSize_ = false;
 
 public:
     void

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

@@ -63,36 +63,33 @@ NFTokenAcceptOffer::preflight(PreflightContext const& ctx)
 TER
 NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
 {
-    auto const checkOffer = [&ctx](std::optional<uint256> id) -> TER {
+    auto const checkOffer = [&ctx](std::optional<uint256> id)
+        -> std::pair<std::shared_ptr<const SLE>, TER> {
         if (id)
         {
-            auto const offer = ctx.view.read(keylet::nftoffer(*id));
+            auto offerSLE = ctx.view.read(keylet::nftoffer(*id));
 
-            if (!offer)
-                return tecOBJECT_NOT_FOUND;
+            if (!offerSLE)
+                return {nullptr, tecOBJECT_NOT_FOUND};
 
-            if (hasExpired(ctx.view, (*offer)[~sfExpiration]))
-                return tecEXPIRED;
-        }
+            if (hasExpired(ctx.view, (*offerSLE)[~sfExpiration]))
+                return {nullptr, tecEXPIRED};
 
-        return tesSUCCESS;
+            return {std::move(offerSLE), tesSUCCESS};
+        }
+        return {nullptr, tesSUCCESS};
     };
 
-    auto const buy = ctx.tx[~sfNFTokenBuyOffer];
-    auto const sell = ctx.tx[~sfNFTokenSellOffer];
-
-    if (auto const ret = checkOffer(buy); !isTesSuccess(ret))
-        return ret;
-
-    if (auto const ret = checkOffer(sell); !isTesSuccess(ret))
-        return ret;
+    auto const [bo, err1] = checkOffer(ctx.tx[~sfNFTokenBuyOffer]);
+    if (!isTesSuccess(err1))
+        return err1;
+    auto const [so, err2] = checkOffer(ctx.tx[~sfNFTokenSellOffer]);
+    if (!isTesSuccess(err2))
+        return err2;
 
-    if (buy && sell)
+    if (bo && so)
     {
         // Brokered mode:
-        auto const bo = ctx.view.read(keylet::nftoffer(*buy));
-        auto const so = ctx.view.read(keylet::nftoffer(*sell));
-
         // The two offers being brokered must be for the same token:
         if ((*bo)[sfNFTokenID] != (*so)[sfNFTokenID])
             return tecNFTOKEN_BUY_SELL_MISMATCH;
@@ -131,10 +128,8 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
         }
     }
 
-    if (buy)
+    if (bo)
     {
-        auto const bo = ctx.view.read(keylet::nftoffer(*buy));
-
         if (((*bo)[sfFlags] & lsfSellNFToken) == lsfSellNFToken)
             return tecNFTOKEN_OFFER_TYPE_MISMATCH;
 
@@ -143,7 +138,7 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
             return tecCANT_ACCEPT_OWN_NFTOKEN_OFFER;
 
         // If not in bridged mode, the account must own the token:
-        if (!sell &&
+        if (!so &&
             !nft::findToken(ctx.view, ctx.tx[sfAccount], (*bo)[sfNFTokenID]))
             return tecNO_PERMISSION;
 
@@ -160,10 +155,8 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
             return tecINSUFFICIENT_FUNDS;
     }
 
-    if (sell)
+    if (so)
     {
-        auto const so = ctx.view.read(keylet::nftoffer(*sell));
-
         if (((*so)[sfFlags] & lsfSellNFToken) != lsfSellNFToken)
             return tecNFTOKEN_OFFER_TYPE_MISMATCH;
 
@@ -176,7 +169,7 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
             return tecNO_PERMISSION;
 
         // If not in bridged mode...
-        if (!buy)
+        if (!bo)
         {
             // If the offer has a Destination field, the acceptor must be the
             // Destination.

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

@@ -77,27 +77,9 @@ NFTokenBurn::preclaim(PreclaimContext const& ctx)
         }
     }
 
-    auto const id = ctx.tx[sfNFTokenID];
-
-    std::size_t totalOffers = 0;
-
-    {
-        Dir buys(ctx.view, keylet::nft_buys(id));
-        totalOffers += std::distance(buys.begin(), buys.end());
-    }
-
-    if (totalOffers > maxDeletableTokenOfferEntries)
-        return tefTOO_BIG;
-
-    {
-        Dir sells(ctx.view, keylet::nft_sells(id));
-        totalOffers += std::distance(sells.begin(), sells.end());
-    }
-
-    if (totalOffers > maxDeletableTokenOfferEntries)
-        return tefTOO_BIG;
-
-    return tesSUCCESS;
+    // 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]);
 }
 
 TER

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

@@ -21,6 +21,7 @@
 #include <ripple/basics/algorithm.h>
 #include <ripple/ledger/Directory.h>
 #include <ripple/ledger/View.h>
+#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/STAccount.h>
 #include <ripple/protocol/STArray.h>
 #include <ripple/protocol/TxFlags.h>
@@ -131,15 +132,40 @@ getPageForToken(
                         cmp;
                 });
 
-        // If splitIter == begin(), then the entire page is filled with
-        // equivalent tokens.  We cannot split the page, so we cannot
-        // insert the requested token.
-        //
         // There should be no circumstance when splitIter == end(), but if it
         // were to happen we should bail out because something is confused.
-        if (splitIter == narr.begin() || splitIter == narr.end())
+        if (splitIter == narr.end())
             return nullptr;
 
+        // If splitIter == begin(), then the entire page is filled with
+        // equivalent tokens.  This requires special handling.
+        if (splitIter == narr.begin())
+        {
+            // Prior to fixNFTokenDirV1 we simply stopped.
+            if (!view.rules().enabled(fixNFTokenDirV1))
+                return nullptr;
+            else
+            {
+                // This would be an ideal place for the spaceship operator...
+                int const relation = compare(id & nft::pageMask, cmp);
+                if (relation == 0)
+                    // If the passed in id belongs exactly on this (full) page
+                    // this account simply cannot store the NFT.
+                    return nullptr;
+
+                else if (relation > 0)
+                    // We need to leave the entire contents of this page in
+                    // narr so carr stays empty.  The new NFT will be
+                    // inserted in carr.  This keeps the NFTs that must be
+                    // together all on their own page.
+                    splitIter = narr.end();
+
+                // If neither of those conditions apply then put all of
+                // narr into carr and produce an empty narr where the new NFT
+                // will be inserted.  Leave the split at narr.begin().
+            }
+        }
+
         // Split narr at splitIter.
         STArray newCarr(
             std::make_move_iterator(splitIter),
@@ -148,8 +174,20 @@ getPageForToken(
         std::swap(carr, newCarr);
     }
 
-    auto np = std::make_shared<SLE>(
-        keylet::nftpage(base, carr[0].getFieldH256(sfNFTokenID)));
+    // Determine the ID for the page index.  This decision is conditional on
+    // fixNFTokenDirV1 being enabled.  But the condition for the decision
+    // is not possible unless fixNFTokenDirV1 is enabled.
+    //
+    // Note that we use uint256::next() because there's a subtlety in the way
+    // NFT pages are structured.  The low 96-bits of NFT ID must be strictly
+    // less than the low 96-bits of the enclosing page's index.  In order to
+    // accommodate that requirement we use an index one higher than the
+    // largest NFT in the page.
+    uint256 const tokenIDForNewPage = narr.size() == dirMaxTokensPerPage
+        ? narr[dirMaxTokensPerPage - 1].getFieldH256(sfNFTokenID).next()
+        : carr[0].getFieldH256(sfNFTokenID);
+
+    auto np = std::make_shared<SLE>(keylet::nftpage(base, tokenIDForNewPage));
     np->setFieldArray(sfNFTokens, narr);
     np->setFieldH256(sfNextPageMin, cp->key());
 
@@ -172,10 +210,17 @@ getPageForToken(
 
     createCallback(view, owner);
 
-    return (first.key <= np->key()) ? np : cp;
+    // fixNFTokenDirV1 corrects a bug in the initial implementation that
+    // would put an NFT in the wrong page.  The problem was caused by an
+    // off-by-one subtlety that the NFT can only be stored in the first page
+    // with a key that's strictly greater than `first`
+    if (!view.rules().enabled(fixNFTokenDirV1))
+        return (first.key <= np->key()) ? np : cp;
+
+    return (first.key < np->key()) ? np : cp;
 }
 
-static bool
+bool
 compareTokens(uint256 const& a, uint256 const& b)
 {
     // The sort of NFTokens needs to be fully deterministic, but the sort
@@ -505,6 +550,33 @@ removeAllTokenOffers(ApplyView& view, Keylet const& directory)
     });
 }
 
+TER
+notTooManyOffers(ReadView const& view, uint256 const& nftokenID)
+{
+    std::size_t totalOffers = 0;
+
+    {
+        Dir buys(view, keylet::nft_buys(nftokenID));
+        for (auto iter = buys.begin(); iter != buys.end(); iter.next_page())
+        {
+            totalOffers += iter.page_size();
+            if (totalOffers > maxDeletableTokenOfferEntries)
+                return tefTOO_BIG;
+        }
+    }
+
+    {
+        Dir sells(view, keylet::nft_sells(nftokenID));
+        for (auto iter = sells.begin(); iter != sells.end(); iter.next_page())
+        {
+            totalOffers += iter.page_size();
+            if (totalOffers > maxDeletableTokenOfferEntries)
+                return tefTOO_BIG;
+        }
+    }
+    return tesSUCCESS;
+}
+
 bool
 deleteTokenOffer(ApplyView& view, std::shared_ptr<SLE> const& offer)
 {