-
-
Notifications
You must be signed in to change notification settings - Fork 291
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: unknown block sync to subscribe/unsubscribe to network events #5654
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
64c049c
to
dd102fe
Compare
dd102fe
to
07e47b9
Compare
.catch((e) => { | ||
this.logger.error("Error subscribing to gossip core topics", {}, e); | ||
}); | ||
} |
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.
Can we rebundle this if statement as before? If you need you can re-order the conditions
if (
state === SyncState.Synced &&
this.chain.clock.currentEpoch >= MIN_EPOCH_TO_START_GOSSIP &&
!this.network.isSubscribedToGossipCoreTopics()
) {
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.
yes we can do it but then the below this.unknownBlockSync.subscribeToNetwork();
has to depend on this.network.isSubscribedToGossipCoreTopics()
. Logically it's the same but it'll make it hard to reason about later.
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.
I can't think of any immediate issues with this, let's merge it. Added a minor comment to reduce diff
🎉 This PR is included in v1.9.0 🎉 |
Motivation
Unknown block sync received unknown block roots while the node is syncing, this lead to:
lodestar/packages/beacon-node/src/sync/unknownBlock.ts
Line 228 in 2eddf46
Description
network.subscribeGossipCoreTopics()
Closes #5653