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;
+            auto const& nftokens = sle->getFieldArray(sfNFTokens);
 
-            if (auto uri = obj[~sfURI]; uri && uri->empty())
-                badURI_ = true;
+            // 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 @@ public:
 
 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 {std::move(offerSLE), tesSUCCESS};
         }
-
-        return tesSUCCESS;
+        return {nullptr, tesSUCCESS};
     };
 
-    auto const buy = ctx.tx[~sfNFTokenBuyOffer];
-    auto const sell = ctx.tx[~sfNFTokenSellOffer];
+    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 (auto const ret = checkOffer(buy); !isTesSuccess(ret))
-        return ret;
-
-    if (auto const ret = checkOffer(sell); !isTesSuccess(ret))
-        return ret;
-
-    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)
 {

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

@@ -53,15 +53,14 @@ constexpr std::uint16_t const flagOnlyXRP = 0x0002;
 constexpr std::uint16_t const flagCreateTrustLines = 0x0004;
 constexpr std::uint16_t const flagTransferable = 0x0008;
 
-/** Erases the specified offer from the specified token offer directory.
-
- */
-void
-removeTokenOffer(ApplyView& view, uint256 const& id);
-
+/** Deletes all offers from the specified token offer directory. */
 void
 removeAllTokenOffers(ApplyView& view, Keylet const& directory);
 
+/** Returns tesSUCCESS if NFToken has few enough offers that it can be burned */
+TER
+notTooManyOffers(ReadView const& view, uint256 const& nftokenID);
+
 /** Finds the specified token in the owner's token directory. */
 std::optional<STObject>
 findToken(
@@ -179,6 +178,9 @@ getIssuer(uint256 const& id)
     return AccountID::fromVoid(id.data() + 4);
 }
 
+bool
+compareTokens(uint256 const& a, uint256 const& b);
+
 }  // namespace nft
 
 }  // namespace ripple

src/ripple/ledger/Directory.h

@@ -79,6 +79,12 @@ public:
     const_iterator
     operator++(int);
 
+    const_iterator&
+    next_page();
+
+    std::size_t
+    page_size();
+
     Keylet const&
     page() const
     {

src/ripple/ledger/impl/Directory.cpp

@@ -1,5 +1,4 @@
-//------------
-//------------------------------------------------------------------
+//------------------------------------------------------------------------------
 /*
     This file is part of rippled: https://github.com/ripple/rippled
     Copyright (c) 2012, 2015 Ripple Labs Inc.
@@ -81,35 +80,11 @@ const_iterator::operator++()
     if (++it_ != std::end(*indexes_))
     {
         index_ = *it_;
-    }
-    else
-    {
-        auto const next = sle_->getFieldU64(sfIndexNext);
-        if (next == 0)
-        {
-            page_.key = root_.key;
-            index_ = beast::zero;
-        }
-        else
-        {
-            page_ = keylet::page(root_, next);
-            sle_ = view_->read(page_);
-            assert(sle_);
-            indexes_ = &sle_->getFieldV256(sfIndexes);
-            if (indexes_->empty())
-            {
-                index_ = beast::zero;
-            }
-            else
-            {
-                it_ = std::begin(*indexes_);
-                index_ = *it_;
-            }
-        }
+        cache_ = std::nullopt;
+        return *this;
     }
 
-    cache_ = std::nullopt;
-    return *this;
+    return next_page();
 }
 
 const_iterator
@@ -121,4 +96,39 @@ const_iterator::operator++(int)
     return tmp;
 }
 
+const_iterator&
+const_iterator::next_page()
+{
+    auto const next = sle_->getFieldU64(sfIndexNext);
+    if (next == 0)
+    {
+        page_.key = root_.key;
+        index_ = beast::zero;
+    }
+    else
+    {
+        page_ = keylet::page(root_, next);
+        sle_ = view_->read(page_);
+        assert(sle_);
+        indexes_ = &sle_->getFieldV256(sfIndexes);
+        if (indexes_->empty())
+        {
+            index_ = beast::zero;
+        }
+        else
+        {
+            it_ = std::begin(*indexes_);
+            index_ = *it_;
+        }
+    }
+    cache_ = std::nullopt;
+    return *this;
+}
+
+std::size_t
+const_iterator::page_size()
+{
+    return indexes_->size();
+}
+
 }  // namespace ripple

src/ripple/protocol/Feature.h

@@ -74,7 +74,7 @@ namespace detail {
 // Feature.cpp. Because it's only used to reserve storage, and determine how
 // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
 // the actual number of amendments. A LogicError on startup will verify this.
-static constexpr std::size_t numFeatures = 47;
+static constexpr std::size_t numFeatures = 48;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -335,6 +335,7 @@ extern uint256 const fixRmSmallIncreasedQOffers;
 extern uint256 const featureCheckCashMakesTrustLine;
 extern uint256 const featureNonFungibleTokensV1;
 extern uint256 const featureExpandedSignerList;
+extern uint256 const fixNFTokenDirV1;
 
 }  // namespace ripple
 

src/ripple/protocol/impl/Feature.cpp

@@ -439,6 +439,7 @@ REGISTER_FIX    (fixRmSmallIncreasedQOffers,    Supported::yes, DefaultVote::yes
 REGISTER_FEATURE(CheckCashMakesTrustLine,       Supported::yes, DefaultVote::no);
 REGISTER_FEATURE(NonFungibleTokensV1,           Supported::yes, DefaultVote::no);
 REGISTER_FEATURE(ExpandedSignerList,            Supported::yes, DefaultVote::no);
+REGISTER_FIX    (fixNFTokenDirV1,               Supported::yes, DefaultVote::no);
 
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.

src/ripple/rpc/handlers/AccountObjects.cpp

@@ -26,6 +26,7 @@
 #include <ripple/protocol/Indexes.h>
 #include <ripple/protocol/LedgerFormats.h>
 #include <ripple/protocol/jss.h>
+#include <ripple/protocol/nftPageMask.h>
 #include <ripple/resource/Fees.h>
 #include <ripple/rpc/Context.h>
 #include <ripple/rpc/impl/RPCHelpers.h>
@@ -101,21 +102,41 @@ doAccountNFTs(RPC::JsonContext& context)
     auto& nfts = (result[jss::account_nfts] = Json::arrayValue);
 
     // Continue iteration from the current page:
-
+    bool pastMarker = marker.isZero();
+    uint256 const maskedMarker = marker & nft::pageMask;
     while (cp)
     {
         auto arr = cp->getFieldArray(sfNFTokens);
 
         for (auto const& o : arr)
         {
-            if (o.getFieldH256(sfNFTokenID) <= marker)
+            // Scrolling past the marker gets weird.  We need to look at
+            // a couple of conditions.
+            //
+            //  1. If the low 96-bits don't match, then we compare only
+            //     against the low 96-bits, since that's what determines
+            //     the sort order of the pages.
+            //
+            //  2. However, within one page there can be a number of
+            //     NFTokenIDs that all have the same low 96 bits.  If we're
+            //     in that case then we need to compare against the full
+            //     256 bits.
+            uint256 const nftokenID = o[sfNFTokenID];
+            uint256 const maskedNftokenID = nftokenID & nft::pageMask;
+
+            if (!pastMarker && maskedNftokenID < maskedMarker)
                 continue;
 
+            if (!pastMarker && maskedNftokenID == maskedMarker &&
+                nftokenID <= marker)
+                continue;
+
+            pastMarker = true;
+
             {
                 Json::Value& obj = nfts.append(o.getJson(JsonOptions::none));
 
                 // Pull out the components of the nft ID.
-                uint256 const nftokenID = o[sfNFTokenID];
                 obj[sfFlags.jsonName] = nft::getFlags(nftokenID);
                 obj[sfIssuer.jsonName] = to_string(nft::getIssuer(nftokenID));
                 obj[sfNFTokenTaxon.jsonName] =

src/test/app/NFTokenBurn_test.cpp

@@ -595,8 +595,11 @@ public:
     run() override
     {
         using namespace test::jtx;
-        auto const sa = supported_amendments();
-        testWithFeats(sa);
+        FeatureBitset const all{supported_amendments()};
+        FeatureBitset const fixNFTDir{fixNFTokenDirV1};
+
+        testWithFeats(all - fixNFTDir);
+        testWithFeats(all);
     }
 };
 

src/test/app/NFToken_test.cpp

@@ -482,15 +482,14 @@ class NFToken_test : public beast::unit_test::suite
                 if (replacement->getFieldU32(sfMintedNFTokens) != 1)
                     return false;  // Unexpected test conditions.
 
-                // Now replace the sfMintedNFTokens with its maximum value.
-                (*replacement)[sfMintedNFTokens] =
-                    std::numeric_limits<std::uint32_t>::max();
+                // Now replace sfMintedNFTokens with the largest valid value.
+                (*replacement)[sfMintedNFTokens] = 0xFFFF'FFFE;
                 view.rawReplace(replacement);
                 return true;
             });
 
-        // alice should not be able to mint any tokens because she has already
-        // minted the maximum allowed by a single account.
+        // See whether alice is at the boundary that causes an error.
+        env(token::mint(alice, 0u), ter(tesSUCCESS));
         env(token::mint(alice, 0u), ter(tecMAX_SEQUENCE_REACHED));
     }
 
@@ -4069,6 +4068,87 @@ class NFToken_test : public beast::unit_test::suite
         }
     }
 
+    void
+    testNFTokenOfferOwner(FeatureBitset features)
+    {
+        // Verify the Owner field of an offer behaves as expected.
+        testcase("NFToken offer owner");
+
+        using namespace test::jtx;
+
+        Env env{*this, features};
+
+        Account const issuer{"issuer"};
+        Account const buyer1{"buyer1"};
+        Account const buyer2{"buyer2"};
+        env.fund(XRP(10000), issuer, buyer1, buyer2);
+        env.close();
+
+        // issuer creates an NFT.
+        uint256 const nftId{token::getNextID(env, issuer, 0u, tfTransferable)};
+        env(token::mint(issuer, 0u), txflags(tfTransferable));
+        env.close();
+
+        // Prove that issuer now owns nftId.
+        BEAST_EXPECT(nftCount(env, issuer) == 1);
+        BEAST_EXPECT(nftCount(env, buyer1) == 0);
+        BEAST_EXPECT(nftCount(env, buyer2) == 0);
+
+        // Both buyer1 and buyer2 create buy offers for nftId.
+        uint256 const buyer1OfferIndex =
+            keylet::nftoffer(buyer1, env.seq(buyer1)).key;
+        env(token::createOffer(buyer1, nftId, XRP(100)), token::owner(issuer));
+        uint256 const buyer2OfferIndex =
+            keylet::nftoffer(buyer2, env.seq(buyer2)).key;
+        env(token::createOffer(buyer2, nftId, XRP(100)), token::owner(issuer));
+        env.close();
+
+        // Lambda that counts the number of buy offers for a given NFT.
+        auto nftBuyOfferCount = [&env](uint256 const& nftId) -> std::size_t {
+            // We know that in this case not very many offers will be
+            // returned, so we skip the marker stuff.
+            Json::Value params;
+            params[jss::nft_id] = to_string(nftId);
+            Json::Value buyOffers =
+                env.rpc("json", "nft_buy_offers", to_string(params));
+
+            if (buyOffers.isMember(jss::result) &&
+                buyOffers[jss::result].isMember(jss::offers))
+                return buyOffers[jss::result][jss::offers].size();
+
+            return 0;
+        };
+
+        // Show there are two buy offers for nftId.
+        BEAST_EXPECT(nftBuyOfferCount(nftId) == 2);
+
+        // issuer accepts buyer1's offer.
+        env(token::acceptBuyOffer(issuer, buyer1OfferIndex));
+        env.close();
+
+        // Prove that buyer1 now owns nftId.
+        BEAST_EXPECT(nftCount(env, issuer) == 0);
+        BEAST_EXPECT(nftCount(env, buyer1) == 1);
+        BEAST_EXPECT(nftCount(env, buyer2) == 0);
+
+        // buyer1's offer was consumed, but buyer2's offer is still in the
+        // ledger.
+        BEAST_EXPECT(nftBuyOfferCount(nftId) == 1);
+
+        // buyer1 can now accept buyer2's offer, even though buyer2's
+        // NFTokenCreateOffer transaction specified the NFT Owner as issuer.
+        env(token::acceptBuyOffer(buyer1, buyer2OfferIndex));
+        env.close();
+
+        // Prove that buyer2 now owns nftId.
+        BEAST_EXPECT(nftCount(env, issuer) == 0);
+        BEAST_EXPECT(nftCount(env, buyer1) == 0);
+        BEAST_EXPECT(nftCount(env, buyer2) == 1);
+
+        // All of the NFTokenOffers are now consumed.
+        BEAST_EXPECT(nftBuyOfferCount(nftId) == 0);
+    }
+
     void
     testNFTokenWithTickets(FeatureBitset features)
     {
@@ -4248,6 +4328,235 @@ class NFToken_test : public beast::unit_test::suite
         env.close();
     }
 
+    void
+    testNftXxxOffers(FeatureBitset features)
+    {
+        testcase("nft_buy_offers and nft_sell_offers");
+
+        // The default limit on returned NFToken offers is 250, so we need
+        // to produce more than 250 offers of each kind in order to exercise
+        // the marker.
+
+        // Fortunately there's nothing in the rules that says an account
+        // can't hold more than one offer for the same NFT.  So we only
+        // need two accounts to generate the necessary offers.
+        using namespace test::jtx;
+
+        Env env{*this, features};
+
+        Account const issuer{"issuer"};
+        Account const buyer{"buyer"};
+
+        // A lot of offers requires a lot for reserve.
+        env.fund(XRP(1000000), issuer, buyer);
+        env.close();
+
+        // Create an NFT that we'll make offers for.
+        uint256 const nftID{token::getNextID(env, issuer, 0u, tfTransferable)};
+        env(token::mint(issuer, 0), txflags(tfTransferable));
+        env.close();
+
+        // A lambda that validates nft_XXX_offers query responses.
+        auto checkOffers = [this, &env, &nftID](
+                               char const* request,
+                               int expectCount,
+                               int expectMarkerCount,
+                               int line) {
+            int markerCount = 0;
+            Json::Value allOffers(Json::arrayValue);
+            std::string marker;
+
+            // The do/while collects results until no marker is returned.
+            do
+            {
+                Json::Value nftOffers = [&env, &nftID, &request, &marker]() {
+                    Json::Value params;
+                    params[jss::nft_id] = to_string(nftID);
+
+                    if (!marker.empty())
+                        params[jss::marker] = marker;
+                    return env.rpc("json", request, to_string(params));
+                }();
+
+                // If there are no offers for the NFT we get an error
+                if (expectCount == 0)
+                {
+                    if (expect(
+                            nftOffers.isMember(jss::result),
+                            "expected \"result\"",
+                            __FILE__,
+                            line))
+                    {
+                        if (expect(
+                                nftOffers[jss::result].isMember(jss::error),
+                                "expected \"error\"",
+                                __FILE__,
+                                line))
+                        {
+                            expect(
+                                nftOffers[jss::result][jss::error].asString() ==
+                                    "objectNotFound",
+                                "expected \"objectNotFound\"",
+                                __FILE__,
+                                line);
+                        }
+                    }
+                    break;
+                }
+
+                marker.clear();
+                if (expect(
+                        nftOffers.isMember(jss::result),
+                        "expected \"result\"",
+                        __FILE__,
+                        line))
+                {
+                    Json::Value& result = nftOffers[jss::result];
+
+                    if (result.isMember(jss::marker))
+                    {
+                        ++markerCount;
+                        marker = result[jss::marker].asString();
+                    }
+
+                    if (expect(
+                            result.isMember(jss::offers),
+                            "expected \"offers\"",
+                            __FILE__,
+                            line))
+                    {
+                        Json::Value& someOffers = result[jss::offers];
+                        for (std::size_t i = 0; i < someOffers.size(); ++i)
+                            allOffers.append(someOffers[i]);
+                    }
+                }
+            } while (!marker.empty());
+
+            // Verify the contents of allOffers makes sense.
+            expect(
+                allOffers.size() == expectCount,
+                "Unexpected returned offer count",
+                __FILE__,
+                line);
+            expect(
+                markerCount == expectMarkerCount,
+                "Unexpected marker count",
+                __FILE__,
+                line);
+            std::optional<int> globalFlags;
+            std::set<std::string> offerIndexes;
+            std::set<std::string> amounts;
+            for (Json::Value const& offer : allOffers)
+            {
+                // The flags on all found offers should be the same.
+                if (!globalFlags)
+                    globalFlags = offer[jss::flags].asInt();
+
+                expect(
+                    *globalFlags == offer[jss::flags].asInt(),
+                    "Inconsistent flags returned",
+                    __FILE__,
+                    line);
+
+                // The test conditions should produce unique indexes and
+                // amounts for all offers.
+                offerIndexes.insert(offer[jss::nft_offer_index].asString());
+                amounts.insert(offer[jss::amount].asString());
+            }
+
+            expect(
+                offerIndexes.size() == expectCount,
+                "Duplicate indexes returned?",
+                __FILE__,
+                line);
+            expect(
+                amounts.size() == expectCount,
+                "Duplicate amounts returned?",
+                __FILE__,
+                line);
+        };
+
+        // There are no sell offers.
+        checkOffers("nft_sell_offers", 0, false, __LINE__);
+
+        // A lambda that generates sell offers.
+        STAmount sellPrice = XRP(0);
+        auto makeSellOffers =
+            [&env, &issuer, &nftID, &sellPrice](STAmount const& limit) {
+                // Save a little test time by not closing too often.
+                int offerCount = 0;
+                while (sellPrice < limit)
+                {
+                    sellPrice += XRP(1);
+                    env(token::createOffer(issuer, nftID, sellPrice),
+                        txflags(tfSellNFToken));
+                    if (++offerCount % 10 == 0)
+                        env.close();
+                }
+                env.close();
+            };
+
+        // There is one sell offer.
+        makeSellOffers(XRP(1));
+        checkOffers("nft_sell_offers", 1, 0, __LINE__);
+
+        // There are 250 sell offers.
+        makeSellOffers(XRP(250));
+        checkOffers("nft_sell_offers", 250, 0, __LINE__);
+
+        // There are 251 sell offers.
+        makeSellOffers(XRP(251));
+        checkOffers("nft_sell_offers", 251, 1, __LINE__);
+
+        // There are 500 sell offers.
+        makeSellOffers(XRP(500));
+        checkOffers("nft_sell_offers", 500, 1, __LINE__);
+
+        // There are 501 sell offers.
+        makeSellOffers(XRP(501));
+        checkOffers("nft_sell_offers", 501, 2, __LINE__);
+
+        // There are no buy offers.
+        checkOffers("nft_buy_offers", 0, 0, __LINE__);
+
+        // A lambda that generates buy offers.
+        STAmount buyPrice = XRP(0);
+        auto makeBuyOffers =
+            [&env, &buyer, &issuer, &nftID, &buyPrice](STAmount const& limit) {
+                // Save a little test time by not closing too often.
+                int offerCount = 0;
+                while (buyPrice < limit)
+                {
+                    buyPrice += XRP(1);
+                    env(token::createOffer(buyer, nftID, buyPrice),
+                        token::owner(issuer));
+                    if (++offerCount % 10 == 0)
+                        env.close();
+                }
+                env.close();
+            };
+
+        // There is one buy offer;
+        makeBuyOffers(XRP(1));
+        checkOffers("nft_buy_offers", 1, 0, __LINE__);
+
+        // There are 250 buy offers.
+        makeBuyOffers(XRP(250));
+        checkOffers("nft_buy_offers", 250, 0, __LINE__);
+
+        // There are 251 buy offers.
+        makeBuyOffers(XRP(251));
+        checkOffers("nft_buy_offers", 251, 1, __LINE__);
+
+        // There are 500 buy offers.
+        makeBuyOffers(XRP(500));
+        checkOffers("nft_buy_offers", 500, 1, __LINE__);
+
+        // There are 501 buy offers.
+        makeBuyOffers(XRP(501));
+        checkOffers("nft_buy_offers", 501, 2, __LINE__);
+    }
+
     void
     testWithFeats(FeatureBitset features)
     {
@@ -4271,8 +4580,10 @@ class NFToken_test : public beast::unit_test::suite
         testCancelOffers(features);
         testCancelTooManyOffers(features);
         testBrokeredAccept(features);
+        testNFTokenOfferOwner(features);
         testNFTokenWithTickets(features);
         testNFTokenDeleteAccount(features);
+        testNftXxxOffers(features);
     }
 
 public:
@@ -4280,8 +4591,11 @@ public:
     run() override
     {
         using namespace test::jtx;
-        auto const sa = supported_amendments();
-        testWithFeats(sa);
+        FeatureBitset const all{supported_amendments()};
+        FeatureBitset const fixNFTDir{fixNFTokenDirV1};
+
+        testWithFeats(all - fixNFTDir);
+        testWithFeats(all);
     }
 };