From 9b2d563decfad222a5a7e6d47dd05f935b95a07f Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 20 Mar 2023 10:22:15 -0700 Subject: [PATCH] fix: support RPC markers for any ledger object: (#4361) There were situations where `marker`s returned by `account_lines` did not work on subsequent requests, returning "Invalid Parameters". This was caused by the optimization implemented in "Enforce account RPC limits by account objects traversed": https://github.com/XRPLF/rippled/pull/4040/commits/e28989638d56d31cc9225da220a5bbe5bb1a046e?diff=unified&w=1 Previously, the ledger traversal would find up to `limit` account lines, and if there were more, the marker would be derived from the key of the next account line. After the change, ledger traversal would _consider_ up to `limit` account objects of any kind found in the account's directory structure. If there were more, the marker would be derived from the key of the next object, regardless of type. With this optimization, it is expected that `account_lines` may return fewer than `limit` account lines - even 0 - along with a marker indicating that there are may be more available. The problem is that this optimization did not update the `RPC::isOwnedByAccount` helper function to handle those other object types. Additionally, XLS-20 added `ltNFTOKEN_OFFER` ledger objects to objects that have been added to the account's directory structure, but did not update `RPC::isOwnedByAccount` to be able to handle those objects. The `marker` provided in the example for #4354 includes the key for an `ltNFTOKEN_OFFER`. When that `marker` is used on subsequent calls, it is not recognized as valid, and so the request fails. * Add unit test that walks all the object types and verifies that all of their indexes can work as a marker. * Fix #4340 * Fix #4354 --- src/ripple/rpc/handlers/AccountChannels.cpp | 2 +- src/ripple/rpc/handlers/AccountLines.cpp | 2 +- src/ripple/rpc/handlers/AccountOffers.cpp | 2 +- src/ripple/rpc/impl/RPCHelpers.cpp | 19 +- src/ripple/rpc/impl/RPCHelpers.h | 2 +- src/test/rpc/AccountLinesRPC_test.cpp | 238 +++++++++++++++++++- 6 files changed, 257 insertions(+), 8 deletions(-) diff --git a/src/ripple/rpc/handlers/AccountChannels.cpp b/src/ripple/rpc/handlers/AccountChannels.cpp index e5059d3ffc..9e5c9ca2c4 100644 --- a/src/ripple/rpc/handlers/AccountChannels.cpp +++ b/src/ripple/rpc/handlers/AccountChannels.cpp @@ -151,7 +151,7 @@ doAccountChannels(RPC::JsonContext& context) if (!sle) return rpcError(rpcINVALID_PARAMS); - if (!RPC::isOwnedByAccount(*ledger, sle, accountID)) + if (!RPC::isRelatedToAccount(*ledger, sle, accountID)) return rpcError(rpcINVALID_PARAMS); } diff --git a/src/ripple/rpc/handlers/AccountLines.cpp b/src/ripple/rpc/handlers/AccountLines.cpp index 364d40673f..adba2acaa7 100644 --- a/src/ripple/rpc/handlers/AccountLines.cpp +++ b/src/ripple/rpc/handlers/AccountLines.cpp @@ -177,7 +177,7 @@ doAccountLines(RPC::JsonContext& context) if (!sle) return rpcError(rpcINVALID_PARAMS); - if (!RPC::isOwnedByAccount(*ledger, sle, accountID)) + if (!RPC::isRelatedToAccount(*ledger, sle, accountID)) return rpcError(rpcINVALID_PARAMS); } diff --git a/src/ripple/rpc/handlers/AccountOffers.cpp b/src/ripple/rpc/handlers/AccountOffers.cpp index e957fe8a8e..409d071fb0 100644 --- a/src/ripple/rpc/handlers/AccountOffers.cpp +++ b/src/ripple/rpc/handlers/AccountOffers.cpp @@ -128,7 +128,7 @@ doAccountOffers(RPC::JsonContext& context) if (!sle) return rpcError(rpcINVALID_PARAMS); - if (!RPC::isOwnedByAccount(*ledger, sle, accountID)) + if (!RPC::isRelatedToAccount(*ledger, sle, accountID)) return rpcError(rpcINVALID_PARAMS); } diff --git a/src/ripple/rpc/impl/RPCHelpers.cpp b/src/ripple/rpc/impl/RPCHelpers.cpp index 3d1bfe6375..ad84d7b12f 100644 --- a/src/ripple/rpc/impl/RPCHelpers.cpp +++ b/src/ripple/rpc/impl/RPCHelpers.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -109,7 +110,7 @@ getStartHint(std::shared_ptr const& sle, AccountID const& accountID) } bool -isOwnedByAccount( +isRelatedToAccount( ReadView const& ledger, std::shared_ptr const& sle, AccountID const& accountID) @@ -121,13 +122,27 @@ isOwnedByAccount( } else if (sle->isFieldPresent(sfAccount)) { - return sle->getAccountID(sfAccount) == accountID; + // If there's an sfAccount present, also test the sfDestination, if + // present. This will match objects such as Escrows (ltESCROW), Payment + // Channels (ltPAYCHAN), and Checks (ltCHECK) because those are added to + // the Destination account's directory. It intentionally EXCLUDES + // NFToken Offers (ltNFTOKEN_OFFER). NFToken Offers are NOT added to the + // Destination account's directory. + return sle->getAccountID(sfAccount) == accountID || + (sle->isFieldPresent(sfDestination) && + sle->getAccountID(sfDestination) == accountID); } else if (sle->getType() == ltSIGNER_LIST) { Keylet const accountSignerList = keylet::signers(accountID); return sle->key() == accountSignerList.key; } + else if (sle->getType() == ltNFTOKEN_OFFER) + { + // Do not check the sfDestination field. NFToken Offers are NOT added to + // the Destination account's directory. + return sle->getAccountID(sfOwner) == accountID; + } return false; } diff --git a/src/ripple/rpc/impl/RPCHelpers.h b/src/ripple/rpc/impl/RPCHelpers.h index 2aa62f3474..12f27641dd 100644 --- a/src/ripple/rpc/impl/RPCHelpers.h +++ b/src/ripple/rpc/impl/RPCHelpers.h @@ -86,7 +86,7 @@ getStartHint(std::shared_ptr const& sle, AccountID const& accountID); * @param account - The account being tested for SLE ownership. */ bool -isOwnedByAccount( +isRelatedToAccount( ReadView const& ledger, std::shared_ptr const& sle, AccountID const& accountID); diff --git a/src/test/rpc/AccountLinesRPC_test.cpp b/src/test/rpc/AccountLinesRPC_test.cpp index bdd376b3aa..cdc6192209 100644 --- a/src/test/rpc/AccountLinesRPC_test.cpp +++ b/src/test/rpc/AccountLinesRPC_test.cpp @@ -33,7 +33,7 @@ public: void testAccountLines() { - testcase("acccount_lines"); + testcase("account_lines"); using namespace test::jtx; Env env(*this); @@ -524,11 +524,244 @@ public: RPC::make_error(rpcINVALID_PARAMS)[jss::error_message]); } + void + testAccountLinesWalkMarkers() + { + testcase("Marker can point to any appropriate ledger entry type"); + using namespace test::jtx; + using namespace std::chrono_literals; + Env env(*this); + + // The goal of this test is observe account_lines RPC calls return an + // error message when the SLE pointed to by the marker is not owned by + // the Account being traversed. + // + // To start, we'll create an environment with some trust lines, offers + // and a signers list. + Account const alice{"alice"}; + Account const becky{"becky"}; + Account const gw1{"gw1"}; + env.fund(XRP(10000), alice, becky, gw1); + env.close(); + + // A couple of helper lambdas + auto escrow = [&env]( + Account const& account, + Account const& to, + STAmount const& amount) { + Json::Value jv; + jv[jss::TransactionType] = jss::EscrowCreate; + jv[jss::Flags] = tfUniversal; + jv[jss::Account] = account.human(); + jv[jss::Destination] = to.human(); + jv[jss::Amount] = amount.getJson(JsonOptions::none); + NetClock::time_point finish = env.now() + 1s; + jv[sfFinishAfter.jsonName] = finish.time_since_epoch().count(); + return jv; + }; + + auto payChan = [](Account const& account, + Account const& to, + STAmount const& amount, + NetClock::duration const& settleDelay, + PublicKey const& pk) { + Json::Value jv; + jv[jss::TransactionType] = jss::PaymentChannelCreate; + jv[jss::Flags] = tfUniversal; + jv[jss::Account] = account.human(); + jv[jss::Destination] = to.human(); + jv[jss::Amount] = amount.getJson(JsonOptions::none); + jv["SettleDelay"] = settleDelay.count(); + jv["PublicKey"] = strHex(pk.slice()); + return jv; + }; + + // Test all available object types. Not all of these objects will be + // included in the search, nor found by `account_objects`. If that ever + // changes for any reason, this test will help catch that. + // + // SignerList, for alice + Account const bogie{"bogie"}; + env(signers(alice, 2, {{bogie, 3}})); + env.close(); + + // SignerList, includes alice + env(signers(becky, 2, {{alice, 3}})); + env.close(); + + // Trust lines + auto const EUR = gw1["EUR"]; + env(trust(alice, EUR(200))); + env(trust(becky, EUR(200))); + env.close(); + + // Escrow, in each direction + env(escrow(alice, becky, XRP(1000))); + env(escrow(becky, alice, XRP(1000))); + + // Pay channels, in each direction + env(payChan(alice, becky, XRP(1000), 100s, alice.pk())); + env(payChan(becky, alice, XRP(1000), 100s, becky.pk())); + + // Mint NFTs, for each account + uint256 const aliceNFtokenID = + token::getNextID(env, alice, 0, tfTransferable); + env(token::mint(alice, 0), txflags(tfTransferable)); + + uint256 const beckyNFtokenID = + token::getNextID(env, becky, 0, tfTransferable); + env(token::mint(becky, 0), txflags(tfTransferable)); + + // NFT Offers, for each other's NFTs + env(token::createOffer(alice, beckyNFtokenID, drops(1)), + token::owner(becky)); + env(token::createOffer(becky, aliceNFtokenID, drops(1)), + token::owner(alice)); + + env(token::createOffer(becky, beckyNFtokenID, drops(1)), + txflags(tfSellNFToken), + token::destination(alice)); + env(token::createOffer(alice, aliceNFtokenID, drops(1)), + txflags(tfSellNFToken), + token::destination(becky)); + + env(token::createOffer(gw1, beckyNFtokenID, drops(1)), + token::owner(becky), + token::destination(alice)); + env(token::createOffer(gw1, aliceNFtokenID, drops(1)), + token::owner(alice), + token::destination(becky)); + + env(token::createOffer(becky, beckyNFtokenID, drops(1)), + txflags(tfSellNFToken)); + env(token::createOffer(alice, aliceNFtokenID, drops(1)), + txflags(tfSellNFToken)); + + // Checks, in each direction + env(check::create(alice, becky, XRP(50))); + env(check::create(becky, alice, XRP(50))); + + // Deposit preauth, in each direction + env(deposit::auth(alice, becky)); + env(deposit::auth(becky, alice)); + + // Offers, one where alice is the owner, and one where alice is the + // issuer + auto const USDalice = alice["USD"]; + env(offer(alice, EUR(10), XRP(100))); + env(offer(becky, USDalice(10), XRP(100))); + + // Tickets + env(ticket::create(alice, 2)); + + // Add another trustline for good measure + auto const BTCbecky = becky["BTC"]; + env(trust(alice, BTCbecky(200))); + + env.close(); + + { + // Now make repeated calls to `account_lines` with a limit of 1. + // That should iterate all of alice's relevant objects, even though + // the list will be empty for most calls. + auto getNextLine = [](Env& env, + Account const& alice, + std::optional const marker) { + Json::Value params(Json::objectValue); + params[jss::account] = alice.human(); + params[jss::limit] = 1; + if (marker) + params[jss::marker] = *marker; + + return env.rpc("json", "account_lines", to_string(params)); + }; + + auto aliceLines = getNextLine(env, alice, std::nullopt); + constexpr std::size_t expectedIterations = 16; + constexpr std::size_t expectedLines = 2; + std::size_t foundLines = 0; + + auto hasMarker = [](auto const& aliceLines) { + return aliceLines[jss::result].isMember(jss::marker); + }; + auto marker = [](auto const& aliceLines) { + return aliceLines[jss::result][jss::marker].asString(); + }; + auto checkLines = [](auto const& aliceLines) { + return aliceLines.isMember(jss::result) && + !aliceLines[jss::result].isMember(jss::error_message) && + aliceLines[jss::result].isMember(jss::lines) && + aliceLines[jss::result][jss::lines].isArray() && + aliceLines[jss::result][jss::lines].size() <= 1; + }; + + BEAST_EXPECT(hasMarker(aliceLines)); + BEAST_EXPECT(checkLines(aliceLines)); + BEAST_EXPECT(aliceLines[jss::result][jss::lines].size() == 0); + + int iterations = 1; + + while (hasMarker(aliceLines)) + { + // Iterate through the markers + aliceLines = getNextLine(env, alice, marker(aliceLines)); + BEAST_EXPECT(checkLines(aliceLines)); + foundLines += aliceLines[jss::result][jss::lines].size(); + ++iterations; + } + BEAST_EXPECT(expectedLines == foundLines); + + Json::Value const aliceObjects = env.rpc( + "json", + "account_objects", + R"({"account": ")" + alice.human() + + R"(", )" + R"("limit": 200})"); + BEAST_EXPECT(aliceObjects.isMember(jss::result)); + BEAST_EXPECT( + !aliceObjects[jss::result].isMember(jss::error_message)); + BEAST_EXPECT( + aliceObjects[jss::result].isMember(jss::account_objects)); + BEAST_EXPECT( + aliceObjects[jss::result][jss::account_objects].isArray()); + // account_objects does not currently return NFTPages. If + // that ever changes, without also changing account_lines, + // this test will need to be updated. + BEAST_EXPECT( + aliceObjects[jss::result][jss::account_objects].size() == + iterations); + // If ledger object association ever changes, for whatever + // reason, this test will need to be updated. + BEAST_EXPECTS( + iterations == expectedIterations, std::to_string(iterations)); + + // Get becky's objects just to confirm that they're symmetrical + Json::Value const beckyObjects = env.rpc( + "json", + "account_objects", + R"({"account": ")" + becky.human() + + R"(", )" + R"("limit": 200})"); + BEAST_EXPECT(beckyObjects.isMember(jss::result)); + BEAST_EXPECT( + !beckyObjects[jss::result].isMember(jss::error_message)); + BEAST_EXPECT( + beckyObjects[jss::result].isMember(jss::account_objects)); + BEAST_EXPECT( + beckyObjects[jss::result][jss::account_objects].isArray()); + // becky should have the same number of objects as alice, except the + // 2 tickets that only alice created. + BEAST_EXPECT( + beckyObjects[jss::result][jss::account_objects].size() == + aliceObjects[jss::result][jss::account_objects].size() - 2); + } + } + // test API V2 void testAccountLines2() { - testcase("V2: acccount_lines"); + testcase("V2: account_lines"); using namespace test::jtx; Env env(*this); @@ -1234,6 +1467,7 @@ public: testAccountLines(); testAccountLinesMarker(); testAccountLineDelete(); + testAccountLinesWalkMarkers(); testAccountLines2(); testAccountLineDelete2(); }