Patch Initial Sync For Non Finalized Blocks #9452
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nisdas
requested review from
kasey,
jmozah and
rauljordan
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## develop #9452 +/- ##
========================================
Coverage 60.93% 60.94%
========================================
Files 582 582
Lines 42928 42929 +1
========================================
+ Hits 26158 26161 +3
+ Misses 12813 12804 -9
- Partials 3957 3964 +7 |
Comment on lines
307
to
308
| blockExistsInDB := s.cfg.DB.HasBlock(ctx, blkRoot) || s.cfg.Chain.HasInitSyncBlock(blkRoot) | ||
| if blk.Block().Slot() <= finalizedSlot || blockExistsInDB && s.cfg.Chain.HeadSlot() >= blk.Block().Slot() { |
There was a problem hiding this comment.
Suggested change
| blockExistsInDB := s.cfg.DB.HasBlock(ctx, blkRoot) || s.cfg.Chain.HasInitSyncBlock(blkRoot) | |
| if blk.Block().Slot() <= finalizedSlot || blockExistsInDB && s.cfg.Chain.HeadSlot() >= blk.Block().Slot() { | |
| if blk.Block().Slot() <= finalizedSlot || (s.cfg.DB.HasBlock(ctx, blkRoot) || s.cfg.Chain.HasInitSyncBlock(blkRoot)) && s.cfg.Chain.HeadSlot() >= blk.Block().Slot() { |
This is kind of a nasty long line, but blk.Block().Slot() <= finalizedSlot is true for most of initial sync. Putting this boolean first in the statement could reduce the number of db lookups.
| @@ -304,7 +304,8 @@ func (s *Service) isProcessedBlock(ctx context.Context, blk block.SignedBeaconBl | |||
| if err != nil { | |||
| return false | |||
| } | |||
| if blk.Block().Slot() <= finalizedSlot || (s.cfg.DB.HasBlock(ctx, blkRoot) || s.cfg.Chain.HasInitSyncBlock(blkRoot)) { | |||
| blockExistsInDB := s.cfg.DB.HasBlock(ctx, blkRoot) || s.cfg.Chain.HasInitSyncBlock(blkRoot) | |||
| if blk.Block().Slot() <= finalizedSlot || blockExistsInDB && s.cfg.Chain.HeadSlot() >= blk.Block().Slot() { | |||
There was a problem hiding this comment.
Also a good candidate for a one liner comment so readers can quickly understand what this does
| @@ -304,7 +304,8 @@ func (s *Service) isProcessedBlock(ctx context.Context, blk block.SignedBeaconBl | |||
| if err != nil { | |||
| return false | |||
| } | |||
| if blk.Block().Slot() <= finalizedSlot || (s.cfg.DB.HasBlock(ctx, blkRoot) || s.cfg.Chain.HasInitSyncBlock(blkRoot)) { | |||
| blockExistsInDB := s.cfg.DB.HasBlock(ctx, blkRoot) || s.cfg.Chain.HasInitSyncBlock(blkRoot) | |||
| if blk.Block().Slot() <= finalizedSlot || blockExistsInDB && s.cfg.Chain.HeadSlot() >= blk.Block().Slot() { | |||
There was a problem hiding this comment.
Curious on whether we have unit test for this as well
There was a problem hiding this comment.
we do have a unit test, which correctly failed when I had an off by 1 :) 09c3bee
rauljordan
approved these changes
Aug 24, 2021
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 27, 2021
* fix for now * off by 1 * preston's review (cherry picked from commit 114a14a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Bug Fix
What does this PR do? Why is it needed?
behind. Validation of responses is done in the block fetcher, so it should be fine to filter it this way.
Which issues(s) does this PR fix?
N.A
Other notes for review