[Merged by Bors] - Reduce false positive logging for late builder blocks #4073
Conversation
This file contains hidden or 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
paulhauner
added
ready-for-review
michaelsproul
approved these changes
Mar 13, 2023
michaelsproul
added
waiting-on-author
Co-authored-by: Michael Sproul <micsproul@gmail.com>
paulhauner
force-pushed
the
builder-late-block-logging
branch
from
3826037 to
bcecf95
Compare
|
Oops, I messed up an attempted sneaky-rebase and had to do a public rebase! |
paulhauner
added
ready-for-merge
michaelsproul
approved these changes
Mar 13, 2023
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Mar 14, 2023
## Issue Addressed NA ## Proposed Changes When producing a block from a builder, there are two points where we could consider the block "broadcast": 1. When the blinded block is published to the builder. 2. When the un-blinded block is published to the P2P network (this is always *after* the previous step). Our logging for late block broadcasts was using (2) for builder-blocks, which was creating a lot of false-positive logs. This is because the builder publishes the block on the P2P network themselves before returning it to us and we perform (2). For clarity, the logs were false-positives because we claim that the block was published late by us when it was actually published earlier by the builder. This PR changes our logging behavior so we do our logging at (1) instead. It also updates our metrics for block broadcast to distinguish between local and builder blocks. I believe the metrics change will be natively compatible with existing Grafana dashboards. ## Additional Info One could argue that the builder *should* return the block to us faster, however that's not the case. I think it's more important that we don't desensitize users with false-positives.
|
Build failed (retrying...): |
bors bot
pushed a commit
that referenced
this pull request
Mar 14, 2023
## Issue Addressed NA ## Proposed Changes When producing a block from a builder, there are two points where we could consider the block "broadcast": 1. When the blinded block is published to the builder. 2. When the un-blinded block is published to the P2P network (this is always *after* the previous step). Our logging for late block broadcasts was using (2) for builder-blocks, which was creating a lot of false-positive logs. This is because the builder publishes the block on the P2P network themselves before returning it to us and we perform (2). For clarity, the logs were false-positives because we claim that the block was published late by us when it was actually published earlier by the builder. This PR changes our logging behavior so we do our logging at (1) instead. It also updates our metrics for block broadcast to distinguish between local and builder blocks. I believe the metrics change will be natively compatible with existing Grafana dashboards. ## Additional Info One could argue that the builder *should* return the block to us faster, however that's not the case. I think it's more important that we don't desensitize users with false-positives.
|
Build failed (retrying...): |
|
I think this might have broken block unblinding (based on the CI failures). Haven't looked in detail yet |
bors bot
pushed a commit
that referenced
this pull request
Mar 14, 2023
## Issue Addressed NA ## Proposed Changes When producing a block from a builder, there are two points where we could consider the block "broadcast": 1. When the blinded block is published to the builder. 2. When the un-blinded block is published to the P2P network (this is always *after* the previous step). Our logging for late block broadcasts was using (2) for builder-blocks, which was creating a lot of false-positive logs. This is because the builder publishes the block on the P2P network themselves before returning it to us and we perform (2). For clarity, the logs were false-positives because we claim that the block was published late by us when it was actually published earlier by the builder. This PR changes our logging behavior so we do our logging at (1) instead. It also updates our metrics for block broadcast to distinguish between local and builder blocks. I believe the metrics change will be natively compatible with existing Grafana dashboards. ## Additional Info One could argue that the builder *should* return the block to us faster, however that's not the case. I think it's more important that we don't desensitize users with false-positives.
|
Build failed: |
michaelsproul
added
waiting-on-author
realbigsean
reviewed
Mar 14, 2023
| .try_into_full_block(Some(full_payload)) | ||
| .map(Arc::new) | ||
| .map(ProvenancedBlock::Builder), | ||
| None => None, |
There was a problem hiding this comment.
this might be what's causing issues in CI, with pre-merge blocks this will return None here causing the Unable to add payload to block error where before calling block.try_into_full_block(None) on a pre-merge block would still return the block
There was a problem hiding this comment.
Oooh, super shoddy oversight on my behalf. Thanks for point this out and thanks tests!
Fixed in 408af7f.
paulhauner
removed
the
waiting-on-author
michaelsproul
added
ready-for-merge
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Mar 17, 2023
## Issue Addressed NA ## Proposed Changes When producing a block from a builder, there are two points where we could consider the block "broadcast": 1. When the blinded block is published to the builder. 2. When the un-blinded block is published to the P2P network (this is always *after* the previous step). Our logging for late block broadcasts was using (2) for builder-blocks, which was creating a lot of false-positive logs. This is because the builder publishes the block on the P2P network themselves before returning it to us and we perform (2). For clarity, the logs were false-positives because we claim that the block was published late by us when it was actually published earlier by the builder. This PR changes our logging behavior so we do our logging at (1) instead. It also updates our metrics for block broadcast to distinguish between local and builder blocks. I believe the metrics change will be natively compatible with existing Grafana dashboards. ## Additional Info One could argue that the builder *should* return the block to us faster, however that's not the case. I think it's more important that we don't desensitize users with false-positives.
bors
bot
changed the title
bors bot
pushed a commit
that referenced
this pull request
Jun 7, 2023
## Issue Addressed NA ## Proposed Changes Downgrade a `CRIT` to an `ERRO` when there's an `Irrecoverable` error whilst publishing a blinded block. It's quite common for builders successfully broadcast a block to the network whilst failing to respond to the BN when it publishes a signed, blinded block. The VC is currently raising a `CRIT` when this happens and I think that's excessive. These changes have the same intent as #4073. In that PR I only managed to remove the `CRIT`s in the BN but missed this one in the VC. I've also tidied the log messages to: - Give them all the same title (*"Error whilst producing block"*) to help with grepping. - Include the `block_slot` so it's easy to look up the slot in an explorer and see if it was actually skipped. ## Additional Info This PR should not change any logic beyond logging.
bors bot
pushed a commit
that referenced
this pull request
Jun 7, 2023
## Issue Addressed NA ## Proposed Changes Downgrade a `CRIT` to an `ERRO` when there's an `Irrecoverable` error whilst publishing a blinded block. It's quite common for builders successfully broadcast a block to the network whilst failing to respond to the BN when it publishes a signed, blinded block. The VC is currently raising a `CRIT` when this happens and I think that's excessive. These changes have the same intent as #4073. In that PR I only managed to remove the `CRIT`s in the BN but missed this one in the VC. I've also tidied the log messages to: - Give them all the same title (*"Error whilst producing block"*) to help with grepping. - Include the `block_slot` so it's easy to look up the slot in an explorer and see if it was actually skipped. ## Additional Info This PR should not change any logic beyond logging.
bors bot
pushed a commit
that referenced
this pull request
Jun 7, 2023
## Issue Addressed NA ## Proposed Changes Downgrade a `CRIT` to an `ERRO` when there's an `Irrecoverable` error whilst publishing a blinded block. It's quite common for builders successfully broadcast a block to the network whilst failing to respond to the BN when it publishes a signed, blinded block. The VC is currently raising a `CRIT` when this happens and I think that's excessive. These changes have the same intent as #4073. In that PR I only managed to remove the `CRIT`s in the BN but missed this one in the VC. I've also tidied the log messages to: - Give them all the same title (*"Error whilst producing block"*) to help with grepping. - Include the `block_slot` so it's easy to look up the slot in an explorer and see if it was actually skipped. ## Additional Info This PR should not change any logic beyond logging.
ghost
pushed a commit
to oone-world/lighthouse
that referenced
this pull request
Jul 13, 2023
## Issue Addressed NA ## Proposed Changes Downgrade a `CRIT` to an `ERRO` when there's an `Irrecoverable` error whilst publishing a blinded block. It's quite common for builders successfully broadcast a block to the network whilst failing to respond to the BN when it publishes a signed, blinded block. The VC is currently raising a `CRIT` when this happens and I think that's excessive. These changes have the same intent as sigp#4073. In that PR I only managed to remove the `CRIT`s in the BN but missed this one in the VC. I've also tidied the log messages to: - Give them all the same title (*"Error whilst producing block"*) to help with grepping. - Include the `block_slot` so it's easy to look up the slot in an explorer and see if it was actually skipped. ## Additional Info This PR should not change any logic beyond logging.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
## Issue Addressed NA ## Proposed Changes Downgrade a `CRIT` to an `ERRO` when there's an `Irrecoverable` error whilst publishing a blinded block. It's quite common for builders successfully broadcast a block to the network whilst failing to respond to the BN when it publishes a signed, blinded block. The VC is currently raising a `CRIT` when this happens and I think that's excessive. These changes have the same intent as sigp#4073. In that PR I only managed to remove the `CRIT`s in the BN but missed this one in the VC. I've also tidied the log messages to: - Give them all the same title (*"Error whilst producing block"*) to help with grepping. - Include the `block_slot` so it's easy to look up the slot in an explorer and see if it was actually skipped. ## Additional Info This PR should not change any logic beyond logging.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
NA When producing a block from a builder, there are two points where we could consider the block "broadcast": 1. When the blinded block is published to the builder. 2. When the un-blinded block is published to the P2P network (this is always *after* the previous step). Our logging for late block broadcasts was using (2) for builder-blocks, which was creating a lot of false-positive logs. This is because the builder publishes the block on the P2P network themselves before returning it to us and we perform (2). For clarity, the logs were false-positives because we claim that the block was published late by us when it was actually published earlier by the builder. This PR changes our logging behavior so we do our logging at (1) instead. It also updates our metrics for block broadcast to distinguish between local and builder blocks. I believe the metrics change will be natively compatible with existing Grafana dashboards. One could argue that the builder *should* return the block to us faster, however that's not the case. I think it's more important that we don't desensitize users with false-positives.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
## Issue Addressed NA ## Proposed Changes Downgrade a `CRIT` to an `ERRO` when there's an `Irrecoverable` error whilst publishing a blinded block. It's quite common for builders successfully broadcast a block to the network whilst failing to respond to the BN when it publishes a signed, blinded block. The VC is currently raising a `CRIT` when this happens and I think that's excessive. These changes have the same intent as sigp#4073. In that PR I only managed to remove the `CRIT`s in the BN but missed this one in the VC. I've also tidied the log messages to: - Give them all the same title (*"Error whilst producing block"*) to help with grepping. - Include the `block_slot` so it's easy to look up the slot in an explorer and see if it was actually skipped. ## Additional Info This PR should not change any logic beyond logging.
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.
Issue Addressed
NA
Proposed Changes
When producing a block from a builder, there are two points where we could consider the block "broadcast":
Our logging for late block broadcasts was using (2) for builder-blocks, which was creating a lot of false-positive logs. This is because the builder publishes the block on the P2P network themselves before returning it to us and we perform (2). For clarity, the logs were false-positives because we claim that the block was published late by us when it was actually published earlier by the builder.
This PR changes our logging behavior so we do our logging at (1) instead. It also updates our metrics for block broadcast to distinguish between local and builder blocks. I believe the metrics change will be natively compatible with existing Grafana dashboards.
Additional Info
One could argue that the builder should return the block to us faster, however that's not the case. I think it's more important that we don't desensitize users with false-positives.