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.

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

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

Merged
merged 18 commits into from
Feb 2, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
43 changes: 37 additions & 6 deletions src/ripple/app/tx/impl/NFTokenBurn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -104,9 +109,35 @@ 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, but prefers buy offers first
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious why buy offers for an NFToken are preferred over sell offers.

In my mind there should be a lot fewer sell offers for an NFT than there might be buy offers. I would expect there to almost always be fewer than 500 sell offers. If that's the case, then if we prefer sell offers then we will almost always remove the entire directory of sell NFTokenSellOffers.

The buy offers seem like the ones that might exceed 500. If they do exceed 500, and we delete them first, then we'll be leaving two directory structures behind at the end of this:

  • Whatever remains of the directory containing buy offers plus
  • The entire directory containing sell offers.

It feels like we should design this to at least minimize the amount of ledger trash it leaves behind.

Am I correct that there are probably more buy offers than sell offers? Does the motivation for changing the preference make sense?

At the very least, the comment should explain why we prefer to delete buy offers. That way, if the reason stays important, it's less likely to be changed when someone is maintaining the code in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it is more likely to have more buy offers than sell offers. That's why I tried to when delete buy offers, because I had the impression that buy offer is the root of the problem, and should be deleted first. On the other hand, sell offers are created by the owner and are less of a problem. Obviously I haven't taken optimization into account, and it does make sense to reduce ledge space to delete sell offers first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But I think it also may be more ideal to delete buy offers from user's perspective, because there would be less hassle for the buyer to delete their offers (if their offer was amongst the deleted ones). Buyers may not be aware of their outstanding buy offers when the NFT was burned, and take up unnecessary reserves for them. On the other hand, the owner of the NFT can delete his/her sell offer themself

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a accumulated effort when factoring how every buyer needs to manually cancel their buy offer, so it would probably be favourable to delete buy offers first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

team would like to discuss with @ledhed2222

Copy link
Collaborator Author

@shawnxie999 shawnxie999 Jan 25, 2023

Choose a reason for hiding this comment

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

I'm not sure what how severe it is to having the extra directory on the ledger. Down the line, we are looking to implement (from Scott D.)

When anyone deletes one of their orphaned offers, we should delete as many orphaned offers as we can in that transaction (like as do with expired offers in the payment engine).

With this upcoming implementation, the sell directory is unlikely to hang on the ledger forever. As soon as someone submits a transaction to delete one of their orphaned offers, all the orphaned offers from the burnt NFT will be removed. So I don't think it would be a big problem then.

And I think it might be preferable to help the buyers first, because buyers are less aware of the orphaned offers. While the owner of the NFT who created sell offers is much more aware of his/her orphaned offers, and is more likely to delete them in batches themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm curious - if i have an NFT with 600 buy offers, which I burn, there are now 100 orphaned offers. what parties are able to cancel the orphaned offers at that point, other than the person who owns the offer (and if they are expired, anyone)? presumably I should still be able to cancel them since I am listed as the "owner" on the offer.

but generally, I don't think i have a huge POV/opinion on which offers get cancelled with higher priority. what scott is saying makes sense. i might go with that.

however, one thing i'm wondering is whether we can included a warning if the token burn did not successfully cancel all offers. something like Not all offers for this NFT could be cancelled. You should immediately cancel any remaining offers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently there are two parties that have permission to cancel NFTokenOffers, whether those offers are orphaned or not:

  • The Owner of the offer can always cancel the offer.
  • If there's a Destination, then the Destination can cancel the offer.

That's assuming that the offer has not expired, in which case the offer can be canceled by anybody.

Here's the code: https://github.com/XRPLF/rippled/blob/develop/src/ripple/app/tx/impl/NFTokenCancelOffer.cpp#L77-L87

Regarding a warning, that's not really something that the ledger internals can provide. There are three types of responses:

  • tesSUCCESS is what you get if the transaction succeeded. If the NFTokenBurn succeeds, whether it leaves orphan offers or not, the appropriate response is tesSUCCESS.
  • tec* is the range of responses where the ledger did not do what you asked, but burned your fee anyway. That's not what is happening here, since the burn was successful.
  • The are other error types (ter, ter, and so forth) to indicate other kinds of failures. But they all mean that your transaction did not change the ledger in any way.

At any rate, such a warning would be returned to the wrong person.

The person who is leaving the orphaned offers is the issuer burning the token. They really don't care whether there are left over offers in the ledger. The only folks who care are the offer owners. And they only care once they want to recover the reserve on their offers. So they may not be motivated to cancel their offers for a long time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@seelabs' suggestion may help, but I have doubts about its effectiveness in a larger scale. Here's why.

Offers on the DEX are encountered whenever someone wants to trade a currency pair. There are some currency pairs that are very popular, so their order books get a lot of traffic. There are other currency pairs that see a lot less traffic. For those low-traffic currencies, dead offers can sit around for a long time before the DEX stumbles across them and removes them.

Compared to currency pairs, there are lots of NFTs on the ledger. And there are up to two order books for every individual NFT in the ledger. I would be astonished if any NFTs are traded frequently. The bulk of NFTs, and offers for them, will just sit on the ledger. Any single NFToken will probably be traded relatively infrequently.

So I doubt that suggestion will help much.

But at a more meta level, what I hear you saying is that the patch you are providing for this problem is insufficient. We need yet another patch to clean up what is left behind by this patch. Amendments are not free. We should get everything that we think is needed into one amendment.

Copy link
Collaborator Author

@shawnxie999 shawnxie999 Jan 26, 2023

Choose a reason for hiding this comment

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

@scottschurr (cc. @seelabs )

But at a more meta level, what I hear you saying is that the patch you are providing for this problem is insufficient. We need yet another patch to clean up what is left behind by this patch. Amendments are not free. We should get everything that we think is needed into one amendment.

Our team's idea is try to get this change into the next release(1.10), so that we can get voting and hear community's voice as early as possible. The voting may or may not be in our favor, but we want to get it into next release to get a feel what the community likes, and perhaps make more adjustments from there.

While I do agree that it feels more robust to include the patch of cleaning orphaned offers into the existing one, but unfortunately this was not considered in scoping, and including it would push back the timeline even further.

int const deletedBuyOffers = nft::removeTokenOffersWithLimit(
view(),
keylet::nft_buys(ctx_.tx[sfNFTokenID]),
maxDeletableTokenOfferEntries);

if (maxDeletableTokenOfferEntries - deletedBuyOffers > 0)
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
{
nft::removeTokenOffersWithLimit(
view(),
keylet::nft_sells(ctx_.tx[sfNFTokenID]),
maxDeletableTokenOfferEntries - deletedBuyOffers);
}
}
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());
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
}

return tesSUCCESS;
}
Expand Down
64 changes: 41 additions & 23 deletions src/ripple/app/tx/impl/details/NFTokenUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -520,34 +520,52 @@ findTokenAndPage(
}
return std::nullopt;
}
void
removeAllTokenOffers(ApplyView& view, Keylet const& directory)
{
view.dirDelete(directory, [&view](uint256 const& id) {
auto offer = view.peek(Keylet{ltNFTOKEN_OFFER, id});

if (!offer)
Throw<std::runtime_error>(
"Offer " + to_string(id) + " not found in ledger!");
int
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
removeTokenOffersWithLimit(
ApplyView& view,
Keylet const& directory,
int maxDeletableOffers)
{
if (maxDeletableOffers == 0)
return 0;

auto const owner = (*offer)[sfOwner];
std::optional<std::uint64_t> pageIndex{0};
int deletedOffersCount = 0;

if (!view.dirRemove(
keylet::ownerDir(owner),
(*offer)[sfOwnerNode],
offer->key(),
false))
Throw<std::runtime_error>(
"Offer " + to_string(id) + " not found in owner directory!");
do
{
auto const page = view.peek(keylet::page(directory, *pageIndex));
if (!page)
break;

auto offerIndexes = page->getFieldV256(sfIndexes);

// 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.
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
for (int i = offerIndexes.size() - 1; i >= 0; --i)
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
{
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!");
}

adjustOwnerCount(
view,
view.peek(keylet::account(owner)),
-1,
beast::Journal{beast::Journal::getNullSink()});
if (maxDeletableOffers == deletedOffersCount)
break;
}
pageIndex = (*page)[~sfIndexNext];
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to move this update of pageIndex somewhere before the for loop but below the

if (!page)

My concern if that if you delete the last offer out of the directory page, then ApplyView::dirRemove() will remove the directory node:
https://github.com/XRPLF/rippled/blob/develop/src/ripple/ledger/impl/ApplyView.cpp#L300-L301

The IndexNext value that the code is currently reading may be from a directory node that has been erased. Please update your local copy before the node you are reading from is erased.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

interesting point, ill update my code. But weirdly I have never ran into this problem throughout all my time doing testing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to clarify, this situation is a problem because the view.peek() returns a shared_ptr right? If the page is merely a copy of the directory, then this wouldn't be a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you didn't see a problem, but that still doesn't mean it was a good idea.

Just to clarify, this situation is a problem because the view.peek() returns a shared_ptr right?

Yes, but it's better to take a larger view on this. For every C++ value you can either have a copy or a reference. The shared_ptr is a kind of reference. Whenever you have a reference you can't really control when the thing referred to will go away. In contrast, if you have a (local) copy, then you control all of the interactions with that value, including who has references to it.

That's a generalization. There are specifics to lifetime rules using shared_ptr to const or non-const values. But we don't need to explore those specifics here.

As it turns out, the ApplyView implementation saves up all of the deletes for the end of the transaction. That's why you were still able to read IndexNext from the page node after the call to erase. But it still was not a good idea. After the call to erase on the node, what you had in your hands is called a "stale reference". The value you want may still be there, but the implementation has every right to remove it due to some completely different software change. What you had was a latent crash. Thankfully that's fixed now.

} while (pageIndex.value_or(0) && maxDeletableOffers != deletedOffersCount);

view.erase(offer);
});
return deletedOffersCount;
}

TER
Expand Down
10 changes: 7 additions & 3 deletions src/ripple/app/tx/impl/details/NFTokenUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
int
removeTokenOffersWithLimit(
ApplyView& view,
Keylet const& directory,
int maxDeletableOffers);

/** Returns tesSUCCESS if NFToken has few enough offers that it can be burned */
TER
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/protocol/Protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ std::size_t constexpr maxDeletableDirEntries = 1000;
std::size_t constexpr maxTokenOfferCancelCount = 500;

/** The maximum number of offers in an offer directory for NFT to be burnable */
std::size_t constexpr maxDeletableTokenOfferEntries = 500;
int constexpr maxDeletableTokenOfferEntries = 500;
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved

/** The maximum token transfer fee allowed.

Expand Down
Loading