Introduce fixNFTokenDirV1 amendment:

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)
 {