Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce fixNFTokenDirV1 amendment: #4155

Closed
wants to merge 2 commits into from
Closed

Introduce fixNFTokenDirV1 amendment: #4155

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2893a44
Introduce fixNFTokenDirV1 amendment:
scottschurr Apr 19, 2022
5040146
[FOLD] Update selected comments as suggested during review
scottschurr May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 53 additions & 13 deletions src/ripple/app/tx/impl/InvariantCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -493,23 +495,24 @@ 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])
if (prev)
{
if (account != (*prev & accountBits))
badLink_ = true;

if (limit <= (*prev & pageBits))
if (hiLimit <= (*prev & pageBits))
badLink_ = true;
}

Expand All @@ -518,17 +521,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;
}
}
};

Expand Down Expand Up @@ -559,12 +587,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;
}

thejohnfreeman marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

Expand Down
4 changes: 3 additions & 1 deletion src/ripple/app/tx/impl/InvariantCheck.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
49 changes: 21 additions & 28 deletions src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand All @@ -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;

Expand All @@ -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;

Expand All @@ -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.
Expand Down
24 changes: 3 additions & 21 deletions src/ripple/app/tx/impl/NFTokenBurn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
89 changes: 80 additions & 9 deletions src/ripple/app/tx/impl/details/NFTokenUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -131,15 +132,39 @@ 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 move the entire contents of this page to
// narr and leave carr empty. This keeps the NFTs
// that must be together all on their own page.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this comment and the next confusing. Below, on lines 168 - 174, we move a (possibly empty) range from narr to carr. This comment says "move the contents to narr and leave carr empty", when it really means "leave narr full and carr empty" (i.e. no move is performed).

The next comment says "leave carr intact and produce an empty narr" when it really means "empty the contents of narr into carr" (i.e. carr is not left intact).

After reading this function many many times, I finally understand that narr means "new array", the array of tokens that will be placed into np, the "new page", and which will all have a strictly lesser low-96-bits than the tokens in cp, the "current page" that will be left at its same page key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on these comments. Thanks for reading carefully. Sorry I messed them up the first time. How's this for the first one?

// 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.

And for the second comment...

// 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().

splitIter = narr.end();

// If neither of those conditions apply then we need to leave
// carr intact 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),
Expand All @@ -148,8 +173,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());

Expand All @@ -172,10 +209,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
Expand Down Expand Up @@ -505,6 +549,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)
{
Expand Down
Loading