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 new BlobSidecar #7668

Merged
merged 6 commits into from
Nov 8, 2023
Merged

Add new BlobSidecar #7668

merged 6 commits into from
Nov 8, 2023

Conversation

zilm13
Copy link
Contributor

@zilm13 zilm13 commented Nov 6, 2023

PR Description

Includes

  • BlobSidecar and its schema
  • Property test
  • new randomBlobSidecar

Part of #7654

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@zilm13 zilm13 self-assigned this Nov 7, 2023
Copy link
Contributor

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

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

LGTM with few nits

@@ -7,4 +7,6 @@ FIELD_ELEMENTS_PER_BLOB: 4096
# `uint64(2**12)` (= 4096)
MAX_BLOB_COMMITMENTS_PER_BLOCK: 4096
# `uint64(6)`
MAX_BLOBS_PER_BLOCK: 6
Copy link
Contributor

Choose a reason for hiding this comment

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

There are also 3 deneb.yaml files that need to be updated under ethereum/spec/src/test/resources/tech/pegasys/teku/spec/config/standard/presets

}

public BlobSidecar(
BlobSidecarSchema schema,
Copy link
Contributor

Choose a reason for hiding this comment

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

final 🗡️

@@ -58,6 +59,7 @@ public SpecConfigDeneb build(final SpecConfigCapella specConfig) {
maxRequestBlobSidecars,
minEpochsForBlobSidecarsRequests,
blobSidecarSubnetCount,
kzgCommitmentInclusionProofDepth,
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit but would add this variable under maxBlobsPerBlock to align with the spec ordering

@zilm13 zilm13 enabled auto-merge (squash) November 8, 2023 12:27
@zilm13 zilm13 merged commit 6fd875d into Consensys:master Nov 8, 2023
4 checks passed
@zilm13 zilm13 deleted the new-blobsidecar branch November 8, 2023 13:35
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.

2 participants