EIP4844: couple beacon block and blob sidecar for p2p #3046
Conversation
|
👍 on coupling |
specs/eip4844/p2p-interface.md
Outdated
| ```python | ||
| class SignedBeaconBlockAndBlobsSidecar(Container): | ||
| beacon_block: SignedBeaconBlock | ||
| blobs_sidecar: SignedBlobsSidecar |
There was a problem hiding this comment.
Verifying the sidecar signature helps ensure the KZG commitments are worth verifying, but would it really result in DoS / performance issues when anyone changes it to invalid data? Technically we compare the sidecar against the KZG commitments in the beacon block already, so we do not have to sign the sidecar.
There was a problem hiding this comment.
Yeah I don't think we need to sign the side car. This also informs whether or not we send signed or unsigned side cars in blobs by range/root request.
There was a problem hiding this comment.
Yeah, I agree that comparing KZG commitments in the beacon block is sufficient. We don't need the sidecar signature
@realbigsean range/root request will be decoupled and it's unsigned there
specs/eip4844/p2p-interface.md
Outdated
| - _[IGNORE]_ The sidecar is the first sidecar with valid signature received for the `(proposer_index, sidecar.beacon_block_slot)` combination, | ||
| where `proposer_index` is the validator index of the beacon block proposer of `sidecar.beacon_block_slot` |
There was a problem hiding this comment.
We can remove this condition now that it's coupled
There was a problem hiding this comment.
I think we need some clarification on how we want to propagate beacon blocks that don't have sidecars (unlikely but possible). Do we keep using the beacon_block topic in that scenario? Or use the new topic from now one and set the blobs_sidecar to its default ssz value in that case?
Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: protolambda <proto@protolambda.com>
Do we want to support this? It feels like more complexity and takes us back to de-coupling block and blob |
|
I think empty lists in the existing structs here should be sufficient |
specs/eip4844/p2p-interface.md
Outdated
| - [The gossip domain: gossipsub](#the-gossip-domain-gossipsub) | ||
| - [Topics and messages](#topics-and-messages) | ||
| - [Global topics](#global-topics) | ||
| - [`beacon_block`](#beacon_block) | ||
| - [`blobs_sidecar`](#blobs_sidecar) | ||
| - [`beacon_block_and_blob_sidecar`](#beacon_block_and_blob_sidecar) |
There was a problem hiding this comment.
Seems we use "blobs sidecar" everywhere but here where you use "blob sidecar". Is that intentional?
There was a problem hiding this comment.
Typo, I will fix. Thanks!
Agreed. it isn't clear to me from my reading of the spec that we'll use coupled blocks even when there isn't a sidecar. Let's clarify this in the spec. We can deprecate/remove the old topic used for (decoupled) beacon blocks. |
|
should we add the description of how to prepare |
Good point, I'll update that today |
terencechain
force-pushed
the
couple-block-sidecar
branch
from
e8087e4 to
e2d0a4f
Compare
| privkey = privkeys[1] | ||
| spec.get_signed_blobs_sidecar(state, blobs_sidecar, privkey) |
There was a problem hiding this comment.
ah right, it should have been in another test case + verification 😅
Part of #3043
Here we couple beacon block and blob sidecar under a single gossip topic-
beacon_block_and_blob_sidecar. We introduce a new network containerSignedBeaconBlockAndBlobsSidecarwhich consists ofSignedBeaconBlockandSignedBlobsSidecar. We also combined the previous gossipsbeacon_blockandblobs_sidecarvalidations underbeacon_block_and_blob_sidecar. I think we can get rid ofblob.beacon_block_rootandblob.beacon_block_slot, I'm curious to hear more thoughtscc @protolambda @Inphi
Rationale for coupling: https://notes.ethereum.org/RLOGb1hYQ0aWt3hcVgzhgQ?#Gossip