fixNFTokenRemint: prevent NFT re-mint: (#4406)

Without the protocol amendment introduced by this commit, an NFT ID can
be reminted in this manner:

1. Alice creates an account and mints an NFT.
2. Alice burns the NFT with an `NFTokenBurn` transaction.
3. Alice deletes her account with an `AccountDelete` transaction.
4. Alice re-creates her account.
5. Alice mints an NFT with an `NFTokenMint` transaction with params:
   `NFTokenTaxon` = 0, `Flags` = 9).

This will mint a NFT with the same `NFTokenID` as the one minted in step
1. The params that construct the NFT ID will cause a collision in
`NFTokenID` if their values are equal before and after the remint.

With the `fixNFTokenRemint` amendment, there is a new sequence number
construct which avoids this scenario:

- A new `AccountRoot` field, `FirstNFTSequence`, stays constant over
  time.
  - This field is set to the current account sequence when the account
    issues their first NFT.
  - Otherwise, it is not set.
- The sequence of a newly-minted NFT is computed by: `FirstNFTSequence +
  MintedNFTokens`.
  - `MintedNFTokens` is then incremented by 1 for each mint.

Furthermore, there is a new account deletion restriction:

- An account can only be deleted if `FirstNFTSequence + MintedNFTokens +
  256` is less than the current ledger sequence.
  - 256 was chosen because it already exists in the current account
    deletion constraint.

Without this restriction, an NFT may still be remintable. Example
scenario:

1. Alice's account sequence is at 1.
2. Bob is Alice's authorized minter.
3. Bob mints 500 NFTs for Alice. The NFTs will have sequences 1-501, as
   NFT sequence is computed by `FirstNFTokenSequence + MintedNFTokens`).
4. Alice deletes her account at ledger 257 (as required by the existing
   `AccountDelete` amendment).
5. Alice re-creates her account at ledger 258.
6. Alice mints an NFT. `FirstNFTokenSequence` initializes to her account
   sequence (258), and `MintedNFTokens` initializes as 0. This
   newly-minted NFT would have a sequence number of 258, which is a
   duplicate of what she issued through authorized minting before she
   deleted her account.

---------

Signed-off-by: Shawn Xie <shawnxie920@gmail.com>

src/ripple/app/tx/impl/DeleteAccount.cpp

@@ -214,6 +214,23 @@ DeleteAccount::preclaim(PreclaimContext const& ctx)
     if ((*sleAccount)[sfSequence] + seqDelta > ctx.view.seq())
         return tecTOO_SOON;
 
+    // When fixNFTokenRemint is enabled, we don't allow an account to be
+    // deleted if <FirstNFTokenSequence + MintedNFTokens> is within 256 of the
+    // current ledger. This is to prevent having duplicate NFTokenIDs after
+    // account re-creation.
+    //
+    // Without this restriction, duplicate NFTokenIDs can be reproduced when
+    // authorized minting is involved. Because when the minter mints a NFToken,
+    // the issuer's sequence does not change. So when the issuer re-creates
+    // their account and mints a NFToken, it is possible that the
+    // NFTokenSequence of this NFToken is the same as the one that the
+    // authorized minter minted in a previous ledger.
+    if (ctx.view.rules().enabled(fixNFTokenRemint) &&
+        ((*sleAccount)[~sfFirstNFTokenSequence].value_or(0) +
+             (*sleAccount)[~sfMintedNFTokens].value_or(0) + seqDelta >
+         ctx.view.seq()))
+        return tecTOO_SOON;
+
     // Verify that the account does not own any objects that would prevent
     // the account from being deleted.
     Keylet const ownerDirKeylet{keylet::ownerDir(account)};

src/ripple/app/tx/impl/NFTokenMint.cpp

@@ -160,15 +160,66 @@ NFTokenMint::doApply()
             // Should not happen.  Checked in preclaim.
             return Unexpected(tecNO_ISSUER);
 
-        // Get the unique sequence number for this token:
+        if (!ctx_.view().rules().enabled(fixNFTokenRemint))
-        std::uint32_t const tokenSeq = (*root)[~sfMintedNFTokens].value_or(0);
         {
-            std::uint32_t const nextTokenSeq = tokenSeq + 1;
+            // Get the unique sequence number for this token:
-            if (nextTokenSeq < tokenSeq)
+            std::uint32_t const tokenSeq =
-                return Unexpected(tecMAX_SEQUENCE_REACHED);
+                (*root)[~sfMintedNFTokens].value_or(0);
+            {
+                std::uint32_t const nextTokenSeq = tokenSeq + 1;
+                if (nextTokenSeq < tokenSeq)
+                    return Unexpected(tecMAX_SEQUENCE_REACHED);
 
-            (*root)[sfMintedNFTokens] = nextTokenSeq;
+                (*root)[sfMintedNFTokens] = nextTokenSeq;
+            }
+            ctx_.view().update(root);
+            return tokenSeq;
         }
+
+        // With fixNFTokenRemint amendment enabled:
+        //
+        // If the issuer hasn't minted an NFToken before we must add a
+        // FirstNFTokenSequence field to the issuer's AccountRoot.  The
+        // value of the FirstNFTokenSequence must equal the issuer's
+        // current account sequence.
+        //
+        // There are three situations:
+        //  o If the first token is being minted by the issuer and
+        //     * If the transaction consumes a Sequence number, then the
+        //       Sequence has been pre-incremented by the time we get here in
+        //       doApply.  We must decrement the value in the Sequence field.
+        //     * Otherwise the transaction uses a Ticket so the Sequence has
+        //       not been pre-incremented.  We use the Sequence value as is.
+        //  o The first token is being minted by an authorized minter.  In
+        //    this case the issuer's Sequence field has been left untouched.
+        //    We use the issuer's Sequence value as is.
+        if (!root->isFieldPresent(sfFirstNFTokenSequence))
+        {
+            std::uint32_t const acctSeq = root->at(sfSequence);
+
+            root->at(sfFirstNFTokenSequence) =
+                ctx_.tx.isFieldPresent(sfIssuer) ||
+                    ctx_.tx.getSeqProxy().isTicket()
+                ? acctSeq
+                : acctSeq - 1;
+        }
+
+        std::uint32_t const mintedNftCnt =
+            (*root)[~sfMintedNFTokens].value_or(0u);
+
+        (*root)[sfMintedNFTokens] = mintedNftCnt + 1u;
+        if ((*root)[sfMintedNFTokens] == 0u)
+            return Unexpected(tecMAX_SEQUENCE_REACHED);
+
+        // Get the unique sequence number of this token by
+        // sfFirstNFTokenSequence + sfMintedNFTokens
+        std::uint32_t const offset = (*root)[sfFirstNFTokenSequence];
+        std::uint32_t const tokenSeq = offset + mintedNftCnt;
+
+        // Check for more overflow cases
+        if (tokenSeq + 1u == 0u || tokenSeq < offset)
+            return Unexpected(tecMAX_SEQUENCE_REACHED);
+
         ctx_.view().update(root);
         return tokenSeq;
     }();

src/ripple/protocol/Feature.h

@@ -74,7 +74,7 @@ namespace detail {
 // Feature.cpp. Because it's only used to reserve storage, and determine how
 // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
 // the actual number of amendments. A LogicError on startup will verify this.
-static constexpr std::size_t numFeatures = 57;
+static constexpr std::size_t numFeatures = 58;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -344,6 +344,7 @@ extern uint256 const featureDisallowIncoming;
 extern uint256 const featureXRPFees;
 extern uint256 const fixUniversalNumber;
 extern uint256 const fixNonFungibleTokensV1_2;
+extern uint256 const fixNFTokenRemint;
 
 }  // namespace ripple
 

src/ripple/protocol/SField.h

@@ -400,6 +400,7 @@ extern SF_UINT32 const sfMintedNFTokens;
 extern SF_UINT32 const sfBurnedNFTokens;
 extern SF_UINT32 const sfHookStateCount;
 extern SF_UINT32 const sfEmitGeneration;
+extern SF_UINT32 const sfFirstNFTokenSequence;
 
 // 64-bit integers (common)
 extern SF_UINT64 const sfIndexNext;

src/ripple/protocol/impl/Feature.cpp

@@ -454,6 +454,7 @@ REGISTER_FEATURE(DisallowIncoming,              Supported::yes, DefaultVote::no)
 REGISTER_FEATURE(XRPFees,                       Supported::yes, DefaultVote::no);
 REGISTER_FIX    (fixUniversalNumber,            Supported::yes, DefaultVote::no);
 REGISTER_FIX    (fixNonFungibleTokensV1_2,      Supported::yes, DefaultVote::no);
+REGISTER_FIX    (fixNFTokenRemint,              Supported::yes, DefaultVote::no);
 
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.

src/ripple/protocol/impl/LedgerFormats.cpp

@@ -55,6 +55,7 @@ LedgerFormats::LedgerFormats()
             {sfNFTokenMinter,        soeOPTIONAL},
             {sfMintedNFTokens,       soeDEFAULT},
             {sfBurnedNFTokens,       soeDEFAULT},
+            {sfFirstNFTokenSequence, soeOPTIONAL},
         },
         commonFields);
 

src/ripple/protocol/impl/SField.cpp

@@ -150,6 +150,9 @@ CONSTRUCT_TYPED_SFIELD(sfMintedNFTokens,        "MintedNFTokens",       UINT32,
 CONSTRUCT_TYPED_SFIELD(sfBurnedNFTokens,        "BurnedNFTokens",       UINT32,    44);
 CONSTRUCT_TYPED_SFIELD(sfHookStateCount,        "HookStateCount",       UINT32,    45);
 CONSTRUCT_TYPED_SFIELD(sfEmitGeneration,        "EmitGeneration",       UINT32,    46);
+// Three field values of 47, 48 and 49 are reserved for 
+// LockCount(Hooks), VoteWeight(AMM), DiscountedFee(AMM)
+CONSTRUCT_TYPED_SFIELD(sfFirstNFTokenSequence,  "FirstNFTokenSequence", UINT32,    50);
 
 // 64-bit integers (common)
 CONSTRUCT_TYPED_SFIELD(sfIndexNext,             "IndexNext",            UINT64,     1);

src/test/app/NFTokenBurn_test.cpp

@@ -380,8 +380,16 @@ class NFTokenBurn_test : public beast::unit_test::suite
             auto internalTaxon = [&env](
                                      Account const& acct,
                                      std::uint32_t taxon) -> std::uint32_t {
-                std::uint32_t const tokenSeq = {
+                std::uint32_t tokenSeq =
-                    env.le(acct)->at(~sfMintedNFTokens).value_or(0)};
+                    env.le(acct)->at(~sfMintedNFTokens).value_or(0);
+
+                // If fixNFTokenRemint amendment is on, we must
+                // add FirstNFTokenSequence.
+                if (env.current()->rules().enabled(fixNFTokenRemint))
+                    tokenSeq += env.le(acct)
+                                    ->at(~sfFirstNFTokenSequence)
+                                    .value_or(env.seq(acct));
+
                 return toUInt32(
                     nft::cipheredTaxon(tokenSeq, nft::toTaxon(taxon)));
             };
@@ -786,8 +794,10 @@ public:
         FeatureBitset const all{supported_amendments()};
         FeatureBitset const fixNFTDir{fixNFTokenDirV1};
 
-        testWithFeats(all - fixNonFungibleTokensV1_2 - fixNFTDir);
+        testWithFeats(
-        testWithFeats(all - fixNonFungibleTokensV1_2);
+            all - fixNonFungibleTokensV1_2 - fixNFTDir - fixNFTokenRemint);
+        testWithFeats(all - fixNonFungibleTokensV1_2 - fixNFTokenRemint);
+        testWithFeats(all - fixNFTokenRemint);
         testWithFeats(all);
     }
 };

src/test/app/NFTokenDir_test.cpp

@@ -190,8 +190,14 @@ class NFTokenDir_test : public beast::unit_test::suite
                     Account const& account = accounts.emplace_back(
                         Account::base58Seed, std::string(seed));
                     env.fund(XRP(10000), account);
-                    env.close();
+
+                    // Do not close the ledger inside the loop.  If
+                    // fixNFTokenRemint is enabled and accounts are initialized
+                    // at different ledgers, they will have different account
+                    // sequences.  That would cause the accounts to have
+                    // different NFTokenID sequence numbers.
                 }
+                env.close();
 
                 // All of the accounts create one NFT and and offer that NFT to
                 // buyer.
@@ -408,8 +414,14 @@ class NFTokenDir_test : public beast::unit_test::suite
                     Account const& account = accounts.emplace_back(
                         Account::base58Seed, std::string(seed));
                     env.fund(XRP(10000), account);
-                    env.close();
+
+                    // Do not close the ledger inside the loop.  If
+                    // fixNFTokenRemint is enabled and accounts are initialized
+                    // at different ledgers, they will have different account
+                    // sequences.  That would cause the accounts to have
+                    // different NFTokenID sequence numbers.
                 }
+                env.close();
 
                 // All of the accounts create one NFT and and offer that NFT to
                 // buyer.
@@ -652,8 +664,14 @@ class NFTokenDir_test : public beast::unit_test::suite
             Account const& account =
                 accounts.emplace_back(Account::base58Seed, std::string(seed));
             env.fund(XRP(10000), account);
-            env.close();
+
+            // Do not close the ledger inside the loop.  If
+            // fixNFTokenRemint is enabled and accounts are initialized
+            // at different ledgers, they will have different account
+            // sequences.  That would cause the accounts to have
+            // different NFTokenID sequence numbers.
         }
+        env.close();
 
         // All of the accounts create one NFT and and offer that NFT to buyer.
         std::vector<uint256> nftIDs;
@@ -827,8 +845,14 @@ class NFTokenDir_test : public beast::unit_test::suite
             Account const& account =
                 accounts.emplace_back(Account::base58Seed, std::string(seed));
             env.fund(XRP(10000), account);
-            env.close();
+
+            // Do not close the ledger inside the loop.  If
+            // fixNFTokenRemint is enabled and accounts are initialized
+            // at different ledgers, they will have different account
+            // sequences.  That would cause the accounts to have
+            // different NFTokenID sequence numbers.
         }
+        env.close();
 
         // All of the accounts create seven consecutive NFTs and and offer
         // those NFTs to buyer.
@@ -1078,7 +1102,8 @@ public:
         FeatureBitset const fixNFTDir{
             fixNFTokenDirV1, featureNonFungibleTokensV1_1};
 
-        testWithFeats(all - fixNFTDir);
+        testWithFeats(all - fixNFTDir - fixNFTokenRemint);
+        testWithFeats(all - fixNFTokenRemint);
         testWithFeats(all);
     }
 };

src/test/app/NFToken_test.cpp

@@ -215,8 +215,8 @@ class NFToken_test : public beast::unit_test::suite
         Account const minter{"minter"};
 
         // Fund alice and minter enough to exist, but not enough to meet
-        // the reserve for creating their first NFT.  Account reserve for unit
+        // the reserve for creating their first NFT.  Account reserve for
-        // tests is 200 XRP, not 20.
+        // unit tests is 200 XRP, not 20.
         env.fund(XRP(200), alice, minter);
         env.close();
         BEAST_EXPECT(env.balance(alice) == XRP(200));
@@ -224,7 +224,8 @@ class NFToken_test : public beast::unit_test::suite
         BEAST_EXPECT(ownerCount(env, alice) == 0);
         BEAST_EXPECT(ownerCount(env, minter) == 0);
 
-        // alice does not have enough XRP to cover the reserve for an NFT page.
+        // alice does not have enough XRP to cover the reserve for an NFT
+        // page.
         env(token::mint(alice, 0u), ter(tecINSUFFICIENT_RESERVE));
         env.close();
         BEAST_EXPECT(ownerCount(env, alice) == 0);
@@ -260,7 +261,8 @@ class NFToken_test : public beast::unit_test::suite
             oneCheck("burned", burnedCount(env, alice), burned);
         };
 
-        // alice still does not have enough XRP for the reserve of an NFT page.
+        // alice still does not have enough XRP for the reserve of an NFT
+        // page.
         env(token::mint(alice, 0u), ter(tecINSUFFICIENT_RESERVE));
         env.close();
         checkAliceOwnerMintedBurned(0, 0, 0, __LINE__);
@@ -292,7 +294,8 @@ class NFToken_test : public beast::unit_test::suite
         env(pay(env.master, alice, XRP(50) + drops(329)));
         env.close();
 
-        // alice still does not have enough XRP for the reserve of an NFT page.
+        // alice still does not have enough XRP for the reserve of an NFT
+        // page.
         env(token::mint(alice), ter(tecINSUFFICIENT_RESERVE));
         env.close();
         checkAliceOwnerMintedBurned(1, 32, 0, __LINE__);
@@ -311,18 +314,20 @@ class NFToken_test : public beast::unit_test::suite
 
         while (seq < 33)
         {
-            env(token::burn(alice, token::getID(alice, 0, seq++)));
+            env(token::burn(alice, token::getID(env, alice, 0, seq++)));
             env.close();
             checkAliceOwnerMintedBurned((33 - seq) ? 1 : 0, 33, seq, __LINE__);
         }
 
         // alice burns a non-existent NFT.
-        env(token::burn(alice, token::getID(alice, 197, 5)), ter(tecNO_ENTRY));
+        env(token::burn(alice, token::getID(env, alice, 197, 5)),
+            ter(tecNO_ENTRY));
         env.close();
         checkAliceOwnerMintedBurned(0, 33, 33, __LINE__);
 
-        // That was fun!  Now let's see what happens when we let someone else
+        // That was fun!  Now let's see what happens when we let someone
-        // mint NFTs on alice's behalf.  alice gives permission to minter.
+        // else mint NFTs on alice's behalf.  alice gives permission to
+        // minter.
         env(token::setMinter(alice, minter));
         env.close();
         BEAST_EXPECT(
@@ -373,9 +378,9 @@ class NFToken_test : public beast::unit_test::suite
         env.close();
         checkMintersOwnerMintedBurned(0, 33, nftSeq, 0, 0, 0, __LINE__);
 
-        // minter still does not have enough XRP for the reserve of an NFT page.
+        // minter still does not have enough XRP for the reserve of an NFT
-        // Just for grins (and code coverage), minter mints NFTs that include
+        // page. Just for grins (and code coverage), minter mints NFTs that
-        // a URI.
+        // include a URI.
         env(token::mint(minter),
             token::issuer(alice),
             token::uri("uri"),
@@ -400,7 +405,8 @@ class NFToken_test : public beast::unit_test::suite
             checkMintersOwnerMintedBurned(0, i + 34, nftSeq, 1, 0, 0, __LINE__);
         }
 
-        // Pay minter almost enough for the reserve of an additional NFT page.
+        // Pay minter almost enough for the reserve of an additional NFT
+        // page.
         env(pay(env.master, minter, XRP(50) + drops(319)));
         env.close();
 
@@ -425,19 +431,20 @@ class NFToken_test : public beast::unit_test::suite
         // minter burns the NFTs she created.
         while (nftSeq < 65)
         {
-            env(token::burn(minter, token::getID(alice, 0, nftSeq++)));
+            env(token::burn(minter, token::getID(env, alice, 0, nftSeq++)));
             env.close();
             checkMintersOwnerMintedBurned(
                 0, 66, nftSeq, (65 - seq) ? 1 : 0, 0, 0, __LINE__);
         }
 
-        // minter has one more NFT to burn.  Should take her owner count to 0.
+        // minter has one more NFT to burn.  Should take her owner count to
-        env(token::burn(minter, token::getID(alice, 0, nftSeq++)));
+        // 0.
+        env(token::burn(minter, token::getID(env, alice, 0, nftSeq++)));
         env.close();
         checkMintersOwnerMintedBurned(0, 66, nftSeq, 0, 0, 0, __LINE__);
 
         // minter burns a non-existent NFT.
-        env(token::burn(minter, token::getID(alice, 2009, 3)),
+        env(token::burn(minter, token::getID(env, alice, 2009, 3)),
             ter(tecNO_ENTRY));
         env.close();
         checkMintersOwnerMintedBurned(0, 66, nftSeq, 0, 0, 0, __LINE__);
@@ -475,7 +482,7 @@ class NFToken_test : public beast::unit_test::suite
         // checks with this modify() call.  If you call close() between
         // here and the end of the test all the effort will be lost.
         env.app().openLedger().modify(
-            [&alice](OpenView& view, beast::Journal j) {
+            [&alice, &env](OpenView& view, beast::Journal j) {
                 // Get the account root we want to hijack.
                 auto const sle = view.read(keylet::account(alice.id()));
                 if (!sle)
@@ -487,8 +494,23 @@ class NFToken_test : public beast::unit_test::suite
                 if (replacement->getFieldU32(sfMintedNFTokens) != 1)
                     return false;  // Unexpected test conditions.
 
-                // Now replace sfMintedNFTokens with the largest valid value.
+                if (env.current()->rules().enabled(fixNFTokenRemint))
-                (*replacement)[sfMintedNFTokens] = 0xFFFF'FFFE;
+                {
+                    // If fixNFTokenRemint is enabled, sequence number is
+                    // generated by sfFirstNFTokenSequence + sfMintedNFTokens.
+                    // We can replace the two fields with any numbers as long as
+                    // they add up to the largest valid number. In our case,
+                    // sfFirstNFTokenSequence is set to the largest valid
+                    // number, and sfMintedNFTokens is set to zero.
+                    (*replacement)[sfFirstNFTokenSequence] = 0xFFFF'FFFE;
+                    (*replacement)[sfMintedNFTokens] = 0x0000'0000;
+                }
+                else
+                {
+                    // Now replace sfMintedNFTokens with the largest valid
+                    // value.
+                    (*replacement)[sfMintedNFTokens] = 0xFFFF'FFFE;
+                }
                 view.rawReplace(replacement);
                 return true;
             });
@@ -623,7 +645,8 @@ class NFToken_test : public beast::unit_test::suite
         // preclaim
 
         // Try to burn a token that doesn't exist.
-        env(token::burn(alice, token::getID(alice, 0, 1)), ter(tecNO_ENTRY));
+        env(token::burn(alice, token::getID(env, alice, 0, 1)),
+            ter(tecNO_ENTRY));
         env.close();
         BEAST_EXPECT(ownerCount(env, buyer) == 0);
 
@@ -769,14 +792,16 @@ class NFToken_test : public beast::unit_test::suite
         BEAST_EXPECT(ownerCount(env, buyer) == 0);
 
         // The nftID must be present in the ledger.
-        env(token::createOffer(buyer, token::getID(alice, 0, 1), XRP(1000)),
+        env(token::createOffer(
+                buyer, token::getID(env, alice, 0, 1), XRP(1000)),
             token::owner(alice),
             ter(tecNO_ENTRY));
         env.close();
         BEAST_EXPECT(ownerCount(env, buyer) == 0);
 
         // The nftID must be present in the ledger of a sell offer too.
-        env(token::createOffer(alice, token::getID(alice, 0, 1), XRP(1000)),
+        env(token::createOffer(
+                alice, token::getID(env, alice, 0, 1), XRP(1000)),
             txflags(tfSellNFToken),
             ter(tecNO_ENTRY));
         env.close();
@@ -2554,6 +2579,7 @@ class NFToken_test : public beast::unit_test::suite
                 };
 
                 uint256 const nftAliceID = token::getID(
+                    env,
                     alice,
                     taxon,
                     rand_int<std::uint32_t>(),
@@ -2562,6 +2588,7 @@ class NFToken_test : public beast::unit_test::suite
                 check(taxon, nftAliceID);
 
                 uint256 const nftBeckyID = token::getID(
+                    env,
                     becky,
                     taxon,
                     rand_int<std::uint32_t>(),
@@ -6038,6 +6065,506 @@ class NFToken_test : public beast::unit_test::suite
         }
     }
 
+    void
+    testFixNFTokenRemint(FeatureBitset features)
+    {
+        using namespace test::jtx;
+
+        testcase("fixNFTokenRemint");
+
+        // Returns the current ledger sequence
+        auto openLedgerSeq = [](Env& env) { return env.current()->seq(); };
+
+        // Close the ledger until the ledger sequence is large enough to delete
+        // the account (no longer within <Sequence + 256>)
+        // This is enforced by the featureDeletableAccounts amendment
+        auto incLgrSeqForAcctDel = [&](Env& env, Account const& acct) {
+            int const delta = [&]() -> int {
+                if (env.seq(acct) + 255 > openLedgerSeq(env))
+                    return env.seq(acct) - openLedgerSeq(env) + 255;
+                return 0;
+            }();
+            BEAST_EXPECT(delta >= 0);
+            for (int i = 0; i < delta; ++i)
+                env.close();
+            BEAST_EXPECT(openLedgerSeq(env) == env.seq(acct) + 255);
+        };
+
+        // Close the ledger until the ledger sequence is no longer
+        // within <FirstNFTokenSequence + MintedNFTokens + 256>.
+        // This is enforced by the fixNFTokenRemint amendment.
+        auto incLgrSeqForFixNftRemint = [&](Env& env, Account const& acct) {
+            int delta = 0;
+            auto const deletableLgrSeq =
+                (*env.le(acct))[~sfFirstNFTokenSequence].value_or(0) +
+                (*env.le(acct))[sfMintedNFTokens] + 255;
+
+            if (deletableLgrSeq > openLedgerSeq(env))
+                delta = deletableLgrSeq - openLedgerSeq(env);
+
+            BEAST_EXPECT(delta >= 0);
+            for (int i = 0; i < delta; ++i)
+                env.close();
+            BEAST_EXPECT(openLedgerSeq(env) == deletableLgrSeq);
+        };
+
+        // We check if NFTokenIDs can be duplicated by
+        // re-creation of an account
+        {
+            Env env{*this, features};
+            Account const alice("alice");
+            Account const becky("becky");
+
+            env.fund(XRP(10000), alice, becky);
+            env.close();
+
+            // alice mint and burn a NFT
+            uint256 const prevNFTokenID = token::getNextID(env, alice, 0u);
+            env(token::mint(alice));
+            env.close();
+            env(token::burn(alice, prevNFTokenID));
+            env.close();
+
+            // alice has minted 1 NFToken
+            BEAST_EXPECT((*env.le(alice))[sfMintedNFTokens] == 1);
+
+            // Close enough ledgers to delete alice's account
+            incLgrSeqForAcctDel(env, alice);
+
+            // alice's account is deleted
+            Keylet const aliceAcctKey{keylet::account(alice.id())};
+            auto const acctDelFee{drops(env.current()->fees().increment)};
+            env(acctdelete(alice, becky), fee(acctDelFee));
+            env.close();
+
+            // alice's account root is gone from the most recently
+            // closed ledger and the current ledger.
+            BEAST_EXPECT(!env.closed()->exists(aliceAcctKey));
+            BEAST_EXPECT(!env.current()->exists(aliceAcctKey));
+
+            // Fund alice to re-create her account
+            env.fund(XRP(10000), alice);
+            env.close();
+
+            // alice's account now exists and has minted 0 NFTokens
+            BEAST_EXPECT(env.closed()->exists(aliceAcctKey));
+            BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+            BEAST_EXPECT((*env.le(alice))[sfMintedNFTokens] == 0);
+
+            // alice mints a NFT with same params as prevNFTokenID
+            uint256 const remintNFTokenID = token::getNextID(env, alice, 0u);
+            env(token::mint(alice));
+            env.close();
+
+            // burn the NFT to make sure alice owns remintNFTokenID
+            env(token::burn(alice, remintNFTokenID));
+            env.close();
+
+            if (features[fixNFTokenRemint])
+                // Check that two NFTs don't have the same ID
+                BEAST_EXPECT(remintNFTokenID != prevNFTokenID);
+            else
+                // Check that two NFTs have the same ID
+                BEAST_EXPECT(remintNFTokenID == prevNFTokenID);
+        }
+
+        // Test if the issuer account can be deleted after an authorized
+        // minter mints and burns a batch of NFTokens.
+        {
+            Env env{*this, features};
+            Account const alice("alice");
+            Account const becky("becky");
+            Account const minter{"minter"};
+
+            env.fund(XRP(10000), alice, becky, minter);
+            env.close();
+
+            // alice sets minter as her authorized minter
+            env(token::setMinter(alice, minter));
+            env.close();
+
+            // minter mints 500 NFTs for alice
+            std::vector<uint256> nftIDs;
+            nftIDs.reserve(500);
+            for (int i = 0; i < 500; i++)
+            {
+                uint256 const nftokenID = token::getNextID(env, alice, 0u);
+                nftIDs.push_back(nftokenID);
+                env(token::mint(minter), token::issuer(alice));
+            }
+            env.close();
+
+            // minter burns 500 NFTs
+            for (auto const nftokenID : nftIDs)
+            {
+                env(token::burn(minter, nftokenID));
+            }
+            env.close();
+
+            // Increment ledger sequence to the number that is
+            // enforced by the featureDeletableAccounts amendment
+            incLgrSeqForAcctDel(env, alice);
+
+            // Verify that alice's account root is present.
+            Keylet const aliceAcctKey{keylet::account(alice.id())};
+            BEAST_EXPECT(env.closed()->exists(aliceAcctKey));
+            BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+
+            auto const acctDelFee{drops(env.current()->fees().increment)};
+
+            if (!features[fixNFTokenRemint])
+            {
+                // alice's account can be successfully deleted.
+                env(acctdelete(alice, becky), fee(acctDelFee));
+                env.close();
+                BEAST_EXPECT(!env.current()->exists(aliceAcctKey));
+
+                // Fund alice to re-create her account
+                env.fund(XRP(10000), alice);
+                env.close();
+
+                // alice's account now exists and has minted 0 NFTokens
+                BEAST_EXPECT(env.closed()->exists(aliceAcctKey));
+                BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+                BEAST_EXPECT((*env.le(alice))[sfMintedNFTokens] == 0);
+
+                // alice mints a NFT with same params as the first one before
+                // the account delete.
+                uint256 const remintNFTokenID =
+                    token::getNextID(env, alice, 0u);
+                env(token::mint(alice));
+                env.close();
+
+                // burn the NFT to make sure alice owns remintNFTokenID
+                env(token::burn(alice, remintNFTokenID));
+                env.close();
+
+                // The new NFT minted has the same ID as one of the NFTs
+                // authorized minter minted for alice
+                BEAST_EXPECT(
+                    std::find(nftIDs.begin(), nftIDs.end(), remintNFTokenID) !=
+                    nftIDs.end());
+            }
+            else if (features[fixNFTokenRemint])
+            {
+                // alice tries to delete her account, but is unsuccessful.
+                // Due to authorized minting, alice's account sequence does not
+                // advance while minter mints NFTokens for her.
+                // The new account deletion retriction <FirstNFTokenSequence +
+                // MintedNFTokens + 256> enabled by this amendment will enforce
+                // alice to wait for more ledgers to close before she can
+                // delete her account, to prevent duplicate NFTokenIDs
+                env(acctdelete(alice, becky),
+                    fee(acctDelFee),
+                    ter(tecTOO_SOON));
+                env.close();
+
+                // alice's account is still present
+                BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+
+                // Close more ledgers until it is no longer within
+                // <FirstNFTokenSequence + MintedNFTokens + 256>
+                // to be able to delete alice's account
+                incLgrSeqForFixNftRemint(env, alice);
+
+                // alice's account is deleted
+                env(acctdelete(alice, becky), fee(acctDelFee));
+                env.close();
+
+                // alice's account root is gone from the most recently
+                // closed ledger and the current ledger.
+                BEAST_EXPECT(!env.closed()->exists(aliceAcctKey));
+                BEAST_EXPECT(!env.current()->exists(aliceAcctKey));
+
+                // Fund alice to re-create her account
+                env.fund(XRP(10000), alice);
+                env.close();
+
+                // alice's account now exists and has minted 0 NFTokens
+                BEAST_EXPECT(env.closed()->exists(aliceAcctKey));
+                BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+                BEAST_EXPECT((*env.le(alice))[sfMintedNFTokens] == 0);
+
+                // alice mints a NFT with same params as the first one before
+                // the account delete.
+                uint256 const remintNFTokenID =
+                    token::getNextID(env, alice, 0u);
+                env(token::mint(alice));
+                env.close();
+
+                // burn the NFT to make sure alice owns remintNFTokenID
+                env(token::burn(alice, remintNFTokenID));
+                env.close();
+
+                // The new NFT minted will not have the same ID
+                // as any of the NFTs authorized minter minted
+                BEAST_EXPECT(
+                    std::find(nftIDs.begin(), nftIDs.end(), remintNFTokenID) ==
+                    nftIDs.end());
+            }
+        }
+
+        // When an account mints and burns a batch of NFTokens using tickets,
+        // see if the the account can be deleted.
+        {
+            Env env{*this, features};
+
+            Account const alice{"alice"};
+            Account const becky{"becky"};
+            env.fund(XRP(10000), alice, becky);
+            env.close();
+
+            // alice grab enough tickets for all of the following
+            // transactions.  Note that once the tickets are acquired alice's
+            // account sequence number should not advance.
+            std::uint32_t aliceTicketSeq{env.seq(alice) + 1};
+            env(ticket::create(alice, 100));
+            env.close();
+
+            BEAST_EXPECT(ticketCount(env, alice) == 100);
+            BEAST_EXPECT(ownerCount(env, alice) == 100);
+
+            // alice mints 50 NFTs using tickets
+            std::vector<uint256> nftIDs;
+            nftIDs.reserve(50);
+            for (int i = 0; i < 50; i++)
+            {
+                nftIDs.push_back(token::getNextID(env, alice, 0u));
+                env(token::mint(alice, 0u), ticket::use(aliceTicketSeq++));
+                env.close();
+            }
+
+            // alice burns 50 NFTs using tickets
+            for (auto const nftokenID : nftIDs)
+            {
+                env(token::burn(alice, nftokenID),
+                    ticket::use(aliceTicketSeq++));
+            }
+            env.close();
+
+            BEAST_EXPECT(ticketCount(env, alice) == 0);
+
+            // Increment ledger sequence to the number that is
+            // enforced by the featureDeletableAccounts amendment
+            incLgrSeqForAcctDel(env, alice);
+
+            // Verify that alice's account root is present.
+            Keylet const aliceAcctKey{keylet::account(alice.id())};
+            BEAST_EXPECT(env.closed()->exists(aliceAcctKey));
+            BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+
+            auto const acctDelFee{drops(env.current()->fees().increment)};
+
+            if (!features[fixNFTokenRemint])
+            {
+                // alice tries to delete her account, and is successful.
+                env(acctdelete(alice, becky), fee(acctDelFee));
+                env.close();
+
+                // alice's account root is gone from the most recently
+                // closed ledger and the current ledger.
+                BEAST_EXPECT(!env.closed()->exists(aliceAcctKey));
+                BEAST_EXPECT(!env.current()->exists(aliceAcctKey));
+
+                // Fund alice to re-create her account
+                env.fund(XRP(10000), alice);
+                env.close();
+
+                // alice's account now exists and has minted 0 NFTokens
+                BEAST_EXPECT(env.closed()->exists(aliceAcctKey));
+                BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+                BEAST_EXPECT((*env.le(alice))[sfMintedNFTokens] == 0);
+
+                // alice mints a NFT with same params as the first one before
+                // the account delete.
+                uint256 const remintNFTokenID =
+                    token::getNextID(env, alice, 0u);
+                env(token::mint(alice));
+                env.close();
+
+                // burn the NFT to make sure alice owns remintNFTokenID
+                env(token::burn(alice, remintNFTokenID));
+                env.close();
+
+                // The new NFT minted will have the same ID
+                // as one of NFTs minted using tickets
+                BEAST_EXPECT(
+                    std::find(nftIDs.begin(), nftIDs.end(), remintNFTokenID) !=
+                    nftIDs.end());
+            }
+            else if (features[fixNFTokenRemint])
+            {
+                // alice tries to delete her account, but is unsuccessful.
+                // Due to authorized minting, alice's account sequence does not
+                // advance while minter mints NFTokens for her using tickets.
+                // The new account deletion retriction <FirstNFTokenSequence +
+                // MintedNFTokens + 256> enabled by this amendment will enforce
+                // alice to wait for more ledgers to close before she can
+                // delete her account, to prevent duplicate NFTokenIDs
+                env(acctdelete(alice, becky),
+                    fee(acctDelFee),
+                    ter(tecTOO_SOON));
+                env.close();
+
+                // alice's account is still present
+                BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+
+                // Close more ledgers until it is no longer within
+                // <FirstNFTokenSequence + MintedNFTokens + 256>
+                // to be able to delete alice's account
+                incLgrSeqForFixNftRemint(env, alice);
+
+                // alice's account is deleted
+                env(acctdelete(alice, becky), fee(acctDelFee));
+                env.close();
+
+                // alice's account root is gone from the most recently
+                // closed ledger and the current ledger.
+                BEAST_EXPECT(!env.closed()->exists(aliceAcctKey));
+                BEAST_EXPECT(!env.current()->exists(aliceAcctKey));
+
+                // Fund alice to re-create her account
+                env.fund(XRP(10000), alice);
+                env.close();
+
+                // alice's account now exists and has minted 0 NFTokens
+                BEAST_EXPECT(env.closed()->exists(aliceAcctKey));
+                BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+                BEAST_EXPECT((*env.le(alice))[sfMintedNFTokens] == 0);
+
+                // alice mints a NFT with same params as the first one before
+                // the account delete.
+                uint256 const remintNFTokenID =
+                    token::getNextID(env, alice, 0u);
+                env(token::mint(alice));
+                env.close();
+
+                // burn the NFT to make sure alice owns remintNFTokenID
+                env(token::burn(alice, remintNFTokenID));
+                env.close();
+
+                // The new NFT minted will not have the same ID
+                // as any of the NFTs authorized minter minted using tickets
+                BEAST_EXPECT(
+                    std::find(nftIDs.begin(), nftIDs.end(), remintNFTokenID) ==
+                    nftIDs.end());
+            }
+        }
+        // If fixNFTokenRemint is enabled,
+        // when an authorized minter mints and burns a batch of NFTokens using
+        // tickets, issuer's account needs to wait a longer time before it can
+        // deleted.
+        // After the issuer's account is re-created and mints a NFT, it should
+        // not have the same NFTokenID as the ones authorized minter minted.
+        if (features[fixNFTokenRemint])
+        {
+            Env env{*this, features};
+            Account const alice("alice");
+            Account const becky("becky");
+            Account const minter{"minter"};
+
+            env.fund(XRP(10000), alice, becky, minter);
+            env.close();
+
+            // alice sets minter as her authorized minter
+            env(token::setMinter(alice, minter));
+            env.close();
+
+            // minter creates 100 tickets
+            std::uint32_t minterTicketSeq{env.seq(minter) + 1};
+            env(ticket::create(minter, 100));
+            env.close();
+
+            BEAST_EXPECT(ticketCount(env, minter) == 100);
+            BEAST_EXPECT(ownerCount(env, minter) == 100);
+
+            // minter mints 50 NFTs for alice using tickets
+            std::vector<uint256> nftIDs;
+            nftIDs.reserve(50);
+            for (int i = 0; i < 50; i++)
+            {
+                uint256 const nftokenID = token::getNextID(env, alice, 0u);
+                nftIDs.push_back(nftokenID);
+                env(token::mint(minter),
+                    token::issuer(alice),
+                    ticket::use(minterTicketSeq++));
+            }
+            env.close();
+
+            // minter burns 50 NFTs using tickets
+            for (auto const nftokenID : nftIDs)
+            {
+                env(token::burn(minter, nftokenID),
+                    ticket::use(minterTicketSeq++));
+            }
+            env.close();
+
+            BEAST_EXPECT(ticketCount(env, minter) == 0);
+
+            // Increment ledger sequence to the number that is
+            // enforced by the featureDeletableAccounts amendment
+            incLgrSeqForAcctDel(env, alice);
+
+            // Verify that alice's account root is present.
+            Keylet const aliceAcctKey{keylet::account(alice.id())};
+            BEAST_EXPECT(env.closed()->exists(aliceAcctKey));
+            BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+
+            // alice tries to delete her account, but is unsuccessful.
+            // Due to authorized minting, alice's account sequence does not
+            // advance while minter mints NFTokens for her using tickets.
+            // The new account deletion retriction <FirstNFTokenSequence +
+            // MintedNFTokens + 256> enabled by this amendment will enforce
+            // alice to wait for more ledgers to close before she can delete her
+            // account, to prevent duplicate NFTokenIDs
+            auto const acctDelFee{drops(env.current()->fees().increment)};
+            env(acctdelete(alice, becky), fee(acctDelFee), ter(tecTOO_SOON));
+            env.close();
+
+            // alice's account is still present
+            BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+
+            // Close more ledgers until it is no longer within
+            // <FirstNFTokenSequence + MintedNFTokens + 256>
+            // to be able to delete alice's account
+            incLgrSeqForFixNftRemint(env, alice);
+
+            // alice's account is deleted
+            env(acctdelete(alice, becky), fee(acctDelFee));
+            env.close();
+
+            // alice's account root is gone from the most recently
+            // closed ledger and the current ledger.
+            BEAST_EXPECT(!env.closed()->exists(aliceAcctKey));
+            BEAST_EXPECT(!env.current()->exists(aliceAcctKey));
+
+            // Fund alice to re-create her account
+            env.fund(XRP(10000), alice);
+            env.close();
+
+            // alice's account now exists and has minted 0 NFTokens
+            BEAST_EXPECT(env.closed()->exists(aliceAcctKey));
+            BEAST_EXPECT(env.current()->exists(aliceAcctKey));
+            BEAST_EXPECT((*env.le(alice))[sfMintedNFTokens] == 0);
+
+            // The new NFT minted will not have the same ID
+            // as any of the NFTs authorized minter minted using tickets
+            uint256 const remintNFTokenID = token::getNextID(env, alice, 0u);
+            env(token::mint(alice));
+            env.close();
+
+            // burn the NFT to make sure alice owns remintNFTokenID
+            env(token::burn(alice, remintNFTokenID));
+            env.close();
+
+            // The new NFT minted will not have the same ID
+            // as one of NFTs authorized minter minted using tickets
+            BEAST_EXPECT(
+                std::find(nftIDs.begin(), nftIDs.end(), remintNFTokenID) ==
+                nftIDs.end());
+        }
+    }
+
     void
     testWithFeats(FeatureBitset features)
     {
@@ -6069,6 +6596,7 @@ class NFToken_test : public beast::unit_test::suite
         testFixNFTokenNegOffer(features);
         testIOUWithTransferFee(features);
         testBrokeredSaleToSelf(features);
+        testFixNFTokenRemint(features);
     }
 
 public:
@@ -6079,9 +6607,13 @@ public:
         FeatureBitset const all{supported_amendments()};
         FeatureBitset const fixNFTDir{fixNFTokenDirV1};
 
-        testWithFeats(all - fixNFTDir - fixNonFungibleTokensV1_2);
+        testWithFeats(
-        testWithFeats(all - disallowIncoming - fixNonFungibleTokensV1_2);
+            all - fixNFTDir - fixNonFungibleTokensV1_2 - fixNFTokenRemint);
-        testWithFeats(all - fixNonFungibleTokensV1_2);
+        testWithFeats(
+            all - disallowIncoming - fixNonFungibleTokensV1_2 -
+            fixNFTokenRemint);
+        testWithFeats(all - fixNonFungibleTokensV1_2 - fixNFTokenRemint);
+        testWithFeats(all - fixNFTokenRemint);
         testWithFeats(all);
     }
 };

src/test/jtx/impl/token.cpp

@@ -68,17 +68,26 @@ getNextID(
     // Get the nftSeq from the account root of the issuer.
     std::uint32_t const nftSeq = {
         env.le(issuer)->at(~sfMintedNFTokens).value_or(0)};
-    return getID(issuer, nfTokenTaxon, nftSeq, flags, xferFee);
+    return token::getID(env, issuer, nfTokenTaxon, nftSeq, flags, xferFee);
 }
 
 uint256
 getID(
+    jtx::Env const& env,
     jtx::Account const& issuer,
     std::uint32_t nfTokenTaxon,
     std::uint32_t nftSeq,
     std::uint16_t flags,
     std::uint16_t xferFee)
 {
+    if (env.current()->rules().enabled(fixNFTokenRemint))
+    {
+        // If fixNFTokenRemint is enabled, we must add issuer's
+        // FirstNFTokenSequence to offset the starting NFT sequence number.
+        nftSeq += env.le(issuer)
+                      ->at(~sfFirstNFTokenSequence)
+                      .value_or(env.seq(issuer));
+    }
     return ripple::NFTokenMint::createNFTokenID(
         flags, xferFee, issuer, nft::toTaxon(nfTokenTaxon), nftSeq);
 }

src/test/jtx/token.h

@@ -95,6 +95,7 @@ getNextID(
 /** Get the NFTokenID for a particular nftSequence. */
 uint256
 getID(
+    jtx::Env const& env,
     jtx::Account const& account,
     std::uint32_t tokenTaxon,
     std::uint32_t nftSeq,