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

Reject Blobs Which Reference Already Finalized Parent Blocks #3529

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

nisdas
Copy link
Contributor

@nisdas nisdas commented Oct 25, 2023

Currently we will allow blobs to reference parent blocks which are already finalized. This could allow malicious peers to perform DOS attacks by referencing a very old parent block. Due to the requirements for nodes to verify proposer shuffling for these blobs, you could have a malicous peer force a honest node to perform an excessive amount of epoch transitions to verify the shuffling for the proposer. In order to mitigate this we can simply add the same gossip validation condition that we have for beacon blocks for blobs:

[REJECT] The current finalized_checkpoint is an ancestor of block -- i.e. get_checkpoint_block(store, block.parent_root, store.finalized_checkpoint.epoch) == store.finalized_checkpoint.root

This was encountered when running an Antithesis experiment.

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

makes sense. thanks!

@ppopth
Copy link
Member

ppopth commented Oct 25, 2023

Should this be a REJECT or an IGNORE? I'm not sure.

It should probably be just an IGNORE because the block proposer sometimes just doesn't have the latest finalized checkpoint in their view yet without the intention to be malicious.

@nisdas
Copy link
Contributor Author

nisdas commented Oct 26, 2023

@ppopth This follows from here #1985

I think the reason it was reject was because any proposer building on an old parent would technically be reverting the finalized checkpoint. It is possible for a node to be stuck and to produce such blocks, but if that happens I think being more harsh here is warranted.

@ppopth
Copy link
Member

ppopth commented Oct 26, 2023

This follows from here #1985

That makes sense then. Thanks.

Copy link
Collaborator

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

This change should be incorporated into #3531

@tbenr
Copy link
Contributor

tbenr commented Oct 30, 2023

Maybe we could merge this to officially signal that this is go to go and rebase #3531 afterwards since it is still under refinement?

@djrtwo
Copy link
Contributor

djrtwo commented Oct 30, 2023

Merging -- can incorporate into #3531 as @tbenr suggests

@djrtwo djrtwo merged commit 914fe92 into ethereum:dev Oct 30, 2023
26 checks passed
@djrtwo djrtwo mentioned this pull request Oct 30, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants