Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant bounds check from getBlock and getBlockTime #33901

Merged
merged 6 commits into from
Nov 8, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Oct 27, 2023

Problem

See #33859 for some additional details but the TLDR is:

  • The current logic in rpc.rs looks up min/max roots from the Blockstore to perform bounds check for the sake of error reporting
  • Looking up these roots is currently expensive as it is done by constructing an iterator
  • Regardless of the overhead of looking up min/max roots, some of these checks are repetitive

Summary of Changes

Remove / condense code to avoid repeating checks that have already occurred. Namely,

  • Blockstore::is_skipped(), which was called by these two RPC methods, has been removed as it was redundant
    • This avoids the overhead of (2) rocksdb iterators
  • Rename check_blockstore_root() ==> check_blockstore_bounds() and add a bunch of comments about the handling of Blockstore errors at the RPC level

@steviez steviez added the noCI Suppress CI on this Pull Request label Oct 27, 2023
@steviez steviez changed the title Remove redundant bounds check from getBlock and getBlockTim Remove redundant bounds check from getBlock and getBlockTime Oct 27, 2023
@steviez
Copy link
Contributor Author

steviez commented Oct 27, 2023

@CriesofCarrots - This doesn't compile yet, but I think you should be able to get the gist. Hopefully my comments inline help illustrate why I think the call to Blockstore::is_skipped() is redundant. This RPC helper is the only caller of Blockstore::is_skipped(), so we can hypothetically remove it as part of this PR too.

One downside is that this function is no longer self contained as it relies on checks from the actual blockstore fetch. This is obviously not ideal, but saving the creation of the two extra iterators is pretty big I think.

Also, thinking on it more, we likely don't even need to check for greater than max root in the helper; we explicitly check for lower than latest root at the top level when we decide to call get_rooted_block_time()

rpc/src/rpc.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Show resolved Hide resolved
@steviez steviez removed the noCI Suppress CI on this Pull Request label Nov 6, 2023
Assume it is more likely for someone to request a slot that has already
occurred but is not on disk rather than a slot that has not occurred
yet. With this, take advantage of short circuiting && operator.
Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #33901 (4bd6503) into master (70d97d3) will increase coverage by 0.0%.
Report is 3 commits behind head on master.
The diff coverage is 41.6%.

@@           Coverage Diff           @@
##           master   #33901   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         811      811           
  Lines      219332   219318   -14     
=======================================
+ Hits       179703   179707    +4     
+ Misses      39629    39611   -18     

@steviez steviez marked this pull request as ready for review November 7, 2023 02:19
rpc/src/rpc.rs Show resolved Hide resolved
rpc/src/rpc.rs Show resolved Hide resolved
@steviez steviez merged commit 03a456e into solana-labs:master Nov 8, 2023
32 checks passed
@steviez steviez deleted the rpc_root_bounds_check branch November 8, 2023 18:09
steviez added a commit to steviez/solana that referenced this pull request Nov 8, 2023
steviez added a commit to steviez/solana that referenced this pull request Nov 8, 2023
steviez added a commit that referenced this pull request Nov 9, 2023
#33996)

Revert " Remove redundant bounds check from getBlock and getBlockTime (#33901)"

This reverts commit 03a456e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants