From 305c9a8d61b919f8be18ff5345ccd7b050c64ddd Mon Sep 17 00:00:00 2001 From: Shawn Xie <35279399+shawnxie999@users.noreply.github.com> Date: Mon, 20 Mar 2023 17:47:46 -0400 Subject: [PATCH] `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 --- src/ripple/app/tx/impl/DeleteAccount.cpp | 17 + src/ripple/app/tx/impl/NFTokenMint.cpp | 63 ++- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/SField.h | 1 + src/ripple/protocol/impl/Feature.cpp | 1 + src/ripple/protocol/impl/LedgerFormats.cpp | 1 + src/ripple/protocol/impl/SField.cpp | 3 + src/test/app/NFTokenBurn_test.cpp | 18 +- src/test/app/NFTokenDir_test.cpp | 35 +- src/test/app/NFToken_test.cpp | 584 ++++++++++++++++++++- src/test/jtx/impl/token.cpp | 11 +- src/test/jtx/token.h | 1 + 12 files changed, 695 insertions(+), 43 deletions(-) diff --git a/src/ripple/app/tx/impl/DeleteAccount.cpp b/src/ripple/app/tx/impl/DeleteAccount.cpp index 3d9d83c0d..62cc9e1fb 100644 --- a/src/ripple/app/tx/impl/DeleteAccount.cpp +++ b/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 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)}; diff --git a/src/ripple/app/tx/impl/NFTokenMint.cpp b/src/ripple/app/tx/impl/NFTokenMint.cpp index f4d3eb856..c26fb1fb1 100644 --- a/src/ripple/app/tx/impl/NFTokenMint.cpp +++ b/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: - 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; + (*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; }(); diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index d53d992d2..62dc327d9 100644 --- a/src/ripple/protocol/Feature.h +++ b/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 diff --git a/src/ripple/protocol/SField.h b/src/ripple/protocol/SField.h index 694eeef5c..5821a43d1 100644 --- a/src/ripple/protocol/SField.h +++ b/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; diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 4fb79e4cc..d15c3fc60 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/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. diff --git a/src/ripple/protocol/impl/LedgerFormats.cpp b/src/ripple/protocol/impl/LedgerFormats.cpp index a540a5d80..c60c8c59d 100644 --- a/src/ripple/protocol/impl/LedgerFormats.cpp +++ b/src/ripple/protocol/impl/LedgerFormats.cpp @@ -55,6 +55,7 @@ LedgerFormats::LedgerFormats() {sfNFTokenMinter, soeOPTIONAL}, {sfMintedNFTokens, soeDEFAULT}, {sfBurnedNFTokens, soeDEFAULT}, + {sfFirstNFTokenSequence, soeOPTIONAL}, }, commonFields); diff --git a/src/ripple/protocol/impl/SField.cpp b/src/ripple/protocol/impl/SField.cpp index f458c2dfe..14c2bd5c3 100644 --- a/src/ripple/protocol/impl/SField.cpp +++ b/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); diff --git a/src/test/app/NFTokenBurn_test.cpp b/src/test/app/NFTokenBurn_test.cpp index 096fd5ce1..75c32385a 100644 --- a/src/test/app/NFTokenBurn_test.cpp +++ b/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 = { - env.le(acct)->at(~sfMintedNFTokens).value_or(0)}; + std::uint32_t tokenSeq = + 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(all - fixNonFungibleTokensV1_2); + testWithFeats( + all - fixNonFungibleTokensV1_2 - fixNFTDir - fixNFTokenRemint); + testWithFeats(all - fixNonFungibleTokensV1_2 - fixNFTokenRemint); + testWithFeats(all - fixNFTokenRemint); testWithFeats(all); } }; diff --git a/src/test/app/NFTokenDir_test.cpp b/src/test/app/NFTokenDir_test.cpp index d50bd1584..e9addfa83 100644 --- a/src/test/app/NFTokenDir_test.cpp +++ b/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 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); } }; diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 150622c73..81abee34b 100644 --- a/src/test/app/NFToken_test.cpp +++ b/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 - // tests is 200 XRP, not 20. + // the reserve for creating their first NFT. Account reserve for + // 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 - // mint NFTs on alice's behalf. alice gives permission to minter. + // That was fun! Now let's see what happens when we let someone + // 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. - // Just for grins (and code coverage), minter mints NFTs that include - // a URI. + // minter still does not have enough XRP for the reserve of an NFT + // page. Just for grins (and code coverage), minter mints NFTs that + // 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. - env(token::burn(minter, token::getID(alice, 0, nftSeq++))); + // minter has one more NFT to burn. Should take her owner count to + // 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. - (*replacement)[sfMintedNFTokens] = 0xFFFF'FFFE; + if (env.current()->rules().enabled(fixNFTokenRemint)) + { + // 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(), @@ -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(), @@ -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 ) + // 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 . + // 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 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 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 + // + // 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 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 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 + // + // 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 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 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 + // + // 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(all - disallowIncoming - fixNonFungibleTokensV1_2); - testWithFeats(all - fixNonFungibleTokensV1_2); + testWithFeats( + all - fixNFTDir - fixNonFungibleTokensV1_2 - fixNFTokenRemint); + testWithFeats( + all - disallowIncoming - fixNonFungibleTokensV1_2 - + fixNFTokenRemint); + testWithFeats(all - fixNonFungibleTokensV1_2 - fixNFTokenRemint); + testWithFeats(all - fixNFTokenRemint); testWithFeats(all); } }; diff --git a/src/test/jtx/impl/token.cpp b/src/test/jtx/impl/token.cpp index cfbcfe11c..6c5cae414 100644 --- a/src/test/jtx/impl/token.cpp +++ b/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); } diff --git a/src/test/jtx/token.h b/src/test/jtx/token.h index 44f89087b..150ddfab0 100644 --- a/src/test/jtx/token.h +++ b/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,