-
Notifications
You must be signed in to change notification settings - Fork 975
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
Add new gossip rule to REJECT
sidecars with index >= MAX_BLOBS_PER_BLOCK
#3525
Conversation
@@ -149,6 +149,7 @@ This topic is used to propagate signed blob sidecars, where each blob index maps | |||
The following validations MUST pass before forwarding the `signed_blob_sidecar` on the network, assuming the alias `sidecar = signed_blob_sidecar.message`: | |||
|
|||
- _[REJECT]_ The sidecar is for the correct subnet -- i.e. `compute_subnet_for_blob_sidecar(sidecar.index) == subnet_id`. | |||
- _[REJECT]_ The sidecar's index is consistent with `MAX_BLOBS_PER_BLOCK` -- i.e. `sidecar.index < MAX_BLOBS_PER_BLOCK`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actualy the previous check itself assures that this is not unbounded : compute_subnet_for_blob_sidecar(sidecar.index) == subnet_id
so this is a redundant check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not true though because compute_subnet_for_blob_sidecar
calculates the subnet_id using mod BLOB_SIDECAR_SUBNET_COUNT
, so for example for subnet 4 and index 100, compute_subnet_for_blob_sidecar(100) == 4
thus valid according to the rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh in that case do we want it to be hardfork independent MAX_BLOB_COMMITMENTS_PER_BLOCK
or MAX_BLOBS_PER_BLOCK
? (although the tighter bound of max blobs per block makes sense)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe @tbenr and @StefanBratanov are correct and that this should be bounded
There is an equivalent condition for beacon_attestation subnets here -- https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/p2p-interface.md#beacon_attestation_subnet_id
REJECT] The committee index is within the expected range -- i.e. data.index < get_committee_count_per_slot(state, data.target.epoch)
I would argue that this should be the first condition -- before the second condition that calls compute_subnet_for_blob_side_car
. Tossing out junk first (and mirroring the order in the attestation subnet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that this should be the first condition ...
True, we could save the function call but currently computes just a modulo.
Teku will perform this in this order in any case because we do the subnet_id
check closer to the networking layer and the actual gossip validation runs on top of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant check already enforced by subnet id check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I"m going to get this into the release today
Thanks @tbenr for finding this! |
Introduce a new rule to avoid propagation of sidecars that have an index >=
MAX_BLOBS_PER_BLOCK
.Current rules allow proposer to publish an unbounded number of sidecars that will be accepted by clients.