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.

Sign up for GitHub

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

Jump to bottom

Add new gossip rule to REJECT sidecars with index >= MAX_BLOBS_PER_BLOCK #3525

Merged
merged 2 commits into from Oct 18, 2023
Merged

Add new gossip rule to REJECT sidecars with index >= MAX_BLOBS_PER_BLOCK #3525

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27ad39b
Add new gossip rule to `REJECT` sidecars with index >= MAX_BLOBS_PER_…
tbenr Oct 18, 2023
a2d021d
re-order conditions
djrtwo Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions specs/deneb/p2p-interface.md
View file
Open in desktop
Expand Up @@ -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`.
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

@g11tech g11tech Oct 18, 2023 •
edited

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)

Copy link
Contributor

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)

Copy link
Contributor Author

@tbenr tbenr Oct 19, 2023 •
edited

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.

- _[IGNORE]_ The sidecar is not from a future slot (with a `MAXIMUM_GOSSIP_CLOCK_DISPARITY` allowance) -- i.e. validate that `sidecar.slot <= current_slot` (a client MAY queue future sidecars for processing at the appropriate slot).
- _[IGNORE]_ The sidecar is from a slot greater than the latest finalized slot -- i.e. validate that `sidecar.slot > compute_start_slot_at_epoch(state.finalized_checkpoint.epoch)`
- _[IGNORE]_ The sidecar's block's parent (defined by `sidecar.block_parent_root`) has been seen (via both gossip and non-gossip sources) (a client MAY queue sidecars for processing once the parent block is retrieved).
Expand Down