Add an RPC API that can query the list of Top N secondary index keys and their sizes #28887

IntokuSatori · 2022-11-19T01:45:23Z

Problem

Secondary account indexes can become quite large for certain keys which have many associated accounts in the system and thus require many GB of ram to store and can cause the validator to OOM. When the RPC call is made to the validator it also will just spin in a core and allocate memory and the resulting request will be too large to transmit over a single RPC request requiring many seconds and hitting connection timeouts. So often even indexing these keys is not useful at least with the current apis.

Summary of Changes

Add an RPC API that can query the list of secondary index keys and their sizes such that we can figure out which keys are taking a lot of ram and add them to the exclusion list.

Fixes #
#28666

mvines · 2022-11-29T00:22:43Z

Just a thought, this proposed RPC API is really just an admin function correct? If so did you consider adding this to the IPC API instead, such that can be accessible to the node operator without adding to the public RPC API surface

IntokuSatori · 2022-11-29T00:30:18Z

Just a thought, this proposed RPC API is really just an admin function correct? If so did you consider adding this to the IPC API instead, such that can be accessible to the node operator without adding to the public RPC API surface

I was not even aware of the IPC API, @CriesofCarrots can you comment? I guess I can move it, my one worry though is how much work I put into enabling secondary indexes on the RPC test infrastructure in a prior PR that I made. Would I need to re-do all that work to support testing under the IPC API? If so I'd prefer to leave it here where I already have the unit test infra in place to support it.

One thing I know is that this is not exposed on the RPC client interface, tho it does expose it as a new RPC API call that could be added to the RPC client later.

CriesofCarrots · 2022-11-29T00:33:43Z

Just a thought, this proposed RPC API is really just an admin function correct? If so did you consider adding this to the IPC API instead, such that can be accessible to the node operator without adding to the public RPC API surface

Interesting, not a bad idea @mvines , but do we need an additional set of apis on AdminRpc that is only pulled in for RPC nodes? Because this is one that won't be relevant to most validators (only those running with the AccountsData set of rpc endpoints).

mvines · 2022-11-29T00:34:36Z

was not even aware of the IPC API, @CriesofCarrots can you comment?

This guy: https://github.com/solana-labs/solana/blob/master/validator/src/admin_rpc_service.rs

Here's an example of adding to it: #22087

mvines · 2022-11-29T00:36:27Z

do we need an additional set of apis on AdminRpc that is only pulled in for RPC nodes?

I guess the AdminRpc endpoint could be disabled when the RPC subsystem isn't active.

But this way at least we don't need to worry about another RPC bug bounty when somebody finds a way to DoS node with new RPC endpoints that are really only for admin usage :)

IntokuSatori · 2022-11-29T00:45:31Z

This guy: https://github.com/solana-labs/solana/blob/master/validator/src/admin_rpc_service.rs

I don't see any notion of Secondary Indexes in this. This relies on those. Not sure if its even possible to add here without making it aware of those.

CriesofCarrots · 2022-11-29T01:19:49Z

I don't see any notion of Secondary Indexes in this. This relies on those. Not sure if its even possible to add here without making it aware of those.

More chatter: https://discord.com/channels/428295358100013066/439194979856809985/1046949163604131860
mvines pointed out that there is already a BankForks in the admin rpc service, which is all we really need from JsonRpcRequestProcessor. We could rework some of the early-return logic to avoid needing AccountSecondaryIndexes, which doesn't seem like a big deal, or do a little plumbing to provide them to the admin service, which also doesn't seem terrible.

I'll do a little investigation tomorrow. You can probably keep moving forward on this PR, though.

IntokuSatori · 2022-12-19T22:21:44Z

@CriesofCarrots Will refactor to move into the Admin RPC Service after: #29003 gets merged.

IntokuSatori · 2022-12-21T04:22:38Z

Admin RPC API added in separate PR to follow: #29352

IntokuSatori · 2023-01-25T02:59:36Z

@CriesofCarrots @t-nelson Good to merge? Addressed the PR comments. People are asking for the feature, if all looks good.

core/src/sigverify_shreds.rs

runtime/src/secondary_index.rs

IntokuSatori · 2023-01-28T04:46:45Z

@CriesofCarrots See my standup on slack for context on delays. Still need to remove the "unsafe" parts we discussed when I added my own bit packing function. Will do Sunday and ping you when its done.

CriesofCarrots

Mostly suggestions to simplify your test harness, and note some test cases that seem to be missing. Otherwise, just nits on the prod code.

runtime/src/secondary_index.rs

runtime/src/accounts_db.rs

runtime/src/secondary_index.rs

runtime/src/accounts_db.rs

…eys and their sizes.

…and their sizes (#28887) Co-authored-by: K-anon <IntokuSatori@users.noreply.github.com> (cherry picked from commit 1e3d634)

…ex keys and their sizes (solana-labs#28887)" This reverts commit 1e3d634.

…ex keys and their sizes (#28887)" (#33121) * Revert "Add an RPC API that can query the list of Top N secondary index keys and their sizes (#28887)" This reverts commit 1e3d634. * Revert "Add Admin RPC Front End for Top N Secondary Index Key Sizes Query. (#29352)" This reverts commit aa353e4. * fix test uses * fmt

…ex keys and their sizes (#28887)" (#33121) * Revert "Add an RPC API that can query the list of Top N secondary index keys and their sizes (#28887)" This reverts commit 1e3d634. * Revert "Add Admin RPC Front End for Top N Secondary Index Key Sizes Query. (#29352)" This reverts commit aa353e4. * fix test uses * fmt (cherry picked from commit fb1ba21) # Conflicts: # validator/src/admin_rpc_service.rs

…ary index keys and their sizes (#28887)" (backport of #33121) (#33122) * Revert "Add an RPC API that can query the list of Top N secondary index keys and their sizes (#28887)" (#33121) * Revert "Add an RPC API that can query the list of Top N secondary index keys and their sizes (#28887)" This reverts commit 1e3d634. * Revert "Add Admin RPC Front End for Top N Secondary Index Key Sizes Query. (#29352)" This reverts commit aa353e4. * fix test uses * fmt (cherry picked from commit fb1ba21) # Conflicts: # validator/src/admin_rpc_service.rs * fix compile errors --------- Co-authored-by: Jeff Washington (jwash) <jeff.washington@solana.com>

CriesofCarrots mentioned this pull request Dec 1, 2022

Move getSecondaryIndexKeySize RPC to admin_rpc_service #29003

Merged

IntokuSatori force-pushed the eng/PR-28666 branch 2 times, most recently from 08cbfc0 to 7922384 Compare December 5, 2022 22:06

IntokuSatori force-pushed the eng/PR-28666 branch 5 times, most recently from d202e30 to cd976af Compare December 17, 2022 02:40

IntokuSatori marked this pull request as ready for review December 17, 2022 02:40

IntokuSatori force-pushed the eng/PR-28666 branch from cd976af to 58695c5 Compare December 17, 2022 02:44

IntokuSatori force-pushed the eng/PR-28666 branch 5 times, most recently from ff6f96d to 76a1151 Compare December 21, 2022 04:10

IntokuSatori requested review from CriesofCarrots and mvines December 21, 2022 04:21

IntokuSatori mentioned this pull request Dec 21, 2022

Add Admin RPC Front End for Top N Secondary Index Key Sizes Query. #29352

Merged

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 4, 2023

IntokuSatori force-pushed the eng/PR-28666 branch 2 times, most recently from 7295a69 to c561724 Compare January 5, 2023 00:33

IntokuSatori force-pushed the eng/PR-28666 branch 5 times, most recently from c06c7d9 to aaa9950 Compare January 25, 2023 00:29

CriesofCarrots reviewed Jan 25, 2023

View reviewed changes

IntokuSatori force-pushed the eng/PR-28666 branch 4 times, most recently from 43d2fa5 to fd5f861 Compare January 31, 2023 04:58

CriesofCarrots reviewed Jan 31, 2023

View reviewed changes

IntokuSatori force-pushed the eng/PR-28666 branch from fd5f861 to 5d13e1f Compare January 31, 2023 06:32

CriesofCarrots reviewed Jan 31, 2023

View reviewed changes

IntokuSatori force-pushed the eng/PR-28666 branch 2 times, most recently from 914fb08 to 78fbc14 Compare January 31, 2023 18:04

Add AccountsDB API that can query the list of Top N secondary index k…

87322df

…eys and their sizes.

IntokuSatori force-pushed the eng/PR-28666 branch from 78fbc14 to 87322df Compare January 31, 2023 18:15

CriesofCarrots approved these changes Jan 31, 2023

View reviewed changes

IntokuSatori merged commit 1e3d634 into solana-labs:master Jan 31, 2023

IntokuSatori added the v1.15 (abandoned) The v1.15 branch has been abandoned label Feb 1, 2023

mergify bot mentioned this pull request Feb 1, 2023

Add an RPC API that can query the list of Top N secondary index keys and their sizes (backport #28887) #30058

Closed

mergify bot pushed a commit that referenced this pull request Feb 1, 2023

Add an RPC API that can query the list of Top N secondary index keys …

61fdb75

…and their sizes (#28887) Co-authored-by: K-anon <IntokuSatori@users.noreply.github.com> (cherry picked from commit 1e3d634)

IntokuSatori removed the v1.15 (abandoned) The v1.15 branch has been abandoned label Feb 1, 2023

jeffwashington added a commit to jeffwashington/solana that referenced this pull request Sep 1, 2023

Revert "Add an RPC API that can query the list of Top N secondary ind…

f792792

…ex keys and their sizes (solana-labs#28887)" This reverts commit 1e3d634.

jeffwashington added a commit to jeffwashington/solana that referenced this pull request Sep 1, 2023

Revert "Add an RPC API that can query the list of Top N secondary ind…

3948b5b

…ex keys and their sizes (solana-labs#28887)" This reverts commit 1e3d634.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an RPC API that can query the list of Top N secondary index keys and their sizes #28887

Add an RPC API that can query the list of Top N secondary index keys and their sizes #28887

IntokuSatori commented Nov 19, 2022 •

edited

Loading

mvines commented Nov 29, 2022

IntokuSatori commented Nov 29, 2022

CriesofCarrots commented Nov 29, 2022

mvines commented Nov 29, 2022

mvines commented Nov 29, 2022 •

edited

Loading

IntokuSatori commented Nov 29, 2022

CriesofCarrots commented Nov 29, 2022

IntokuSatori commented Dec 19, 2022 •

edited

Loading

IntokuSatori commented Dec 21, 2022

IntokuSatori commented Jan 25, 2023

IntokuSatori commented Jan 28, 2023

CriesofCarrots left a comment

Add an RPC API that can query the list of Top N secondary index keys and their sizes #28887

Add an RPC API that can query the list of Top N secondary index keys and their sizes #28887

Conversation

IntokuSatori commented Nov 19, 2022 • edited Loading

Problem

Summary of Changes

mvines commented Nov 29, 2022

IntokuSatori commented Nov 29, 2022

CriesofCarrots commented Nov 29, 2022

mvines commented Nov 29, 2022

mvines commented Nov 29, 2022 • edited Loading

IntokuSatori commented Nov 29, 2022

CriesofCarrots commented Nov 29, 2022

IntokuSatori commented Dec 19, 2022 • edited Loading

IntokuSatori commented Dec 21, 2022

IntokuSatori commented Jan 25, 2023

IntokuSatori commented Jan 28, 2023

CriesofCarrots left a comment

Choose a reason for hiding this comment

IntokuSatori commented Nov 19, 2022 •

edited

Loading

mvines commented Nov 29, 2022 •

edited

Loading

IntokuSatori commented Dec 19, 2022 •

edited

Loading