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.

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

Document ForkDigest-context for EIP-4844 BlobSidecar in Electra #3850

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

etan-status
Copy link
Contributor

Currently, documentation does not state what determines the ForkDigest for BlobSidecar. It makes most sense to use the corresponding slot as that one determines the ForkDigest of the corresponding beacon block.

Furthermore, add "and later" remarks so that ELECTRA_FORK_VERSION is also associated with the deneb.BlobSidecar type.

This is in line with how light-client/p2p-interface.md is done.

Currently, documentation does not state what determines the `ForkDigest`
for `BlobSidecar`. It makes most sense to use the corresponding slot as
that one determines the `ForkDigest` of the corresponding beacon block.

Furthermore, add "and later" remarks so that `ELECTRA_FORK_VERSION` is
also associated with the `deneb.BlobSidecar` type.

This is in line with how `light-client/p2p-interface.md` is done.
Comment on lines 190 to 199
The gossip `ForkDigest`-context is determined based on `compute_fork_version(compute_epoch_at_slot(blob_sidecar.signed_block_header.message.slot))`.

Per `context = compute_fork_digest(fork_version, genesis_validators_root)`:

[0]: # (eth2spec: skip)

| `fork_version` | Chunk SSZ type |
|--------------------------------|-------------------------------|
| `DENEB_FORK_VERSION` and later | `deneb.BlobSidecar` |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ForkDigest-context thing is only for Req/Resp, not GossipSub topics. GossipSub has nothing to do with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call it ForkDigestValue then.

@ppopth
Copy link
Member

ppopth commented Jul 24, 2024

Furthermore, add "and later" remarks so that ELECTRA_FORK_VERSION is also associated with the deneb.BlobSidecar type.

This is in line with how light-client/p2p-interface.md is done.

This is new to the spec (except light-client), but, given that BlobSidecar is not changed, I think that makes sense.

@@ -241,7 +251,7 @@ No more than `MAX_REQUEST_BLOCKS_DENEB` may be requested at a time.

Per `context = compute_fork_digest(fork_version, genesis_validators_root)`:

[1]: # (eth2spec: skip)
[0]: # (eth2spec: skip)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit curious when to use [0] and when to use [1] here. cc: @hwwhww

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, it's inconsistent inside the same document. Overall, [0] seems to be more.. "dominant" throughout the codebase, for these table annotations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It shouldn't really matter either way, it's only used to tell pyspec not to parse the table.

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you!

@jtraglia
Copy link
Member

Will merge after @ppopth and/or @hwwhww also approves.

@ppopth
Copy link
Member

ppopth commented Nov 28, 2024

I'm reluctant to merge this PR since we have never specified how to derive ForkDigestValue in the past gossipsub topics. I think if we want to do so, we have to do it with all topics.

@etan-status
Copy link
Contributor Author

https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/p2p-interface.md#topics-and-messages
It is inherited from phase0

ForkDigestValue - the lowercase hex-encoded (no "0x" prefix) bytes of compute_fork_digest(current_fork_version, genesis_validators_root) where

  • current_fork_version is the fork version of the epoch of the message to be sent on the topic
  • genesis_validators_root is the static Root found in state.genesis_validators_root

Also, it was your suggestion to switch to ForkDigestValue: #3850 (comment)

I'm also fine going back to ForkDigest-context. Just lmk which one you prefer :-)

@ppopth
Copy link
Member

ppopth commented Nov 28, 2024

It is inherited from phase0

What I meant is we didn't specify it in Altair, Bellatrix, or Capella.

Also, it was your suggestion to switch to ForkDigestValue: #3850 (comment)

I just suggested that it should be called ForkDigestValue instead of ForkDigest-context.

@etan-status
Copy link
Contributor Author

Hmm. The difference that's blob sidecars specific is that they are tied to a block, while the other messages are all standalone.

What the explicit spec of the ForkDigestValue does is guaranteeing that, during the fork transition from Deneb to Electra, that the cases where someone broadcasts some blobsidecars on electra and some on deneb is ruled out / as in, that blobs are always to be sent on the same gossip mesh that is also used for the linked beacon block.

In practice, this is (so far) only relevant in edge cases around the fork transition and possibly only createable by a weird implementation or by combining late block proposal with severe clock disparity / processing lag spikes that advance the clock between individual sidecars sending.

In principle we have three classes of gossip:

  • Somewhat perpetually valid concepts: SignedVoluntaryExit, AttesterSlashing, ProposerSlashing, BlsToExecutionChange -- the ForkDigestValue cannot be inferred from the data alone, and is based on the wall clock (as in, use the latest gossip mesh that's available).
    • For AttesterSlashing this works by coincidence because electra.AttesterSlashing is backward compatible in its serialization with phase0.AttesterSlashing.
  • Time-relevant concepts: Attestation, SignedContributionAndProof, SyncCommitteeMessage -- These have a clear slot that associates them with a particular fork, but still use the latest gossip mesh that's available.
    • Electra attestations can still vote for a Deneb head, the slot is not used to determine the SSZ type
  • Slot-specific concepts: SignedBeaconBlock, BlobSidecar, LightClientFinalityUpdate, LightClientOptimisticUpdate -- Here, the slot determines the SSZ type uniquely, and one has to use the correct gossip mesh associated with that type or rely on req/resp only if that (older) mesh is no longer available.
    • This is what this PR is targeting: The BlobSidecar should be sent on the same ForkDigestValue as its SignedBeaconBlock
    • This also ensures forward compatibility in case the BlobSidecar is changed in a future fork, e.g., because someone adds unrelated fields to a different type that affects the length of the blob sidecar inclusion proof.

That explains why this is a BlobSidecar specific thing. It's the first time we have separate parts of blocks being sent through separate topics.

@etan-status
Copy link
Contributor Author

Regarding whether there needs to be any specification for this at all, I wonder about the security implications as well of leaving this implementation-defined.

During the fork transition, CL implementations start building Electra meshes an epoch or so before the fork transition happens. Today, there is nothing that prevents nodes from broadcasting BlobSidecar on Deneb vs Electra meshes -- it's the same format. The PR here addresses that by forcing to use the same mesh as the underlying associated SignedBeaconBlock, i.e., if it is a blob sidecar for a Deneb block, then it has to use Deneb mesh for the blobs; and for Electra block, Electra mesh has to be used for the blobs.

If that is left open, blobs may appear on either of the meshes, or even on both. In case of both, question is if it can be used to introduce latency spikes by doubling the traffic. Answer there is no because there is an IGNORE condition in the blob processing that is global across gossip meshes and prevents forwarding duplicates. But I wonder if, say, some CL subscribe to Electra early, then some node broadcasts some blobs randomly on Electra and later re-broadcasts them on Deneb for some reason, how that interacts with propagation. Because the other nodes that first see it on Electra will only forward on Electra (but not all nodes are subscribed on Electra yet so parts of the network will miss this message). And then later when the re-broadcast on Deneb happens, the nodes that already saw it on Electra will not re-broadcast on Deneb (because of the gossip validation rule), so propagation is hindered. And any node can do that, e.g.., an attacker can listen to Deneb gossip and rebroadcast it on Electra mesh to try and mess with the propagation on Deneb if they have a privileged network position. There is no signature on the blobs.

So, while this is only about edge cases during the fork transition, and it's probably alright to leave it unspecified, I still think (1) there is no downside in restricting the blob to be sent on the same mesh as block, and (2) there may (or may not) be security implications if implementations can just pick whatever mesh they want for large messages such as blobsidecar.

And, also, this is unique for BlobSidecar only. All the other messages either are sent on whatever the latest gossip mesh is anyway, or are tied to a specific mesh because of their type. Only BlobSidecar has the same type on both Deneb and Electra, and is attached to a slot via its associated SignedBeaconBlock, and is also one of the large messages where propagation speed matters.

Copy link
Member

@ppopth ppopth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jtraglia jtraglia merged commit dcab13d into ethereum:dev Dec 3, 2024
26 checks passed
@etan-status etan-status deleted the ef-blobp2p branch December 4, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants