-
Notifications
You must be signed in to change notification settings - Fork 962
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
Check Last Finalized Epoch #357
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@rawfalafel I don't think @nisdas has finished this. He hasn't implemented Get the last finalized epoch from a peer from a received block + tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I take this back then
… into initialsync
… into initialsync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Left a few comments.
beacon-chain/sync/service.go
Outdated
@@ -197,6 +252,116 @@ func (ss *Service) ReceiveActiveState(data *pb.ActiveStateResponse) error { | |||
return nil | |||
} | |||
|
|||
// SetBlockForInitalSync sets the first received block as the base finalized | |||
// block for initial sync. | |||
func (ss *Service) SetBlockForInitalSync(data *pb.BeaconBlockResponse) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetBlockForInitialSync
beacon-chain/sync/service_test.go
Outdated
blockResponse.SlotNumber = 21 | ||
ss.blockBuf <- msg1 | ||
|
||
time.Sleep(interval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to use an interval, please pass in the channel rather than putting in a 1 second timeout.
beacon-chain/sync/service.go
Outdated
|
||
// SetBlockForInitalSync sets the first received block as the base finalized | ||
// block for initial sync. | ||
func (ss *Service) SetBlockForInitalSync(data *pb.BeaconBlockResponse) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetBlockForInitialSync
beacon-chain/sync/service_test.go
Outdated
|
||
func TestTimeChan(t *testing.T) { | ||
hook := logTest.NewGlobal() | ||
interval := time.Duration(1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be avoided by passing the channel into the goroutine. We should really avoid adding extra time to the tests like this. They add up quickly.
beacon-chain/blockchain/service.go
Outdated
if err != nil { | ||
return false, err | ||
} | ||
if !hasActive && !hasCrystallized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be !hasActive || !hasCrystallized
?
beacon-chain/sync/service.go
Outdated
switch ss.syncMode { | ||
case 0: | ||
log.Info("Starting initial sync") | ||
go ss.initialSync(ss.ctx.Done()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you disagree with this?
Codecov Report
@@ Coverage Diff @@
## master #357 +/- ##
=========================================
- Coverage 49.81% 49.8% -0.01%
=========================================
Files 35 35
Lines 2670 2795 +125
=========================================
+ Hits 1330 1392 +62
- Misses 1160 1206 +46
- Partials 180 197 +17
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, then LGTM after builds pass. Awesome 👍
beacon-chain/sync/service.go
Outdated
case 0: | ||
log.Info("Starting initial sync") | ||
go ss.initialSync(time.NewTicker(ss.syncPollingInterval).C, ss.ctx.Done()) | ||
case 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, removed it
This pull request will deal with #351 , where we will determine the last finalized epoch of a connected peer and then start our sync from there.
Requirements for Merge