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

Improve block equivocation check for block publishing V2 API #8043

Merged
merged 13 commits into from
Mar 6, 2024

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented Mar 5, 2024

Based on the discussion happening in the issue.

The objective is to make gossip broadcast validation to fail only in case of equivocating blocks (slashable, proposer proposed two different blocks for the same slot) and let it pass when it is the same block root that is causing the gossip ignore.

In case of "already seen", we can say for sure that the block has been seen but we currently don't know anything about blobs. So the current decision is to publish block and blobs in that condition too, and let the libp2p layer to filter the already seen. If detected we log a debug line with the exception:
image

fixes #8039

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@tbenr tbenr marked this pull request as ready for review March 5, 2024 21:27
@tbenr tbenr requested a review from zilm13 March 5, 2024 21:45
@tbenr tbenr changed the title Improve block equivocation check Improve block equivocation check for block publishing V2 API Mar 6, 2024
Copy link
Contributor

@zilm13 zilm13 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, just a few nits but I still not sure on consequences of publishing duplicate

return EQUIVOCATING_BLOCK_FOR_SLOT_PROPOSER;
}

EquivocationCheckResult performBlockEquivocationCheck(final SignedBeaconBlock block) {
Copy link
Contributor

Choose a reason for hiding this comment

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

and also here I think something like performIntermediateBlockEquivocationCheck will highlight purpose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm... here I disagree because it is called to do the final equivocation check when building the pipeline in buildValidationPipeline

Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

LGTM

@tbenr tbenr enabled auto-merge (squash) March 6, 2024 14:09
@tbenr tbenr merged commit 6e65915 into Consensys:master Mar 6, 2024
15 checks passed
@tbenr tbenr deleted the improve-block-equivocation-check branch March 6, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Occasional failure to public blocks with /eth/v2/beacon/blocks
2 participants