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

Add blob_sidecar event #350

Merged
merged 4 commits into from
Sep 18, 2023
Merged

Add blob_sidecar event #350

merged 4 commits into from
Sep 18, 2023

Conversation

dapplion
Copy link
Collaborator

Discussion points

  • unsure to name the event blob or blob_sidecar. There won't be two different types of blob events so blob is shorter. But blob_sidecar matches the returned container summary.
  • should the event include the kzg_commitment, the versioned hash or both? I tend to say that the version_hash is more useful since that's what the app layer is concerned about. Also one can derive the version hash, see kzg_commitment_to_versioned_hash

Closes #348

@mcdee
Copy link
Contributor

mcdee commented Sep 11, 2023

Rationale behind including the KZG commitment is that https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/beacon-chain.md#beaconblockbody only contains the commitment. So if (for example) you received a blob event but wanted to see if the same blob had shown up in an earlier (orphaned) block you could do so relatively simply.

Not sure about how useful this would be in the real world, but the commitment is the only way that I can see to link blobs to consensus blocks (beyond the obvious other information already in the event).

I would include the versioned hash as well, as that provides a similar function for the execution layer.

@james-prysm
Copy link
Contributor

james-prysm commented Sep 12, 2023

blob_sidecar matches the returned container summary.

casting my vote for blob_sidecar

rkapka
rkapka previously approved these changes Sep 15, 2023
Copy link
Collaborator

@rkapka rkapka 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

rolfyone
rolfyone previously approved these changes Sep 18, 2023
Copy link
Collaborator

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM, just needs to be added to the change list.

@rolfyone rolfyone dismissed stale reviews from rkapka and themself via 5a8326b September 18, 2023 23:49
Copy link
Collaborator

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Add Blob sidecar information to event stream
5 participants