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

Use WaitForSynced in validator client for startup #5465

Merged
merged 27 commits into from
Apr 20, 2020

Conversation

0xKiwi
Copy link
Contributor

@0xKiwi 0xKiwi commented Apr 16, 2020

Part of #5345, meant to be merged after #5366 , this PR changes the validator client to use WaitForSynced to listen for what the validator is ready, rather than pinging if the node is synced every 6 seconds.

This feature is gated by the --wait-for-synced flag on the validator client.
Confirmed in runtime:
Old Startup:

time="2020-04-16 19:40:42" msg="Beacon node is ready 2.852288s after chainstart"
time="2020-04-16 19:40:42" msg="Activations received 2.853108s after chainstart"

New Startup:

time="2020-04-16 19:48:10" msg="Beacon node is ready 45.511ms after chain start"
time="2020-04-16 19:48:10" msg="Activations received 78.148ms after chainstart"

Resolves #5452

@0xKiwi 0xKiwi requested a review from a team as a code owner April 16, 2020 22:53
@prestonvanloon
Copy link
Member

Tests fail. Please wait to merge new features until after Topaz starts

@0xKiwi 0xKiwi added the Blocked Blocked by research or external factors label Apr 16, 2020
@nisdas
Copy link
Member

nisdas commented Apr 18, 2020

Is this unblocked now @0xKiwi ?

@0xKiwi 0xKiwi added Ready For Review A pull request ready for code review and removed Blocked Blocked by research or external factors labels Apr 18, 2020
@0xKiwi
Copy link
Contributor Author

0xKiwi commented Apr 18, 2020

Yes @nisdas

Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

Cant you split the client and server changes into 2 PRs ?

@0xKiwi
Copy link
Contributor Author

0xKiwi commented Apr 18, 2020

I did! This is the validator half but its waiting on #5366 to be merged first so it has changes for both :)

@nisdas
Copy link
Member

nisdas commented Apr 18, 2020

Ah #5366 is meant to be merged first, just saw it. Lets merge that then

@@ -178,6 +178,7 @@ func (s *Service) Syncing() bool {
func (s *Service) Resync() error {
// set it to false since we are syncing again
s.synced = false
// How does the validator handle this?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should there be anything special done here? Missed it in the beacon-chain PR.

Copy link
Member

Choose a reason for hiding this comment

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

Don't think you can, you would have to create new methods in the validator client to handle this.

@0xKiwi 0xKiwi requested a review from nisdas April 19, 2020 05:33
@codecov
Copy link

codecov bot commented Apr 19, 2020

Codecov Report

Merging #5465 into master will increase coverage by 4.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5465      +/-   ##
==========================================
+ Coverage   14.88%   19.00%   +4.11%     
==========================================
  Files           8      238     +230     
  Lines         430    20326   +19896     
==========================================
+ Hits           64     3862    +3798     
- Misses        351    15869   +15518     
- Partials       15      595     +580     

nisdas
nisdas previously approved these changes Apr 19, 2020
if err != nil {
return errors.Wrap(err, "could not setup beacon chain Synced streaming client")
}
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a need for this for loop. seem to happen once

@prylabs-bulldozer prylabs-bulldozer bot merged commit 25102e0 into prysmaticlabs:master Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misleading log message from validator before genesis start
5 participants