ethereum / consensus-specs

Ethereum Proof-of-Stake Consensus Specifications
Creative Commons Zero v1.0 Universal
3.56k stars 970 forks source link

Eip4844 `beacon_block_and_blobs_sidecar` gossip validation rules #3109

Open tbenr opened 1 year ago

tbenr commented 1 year ago

Current rules:

In addition to the gossip validations for the `beacon_block` topic from prior specifications, the following validations MUST pass before forwarding the `signed_beacon_block_and_blobs_sidecar` on the network.  
Alias `signed_beacon_block = signed_beacon_block_and_blobs_sidecar.beacon_block`, `block = signed_beacon_block.message`, `execution_payload = block.body.execution_payload`.
- _[REJECT]_ The KZG commitments of the blobs are all correctly encoded compressed BLS G1 Points.
  -- i.e. `all(bls.KeyValidate(commitment) for commitment in block.body.blob_kzg_commitments)`
- _[REJECT]_ The KZG commitments correspond to the versioned hashes in the transactions list.
  -- i.e. `verify_kzg_commitments_against_transactions(block.body.execution_payload.transactions, block.body.blob_kzg_commitments)`

Alias `sidecar = signed_beacon_block_and_blobs_sidecar.blobs_sidecar`.
- _[IGNORE]_ the `sidecar.beacon_block_slot` is for the current slot (with a `MAXIMUM_GOSSIP_CLOCK_DISPARITY` allowance) -- i.e. `sidecar.beacon_block_slot == block.slot`.
- _[REJECT]_ the `sidecar.blobs` are all well formatted, i.e. the `BLSFieldElement` in valid range (`x < BLS_MODULUS`).
- _[REJECT]_ The KZG proof is a correctly encoded compressed BLS G1 Point -- i.e. `bls.KeyValidate(blobs_sidecar.kzg_aggregated_proof)`
  1. Shouldn't we add check sidecar.beacon_block_root == block.block.hash_tree_root()?
  2. Shouldn't we [REJECT] on slot and block_root checks since signed_beacon_block_and_blobs_sidecar itself is inconsistent thus invalid?
  3. Shouldn't we rely on kzg library verification here by running validate_blobs_sidecar and all the low level KZG\BLS checks will be done internally by the kzg library?

cc @terencechain

djrtwo commented 1 year ago

good points!

  1. I'm not sure we need the beacon slot and root in the sidecar gossip anymore because of the simultaneous delivery (especially if we make our range/root requests coupled as well)
  2. Yes, if we keep it, it should be REJECT
  3. We either need signature or full kzg validation as discussed in #3103
tbenr commented 1 year ago
  1. I'm not sure we need the beacon slot and root in the sidecar gossip anymore because of the simultaneous delivery (especially if we make our range/root requests coupled as well)

Yes, makes sense for the coupled container but if we leave the decoupled "byRange" methods (which was the latest direction if I recall correctly) hey might have more sense

terencechain commented 1 year ago
  1. Shouldn't we rely on kzg library verification here by running validate_blobs_sidecar and all the low level KZG\BLS checks will be done internally by the kzg library?

  2. We either need signature or full kzg validation as discussed in https://github.com/ethereum/consensus-specs/issues/3103

I think we can make an argument to remove all the low-level kzg checks in either signature or full kzg validation cases. If the signature can be validated, is it still worth the low-level kzg checks before forwarding it to peers? They are cheap checks, so maybe why not

tbenr commented 1 year ago

Since this has been merged, shouldn't we simplify\revisit the validations?