From 90d463b925c377878d3abdb27eb3a1530782a668 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Wed, 31 Jan 2024 06:14:57 +0000 Subject: [PATCH 1/8] test: add unit test for redundant payment (#4860) If the payee and payer are the same account, then the transaction fails in preflight with temREDUNDANT. --- src/test/app/TxQ_test.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index ed94ffe1866..31e0b7b8124 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -1019,6 +1019,9 @@ class TxQPosNegFlows_test : public beast::unit_test::suite // Fail in preflight env(pay(alice, bob, XRP(-1000)), ter(temBAD_AMOUNT)); + // Fail in preflight + env(pay(alice, alice, XRP(100)), ter(temREDUNDANT)); + // Fail in preclaim env(noop(alice), fee(XRP(100000)), ter(terINSUF_FEE_B)); } From f9e365828a00900feca14483ad955d0ee6c49310 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 1 Feb 2024 01:29:29 -0500 Subject: [PATCH 2/8] test: check for success/failure of Windows CI unit tests (#4871) * Disable the Windows CI unit tests "allowed to fail" workaround which was previously introduced in #4596. * The runner hardware was upgraded, and the unit tests have been passing since then. --- .github/workflows/windows.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index 7286d2f1f34..e0663b3d18d 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -91,11 +91,7 @@ jobs: # Hard code for now. Move to the matrix if varied options are needed cmake-args: '-Dassert=ON -Dreporting=OFF -Dunity=ON' cmake-target: install - - name: test (permitted to silently fail) + - name: test shell: bash - # Github runners are resource limited, which causes unit tests to fail - # (e.g. OOM). To allow forward progress until self-hosted runners are - # up and running reliably, allow the job to succeed even if tests fail. - continue-on-error: true run: | ${build_dir}/${{ matrix.configuration }}/rippled --unittest --unittest-jobs $(nproc) From 6f00d32f7e698d4f8bd7f23c386c132a04708be2 Mon Sep 17 00:00:00 2001 From: John Freeman Date: Thu, 1 Feb 2024 18:57:29 -0600 Subject: [PATCH 3/8] fix(libxrpl): change library names in Conan recipe (#4831) Use consistent platform-agnostic library names on all platforms. Fix an issue that prevents dependents like validator-keys-tool from linking to libxrpl on Windows. It is bad practice to change the binary base name depending on the platform. CMake already manipulates the base name into a final name that fits the conventions of the platform. Linkers accept base names on the command line and then look for conventional names on disk. --- conanfile.py | 6 +++--- src/secp256k1/src/CMakeLists.txt | 13 ++++++++++--- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/conanfile.py b/conanfile.py index c789cacf568..d9e2a0ae7c4 100644 --- a/conanfile.py +++ b/conanfile.py @@ -147,9 +147,9 @@ def package(self): def package_info(self): libxrpl = self.cpp_info.components['libxrpl'] libxrpl.libs = [ - 'libxrpl_core.a', - 'libed25519.a', - 'libsecp256k1.a', + 'xrpl_core', + 'ed25519', + 'secp256k1', ] # TODO: Fix the protobufs to include each other relative to # `include/`, not `include/ripple/proto/`. diff --git a/src/secp256k1/src/CMakeLists.txt b/src/secp256k1/src/CMakeLists.txt index 93844caa7f2..dace09201f8 100644 --- a/src/secp256k1/src/CMakeLists.txt +++ b/src/secp256k1/src/CMakeLists.txt @@ -64,9 +64,16 @@ elseif(APPLE) endif() elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows") set(${PROJECT_NAME}_windows "secp256k1") - if(MSVC) - set(${PROJECT_NAME}_windows "${PROJECT_NAME}") - endif() + # This step is commented out from the original. It is bad practice to change + # the binary base name depending on the platform. CMake already manipulates + # the base name into a final name that fits the conventions of the platform. + # Linkers accept base names on the command line and then look for + # conventional names on disk. This way, developers can use base names + # everywhere (in the CMake and Conan they write) and the tools will do the + # right thing. + # if(MSVC) + # set(${PROJECT_NAME}_windows "${PROJECT_NAME}") + # endif() set_target_properties(secp256k1 PROPERTIES ARCHIVE_OUTPUT_NAME "${${PROJECT_NAME}_windows}" RUNTIME_OUTPUT_NAME "${${PROJECT_NAME}_windows}-${${PROJECT_NAME}_soversion}" From 828bb64ebc76394cf9a3f7edd42f4588d106932f Mon Sep 17 00:00:00 2001 From: Shawn Xie <35279399+shawnxie999@users.noreply.github.com> Date: Fri, 2 Feb 2024 17:27:21 -0500 Subject: [PATCH 4/8] `fixNFTokenReserve`: ensure NFT tx fails when reserve is not met (#4767) Without this amendment, an NFTokenAcceptOffer transaction can succeed even when the NFToken recipient does not have sufficient reserves for the new NFTokenPage. This allowed accounts to accept NFT sell offers without having a sufficient reserve. (However, there was no issue in brokered mode or when a buy offer is involved.) Instead, the transaction should fail with `tecINSUFFICIENT_RESERVE` as appropriate. The `fixNFTokenReserve` amendment adds checks in the NFTokenAcceptOffer transactor to check if the OwnerCount changed. If it did, then it checks the new reserve requirement. Fix #4679 --- src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp | 79 ++-- src/ripple/app/tx/impl/NFTokenAcceptOffer.h | 6 + src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/impl/Feature.cpp | 1 + src/test/app/NFToken_test.cpp | 340 +++++++++++++++++- 5 files changed, 400 insertions(+), 29 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 61aa7e0629a..02471c1d482 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -301,6 +301,60 @@ NFTokenAcceptOffer::pay( return tesSUCCESS; } +TER +NFTokenAcceptOffer::transferNFToken( + AccountID const& buyer, + AccountID const& seller, + uint256 const& nftokenID) +{ + auto tokenAndPage = nft::findTokenAndPage(view(), seller, nftokenID); + + if (!tokenAndPage) + return tecINTERNAL; + + if (auto const ret = nft::removeToken( + view(), seller, nftokenID, std::move(tokenAndPage->page)); + !isTesSuccess(ret)) + return ret; + + auto const sleBuyer = view().read(keylet::account(buyer)); + if (!sleBuyer) + return tecINTERNAL; + + std::uint32_t const buyerOwnerCountBefore = + sleBuyer->getFieldU32(sfOwnerCount); + + auto const insertRet = + nft::insertToken(view(), buyer, std::move(tokenAndPage->token)); + + // if fixNFTokenReserve is enabled, check if the buyer has sufficient + // reserve to own a new object, if their OwnerCount changed. + // + // There was an issue where the buyer accepts a sell offer, the ledger + // didn't check if the buyer has enough reserve, meaning that buyer can get + // NFTs free of reserve. + if (view().rules().enabled(fixNFTokenReserve)) + { + // To check if there is sufficient reserve, we cannot use mPriorBalance + // because NFT is sold for a price. So we must use the balance after + // the deduction of the potential offer price. A small caveat here is + // that the balance has already deducted the transaction fee, meaning + // that the reserve requirement is a few drops higher. + auto const buyerBalance = sleBuyer->getFieldAmount(sfBalance); + + auto const buyerOwnerCountAfter = sleBuyer->getFieldU32(sfOwnerCount); + if (buyerOwnerCountAfter > buyerOwnerCountBefore) + { + if (auto const reserve = + view().fees().accountReserve(buyerOwnerCountAfter); + buyerBalance < reserve) + return tecINSUFFICIENT_RESERVE; + } + } + + return insertRet; +} + TER NFTokenAcceptOffer::acceptOffer(std::shared_ptr const& offer) { @@ -333,17 +387,7 @@ NFTokenAcceptOffer::acceptOffer(std::shared_ptr const& offer) } // Now transfer the NFT: - auto tokenAndPage = nft::findTokenAndPage(view(), seller, nftokenID); - - if (!tokenAndPage) - return tecINTERNAL; - - if (auto const ret = nft::removeToken( - view(), seller, nftokenID, std::move(tokenAndPage->page)); - !isTesSuccess(ret)) - return ret; - - return nft::insertToken(view(), buyer, std::move(tokenAndPage->token)); + return transferNFToken(buyer, seller, nftokenID); } TER @@ -431,17 +475,8 @@ NFTokenAcceptOffer::doApply() return r; } - auto tokenAndPage = nft::findTokenAndPage(view(), seller, nftokenID); - - if (!tokenAndPage) - return tecINTERNAL; - - if (auto const ret = nft::removeToken( - view(), seller, nftokenID, std::move(tokenAndPage->page)); - !isTesSuccess(ret)) - return ret; - - return nft::insertToken(view(), buyer, std::move(tokenAndPage->token)); + // Now transfer the NFT: + return transferNFToken(buyer, seller, nftokenID); } if (bo) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.h b/src/ripple/app/tx/impl/NFTokenAcceptOffer.h index 2d1b14ba284..e1b26cbecea 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.h +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.h @@ -38,6 +38,12 @@ class NFTokenAcceptOffer : public Transactor std::shared_ptr const& buy, std::shared_ptr const& sell); + TER + transferNFToken( + AccountID const& buyer, + AccountID const& seller, + uint256 const& nfTokenID); + public: static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 3bdfcb15c59..d8ce6dc6280 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 = 65; +static constexpr std::size_t numFeatures = 66; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -352,6 +352,7 @@ extern uint256 const featureXChainBridge; extern uint256 const fixDisallowIncomingV1; extern uint256 const featureDID; extern uint256 const fixFillOrKill; +extern uint256 const fixNFTokenReserve; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 25033d4336e..e7cd72fb866 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -459,6 +459,7 @@ REGISTER_FEATURE(XChainBridge, Supported::yes, VoteBehavior::De REGISTER_FIX (fixDisallowIncomingV1, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(DID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX(fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixNFTokenReserve, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 399f6f54b9a..8740b521132 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6798,6 +6798,320 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite } } + void + testFixNFTokenBuyerReserve(FeatureBitset features) + { + testcase("Test buyer reserve when accepting an offer"); + + using namespace test::jtx; + + // Lambda that mints an NFT and then creates a sell offer + auto mintAndCreateSellOffer = [](test::jtx::Env& env, + test::jtx::Account const& acct, + STAmount const amt) -> uint256 { + // acct mints a NFT + uint256 const nftId{ + token::getNextID(env, acct, 0u, tfTransferable)}; + env(token::mint(acct, 0u), txflags(tfTransferable)); + env.close(); + + // acct makes an sell offer + uint256 const sellOfferIndex = + keylet::nftoffer(acct, env.seq(acct)).key; + env(token::createOffer(acct, nftId, amt), txflags(tfSellNFToken)); + env.close(); + + return sellOfferIndex; + }; + + // Test the behaviors when the buyer makes an accept offer, both before + // and after enabling the amendment. Exercises the precise number of + // reserve in drops that's required to accept the offer + { + Account const alice{"alice"}; + Account const bob{"bob"}; + + Env env{*this, features}; + auto const acctReserve = env.current()->fees().accountReserve(0); + auto const incReserve = env.current()->fees().increment; + + env.fund(XRP(10000), alice); + env.close(); + + // Bob is funded with minimum XRP reserve + env.fund(acctReserve, bob); + env.close(); + + // alice mints an NFT and create a sell offer for 0 XRP + auto const sellOfferIndex = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob owns no object + BEAST_EXPECT(ownerCount(env, bob) == 0); + + // Without fixNFTokenReserve amendment, when bob accepts an NFT sell + // offer, he can get the NFT free of reserve + if (!features[fixNFTokenReserve]) + { + // Bob is able to accept the offer + env(token::acceptSellOffer(bob, sellOfferIndex)); + env.close(); + + // Bob now owns an extra objects + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // This is the wrong behavior, since Bob should need at least + // one incremental reserve. + } + // With fixNFTokenReserve, bob can no longer accept the offer unless + // there is enough reserve. A detail to note is that NFTs(sell + // offer) will not allow one to go below the reserve requirement, + // because buyer's balance is computed after the transaction fee is + // deducted. This means that the reserve requirement will be 10 + // drops higher than normal. + else + { + // Bob is not able to accept the offer with only the account + // reserve (200,000,000 drops) + env(token::acceptSellOffer(bob, sellOfferIndex), + ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + // after prev transaction, Bob owns 199,999,990 drops due to + // burnt tx fee + + BEAST_EXPECT(ownerCount(env, bob) == 0); + + // Send bob an increment reserve and 10 drops (to make up for + // the transaction fee burnt from the prev failed tx) Bob now + // owns 250,000,000 drops + env(pay(env.master, bob, incReserve + drops(10))); + env.close(); + + // However, this transaction will still fail because the reserve + // requirement is 10 drops higher + env(token::acceptSellOffer(bob, sellOfferIndex), + ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + // Send bob an increment reserve and 20 drops + // Bob now owns 250,000,010 drops + env(pay(env.master, bob, drops(20))); + env.close(); + + // Bob is now able to accept the offer + env(token::acceptSellOffer(bob, sellOfferIndex)); + env.close(); + + BEAST_EXPECT(ownerCount(env, bob) == 1); + } + } + + // Now exercise the scenario when the buyer accepts + // many sell offers + { + Account const alice{"alice"}; + Account const bob{"bob"}; + + Env env{*this, features}; + auto const acctReserve = env.current()->fees().accountReserve(0); + auto const incReserve = env.current()->fees().increment; + + env.fund(XRP(10000), alice); + env.close(); + + env.fund(acctReserve + XRP(1), bob); + env.close(); + + if (!features[fixNFTokenReserve]) + { + // Bob can accept many NFTs without having a single reserve! + for (size_t i = 0; i < 200; i++) + { + // alice mints an NFT and creates a sell offer for 0 XRP + auto const sellOfferIndex = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob is able to accept the offer + env(token::acceptSellOffer(bob, sellOfferIndex)); + env.close(); + } + } + else + { + // alice mints the first NFT and creates a sell offer for 0 XRP + auto const sellOfferIndex1 = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob cannot accept this offer because he doesn't have the + // reserve for the NFT + env(token::acceptSellOffer(bob, sellOfferIndex1), + ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + // Give bob enough reserve + env(pay(env.master, bob, drops(incReserve))); + env.close(); + + BEAST_EXPECT(ownerCount(env, bob) == 0); + + // Bob now owns his first NFT + env(token::acceptSellOffer(bob, sellOfferIndex1)); + env.close(); + + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // alice now mints 31 more NFTs and creates an offer for each + // NFT, then sells to bob + for (size_t i = 0; i < 31; i++) + { + // alice mints an NFT and creates a sell offer for 0 XRP + auto const sellOfferIndex = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob can accept the offer because the new NFT is stored in + // an existing NFTokenPage so no new reserve is requried + env(token::acceptSellOffer(bob, sellOfferIndex)); + env.close(); + } + + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // alice now mints the 33rd NFT and creates an sell offer for 0 + // XRP + auto const sellOfferIndex33 = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob fails to accept this NFT because he does not have enough + // reserve for a new NFTokenPage + env(token::acceptSellOffer(bob, sellOfferIndex33), + ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + // Send bob incremental reserve + env(pay(env.master, bob, drops(incReserve))); + env.close(); + + // Bob now has enough reserve to accept the offer and now + // owns one more NFTokenPage + env(token::acceptSellOffer(bob, sellOfferIndex33)); + env.close(); + + BEAST_EXPECT(ownerCount(env, bob) == 2); + } + } + + // Test the behavior when the seller accepts a buy offer. + // The behavior should not change regardless whether fixNFTokenReserve + // is enabled or not, since the ledger is able to guard against + // free NFTokenPages when buy offer is accepted. This is merely an + // additional test to exercise existing offer behavior. + { + Account const alice{"alice"}; + Account const bob{"bob"}; + + Env env{*this, features}; + auto const acctReserve = env.current()->fees().accountReserve(0); + auto const incReserve = env.current()->fees().increment; + + env.fund(XRP(10000), alice); + env.close(); + + // Bob is funded with account reserve + increment reserve + 1 XRP + // increment reserve is for the buy offer, and 1 XRP is for offer + // price + env.fund(acctReserve + incReserve + XRP(1), bob); + env.close(); + + // Alice mints a NFT + uint256 const nftId{ + token::getNextID(env, alice, 0u, tfTransferable)}; + env(token::mint(alice, 0u), txflags(tfTransferable)); + env.close(); + + // Bob makes a buy offer for 1 XRP + auto const buyOfferIndex = keylet::nftoffer(bob, env.seq(bob)).key; + env(token::createOffer(bob, nftId, XRP(1)), token::owner(alice)); + env.close(); + + // accepting the buy offer fails because bob's balance is 10 drops + // lower than the required amount, since the previous tx burnt 10 + // drops for tx fee. + env(token::acceptBuyOffer(alice, buyOfferIndex), + ter(tecINSUFFICIENT_FUNDS)); + env.close(); + + // send Bob 10 drops + env(pay(env.master, bob, drops(10))); + env.close(); + + // Now bob can buy the offer + env(token::acceptBuyOffer(alice, buyOfferIndex)); + env.close(); + } + + // Test the reserve behavior in brokered mode. + // The behavior should not change regardless whether fixNFTokenReserve + // is enabled or not, since the ledger is able to guard against + // free NFTokenPages in brokered mode. This is merely an + // additional test to exercise existing offer behavior. + { + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const broker{"broker"}; + + Env env{*this, features}; + auto const acctReserve = env.current()->fees().accountReserve(0); + auto const incReserve = env.current()->fees().increment; + + env.fund(XRP(10000), alice, broker); + env.close(); + + // Bob is funded with account reserve + incr reserve + 1 XRP(offer + // price) + env.fund(acctReserve + incReserve + XRP(1), bob); + env.close(); + + // Alice mints a NFT + uint256 const nftId{ + token::getNextID(env, alice, 0u, tfTransferable)}; + env(token::mint(alice, 0u), txflags(tfTransferable)); + env.close(); + + // Alice creates sell offer and set broker as destination + uint256 const offerAliceToBroker = + keylet::nftoffer(alice, env.seq(alice)).key; + env(token::createOffer(alice, nftId, XRP(1)), + token::destination(broker), + txflags(tfSellNFToken)); + env.close(); + + // Bob creates buy offer + uint256 const offerBobToBroker = + keylet::nftoffer(bob, env.seq(bob)).key; + env(token::createOffer(bob, nftId, XRP(1)), token::owner(alice)); + env.close(); + + // broker offers. + // Returns insufficient funds, because bob burnt tx fee when he + // created his buy offer, which makes his spendable balance to be + // less than the required amount. + env(token::brokerOffers( + broker, offerBobToBroker, offerAliceToBroker), + ter(tecINSUFFICIENT_FUNDS)); + env.close(); + + // send Bob 10 drops + env(pay(env.master, bob, drops(10))); + env.close(); + + // broker offers. + env(token::brokerOffers( + broker, offerBobToBroker, offerAliceToBroker)); + env.close(); + } + } + void testWithFeats(FeatureBitset features) { @@ -6831,6 +7145,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite testBrokeredSaleToSelf(features); testFixNFTokenRemint(features); testTxJsonMetaFields(features); + testFixNFTokenBuyerReserve(features); } public: @@ -6841,12 +7156,15 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite static FeatureBitset const all{supported_amendments()}; static FeatureBitset const fixNFTDir{fixNFTokenDirV1}; - static std::array const feats{ - all - fixNFTDir - fixNonFungibleTokensV1_2 - fixNFTokenRemint, + static std::array const feats{ + all - fixNFTDir - fixNonFungibleTokensV1_2 - fixNFTokenRemint - + fixNFTokenReserve, all - disallowIncoming - fixNonFungibleTokensV1_2 - - fixNFTokenRemint, - all - fixNonFungibleTokensV1_2 - fixNFTokenRemint, - all - fixNFTokenRemint, + fixNFTokenRemint - fixNFTokenReserve, + all - fixNonFungibleTokensV1_2 - fixNFTokenRemint - + fixNFTokenReserve, + all - fixNFTokenRemint - fixNFTokenReserve, + all - fixNFTokenReserve, all}; if (BEAST_EXPECT(instance < feats.size())) @@ -6890,12 +7208,21 @@ class NFTokenWOTokenRemint_test : public NFTokenBaseUtil_test } }; +class NFTokenWOTokenReserve_test : public NFTokenBaseUtil_test +{ + void + run() override + { + NFTokenBaseUtil_test::run(4); + } +}; + class NFTokenAllFeatures_test : public NFTokenBaseUtil_test { void run() override { - NFTokenBaseUtil_test::run(4, true); + NFTokenBaseUtil_test::run(5, true); } }; @@ -6903,6 +7230,7 @@ BEAST_DEFINE_TESTSUITE_PRIO(NFTokenBaseUtil, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFTokenDisallowIncoming, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOfixV1, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOTokenRemint, tx, ripple, 2); +BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOTokenReserve, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFTokenAllFeatures, tx, ripple, 2); } // namespace ripple From 1e96a1d6eb2e17bf52694e1f6af1d585e10bd9ff Mon Sep 17 00:00:00 2001 From: Michael Legleux Date: Mon, 5 Feb 2024 08:17:32 -0800 Subject: [PATCH 5/8] build: add headers needed in Conan package for libxrpl (#4885) These headers are required in the xrpl Conan package in order for xbridge witness server (xbwd) to build. This change to libxrpl may help any dependents of libxrpl. This addition does not change any C++ code. --- Builds/CMake/RippledCore.cmake | 36 +++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index f9e43810a05..9c0c7016379 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -230,6 +230,11 @@ install ( FILES src/ripple/json/impl/json_assert.h DESTINATION include/ripple/json/impl) + +install ( + FILES + src/ripple/net/RPCErr.h + DESTINATION include/ripple/net) install ( FILES src/ripple/protocol/AccountID.h @@ -302,7 +307,35 @@ install ( src/ripple/protocol/impl/STVar.h src/ripple/protocol/impl/secp256k1.h DESTINATION include/ripple/protocol/impl) - +install ( + FILES + src/ripple/resource/Fees.h + src/ripple/resource/Charge.h + DESTINATION include/ripple/resource) +install ( + FILES + src/ripple/server/Port.h + src/ripple/server/Server.h + src/ripple/server/Session.h + src/ripple/server/SimpleWriter.h + src/ripple/server/Writer.h + src/ripple/server/WSSession.h + src/ripple/server/Handoff.h + DESTINATION include/ripple/server) +install ( + FILES + src/ripple/server/impl/ServerImpl.h + src/ripple/server/impl/io_list.h + src/ripple/server/impl/Door.h + src/ripple/server/impl/PlainHTTPPeer.h + src/ripple/server/impl/PlainWSPeer.h + src/ripple/server/impl/BaseHTTPPeer.h + src/ripple/server/impl/BaseWSPeer.h + src/ripple/server/impl/BasePeer.h + src/ripple/server/impl/LowestLayer.h + src/ripple/server/impl/SSLHTTPPeer.h + src/ripple/server/impl/SSLWSPeer.h + DESTINATION include/ripple/server/impl) #[===================================[ beast/legacy headers installation #]===================================] @@ -369,6 +402,7 @@ install ( src/ripple/beast/utility/PropertyStream.h src/ripple/beast/utility/Zero.h src/ripple/beast/utility/rngfill.h + src/ripple/beast/utility/WrappedSink.h DESTINATION include/ripple/beast/utility) # WARNING!! -- horrible levelization ahead # (these files should be isolated or moved...but From 6d3c21e369cdcde867a2c54f68028f18aef7d83e Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Date: Tue, 6 Feb 2024 20:14:40 -0800 Subject: [PATCH 6/8] feat: allow `port_grpc` to be specified in `[server]` stanza (#4728) Prior to this commit, `port_grpc` could not be added to the [server] stanza. Instead of validating gRPC IP/Port/Protocol information in ServerHandler, validate grpc port info in GRPCServer constructor. This should not break backwards compatibility. gRPC-related config info must be in a section (stanza) called [port_gprc]. * Close #4015 - That was an alternate solution. It was decided that with relaxed validation, it is not necessary to rename port_grpc. * Fix #4557 --- src/ripple/app/main/GRPCServer.cpp | 7 +-- src/ripple/app/misc/NetworkOPs.cpp | 4 +- src/ripple/core/ConfigSections.h | 13 +++--- src/ripple/rpc/impl/ServerHandler.cpp | 9 +++- src/test/jtx/envconfig.h | 5 ++ src/test/jtx/impl/envconfig.cpp | 67 +++++++++++++-------------- src/test/rpc/ReportingETL_test.cpp | 21 +++++---- src/test/rpc/ServerInfo_test.cpp | 3 +- 8 files changed, 73 insertions(+), 56 deletions(-) diff --git a/src/ripple/app/main/GRPCServer.cpp b/src/ripple/app/main/GRPCServer.cpp index 3a5e96b0ed9..a535a4a1a53 100644 --- a/src/ripple/app/main/GRPCServer.cpp +++ b/src/ripple/app/main/GRPCServer.cpp @@ -23,6 +23,7 @@ #include #include +#include namespace ripple { @@ -427,9 +428,9 @@ GRPCServerImpl::GRPCServerImpl(Application& app) : app_(app), journal_(app_.journal("gRPC Server")) { // if present, get endpoint from config - if (app_.config().exists("port_grpc")) + if (app_.config().exists(SECTION_PORT_GRPC)) { - const auto& section = app_.config().section("port_grpc"); + Section const& section = app_.config().section(SECTION_PORT_GRPC); auto const optIp = section.get("ip"); if (!optIp) @@ -659,7 +660,7 @@ GRPCServerImpl::setupListeners() secureGatewayIPs_)); } return requests; -}; +} bool GRPCServerImpl::start() diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 785e1c12c8b..239fd6306e0 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -2719,9 +2719,9 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters) } } - if (app_.config().exists("port_grpc")) + if (app_.config().exists(SECTION_PORT_GRPC)) { - auto const& grpcSection = app_.config().section("port_grpc"); + auto const& grpcSection = app_.config().section(SECTION_PORT_GRPC); auto const optPort = grpcSection.get("port"); if (optPort && grpcSection.get("ip")) { diff --git a/src/ripple/core/ConfigSections.h b/src/ripple/core/ConfigSections.h index 03c702f9a52..b4e460f1cfc 100644 --- a/src/ripple/core/ConfigSections.h +++ b/src/ripple/core/ConfigSections.h @@ -49,6 +49,7 @@ struct ConfigSection // VFALCO TODO Rename and replace these macros with variables. #define SECTION_AMENDMENTS "amendments" #define SECTION_AMENDMENT_MAJORITY_TIME "amendment_majority_time" +#define SECTION_BETA_RPC_API "beta_rpc_api" #define SECTION_CLUSTER_NODES "cluster_nodes" #define SECTION_COMPRESSION "compression" #define SECTION_DEBUG_LOGFILE "debug_logfile" @@ -57,10 +58,13 @@ struct ConfigSection #define SECTION_FETCH_DEPTH "fetch_depth" #define SECTION_HISTORICAL_SHARD_PATHS "historical_shard_paths" #define SECTION_INSIGHT "insight" +#define SECTION_IO_WORKERS "io_workers" #define SECTION_IPS "ips" #define SECTION_IPS_FIXED "ips_fixed" #define SECTION_LEDGER_HISTORY "ledger_history" +#define SECTION_LEDGER_REPLAY "ledger_replay" #define SECTION_MAX_TRANSACTIONS "max_transactions" +#define SECTION_NETWORK_ID "network_id" #define SECTION_NETWORK_QUORUM "network_quorum" #define SECTION_NODE_SEED "node_seed" #define SECTION_NODE_SIZE "node_size" @@ -73,6 +77,8 @@ struct ConfigSection #define SECTION_PEERS_MAX "peers_max" #define SECTION_PEERS_IN_MAX "peers_in_max" #define SECTION_PEERS_OUT_MAX "peers_out_max" +#define SECTION_PORT_GRPC "port_grpc" +#define SECTION_PREFETCH_WORKERS "prefetch_workers" #define SECTION_REDUCE_RELAY "reduce_relay" #define SECTION_RELATIONAL_DB "relational_db" #define SECTION_RELAY_PROPOSALS "relay_proposals" @@ -84,6 +90,7 @@ struct ConfigSection #define SECTION_SSL_VERIFY_FILE "ssl_verify_file" #define SECTION_SSL_VERIFY_DIR "ssl_verify_dir" #define SECTION_SERVER_DOMAIN "server_domain" +#define SECTION_SWEEP_INTERVAL "sweep_interval" #define SECTION_VALIDATORS_FILE "validators_file" #define SECTION_VALIDATION_SEED "validation_seed" #define SECTION_VALIDATOR_KEYS "validator_keys" @@ -94,12 +101,6 @@ struct ConfigSection #define SECTION_VALIDATOR_TOKEN "validator_token" #define SECTION_VETO_AMENDMENTS "veto_amendments" #define SECTION_WORKERS "workers" -#define SECTION_IO_WORKERS "io_workers" -#define SECTION_PREFETCH_WORKERS "prefetch_workers" -#define SECTION_LEDGER_REPLAY "ledger_replay" -#define SECTION_BETA_RPC_API "beta_rpc_api" -#define SECTION_SWEEP_INTERVAL "sweep_interval" -#define SECTION_NETWORK_ID "network_id" } // namespace ripple diff --git a/src/ripple/rpc/impl/ServerHandler.cpp b/src/ripple/rpc/impl/ServerHandler.cpp index f33ecd625aa..424f2f8d001 100644 --- a/src/ripple/rpc/impl/ServerHandler.cpp +++ b/src/ripple/rpc/impl/ServerHandler.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -46,9 +47,7 @@ #include #include #include -#include #include -#include #include namespace ripple { @@ -1149,6 +1148,12 @@ parse_Ports(Config const& config, std::ostream& log) log << "Missing section: [" << name << "]"; Throw(); } + + // grpc ports are parsed by GRPCServer class. Do not validate + // grpc port information in this file. + if (name == SECTION_PORT_GRPC) + continue; + ParsedPort parsed = common; parsed.name = name; parse_Port(parsed, config[name], log); diff --git a/src/test/jtx/envconfig.h b/src/test/jtx/envconfig.h index 59b881539e5..02c99c52f09 100644 --- a/src/test/jtx/envconfig.h +++ b/src/test/jtx/envconfig.h @@ -25,6 +25,11 @@ namespace ripple { namespace test { +// frequently used macros defined here for convinience. +#define PORT_WS "port_ws" +#define PORT_RPC "port_rpc" +#define PORT_PEER "port_peer" + extern std::atomic envUseIPv4; inline const char* diff --git a/src/test/jtx/impl/envconfig.cpp b/src/test/jtx/impl/envconfig.cpp index 7f8163f5ee7..cbca244be6d 100644 --- a/src/test/jtx/impl/envconfig.cpp +++ b/src/test/jtx/impl/envconfig.cpp @@ -20,7 +20,6 @@ #include #include -#include #include namespace ripple { @@ -57,22 +56,22 @@ setupConfigForUnitTests(Config& cfg) cfg.deprecatedClearSection(ConfigSection::importNodeDatabase()); cfg.legacy("database_path", ""); cfg.setupControl(true, true, true); - cfg["server"].append("port_peer"); - cfg["port_peer"].set("ip", getEnvLocalhostAddr()); - cfg["port_peer"].set("port", port_peer); - cfg["port_peer"].set("protocol", "peer"); - - cfg["server"].append("port_rpc"); - cfg["port_rpc"].set("ip", getEnvLocalhostAddr()); - cfg["port_rpc"].set("admin", getEnvLocalhostAddr()); - cfg["port_rpc"].set("port", port_rpc); - cfg["port_rpc"].set("protocol", "http,ws2"); - - cfg["server"].append("port_ws"); - cfg["port_ws"].set("ip", getEnvLocalhostAddr()); - cfg["port_ws"].set("admin", getEnvLocalhostAddr()); - cfg["port_ws"].set("port", port_ws); - cfg["port_ws"].set("protocol", "ws"); + cfg["server"].append(PORT_PEER); + cfg[PORT_PEER].set("ip", getEnvLocalhostAddr()); + cfg[PORT_PEER].set("port", port_peer); + cfg[PORT_PEER].set("protocol", "peer"); + + cfg["server"].append(PORT_RPC); + cfg[PORT_RPC].set("ip", getEnvLocalhostAddr()); + cfg[PORT_RPC].set("admin", getEnvLocalhostAddr()); + cfg[PORT_RPC].set("port", port_rpc); + cfg[PORT_RPC].set("protocol", "http,ws2"); + + cfg["server"].append(PORT_WS); + cfg[PORT_WS].set("ip", getEnvLocalhostAddr()); + cfg[PORT_WS].set("admin", getEnvLocalhostAddr()); + cfg[PORT_WS].set("port", port_ws); + cfg[PORT_WS].set("protocol", "ws"); cfg.SSL_VERIFY = false; } @@ -81,35 +80,35 @@ namespace jtx { std::unique_ptr no_admin(std::unique_ptr cfg) { - (*cfg)["port_rpc"].set("admin", ""); - (*cfg)["port_ws"].set("admin", ""); + (*cfg)[PORT_RPC].set("admin", ""); + (*cfg)[PORT_WS].set("admin", ""); return cfg; } std::unique_ptr secure_gateway(std::unique_ptr cfg) { - (*cfg)["port_rpc"].set("admin", ""); - (*cfg)["port_ws"].set("admin", ""); - (*cfg)["port_rpc"].set("secure_gateway", getEnvLocalhostAddr()); + (*cfg)[PORT_RPC].set("admin", ""); + (*cfg)[PORT_WS].set("admin", ""); + (*cfg)[PORT_RPC].set("secure_gateway", getEnvLocalhostAddr()); return cfg; } std::unique_ptr admin_localnet(std::unique_ptr cfg) { - (*cfg)["port_rpc"].set("admin", "127.0.0.0/8"); - (*cfg)["port_ws"].set("admin", "127.0.0.0/8"); + (*cfg)[PORT_RPC].set("admin", "127.0.0.0/8"); + (*cfg)[PORT_WS].set("admin", "127.0.0.0/8"); return cfg; } std::unique_ptr secure_gateway_localnet(std::unique_ptr cfg) { - (*cfg)["port_rpc"].set("admin", ""); - (*cfg)["port_ws"].set("admin", ""); - (*cfg)["port_rpc"].set("secure_gateway", "127.0.0.0/8"); - (*cfg)["port_ws"].set("secure_gateway", "127.0.0.0/8"); + (*cfg)[PORT_RPC].set("admin", ""); + (*cfg)[PORT_WS].set("admin", ""); + (*cfg)[PORT_RPC].set("secure_gateway", "127.0.0.0/8"); + (*cfg)[PORT_WS].set("secure_gateway", "127.0.0.0/8"); return cfg; } @@ -127,7 +126,7 @@ validator(std::unique_ptr cfg, std::string const& seed) std::unique_ptr port_increment(std::unique_ptr cfg, int increment) { - for (auto const sectionName : {"port_peer", "port_rpc", "port_ws"}) + for (auto const sectionName : {PORT_PEER, PORT_RPC, PORT_WS}) { Section& s = (*cfg)[sectionName]; auto const port = s.get("port"); @@ -143,8 +142,8 @@ std::unique_ptr addGrpcConfig(std::unique_ptr cfg) { std::string port_grpc = std::to_string(port_base + 3); - (*cfg)["port_grpc"].set("ip", getEnvLocalhostAddr()); - (*cfg)["port_grpc"].set("port", port_grpc); + (*cfg)[SECTION_PORT_GRPC].set("ip", getEnvLocalhostAddr()); + (*cfg)[SECTION_PORT_GRPC].set("port", port_grpc); return cfg; } @@ -154,9 +153,9 @@ addGrpcConfigWithSecureGateway( std::string const& secureGateway) { std::string port_grpc = std::to_string(port_base + 3); - (*cfg)["port_grpc"].set("ip", getEnvLocalhostAddr()); - (*cfg)["port_grpc"].set("port", port_grpc); - (*cfg)["port_grpc"].set("secure_gateway", secureGateway); + (*cfg)[SECTION_PORT_GRPC].set("ip", getEnvLocalhostAddr()); + (*cfg)[SECTION_PORT_GRPC].set("port", port_grpc); + (*cfg)[SECTION_PORT_GRPC].set("secure_gateway", secureGateway); return cfg; } diff --git a/src/test/rpc/ReportingETL_test.cpp b/src/test/rpc/ReportingETL_test.cpp index ed055d0fd93..078a51d7bcd 100644 --- a/src/test/rpc/ReportingETL_test.cpp +++ b/src/test/rpc/ReportingETL_test.cpp @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -56,7 +57,8 @@ class ReportingETL_test : public beast::unit_test::suite testcase("GetLedger"); using namespace test::jtx; std::unique_ptr config = envconfig(addGrpcConfig); - std::string grpcPort = *(*config)["port_grpc"].get("port"); + std::string grpcPort = + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); @@ -498,7 +500,8 @@ class ReportingETL_test : public beast::unit_test::suite testcase("GetLedgerData"); using namespace test::jtx; std::unique_ptr config = envconfig(addGrpcConfig); - std::string grpcPort = *(*config)["port_grpc"].get("port"); + std::string grpcPort = + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); auto grpcLedgerData = [&grpcPort]( auto sequence, std::string marker = "") { @@ -620,7 +623,8 @@ class ReportingETL_test : public beast::unit_test::suite testcase("GetLedgerDiff"); using namespace test::jtx; std::unique_ptr config = envconfig(addGrpcConfig); - std::string grpcPort = *(*config)["port_grpc"].get("port"); + std::string grpcPort = + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); auto grpcLedgerDiff = [&grpcPort]( @@ -735,7 +739,8 @@ class ReportingETL_test : public beast::unit_test::suite testcase("GetLedgerDiff"); using namespace test::jtx; std::unique_ptr config = envconfig(addGrpcConfig); - std::string grpcPort = *(*config)["port_grpc"].get("port"); + std::string grpcPort = + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); auto grpcLedgerEntry = [&grpcPort](auto sequence, auto key) { @@ -895,7 +900,7 @@ class ReportingETL_test : public beast::unit_test::suite std::unique_ptr config = envconfig( addGrpcConfigWithSecureGateway, getEnvLocalhostAddr()); std::string grpcPort = - *(*config)["port_grpc"].get("port"); + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); @@ -955,7 +960,7 @@ class ReportingETL_test : public beast::unit_test::suite std::unique_ptr config = envconfig(addGrpcConfigWithSecureGateway, secureGatewayIp); std::string grpcPort = - *(*config)["port_grpc"].get("port"); + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); @@ -1008,7 +1013,7 @@ class ReportingETL_test : public beast::unit_test::suite std::unique_ptr config = envconfig( addGrpcConfigWithSecureGateway, getEnvLocalhostAddr()); std::string grpcPort = - *(*config)["port_grpc"].get("port"); + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); @@ -1065,7 +1070,7 @@ class ReportingETL_test : public beast::unit_test::suite std::unique_ptr config = envconfig(addGrpcConfigWithSecureGateway, secureGatewayIp); std::string grpcPort = - *(*config)["port_grpc"].get("port"); + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); diff --git a/src/test/rpc/ServerInfo_test.cpp b/src/test/rpc/ServerInfo_test.cpp index 1d78f1cc36b..ece98e99a7e 100644 --- a/src/test/rpc/ServerInfo_test.cpp +++ b/src/test/rpc/ServerInfo_test.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -107,7 +108,7 @@ admin = 127.0.0.1 auto const rpc_port = (*config)["port_rpc"].get("port"); auto const grpc_port = - (*config)["port_grpc"].get("port"); + (*config)[SECTION_PORT_GRPC].get("port"); auto const ws_port = (*config)["port_ws"].get("port"); BEAST_EXPECT(grpc_port); BEAST_EXPECT(rpc_port); From be12136b8aa21926b45b3162249410f8779d2467 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Wed, 7 Feb 2024 16:58:12 -0500 Subject: [PATCH 7/8] `fixInnerObjTemplate`: set inner object template (#4906) Add `STObject` constructor to explicitly set the inner object template. This allows certain AMM transactions to apply in the same ledger: There is no issue if the trading fee is greater than or equal to 0.01%. If the trading fee is less than 0.01%, then: - After AMM create, AMM transactions must wait for one ledger to close (3-5 seconds). - After one ledger is validated, all AMM transactions succeed, as appropriate, except for AMMVote. - The first AMMVote which votes for a 0 trading fee in a ledger will succeed. Subsequent AMMVote transactions which vote for a 0 trading fee will wait for the next ledger (3-5 seconds). This behavior repeats for each ledger. This has no effect on the ultimate correctness of AMM. This amendment will allow the transactions described above to succeed as expected, even if the trading fee is 0 and the transactions are applied within one ledger (block). --- src/ripple/app/misc/impl/AMMUtils.cpp | 16 ++- src/ripple/app/tx/impl/AMMBid.cpp | 14 +- src/ripple/app/tx/impl/AMMVote.cpp | 9 +- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/SOTemplate.h | 2 + src/ripple/protocol/STObject.h | 6 +- src/ripple/protocol/impl/Feature.cpp | 1 + .../protocol/impl/InnerObjectFormats.cpp | 3 + src/ripple/protocol/impl/STObject.cpp | 27 +++- src/ripple/rpc/handlers/AMMInfo.cpp | 3 + src/test/app/AMM_test.cpp | 101 +++++++++++++++ src/test/jtx/AMM.h | 89 ++++++++++++- src/test/jtx/impl/AMM.cpp | 120 +++++++++++++++--- 13 files changed, 363 insertions(+), 31 deletions(-) diff --git a/src/ripple/app/misc/impl/AMMUtils.cpp b/src/ripple/app/misc/impl/AMMUtils.cpp index 1d787dbe4ca..d766bc508b6 100644 --- a/src/ripple/app/misc/impl/AMMUtils.cpp +++ b/src/ripple/app/misc/impl/AMMUtils.cpp @@ -138,6 +138,9 @@ std::uint16_t getTradingFee(ReadView const& view, SLE const& ammSle, AccountID const& account) { using namespace std::chrono; + assert( + !view.rules().enabled(fixInnerObjTemplate) || + ammSle.isFieldPresent(sfAuctionSlot)); if (ammSle.isFieldPresent(sfAuctionSlot)) { auto const& auctionSlot = @@ -287,9 +290,10 @@ initializeFeeAuctionVote( Issue const& lptIssue, std::uint16_t tfee) { + auto const& rules = view.rules(); // AMM creator gets the voting slot. STArray voteSlots; - STObject voteEntry{sfVoteEntry}; + STObject voteEntry = STObject::makeInnerObject(sfVoteEntry, rules); if (tfee != 0) voteEntry.setFieldU16(sfTradingFee, tfee); voteEntry.setFieldU32(sfVoteWeight, VOTE_WEIGHT_SCALE_FACTOR); @@ -297,7 +301,15 @@ initializeFeeAuctionVote( voteSlots.push_back(voteEntry); ammSle->setFieldArray(sfVoteSlots, voteSlots); // AMM creator gets the auction slot for free. - auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); + // AuctionSlot is created on AMMCreate and updated on AMMDeposit + // when AMM is in an empty state + if (rules.enabled(fixInnerObjTemplate) && + !ammSle->isFieldPresent(sfAuctionSlot)) + { + STObject auctionSlot = STObject::makeInnerObject(sfAuctionSlot, rules); + ammSle->set(std::move(auctionSlot)); + } + STObject& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); auctionSlot.setAccountID(sfAccount, account); // current + sec in 24h auto const expiration = std::chrono::duration_cast( diff --git a/src/ripple/app/tx/impl/AMMBid.cpp b/src/ripple/app/tx/impl/AMMBid.cpp index 822e72203a6..e49c378ceeb 100644 --- a/src/ripple/app/tx/impl/AMMBid.cpp +++ b/src/ripple/app/tx/impl/AMMBid.cpp @@ -173,8 +173,18 @@ applyBid( return {tecINTERNAL, false}; STAmount const lptAMMBalance = (*ammSle)[sfLPTokenBalance]; auto const lpTokens = ammLPHolds(sb, *ammSle, account_, ctx_.journal); - if (!ammSle->isFieldPresent(sfAuctionSlot)) - ammSle->makeFieldPresent(sfAuctionSlot); + auto const& rules = ctx_.view().rules(); + if (!rules.enabled(fixInnerObjTemplate)) + { + if (!ammSle->isFieldPresent(sfAuctionSlot)) + ammSle->makeFieldPresent(sfAuctionSlot); + } + else + { + assert(ammSle->isFieldPresent(sfAuctionSlot)); + if (!ammSle->isFieldPresent(sfAuctionSlot)) + return {tecINTERNAL, false}; + } auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot); auto const current = duration_cast( diff --git a/src/ripple/app/tx/impl/AMMVote.cpp b/src/ripple/app/tx/impl/AMMVote.cpp index ff0598aaa40..d908a93c383 100644 --- a/src/ripple/app/tx/impl/AMMVote.cpp +++ b/src/ripple/app/tx/impl/AMMVote.cpp @@ -104,6 +104,7 @@ applyVote( Number den{0}; // Account already has vote entry bool foundAccount = false; + auto const& rules = ctx_.view().rules(); // Iterate over the current vote entries and update each entry // per current total tokens balance and each LP tokens balance. // Find the entry with the least tokens and whether the account @@ -119,7 +120,7 @@ applyVote( continue; } auto feeVal = entry[sfTradingFee]; - STObject newEntry{sfVoteEntry}; + STObject newEntry = STObject::makeInnerObject(sfVoteEntry, rules); // The account already has the vote entry. if (account == account_) { @@ -158,7 +159,7 @@ applyVote( { auto update = [&](std::optional const& minPos = std::nullopt) { - STObject newEntry{sfVoteEntry}; + STObject newEntry = STObject::makeInnerObject(sfVoteEntry, rules); if (feeNew != 0) newEntry.setFieldU16(sfTradingFee, feeNew); newEntry.setFieldU32( @@ -199,6 +200,10 @@ applyVote( } } + assert( + !ctx_.view().rules().enabled(fixInnerObjTemplate) || + ammSle->isFieldPresent(sfAuctionSlot)); + // Update the vote entries and the trading/discounted fee. ammSle->setFieldArray(sfVoteSlots, updatedVoteSlots); if (auto const fee = static_cast(num / den)) diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index d8ce6dc6280..8e6483b1dbd 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 = 66; +static constexpr std::size_t numFeatures = 67; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -353,6 +353,7 @@ extern uint256 const fixDisallowIncomingV1; extern uint256 const featureDID; extern uint256 const fixFillOrKill; extern uint256 const fixNFTokenReserve; +extern uint256 const fixInnerObjTemplate; } // namespace ripple diff --git a/src/ripple/protocol/SOTemplate.h b/src/ripple/protocol/SOTemplate.h index 56dcf4492f7..609c2d2c80b 100644 --- a/src/ripple/protocol/SOTemplate.h +++ b/src/ripple/protocol/SOTemplate.h @@ -35,6 +35,8 @@ enum SOEStyle { soeREQUIRED = 0, // required soeOPTIONAL = 1, // optional, may be present with default value soeDEFAULT = 2, // optional, if present, must not have default value + // inner object with the default fields has to be + // constructed with STObject::makeInnerObject() }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/STObject.h b/src/ripple/protocol/STObject.h index 3e3862bf6c8..5476cd01198 100644 --- a/src/ripple/protocol/STObject.h +++ b/src/ripple/protocol/STObject.h @@ -43,6 +43,7 @@ namespace ripple { class STArray; +class Rules; inline void throwFieldNotFound(SField const& field) @@ -102,6 +103,9 @@ class STObject : public STBase, public CountedObject STObject(SerialIter&& sit, SField const& name); explicit STObject(SField const& name); + static STObject + makeInnerObject(SField const& name, Rules const& rules); + iterator begin() const; @@ -339,7 +343,7 @@ class STObject : public STBase, public CountedObject set(std::unique_ptr v); void - set(STBase* v); + set(STBase&& v); void setFieldU8(SField const& field, unsigned char); diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index e7cd72fb866..ab36983edd7 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -460,6 +460,7 @@ REGISTER_FIX (fixDisallowIncomingV1, Supported::yes, VoteBehavior::De REGISTER_FEATURE(DID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX(fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixNFTokenReserve, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX(fixInnerObjTemplate, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/ripple/protocol/impl/InnerObjectFormats.cpp b/src/ripple/protocol/impl/InnerObjectFormats.cpp index 58f4392f536..4350ea180d2 100644 --- a/src/ripple/protocol/impl/InnerObjectFormats.cpp +++ b/src/ripple/protocol/impl/InnerObjectFormats.cpp @@ -25,6 +25,9 @@ namespace ripple { InnerObjectFormats::InnerObjectFormats() { + // inner objects with the default fields have to be + // constructed with STObject::makeInnerObject() + add(sfSignerEntry.jsonName.c_str(), sfSignerEntry.getCode(), { diff --git a/src/ripple/protocol/impl/STObject.cpp b/src/ripple/protocol/impl/STObject.cpp index 5bafbcfce54..7c546a2568e 100644 --- a/src/ripple/protocol/impl/STObject.cpp +++ b/src/ripple/protocol/impl/STObject.cpp @@ -18,7 +18,9 @@ //============================================================================== #include +#include #include +#include #include #include #include @@ -57,6 +59,19 @@ STObject::STObject(SerialIter& sit, SField const& name, int depth) noexcept( set(sit, depth); } +STObject +STObject::makeInnerObject(SField const& name, Rules const& rules) +{ + STObject obj{name}; + if (rules.enabled(fixInnerObjTemplate)) + { + if (SOTemplate const* elements = + InnerObjectFormats::getInstance().findSOTemplateBySField(name)) + obj.set(*elements); + } + return obj; +} + STBase* STObject::copy(std::size_t n, void* buf) const { @@ -630,16 +645,22 @@ STObject::getFieldArray(SField const& field) const void STObject::set(std::unique_ptr v) { - auto const i = getFieldIndex(v->getFName()); + set(std::move(*v.get())); +} + +void +STObject::set(STBase&& v) +{ + auto const i = getFieldIndex(v.getFName()); if (i != -1) { - v_[i] = std::move(*v); + v_[i] = std::move(v); } else { if (!isFree()) Throw("missing field in templated STObject"); - v_.emplace_back(std::move(*v)); + v_.emplace_back(std::move(v)); } } diff --git a/src/ripple/rpc/handlers/AMMInfo.cpp b/src/ripple/rpc/handlers/AMMInfo.cpp index a1be636cafd..c6711fa7b82 100644 --- a/src/ripple/rpc/handlers/AMMInfo.cpp +++ b/src/ripple/rpc/handlers/AMMInfo.cpp @@ -200,6 +200,9 @@ doAMMInfo(RPC::JsonContext& context) } if (voteSlots.size() > 0) ammResult[jss::vote_slots] = std::move(voteSlots); + assert( + !ledger->rules().enabled(fixInnerObjTemplate) || + amm->isFieldPresent(sfAuctionSlot)); if (amm->isFieldPresent(sfAuctionSlot)) { auto const& auctionSlot = diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index b0e5106f9eb..b6828fab773 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4789,6 +4789,106 @@ struct AMM_test : public jtx::AMMTest } } + void + testFixDefaultInnerObj() + { + testcase("Fix Default Inner Object"); + using namespace jtx; + FeatureBitset const all{supported_amendments()}; + + auto test = [&](FeatureBitset features, + TER const& err1, + TER const& err2, + TER const& err3, + TER const& err4, + std::uint16_t tfee, + bool closeLedger, + std::optional extra = std::nullopt) { + Env env(*this, features); + fund(env, gw, {alice}, XRP(1'000), {USD(10)}); + AMM amm( + env, + gw, + XRP(10), + USD(10), + {.tfee = tfee, .close = closeLedger}); + amm.deposit(alice, USD(10), XRP(10)); + amm.vote(VoteArg{.account = alice, .tfee = tfee, .err = ter(err1)}); + amm.withdraw(WithdrawArg{ + .account = gw, .asset1Out = USD(1), .err = ter(err2)}); + // with the amendment disabled and ledger not closed, + // second vote succeeds if the first vote sets the trading fee + // to non-zero; if the first vote sets the trading fee to >0 && <9 + // then the second withdraw succeeds if the second vote sets + // the trading fee so that the discounted fee is non-zero + amm.vote(VoteArg{.account = alice, .tfee = 20, .err = ter(err3)}); + amm.withdraw(WithdrawArg{ + .account = gw, .asset1Out = USD(2), .err = ter(err4)}); + }; + + // ledger is closed after each transaction, vote/withdraw don't fail + // regardless whether the amendment is enabled or not + test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 0, true); + test( + all - fixInnerObjTemplate, + tesSUCCESS, + tesSUCCESS, + tesSUCCESS, + tesSUCCESS, + 0, + true); + // ledger is not closed after each transaction + // vote/withdraw don't fail if the amendment is enabled + test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 0, false); + // vote/withdraw fail if the amendment is not enabled + // second vote/withdraw still fail: second vote fails because + // the initial trading fee is 0, consequently second withdraw fails + // because the second vote fails + test( + all - fixInnerObjTemplate, + tefEXCEPTION, + tefEXCEPTION, + tefEXCEPTION, + tefEXCEPTION, + 0, + false); + // if non-zero trading/discounted fee then vote/withdraw + // don't fail whether the ledger is closed or not and + // the amendment is enabled or not + test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, true); + test( + all - fixInnerObjTemplate, + tesSUCCESS, + tesSUCCESS, + tesSUCCESS, + tesSUCCESS, + 10, + true); + test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 10, false); + test( + all - fixInnerObjTemplate, + tesSUCCESS, + tesSUCCESS, + tesSUCCESS, + tesSUCCESS, + 10, + false); + // non-zero trading fee but discounted fee is 0, vote doesn't fail + // but withdraw fails + test(all, tesSUCCESS, tesSUCCESS, tesSUCCESS, tesSUCCESS, 9, false); + // second vote sets the trading fee to non-zero, consequently + // second withdraw doesn't fail even if the amendment is not + // enabled and the ledger is not closed + test( + all - fixInnerObjTemplate, + tesSUCCESS, + tefEXCEPTION, + tesSUCCESS, + tesSUCCESS, + 9, + false); + } + void testCore() { @@ -4815,6 +4915,7 @@ struct AMM_test : public jtx::AMMTest testClawback(); testAMMID(); testSelection(); + testFixDefaultInnerObj(); } void diff --git a/src/test/jtx/AMM.h b/src/test/jtx/AMM.h index c7c6f3b8477..4c6e8d78a4e 100644 --- a/src/test/jtx/AMM.h +++ b/src/test/jtx/AMM.h @@ -57,6 +57,67 @@ class LPToken } }; +struct CreateArg +{ + bool log = false; + std::uint16_t tfee = 0; + std::uint32_t fee = 0; + std::optional flags = std::nullopt; + std::optional seq = std::nullopt; + std::optional ms = std::nullopt; + std::optional err = std::nullopt; + bool close = true; +}; + +struct DepositArg +{ + std::optional account = std::nullopt; + std::optional tokens = std::nullopt; + std::optional asset1In = std::nullopt; + std::optional asset2In = std::nullopt; + std::optional maxEP = std::nullopt; + std::optional flags = std::nullopt; + std::optional> assets = std::nullopt; + std::optional seq = std::nullopt; + std::optional tfee = std::nullopt; + std::optional err = std::nullopt; +}; + +struct WithdrawArg +{ + std::optional account = std::nullopt; + std::optional tokens = std::nullopt; + std::optional asset1Out = std::nullopt; + std::optional asset2Out = std::nullopt; + std::optional maxEP = std::nullopt; + std::optional flags = std::nullopt; + std::optional> assets = std::nullopt; + std::optional seq = std::nullopt; + std::optional err = std::nullopt; +}; + +struct VoteArg +{ + std::optional account = std::nullopt; + std::uint32_t tfee = 0; + std::optional flags = std::nullopt; + std::optional seq = std::nullopt; + std::optional> assets = std::nullopt; + std::optional err = std::nullopt; +}; + +struct BidArg +{ + std::optional account = std::nullopt; + std::optional> bidMin = std::nullopt; + std::optional> bidMax = std::nullopt; + std::vector authAccounts = {}; + std::optional flags = std::nullopt; + std::optional seq = std::nullopt; + std::optional> assets = std::nullopt; + std::optional err = std::nullopt; +}; + /** Convenience class to test AMM functionality. */ class AMM @@ -91,13 +152,20 @@ class AMM std::optional flags = std::nullopt, std::optional seq = std::nullopt, std::optional ms = std::nullopt, - std::optional const& ter = std::nullopt); + std::optional const& ter = std::nullopt, + bool close = true); AMM(Env& env, Account const& account, STAmount const& asset1, STAmount const& asset2, ter const& ter, - bool log = false); + bool log = false, + bool close = true); + AMM(Env& env, + Account const& account, + STAmount const& asset1, + STAmount const& asset2, + CreateArg const& arg); /** Send amm_info RPC command */ @@ -189,6 +257,9 @@ class AMM std::optional const& tfee = std::nullopt, std::optional const& ter = std::nullopt); + IOUAmount + deposit(DepositArg const& arg); + IOUAmount withdraw( std::optional const& account, @@ -200,14 +271,15 @@ class AMM IOUAmount withdrawAll( std::optional const& account, - std::optional const& asset1OutDetails = std::nullopt) + std::optional const& asset1OutDetails = std::nullopt, + std::optional const& ter = std::nullopt) { return withdraw( account, std::nullopt, asset1OutDetails, asset1OutDetails ? tfOneAssetWithdrawAll : tfWithdrawAll, - std::nullopt); + ter); } IOUAmount @@ -230,6 +302,9 @@ class AMM std::optional const& seq, std::optional const& ter = std::nullopt); + IOUAmount + withdraw(WithdrawArg const& arg); + void vote( std::optional const& account, @@ -239,6 +314,9 @@ class AMM std::optional> const& assets = std::nullopt, std::optional const& ter = std::nullopt); + void + vote(VoteArg const& arg); + void bid(std::optional const& account, std::optional> const& bidMin = @@ -251,6 +329,9 @@ class AMM std::optional> const& assets = std::nullopt, std::optional const& ter = std::nullopt); + void + bid(BidArg const& arg); + AccountID const& ammAccount() const { diff --git a/src/test/jtx/impl/AMM.cpp b/src/test/jtx/impl/AMM.cpp index dee1cb1bf5b..2d5ce90d306 100644 --- a/src/test/jtx/impl/AMM.cpp +++ b/src/test/jtx/impl/AMM.cpp @@ -57,7 +57,8 @@ AMM::AMM( std::optional flags, std::optional seq, std::optional ms, - std::optional const& ter) + std::optional const& ter, + bool close) : env_(env) , creatorAccount_(account) , asset1_(asset1) @@ -65,7 +66,7 @@ AMM::AMM( , ammID_(keylet::amm(asset1_.issue(), asset2_.issue()).key) , initialLPTokens_(initialTokens(asset1, asset2)) , log_(log) - , doClose_(true) + , doClose_(close) , lastPurchasePrice_(0) , bidMin_() , bidMax_() @@ -85,7 +86,8 @@ AMM::AMM( STAmount const& asset1, STAmount const& asset2, ter const& ter, - bool log) + bool log, + bool close) : AMM(env, account, asset1, @@ -96,7 +98,29 @@ AMM::AMM( std::nullopt, std::nullopt, std::nullopt, - ter) + ter, + close) +{ +} + +AMM::AMM( + Env& env, + Account const& account, + STAmount const& asset1, + STAmount const& asset2, + CreateArg const& arg) + : AMM(env, + account, + asset1, + asset2, + arg.log, + arg.tfee, + arg.fee, + arg.flags, + arg.seq, + arg.ms, + arg.err, + arg.close) { } @@ -470,6 +494,22 @@ AMM::deposit( return deposit(account, jv, assets, seq, ter); } +IOUAmount +AMM::deposit(DepositArg const& arg) +{ + return deposit( + arg.account, + arg.tokens, + arg.asset1In, + arg.asset2In, + arg.maxEP, + arg.flags, + arg.assets, + arg.seq, + arg.tfee, + arg.err); +} + IOUAmount AMM::withdraw( std::optional const& account, @@ -574,6 +614,21 @@ AMM::withdraw( return withdraw(account, jv, seq, assets, ter); } +IOUAmount +AMM::withdraw(WithdrawArg const& arg) +{ + return withdraw( + arg.account, + arg.tokens, + arg.asset1Out, + arg.asset2Out, + arg.maxEP, + arg.flags, + arg.assets, + arg.seq, + arg.err); +} + void AMM::vote( std::optional const& account, @@ -595,6 +650,12 @@ AMM::vote( submit(jv, seq, ter); } +void +AMM::vote(VoteArg const& arg) +{ + return vote(arg.account, arg.tfee, arg.flags, arg.seq, arg.assets, arg.err); +} + void AMM::bid( std::optional const& account, @@ -609,6 +670,9 @@ AMM::bid( if (auto const amm = env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()))) { + assert( + !env_.current()->rules().enabled(fixInnerObjTemplate) || + amm->isFieldPresent(sfAuctionSlot)); if (amm->isFieldPresent(sfAuctionSlot)) { auto const& auctionSlot = @@ -663,6 +727,20 @@ AMM::bid( submit(jv, seq, ter); } +void +AMM::bid(BidArg const& arg) +{ + return bid( + arg.account, + arg.bidMin, + arg.bidMax, + arg.authAccounts, + arg.flags, + arg.seq, + arg.assets, + arg.err); +} + void AMM::submit( Json::Value const& jv, @@ -698,20 +776,30 @@ bool AMM::expectAuctionSlot(auto&& cb) const { if (auto const amm = - env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue())); - amm && amm->isFieldPresent(sfAuctionSlot)) + env_.current()->read(keylet::amm(asset1_.issue(), asset2_.issue()))) { - auto const& auctionSlot = - static_cast(amm->peekAtField(sfAuctionSlot)); - if (auctionSlot.isFieldPresent(sfAccount)) + assert( + !env_.current()->rules().enabled(fixInnerObjTemplate) || + amm->isFieldPresent(sfAuctionSlot)); + if (amm->isFieldPresent(sfAuctionSlot)) { - auto const slotFee = auctionSlot[sfDiscountedFee]; - auto const slotInterval = ammAuctionTimeSlot( - env_.app().timeKeeper().now().time_since_epoch().count(), - auctionSlot); - auto const slotPrice = auctionSlot[sfPrice].iou(); - auto const authAccounts = auctionSlot.getFieldArray(sfAuthAccounts); - return cb(slotFee, slotInterval, slotPrice, authAccounts); + auto const& auctionSlot = + static_cast(amm->peekAtField(sfAuctionSlot)); + if (auctionSlot.isFieldPresent(sfAccount)) + { + // This could fail in pre-fixInnerObjTemplate tests + // if the submitted transactions recreate one of + // the failure scenarios. Access as optional + // to avoid the failure. + auto const slotFee = auctionSlot[~sfDiscountedFee].value_or(0); + auto const slotInterval = ammAuctionTimeSlot( + env_.app().timeKeeper().now().time_since_epoch().count(), + auctionSlot); + auto const slotPrice = auctionSlot[sfPrice].iou(); + auto const authAccounts = + auctionSlot.getFieldArray(sfAuthAccounts); + return cb(slotFee, slotInterval, slotPrice, authAccounts); + } } } return false; From da68651f612371d082d7055439f7c55e4358eba9 Mon Sep 17 00:00:00 2001 From: Elliot Lee Date: Wed, 7 Feb 2024 13:59:44 -0800 Subject: [PATCH 8/8] Set version to 2.1.0-rc1 --- src/ripple/protocol/impl/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/protocol/impl/BuildInfo.cpp b/src/ripple/protocol/impl/BuildInfo.cpp index e3102282579..be7404c7d40 100644 --- a/src/ripple/protocol/impl/BuildInfo.cpp +++ b/src/ripple/protocol/impl/BuildInfo.cpp @@ -33,7 +33,7 @@ namespace BuildInfo { // and follow the format described at http://semver.org/ //------------------------------------------------------------------------------ // clang-format off -char const* const versionString = "2.0.1" +char const* const versionString = "2.1.0-rc1" // clang-format on #if defined(DEBUG) || defined(SANITIZER)