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 pre-check of blockstore slots during load_bank_forks #25632

Merged

Conversation

apfitzge
Copy link
Contributor

@apfitzge apfitzge commented May 29, 2022

Problem

Check I added in #25329 was too restrictive. It's possible we having missing blocks in the range, but can still generate a snapshot.

Summary of Changes

Update the blockstore slots check to iterate from the starting slot -> halt_slot using slot_meta.next_slots.

Fixes #

@apfitzge apfitzge force-pushed the bugfix/too_restrictive_slot_precheck branch from 163d099 to e2e3ccc Compare May 31, 2022 17:23
@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #25632 (e2e3ccc) into master (02b26dd) will increase coverage by 0.0%.
The diff coverage is n/a.

❗ Current head e2e3ccc differs from pull request most recent head 91120cb. Consider uploading reports for the commit 91120cb to get more accurate results

@@           Coverage Diff           @@
##           master   #25632   +/-   ##
=======================================
  Coverage    82.1%    82.1%           
=======================================
  Files         621      621           
  Lines      170297   170297           
=======================================
+ Hits       139917   139955   +38     
+ Misses      30380    30342   -38     

@apfitzge apfitzge marked this pull request as ready for review May 31, 2022 21:02
@apfitzge apfitzge requested a review from steviez May 31, 2022 21:03
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
@apfitzge
Copy link
Contributor Author

apfitzge commented Jun 1, 2022

Measured the time this check takes on the case where I ran into in devnet:

starting_slot=136943427
halt_slot=136944100
difference=673
slots_connected took 4ms

so approximately 6us/slot, though probably some noise there since it's a small sample size. Checking an entire epoch's worth of slots that'd take roughly 2.5s base on this measurement.

@apfitzge apfitzge changed the title Remove a check for ALL blocks from starting to halt slot to be present in blockstore Fix pre-check of blockstore slots during load_bank_forks Jun 1, 2022
@steviez steviez added noCI Suppress CI on this Pull Request and removed noCI Suppress CI on this Pull Request labels Jun 1, 2022
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

@apfitzge apfitzge merged commit 934da5e into solana-labs:master Jun 2, 2022
@apfitzge apfitzge deleted the bugfix/too_restrictive_slot_precheck branch June 2, 2022 01:19
lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Jun 17, 2022
lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Jun 18, 2022
lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Jun 22, 2022
lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Jun 27, 2022
lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Jun 28, 2022
lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Jun 28, 2022
lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Jun 29, 2022
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