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

[Merged by Bors] - Don't return errors on HTTP API for already-known messages #3341

Closed
wants to merge 11 commits into from

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Jul 18, 2022

Issue Addressed

Proposed Changes

Return 200 OK rather than an error when a block, attestation or sync message is already known.

Presently, we will log return an error which causes a BN to go "offline" from the VCs perspective which causes the fallback mechanism to do work to try and avoid and upcheck offline nodes. This can be observed as instability in the vc_beacon_nodes_available_count metric.

The current behaviour also causes scary logs for the user. There's nothing to actually be concerned about when we see duplicate messages, this can happen on fallback systems (see code comments).

Additional Info

NA

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Jul 18, 2022
@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jul 21, 2022
@paulhauner paulhauner marked this pull request as ready for review July 21, 2022 00:15
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks good overall, but there are a few merge conflicts

beacon_node/http_api/src/lib.rs Outdated Show resolved Hide resolved
@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Aug 9, 2022
@paulhauner
Copy link
Member Author

Thanks! All conflicts resolved. That publish_block function is a nice improvement!

@paulhauner paulhauner added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 9, 2022
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Approved modulo typo fixes!

beacon_node/http_api/src/publish_blocks.rs Outdated Show resolved Hide resolved
beacon_node/http_api/src/sync_committees.rs Outdated Show resolved Hide resolved
@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Aug 9, 2022
Co-authored-by: Michael Sproul <micsproul@gmail.com>
@paulhauner
Copy link
Member Author

Approved modulo typo fixes!

Thanks! Sorry for zombie-ing those typos again!

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 9, 2022
@paulhauner
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Aug 10, 2022
## Issue Addressed

- Resolves #3266

## Proposed Changes

Return 200 OK rather than an error when a block, attestation or sync message is already known.

Presently, we will log return an error which causes a BN to go "offline" from the VCs perspective which causes the fallback mechanism to do work to try and avoid and upcheck offline nodes. This can be observed as instability in the `vc_beacon_nodes_available_count` metric.

The current behaviour also causes scary logs for the user. There's nothing to *actually* be concerned about when we see duplicate messages, this can happen on fallback systems (see code comments).

## Additional Info

NA
@bors bors bot changed the title Don't return errors on HTTP API for already-known messages [Merged by Bors] - Don't return errors on HTTP API for already-known messages Aug 10, 2022
@bors bors bot closed this Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants