Introduce fixNFTokenDirV1 amendment:

[scottschurr]

High Level Overview of Change

• Fixes an off-by-one when determining which NFTokenPage an NFToken belongs on.
• Improves handling of packed sets of 32 NFTs with identical low 96-bits.
• Fixes marker handling by the account_nfts RPC command.
• Tightens constraints of NFTokenPage invariant checks.

Also adds unit tests to exercise the fixed cases as well as tests for previously untested functionality.

Context of Change

Certain capabilities for testing NFTokenPage functionality became available after the release of 1.9.0. Those test capabilities showed problems in the existing NFTokenPage functionality in specific corner cases. The fixNFTokenDirV1 amendment addresses those new corner cases that were found.

Without the fixNFTokenDirV1 amendment the corner cases would (typically) fail with a tecINVARIANT_FAILED. With the amendment the cases either work as hoped or produce a different error, like tecNO_SUITABLE_TOKEN_PAGE.

Type of Change

• Amendment (Functionality waits for amendment to pass. Once passed may cause amendment blocking.)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Tests (You added tests for code that already exists, or your new feature included in this PR)

[thejohnfreeman]

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.

[scottschurr]

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

[intelliot]

Could you compare the expected behavior in these two hypothetical cases - i.e. what would happen?

1. fixNFTokenDirV1 activates prior to NonFungibleTokensV1
2. NonFungibleTokensV1 activates prior to fixNFTokenDirV1

[Ref #4101]

[scottschurr]

Great question @intelliot.

Could you compare the expected behavior in these two hypothetical cases - i.e. what would happen?

Sure. Let's look at case 2 first:

2. NonFungibleTokensV1 activates prior to fixNFTokenDirV1

First of all, the situations that are covered by fixNFTokenDirV1 are unusual corner cases. So, probably, fixNFTokenDirV1 is not protecting the ledger from anything that will happen in the wild unless the situation is carefully engineered. However there are two distinct problems that have been observed.

Problem 1: Depending on the numeric values of the NFTokenIDs that an account already owns, transactions that attempt to mint or acquire NFTokens with specific unusual numeric relationships to the NFTokens the account already owns may fail with a tecINVARIANT_FAILED code.

Once the fix is applied then these transactions will succeed.

Problem 2: By carefully selecting which NFTokenIDs an account owns, the account can block itself from owning more NFTokens. This requires that the account own at least 32 NFTokens, all 32 of which must be carefully chosen. If this happens then attempting to mint or acquire any NFToken will result in a tecNO_SUITABLE_NFTOKEN_PAGE transaction failure.

Once the fix is applied then this blockage will not occur.

If we have case 1 ...

1. fixNFTokenDirV1 activates prior to NonFungibleTokensV1

then none of the problems described above will occur.

Clearly it's preferable to have the fix go live before the amendment. And since the folks running the validators have the best interest of the ledger at heart I anticipate they'll accomplish that. But if the reverse order occurs it's not the end of the world because the invariant checker will keep bad things from becoming enshrined in the ledger.