-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(bin): warn about CL health only when pipeline is idle #4835
Conversation
Codecov Report
... and 7 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
need clarification on comment
bin/reth/src/node/events.rs
Outdated
} | ||
ConsensusLayerHealthEvent::HaveNotReceivedUpdatesForAWhile(period) => { | ||
warn!(?period, "Beacon client online, but no consensus updates received for a while. Please fix your beacon client to follow the chain!") | ||
// If pipeline is running, consensus layer messages are expected to be neither received nor |
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 is not correct, they're always handled.
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.
right, removed the part about processing. We always process incoming messages, but not expect to receive them when the pipeline is running.
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 also don't think this is correct, we're still getting FCUs, at least with lighthouse, so perhaps this is a client specific thing.
the change makes sense, but comment should reflect 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.
hmm, maybe "If pipeline is running, it's fine to not receive any consensus layer messages."?
bin/reth/src/node/events.rs
Outdated
} | ||
ConsensusLayerHealthEvent::HaveNotReceivedUpdatesForAWhile(period) => { | ||
warn!(?period, "Beacon client online, but no consensus updates received for a while. Please fix your beacon client to follow the chain!") | ||
// If pipeline is running, no consensus layer messages are expected to be received. |
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.
sorry to bikeshed here, but I think this is still not accurate because we know that some clients still send us messages.
this comment should reflect that it can be the case not that it is expected
No description provided.