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

Fix ledger-tool bigtable compare blocks #34373

Merged
merged 10 commits into from
Dec 14, 2023

Conversation

CriesofCarrots
Copy link
Contributor

Problem

We received a report of solana-ledger-tool bigtable compare-blocks returning a list of missing blocks that did actually appear to be present when queried with solana-ledger-tool bigtable block.

GOOGLE_APPLICATION_CREDENTIALS=<credential> ./solana-ledger-tool bigtable compare-blocks 13330000 10000 --reference-credential <other_credential> --rpc-bigtable-instance-name solana-ledger-hdd
[2023-11-12T10:37:34.112098337Z INFO  solana_ledger_tool] solana-ledger-tool 1.16.18 (src:612616be; feat:4033350765, client:SolanaLabs)
[2023-11-12T10:37:34.113806564Z INFO  solana_storage_bigtable::access_token] Requesting token for BigTableData scope
[2023-11-12T10:37:34.200364731Z INFO  solana_storage_bigtable::access_token] Token expires in 3599 seconds
[2023-11-12T10:37:34.477986775Z INFO  solana_ledger_tool::bigtable] owned bigtable 10000 blocks found
[2023-11-12T10:37:34.478114155Z INFO  solana_storage_bigtable::access_token] Requesting token for BigTableData scope
[2023-11-12T10:37:34.511993782Z INFO  solana_storage_bigtable::access_token] Token expires in 3599 seconds
[2023-11-12T10:37:36.822228338Z INFO  solana_ledger_tool::bigtable] reference bigtable 10000 blocks found
{"missing_blocks":[13344032,13344033,13344034,13344035,13344036,13344037,13344038,13344039,13344040,13344041,13344042,13344043],"num_owned_slots":10000,"num_reference_slots":10000,"reference_last_block":13344043}

Upon deeper investigation, we discovered that the owned bigtable contained 12 extra blocks that weren't finalized and uploaded to the reference bigtable.

The reason for the erroneous response is that the compare-blocks method assumes that the responses from get_confirmed_blocks() against both bigtables represent the same slot range. But this is not the case. get_confirmed_blocks() accepts a starting slot and a limit and always returns the same number of blocks if the bigtable instance holds that many. Thus if one bigtable contains blocks that are not present in the other, it will examine a small range of slots.

Also, it's unfortunate that there was no indication about the extra blocks.

Summary of Changes

  • Use the same last_block to limit the lists returned from both bigtables to ensure comparing identical slot ranges. This prevents the error described above.
  • Also return data about any superfluous blocks found in the owned bigtable

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #34373 (a86eb7e) into master (c7a5767) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34373   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         819      819           
  Lines      220919   220919           
=======================================
+ Hits       180853   180874   +21     
+ Misses      40066    40045   -21     

Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

In general, things look pretty good to me. And I think it is a very nice improvement to return the superfluous slots too; the reference table could certainly be off as well so good to report all differences.

Just one or two minor things, I'm not dead set on either of them and I'd be happy with the PR as-is if you agree. I'll wait for you respond before giving a ship it tho

ledger-tool/src/bigtable.rs Outdated Show resolved Hide resolved
ledger-tool/src/bigtable.rs Show resolved Hide resolved
ledger-tool/src/bigtable.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@steviez steviez 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 merged commit 36c1bbf into solana-labs:master Dec 14, 2023
34 checks passed
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