Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Return error for excluded secondary-index keys #17193

Merged
merged 4 commits into from
May 13, 2021

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented May 12, 2021

Problem

As of #17110, nodes running with secondary account indexes can explicitly exclude/include keys from the index. If a key is excluded, a full account scan will run. But since these keys are being excluded explicitly because there are too many indexed values for callers to handle, a full account scan is impractical -- it can only perform worse.

Summary of Changes

  • Add custom rpc error for excluded keys
  • Check keys for exclusion in Rpc before attempting indexed scan -- This operation could be done here:
    if !self.account_indexes.include_key(key) {
    where we are already doing the check. However this would require a bunch of churn to change type signatures from Vec to Result, and anyway it seems more reasonable for the rpc service to decide whether to support the full-accounts scan, instead of a hard block in the runtime.

@CriesofCarrots
Copy link
Contributor Author

CriesofCarrots commented May 12, 2021

@mvines , if you wouldn't mind, does the rpc error seem satisfactory?
Also, it occurred to me that we could make the error more general, and return it for any attempted full accounts scan if some criteria is met: secondary indexes are in place (this seems too heavy-handed to me), some secondary-index keys are included/excluded, or validator arg is supplied.

@@ -1746,13 +1755,22 @@ impl JsonRpcRequestProcessor {
.account_indexes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way you did this is ok. However, the .account_indexes is specified in the config to rpc also from validator. I specifically had to chop out the include/exclude info. So, you could just keep a copy of the include/exclude info in rpc directly. There are 2 sources of truth then, but we already do that with account_indexes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is where I exclude the include/exclude info

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I failed to track where the verbose/concise account_indexes were split. Yeah, let's do that; saves any runtime helpers needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeffwashington done here: 32118af
Can you take a look?

I considered changing the type to Arc<AccountSecondaryIndexes> to reduce to only one source of truth, but the level of churn was very high. Let me know if you prefer that, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect.
I prefer not an arc myself. The secondary config is created once at startup and never changed and looked up on each modification to the accounts index. 2 copies is ok with me.

@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #17193 (32118af) into master (3e0c0ab) will decrease coverage by 0.0%.
The diff coverage is 14.0%.

@@            Coverage Diff            @@
##           master   #17193     +/-   ##
=========================================
- Coverage    82.7%    82.7%   -0.1%     
=========================================
  Files         421      421             
  Lines      117859   117892     +33     
=========================================
+ Hits        97509    97522     +13     
- Misses      20350    20370     +20     

@jeffwashington jeffwashington self-requested a review May 13, 2021 19:28
Copy link
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label May 13, 2021
@mergify mergify bot merged commit 27004f1 into solana-labs:master May 13, 2021
mergify bot pushed a commit that referenced this pull request May 13, 2021
* Add runtime helpers to check secondary indexes for key

* Add custom rpc error

* Check secondary-index key inclusion in rpc

* Clone complete AccountSecondaryIndexes into rpc to avoid bank query

(cherry picked from commit 27004f1)

# Conflicts:
#	core/src/rpc.rs
mergify bot added a commit that referenced this pull request May 13, 2021
)

* Return error for excluded secondary-index keys (#17193)

* Add runtime helpers to check secondary indexes for key

* Add custom rpc error

* Check secondary-index key inclusion in rpc

* Clone complete AccountSecondaryIndexes into rpc to avoid bank query

(cherry picked from commit 27004f1)

# Conflicts:
#	core/src/rpc.rs

* Fix conflicts

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>
Co-authored-by: Tyera Eulberg <tyera@solana.com>
@CriesofCarrots CriesofCarrots deleted the full-scan-rpc-err branch July 28, 2021 22:32
@brooksprumo brooksprumo mentioned this pull request Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants