Enforce account RPC limits by account objects traversed #4032

natenichols · 2021-12-14T17:51:37Z

High Level Overview of Change

Modifies account RPCs to enforce limits by number of account objects traversed.

Context of Change

Previously, we limited account RPCs by the number of objects returned, not the number of objects traversed. This can get expensive when traversing accounts with a lot of owned objects. This PR modifies this behavior to enforce limits based on number of objects traversed. It also slightly changes how we do markers, including the marker startHint in the marker, similar to how account_objects works.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Test Plan

If helpful, please describe the tests that you ran to verify your changes and provide instructions so that others can reproduce.
This section may not be needed if your change includes thoroughly commented unit tests.

Some tests had to be modified, including increasing limits and changing markers.

src/ripple/rpc/impl/RPCHelpers.cpp

ximinez

Looking at these three handlers together, I realize there's a ton of overlapping / boilerplate code that could probably be pulled into one or more common helper functions. That may be beyond the scope of this PR, but it would be pretty cool.

Otherwise, all of my feedback items are minor - comments, variable names, and such.

src/ripple/rpc/handlers/AccountChannels.cpp

src/ripple/rpc/handlers/AccountLines.cpp

src/ripple/rpc/handlers/AccountOffers.cpp

src/ripple/rpc/impl/RPCHelpers.h

This flag, if present, suppresses the output of incoming trustlines in default state. This is primarily motivated by observing that users of Xumm often have many unwanted incoming trustlines in a default state, which are not useful in the vast majority of cases. Being able to suppress those when doing `account_lines` saves bandwidth and resources.

* Just the rep is made atomic to workaround older compilers

seperated -> separated

determing -> determining

* Log load fee values (at debug) received from validations. * Log remote and cluster fee values (at trace) when changed. * Refactor JobQueue::isOverloaded to return sooner if overloaded. * Refactor Transactor::checkFee to only compute fee if ledger is open.

* Sort by fee level (which is the current behavior) then by transaction ID (hash). * Edge case when the account at the end of the queue submits a higher paying transaction to walk backwards and compare against the cheapest transaction from a different account. * Use std::if_any to simplify the JobQueue::isOverloaded loop.

src/ripple/rpc/impl/RPCHelpers.cpp

scottschurr

I've finished my review and I'm okay with what's here. I'd be a lot more comfortable if we check isFieldPresent in getStartHint(). So I hope you'll look at the suggestions and see what you think. But, I'll acknowledge, there's also risk in changing stuff that's been well tested.

src/ripple/rpc/handlers/AccountChannels.cpp

src/ripple/rpc/handlers/AccountLines.cpp

src/ripple/rpc/handlers/AccountOffers.cpp

src/ripple/rpc/impl/RPCHelpers.cpp

src/test/rpc/AccountLinesRPC_test.cpp

cjcobb23 · 2021-12-15T17:23:18Z

Just want to point out this change actually changes the behavior of the RPCs from a client perspective. The call will return less objects than what is specified in limit. It could even return no objects, but a marker, in which case a user should continue paging. None of this is actually API breaking, as the server is not required to honor the limit. I think these changes are appropriate and beneficial, but just wanted to make sure everyone is aware. @mDuo13 chime in if you object to these changes.

natenichols · 2021-12-15T17:30:37Z

@cjcobb23, I'm okay to wait on this PR until 1.8.3, its already running in reporting mode, which is where it is most important.

If we don't feel comfortable shipping it right now, we can wait until the next release. Thoughts?

cjcobb23 · 2021-12-15T17:38:17Z

@natenichols I would like to ship it now, because we are trying to make it easy for others to run reporting mode. I think this will also benefit regular rippled.

ximinez

👍 I think this is appropriate for 1.8.2, FWIW.

scottschurr · 2021-12-15T18:06:50Z

@natenichols, the latest changes look good to me.

Thanks for adding the unit test, however I don't think it is testing as much as you hope for. When I check code coverage I see that, even with the added test, none of the non-manual unit tests exercise the ltSIGNER_LIST branch of isOwnedByAccount().

I'll poke at a way to fix the new unit test so it exercises that branch. But in the mean time, know that we haven't verified that part of the code.

mDuo13

At a glance this looks fine, though it's a bit hard for me to figure out all the consequences this might have on the API.

Are the markers portable across servers, or not? Say you connect via JSON-RPC to a hostname that is backed by a round-robin pool of servers. If you do a request and get a marker back, but then your follow-up request using the marker goes to a different machine, will pagination continue smoothly? Ideally I think all markers should be portable across machines, but that's not a guarantee we provide for all calls. (The account_tx command does guarantee portability of its markers, but that's a different story.)

mDuo13 · 2021-12-15T20:11:42Z

src/ripple/rpc/impl/Tuning.h

@@ -46,7 +46,7 @@ static LimitRange constexpr accountObjects = {10, 200, 400};
 static LimitRange constexpr accountOffers = {10, 200, 400};

 /** Limits for the book_offers command. */
-static LimitRange constexpr bookOffers = {0, 300, 400};
+static LimitRange constexpr bookOffers = {0, 60, 100};


What happens if you specify a limit=200 for example? To my understanding, that would have been allowed in the old API but it's too large in the new API. Depending on the behavior, this may or may not be a breaking change that requires clients to be rewritten:

If the limit is quietly clamped to the new max limit of 100, that's OK. Clients already need to assume that the API may return fewer than their specified limit.

If the command now returns an error like rpcINVALID_PARAMS, that's a problematic breaking change.

Limits are quietly clamped.

These limits also are only applied to non-admin clients

natenichols · 2021-12-15T20:29:57Z

Are the markers portable across servers, or not?

No, they are not portable across servers. There is not a scenario where these changes could make markers portable, since now markers can be the index of a non-RippleState SLE, which is not allowed pre 1.8.2.

If you do a request and get a marker back, but then your follow-up request using the marker goes to a different machine, will pagination continue smoothly?

Yes, as long as all servers in the same cluster are running 1.8.2. If there are different versions of rippled running in the same cluster, then no, because markers are not portable.

natenichols self-assigned this Dec 14, 2021

ximinez self-requested a review December 14, 2021 21:57

ximinez self-assigned this Dec 14, 2021

scottschurr self-requested a review December 14, 2021 23:15

scottschurr reviewed Dec 15, 2021

View reviewed changes

src/ripple/rpc/impl/RPCHelpers.cpp Show resolved Hide resolved

RichardAH added 2 commits December 14, 2021 17:42

Add git commit hash to debug version string

bf013c0

Configurable handling of untrusted validations and proposals

6746b86

ximinez requested changes Dec 15, 2021

View reviewed changes

RichardAH and others added 15 commits December 14, 2021 17:43

Make I/O and prefetch worker threads configurable

cf97dcb

Make basic_seconds_clock::time_point atomic

32a26a6

* Just the rep is made atomic to workaround older compilers

Fix typo in RFC1751.cpp

6f6179a

seperated -> separated

Fix typo in consensus.md

81e7ec8

determing -> determining

Update README.md

b00787e

Add load cost

eb17325

Improve handling of the X-Forwarded-For and Forwarded HTTP headers

ad805eb

NuDBBackend destructor should not throw

8ca2d98

Consensus transaction recovery/deferral completely ignores the TxQ

ae9930b

Reduce TxQ logging severity in several places

8fa3379

Increase TxQ default minimum and target sizes

e3acb61

Improve full & compressed inner node deserialization

45aa014

scottschurr reviewed Dec 15, 2021

View reviewed changes

src/ripple/rpc/impl/RPCHelpers.cpp Outdated Show resolved Hide resolved

scottschurr approved these changes Dec 15, 2021

View reviewed changes

natenichols requested a review from ximinez December 15, 2021 15:40

ximinez requested changes Dec 15, 2021

View reviewed changes

ximinez approved these changes Dec 15, 2021

View reviewed changes

natenichols added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Dec 15, 2021

CJ Cobb and others added 7 commits December 15, 2021 11:18

Only forward to p2p nodes that are in sync

47376a0

Reporting mode always returns age in server_info.

fc04336

Improve names returned by server_info counters

d54f627

Make tx() function against a read-only postgres instance.

c663f1f

make cassandra io threads configurable

72752b1

Log resource limit disconnections.

db720a5

log request and duration for every RPC call

915fe31

mDuo13 reviewed Dec 15, 2021

View reviewed changes

natenichols force-pushed the account-rpc-limits branch from cdd9601 to d03b771 Compare December 15, 2021 20:52

manojsdoshi mentioned this pull request Dec 15, 2021

Proposed 1.8.2-rc1 #4039

Closed

natenichols force-pushed the account-rpc-limits branch from dc77f16 to f0cc98f Compare December 16, 2021 01:05

manojsdoshi mentioned this pull request Dec 16, 2021

Proposed 1.8.2-rc1 #4040

Merged

Enforce account RPC limits by account objects traversed

bf540e9

natenichols force-pushed the account-rpc-limits branch from f0cc98f to bf540e9 Compare December 16, 2021 16:09

manojsdoshi closed this in #4040 Dec 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce account RPC limits by account objects traversed #4032

Enforce account RPC limits by account objects traversed #4032

natenichols commented Dec 14, 2021

ximinez left a comment

scottschurr left a comment

cjcobb23 commented Dec 15, 2021

natenichols commented Dec 15, 2021 •

edited

Loading

cjcobb23 commented Dec 15, 2021

ximinez left a comment

scottschurr commented Dec 15, 2021

mDuo13 left a comment

mDuo13 Dec 15, 2021

natenichols Dec 15, 2021

natenichols Dec 15, 2021

natenichols commented Dec 15, 2021 •

edited

Loading

Enforce account RPC limits by account objects traversed #4032

Enforce account RPC limits by account objects traversed #4032

Conversation

natenichols commented Dec 14, 2021

High Level Overview of Change

Context of Change

Type of Change

Test Plan

ximinez left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

cjcobb23 commented Dec 15, 2021

natenichols commented Dec 15, 2021 • edited Loading

cjcobb23 commented Dec 15, 2021

ximinez left a comment

Choose a reason for hiding this comment

scottschurr commented Dec 15, 2021

mDuo13 left a comment

Choose a reason for hiding this comment

mDuo13 Dec 15, 2021

Choose a reason for hiding this comment

natenichols Dec 15, 2021

Choose a reason for hiding this comment

natenichols Dec 15, 2021

Choose a reason for hiding this comment

natenichols commented Dec 15, 2021 • edited Loading

natenichols commented Dec 15, 2021 •

edited

Loading

natenichols commented Dec 15, 2021 •

edited

Loading