fixNFTokenRemint: prevent NFT re-mint:

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,60 @@ NFTokenMint::doApply()
             // Should not happen.  Checked in preclaim.
             return Unexpected(tecNO_ISSUER);
 
-        // Get the unique sequence number for this token:
-        std::uint32_t const tokenSeq = (*root)[~sfMintedNFTokens].value_or(0);
+        if (!ctx_.view().rules().enabled(fixNFTokenRemint))
         {
-            std::uint32_t const nextTokenSeq = tokenSeq + 1;
-            if (nextTokenSeq < tokenSeq)
-                return Unexpected(tecMAX_SEQUENCE_REACHED);
+            // Get the unique sequence number for this token:
+            std::uint32_t const tokenSeq =
+                (*root)[~sfMintedNFTokens].value_or(0);
+            {
+                std::uint32_t const nextTokenSeq = tokenSeq + 1;
+                if (nextTokenSeq < tokenSeq)
+                    return Unexpected(tecMAX_SEQUENCE_REACHED);
+
+                (*root)[sfMintedNFTokens] = nextTokenSeq;
+            }
+            ctx_.view().update(root);
+            return tokenSeq;
+        }
+
+        // With fixNFTokenRemint amendment enabled:
+        //
+        // If the issuer hasn't minted a NFT before, we must
+        // initialize sfFirstNFTokenSequence to equal to the current account
+        // sequence. In general, we must subtract account sequence by
+        // one, since it is incremented by the transactor beforehand. In
+        // scenarios of AuthorizedMinting or Tickets, we use the
+        // account sequence as it is because it has not been incremented.
+        //
+        // Whether we subtract the acct sequence by 1 is unimportant in real
+        // world use case. But it is needed in order to deterministically
+        // generate the NFTokenSequence for test cases.
+        if (auto fts = (*root)[~sfFirstNFTokenSequence]; !fts)
+        {
+            auto const accSeq = (*root)[sfSequence];
 
-            (*root)[sfMintedNFTokens] = nextTokenSeq;
+            (*root)[sfFirstNFTokenSequence] =
+                (*root)[~sfNFTokenMinter] == ctx_.tx[sfAccount] ||
+                    ctx_.tx.getSeqProxy().isTicket()
+                ? accSeq
+                : accSeq - 1;
         }
+
+        auto const mintedNftCnt = (*root)[~sfMintedNFTokens].value_or(0);
+
+        (*root)[sfMintedNFTokens] = mintedNftCnt + 1;
+        if ((*root)[sfMintedNFTokens] == 0)
+            return Unexpected(tecMAX_SEQUENCE_REACHED);
+
+        // Get the unique sequence number of this token by
+        // sfFirstNFTokenSequence + sfMintedNFTokens
+        auto const offset = (*root)[sfFirstNFTokenSequence];
+        auto const tokenSeq = offset + mintedNftCnt;
+
+        // Check for more overflow cases
+        if (tokenSeq + 1 == 0 || tokenSeq < mintedNftCnt)
+            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,20 @@ 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 = {
-                    env.le(acct)->at(~sfMintedNFTokens).value_or(0)};
+                std::uint32_t tokenSeq;
+
+                // If fixNFTokenRemint amendment is on, we must
+                // generate the NFT sequence using new construct
+                if (env.current()->rules().enabled(fixNFTokenRemint))
+                    tokenSeq = {
+                        env.le(acct)
+                            ->at(~sfFirstNFTokenSequence)
+                            .value_or(env.seq(acct)) +
+                        env.le(acct)->at(~sfMintedNFTokens).value_or(0)};
+                else
+                    tokenSeq = {
+                        env.le(acct)->at(~sfMintedNFTokens).value_or(0)};
+
                 return toUInt32(
                     nft::cipheredTaxon(tokenSeq, nft::toTaxon(taxon)));
             };

src/test/app/NFTokenDir_test.cpp

@@ -190,8 +190,16 @@ 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();
+
+                    // Should not advance ledger if fixNFTokenRemint is
+                    // enabled. Because otherwise, accounts are initialized at
+                    // different ledgers, and will have different account
+                    // sequences, which then causes the sequence number of
+                    // NFTokenIDs to be different
+                    if (!features[fixNFTokenRemint])
+                        env.close();
                 }
+                env.close();
 
                 // All of the accounts create one NFT and and offer that NFT to
                 // buyer.
@@ -408,8 +416,16 @@ 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();
+
+                    // Should not advance ledger if fixNFTokenRemint is
+                    // enabled. Because otherwise, accounts are initialized at
+                    // different ledgers, and will have different account
+                    // sequences, which then causes the sequence number of
+                    // NFTokenIDs to be different
+                    if (!features[fixNFTokenRemint])
+                        env.close();
                 }
+                env.close();
 
                 // All of the accounts create one NFT and and offer that NFT to
                 // buyer.
@@ -647,13 +663,23 @@ class NFTokenDir_test : public beast::unit_test::suite
         // Create accounts for all of the seeds and fund those accounts.
         std::vector<Account> accounts;
         accounts.reserve(seeds.size());
+
+        // If fixNFTokenRemint is not enabled, accounts can be created in
+        // different ledgers.
+        // If fixNFTokenRemint is enabled, all accounts must be created in
+        // the same ledger in order to initialize all accounts with the same
+        // account sequence.
         for (std::string_view const& seed : seeds)
         {
             Account const& account =
                 accounts.emplace_back(Account::base58Seed, std::string(seed));
             env.fund(XRP(10000), account);
-            env.close();
+
+            // Only advance the ledger if fixNFTokenRemint is disabled
+            if (!features[fixNFTokenRemint])
+                env.close();
         }
+        env.close();
 
         // All of the accounts create one NFT and and offer that NFT to buyer.
         std::vector<uint256> nftIDs;
@@ -822,13 +848,23 @@ class NFTokenDir_test : public beast::unit_test::suite
         // Create accounts for all of the seeds and fund those accounts.
         std::vector<Account> accounts;
         accounts.reserve(seeds.size());
+
+        // If fixNFTokenRemint is not enabled, accounts can be created in
+        // different ledgers.
+        // If fixNFTokenRemint is enabled, accounts must be created in the
+        // same ledger in order to initialize all accounts with the same
+        // account sequence.
         for (std::string_view const& seed : seeds)
         {
             Account const& account =
                 accounts.emplace_back(Account::base58Seed, std::string(seed));
             env.fund(XRP(10000), account);
-            env.close();
+
+            // Only advance the ledger if fixNFTokenRemint is disabled
+            if (!features[fixNFTokenRemint])
+                env.close();
         }
+        env.close();
 
         // All of the accounts create seven consecutive NFTs and and offer
         // those NFTs to buyer.
@@ -1078,7 +1114,8 @@ class NFTokenDir_test : public beast::unit_test::suite
         FeatureBitset const fixNFTDir{
             fixNFTokenDirV1, featureNonFungibleTokensV1_1};
 
-        testWithFeats(all - fixNFTDir);
+        testWithFeats(all - fixNFTDir - fixNFTokenRemint);
+        testWithFeats(all - fixNFTokenRemint);
         testWithFeats(all);
     }
 };