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

[shawnxie999]

Ready for Review

High Level Overview of Change

(disable whitespaces to see file comparisons better)

This PR introduces an amendment called fixUnburnableNFToken which allows NFTs with more than 500 offers to be burned. This is enabled by deleting 500 offers but leaving the others untouched.

Context of Change

Current NFT cannot be burned when it has over 500 offers. We want to remove this restriction. So we delete exactly 500 offers, and will leave out other offers untouched if there are more.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Tests (You added tests for code that already exists, or your new feature included in this PR)
• Documentation Updates
• Release

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1203706294512476

[RichardAH]

Hi @shawnxie999
You need to encapsulate your proposed changes inside a ledger amendment, otherwise changes to transactors can/will cause a ledger fork (where nodes who are yet to upgrade to the new code disagree with nodes that have upgraded.)

Have a look at https://github.com/XRPLF/rippled/pull/4336/files for an example of how to do this.

[nbougalis]

Please address the issues identified (the biggest one is the looping bug over the offer directory pages) and ping me again for a follow-up review.

Generally, you should also consider squashing things into a reasonable set of commits, especially when first opening the PR. Subsequent fixes to address concerns can be left as separate commits but marked as [FOLD] to make it easier to review.

[nbougalis]

Looks good, except for some clang-format issues. You'll want to squash this down into a single commit, prior to the Passed label being added.

[scottschurr]

I'd like to make an observation based only on the description of the pull request. I've not looked at the code yet.

The observation is a bit long-winded, and starts with how the ledger prevents duplicate NFTokens from appearing in the ledger.

Each NFToken ID incorporates the 160-bit ID of the issuing account. That account ID plus the MintedNFTokens number associated with that account are sufficient to prevent one NFT from duplicating any other. That is, unless the issuing account is deleted.

Once the issuing account is quiescent for 20 minutes or so, it can deleted. Once an account is deleted it can be recreated, but a recreated account has its account root in an initial state. So a recreated account has a MintedNFTokens value of zero. That is, if the account is recreated it can issue NFTs with identical IDs to the ones that were issued by the account that was deleted.

Specifically in order to prevent duplicate NFTokens in the ledger there is a rule that prevents an account from being deleted if it has ever issued an NFToken that has not also been burned. If an account issues no NFTokens it automatically meets that requirement. If a different account has issued 1700 NFTokens and all 1700 of those NFTokens have been burned, that account also meets the requirements.

This is where canceling all of the offers for a burned NFToken comes in. Let's suppose the behavior proposed by this pull request comes into play. Then we could see this sequence of events play out:

1. Alice creates an account.
2. Alice issues one NFToken.
3. Folks (including Alice) create 500+M offers for Alice's lone NFToken.
4. Alice burns her lone NFToken. That leaves M offers for that NFToken in the ledger, even though that NFToken no longer exists.
5. Alice waits 20 minutes and then deletes her account.
6. Alice re-creates her account.
7. Alice issues one NFToken with an ID that is identical to the one in step 2.
8. There are now M offers in the ledger for Alice's NFT, even though Alice burned the original NFT.

Also note that this scenario is only possible if the behavior proposed by this pull request comes into play. If we don't accept this pull request then there are two possible outcomes:

1. Alice is unable to burn her lone NFToken because there are too many offers for it, or
2. Alice starts out by canceling some of the offers for her lone NFToken. Once she gets the number of offers below 500, then she can burn her NFToken.

Alice can cancel as many as 500 offers for her NFToken in a single transaction. So it would take a particularly concerted effort by attackers to prevent Alice from burning her NFToken. The attacker needs to generates NFToken create offer transactions at 500 times the rate that Alice issues cancel transactions. It seems improbable that this attack would be worth it.

I don't know what usability issue prompted this pull request. But I think that usability issue needs to be balanced against the possibility of there being NFTokenOffers in the ledger for an NFToken that is not the same as the originally intended token.

The scenario is a bit convoluted. I'm happy to work through the details more closely with anyone who's curious.

Thanks for reading.

[shawnxie999]

I'd like to make an observation based only on the description of the pull request. I've not looked at the code yet.

The observation is a bit long-winded, and starts with how the ledger prevents duplicate NFTokens from appearing in the ledger.

Each NFToken ID incorporates the 160-bit ID of the issuing account. That account ID plus the MintedNFTokens number associated with that account are sufficient to prevent one NFT from duplicating any other. That is, unless the issuing account is deleted.

Once the issuing account is quiescent for 20 minutes or so, it can deleted. Once an account is deleted it can be recreated, but a recreated account has its account root in an initial state. So a recreated account has a MintedNFTokens value of zero. That is, if the account is recreated it can issue NFTs with identical IDs to the ones that were issued by the account that was deleted.

Specifically in order to prevent duplicate NFTokens in the ledger there is a rule that prevents an account from being deleted if it has ever issued an NFToken that has not also been burned. If an account issues no NFTokens it automatically meets that requirement. If a different account has issued 1700 NFTokens and all 1700 of those NFTokens have been burned, that account also meets the requirements.

This is where canceling all of the offers for a burned NFToken comes in. Let's suppose the behavior proposed by this pull request comes into play. Then we could see this sequence of events play out:

1. Alice creates an account.
2. Alice issues one NFToken.
3. Folks (including Alice) create 500+M offers for Alice's lone NFToken.
4. Alice burns her lone NFToken. That leaves M offers for that NFToken in the ledger, even though that NFToken no longer exists.
5. Alice waits 20 minutes and then deletes her account.
6. Alice re-creates her account.
7. Alice issues one NFToken with an ID that is identical to the one in step 2.
8. There are now M offers in the ledger for Alice's NFT, even though Alice burned the original NFT.

Also note that this scenario is only possible if the behavior proposed by this pull request comes into play. If we don't accept this pull request then there are two possible outcomes:

1. Alice is unable to burn her lone NFToken because there are too many offers for it, or
2. Alice starts out by canceling some of the offers for her lone NFToken. Once she gets the number of offers below 500, then she can burn her NFToken.

Alice can cancel as many as 500 offers for her NFToken in a single transaction. So it would take a particularly concerted effort by attackers to prevent Alice from burning her NFToken. The attacker needs to generates NFToken create offer transactions at 500 times the rate that Alice issues cancel transactions. It seems improbable that this attack would be worth it.

I don't know what usability issue prompted this pull request. But I think that usability issue needs to be balanced against the possibility of there being NFTokenOffers in the ledger for an NFToken that is not the same as the originally intended token.

The scenario is a bit convoluted. I'm happy to work through the details more closely with anyone who's curious.

Thanks for reading.

Thanks for the informative feedback Scott!

You said that account can be re-created once it has been deleted - so the account would be created with the same address as before? If that is the case, then when you mention

7. Alice issues one NFToken with an ID that is identical to the one in step 2.

That means we have duplicate NFTs that share the same NFTokenID (one is the newly minted, other is the burnt one) on the ledger. To me, it sounds a bit odd that this scenario was allowed to happen in the first place.

Nonetheless, I'm curious if this situation was considered when this approach was proposed, or whether it was forgotten. But I think @ledhed2222 has more context on this.

[scottschurr]

You said that account can be re-created once it has been deleted - so the account would be created with the same address as before?

That's correct. The ID of an account is derived from the secret used to create the account. If an account is deleted from the ledger then, later, an account with the same ID as the deleted account can be recreated by re-using the same secret.

That means we have duplicate NFTs that share the same NFTokenID (one is the newly minted, other is the burnt one) on the ledger. To me, it sounds a bit odd that this scenario was allowed to happen in the first place.

If I open my eyes to the ledger history I have to agree that...

1. There can be an NFT with a particular ID in the ledger history,
2. The history can show that NFT ID being burned and removed from the ledger,
3. The ledger history can show an NFT with that same ID being (re?)minted in a later ledger, and
4. That same NFT ID can be present in the current ledger.

Situations like this became possible when account deletion was enabled a few years back.

However, since an account with any NFTs in the ledger cannot be deleted, there can only be one instance of a particular NFT ID in any given current ledger.

Does that distinction make sense? I'm happy to answer further questions.

I'm curious if this situation was considered when this approach was proposed, or whether it was forgotten.

Account deletion is a little known corner of the ledger that also has strange consequences. It's easy to forget.

[shawnxie999]

@scottschurr
So let's say if this PR is merged in. What are the worst consequences that it would cause?

From my understanding, one ramification would be that it would cause confusion when a NFT is re-minted, left-over offers from the burnt NFT object are carried over to the new one. Just curious in your perspective, how serious of a problem would this scenario be?

[scottschurr]

From my understanding, one ramification would be that it would cause confusion when a NFT is re-minted, left-over offers from the burnt NFT object are carried over to the new one. Just curious in your perspective, how serious of a problem would this scenario be?

@shawnxie999, my opinion on the severity of the problem is irrelevant. Different observers will have different opinions on how serious the matter is. However consider this:

1. Alice mints an NFToken with a specific URL.
2. Bill (amongst over 500 others) makes an offer to buy Alice's NFToken.
3. Alice burns her NFToken, but Bill's offer is left in the ledger.
4. Alice delete's her account.
5. Alice recreate's her account.
6. Alice re-mints her NFToken, giving it the same ID but a different URL.
7. Alice accepts Bill's offer for the NFToken.

Note that the NFToken that Alice re-mints has exactly the same ID as the original one, but the associated URL is different. So, from a legitimate perspective, even though the ID is the same, the NFToken itself is different.

How does Bill feel about having paid for an NFT that is not the same as the one he created the offer for? That depends on Bill. Given the folks who populate the crypto world I'd guess that there's a pretty good likelihood that Bill would be royally torqued once he found out.

So my opinion doesn't matter here. The question is how a user of the ledger would feel. That's above my pay grade.

[intelliot]

Trivial: Let's consider naming this amendment NonFungibleTokensV1_2 because additional fixes will be included in it. It's not semver or anything, but at least it makes sense.

This does not block merging or reviewing - please proceed as before. The name will be finalized later, before feature/nft-fixes gets merged into develop.

[scottschurr]

I left one comment that requires no action. I think this code still meets its intended purpose.