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":

e289896?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

@@ -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);
     }
 

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);
     }
 

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);
     }
 

src/ripple/rpc/impl/RPCHelpers.cpp

@@ -23,6 +23,7 @@
 #include <ripple/app/misc/Transaction.h>
 #include <ripple/app/paths/TrustLine.h>
 #include <ripple/app/rdb/RelationalDatabase.h>
+#include <ripple/app/tx/impl/details/NFTokenUtils.h>
 #include <ripple/ledger/View.h>
 #include <ripple/net/RPCErr.h>
 #include <ripple/protocol/AccountID.h>
@@ -109,7 +110,7 @@ getStartHint(std::shared_ptr<SLE const> const& sle, AccountID const& accountID)
 }
 
 bool
-isOwnedByAccount(
+isRelatedToAccount(
     ReadView const& ledger,
     std::shared_ptr<SLE const> 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;
 }

src/ripple/rpc/impl/RPCHelpers.h

@@ -86,7 +86,7 @@ getStartHint(std::shared_ptr<SLE const> const& sle, AccountID const& accountID);
  * @param account - The account being tested for SLE ownership.
  */
 bool
-isOwnedByAccount(
+isRelatedToAccount(
     ReadView const& ledger,
     std::shared_ptr<SLE const> const& sle,
     AccountID const& accountID);

src/test/rpc/AccountLinesRPC_test.cpp

@@ -33,7 +33,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite
     void
     testAccountLines()
     {
-        testcase("acccount_lines");
+        testcase("account_lines");
 
         using namespace test::jtx;
         Env env(*this);
@@ -524,11 +524,244 @@ class AccountLinesRPC_test : public beast::unit_test::suite
             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<std::string> 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 @@ class AccountLinesRPC_test : public beast::unit_test::suite
         testAccountLines();
         testAccountLinesMarker();
         testAccountLineDelete();
+        testAccountLinesWalkMarkers();
         testAccountLines2();
         testAccountLineDelete2();
     }