Ensure data column sidecars respect the blob limit#4650
Ensure data column sidecars respect the blob limit#4650jtraglia merged 2 commits intoethereum:masterfrom
Conversation
ralexstokes
left a comment
There was a problem hiding this comment.
nice catch!
we had this check from deneb, but the change to columns in fulu leaves a hole here where there is no bound at the networking layer for the size of a given column
as long as it is easy for implementations to inject the blob parameters into their gossip validations (which AFAIK is no problem) then this seems like a lightweight check to tighten the networking layer
note: we do have this check during chain processing so there's no issue there
There was a problem hiding this comment.
This check brings extra safety to the gossip domain 👍
There's opportunity to harden the validation rules further to deal with an edge case that occurs during a fork transition. It's a bit orthogonal, but I'll post here for now, and hoist it later to an issue/PR of its own depending on feedback.
- During a short period, clients remain subscribed to two topic sets, determined by the old and new values of the
fork_digestcomponent in the topic name. - In practice, validator and subscriber logic is shared for a topic across forks (and behavioural changes are implemented as conditional statements within).
- For extra correctness, clients should check that a message was received in the correct topic. This check would be contextual to the message type, so in this case we'd check that
DataColumnSidecar#signed_block_header.message.slotis within bounds for the fork represented by thefork_digestcomponent of the topic the message was effectively received in. - Similarly for other gossiped message types: blocks, attestations, aggregates (this isn't specific to PeerDAS topics).
I don't think this is an absolute must, as the risk is relatively well contained both in time and impact. But given that we're hardening for correctness, it seems fitting to raise.
**Motivation** See ethereum/consensus-specs#4650 **Description** Ensure data column sidecars respect blob limit by checking that `kzgCommitments.length` of each data column sidecar does not exceed `getMaxBlobsPerBlock(epoch)`.
Addresses this spec change ethereum/consensus-specs#4650 Add `max_blobs_per_block` to gossip data column check so we reject large columns before processing. (we currently do this check during processing) Co-Authored-By: Jimmy Chen <jchen.tc@gmail.com>
…p#8198) Addresses this spec change ethereum/consensus-specs#4650 Add `max_blobs_per_block` to gossip data column check so we reject large columns before processing. (we currently do this check during processing) Co-Authored-By: Jimmy Chen <jchen.tc@gmail.com>
The gossip validation for data column sidecars is missing a check which ensures the number of blob KZG commitments in the sidecar respects the max blobs per block limit. This would allow a malicious proposer to broadcast sidecars with a bunch of data to verify (slow) and it would still be propagated causing other nodes to waste resources on this too. I'm not very concerned with this as a DoS, but we should add a check for this.
Instead of adding a line to the gossip verification checks I thought it would be better to put the check in
verify_data_column_sidecar. Code is better than an english sentence 🙂Edit: This came from a report in the fusaka audit competition. Thank you!