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

update Deneb for blob sidecar inclusion proofs #5565

Merged
merged 7 commits into from
Nov 6, 2023

Conversation

etan-status
Copy link
Contributor

@etan-status etan-status commented Nov 4, 2023

BlobSidecar is no longer signed, instead use Merkle proof to link blobs with block.

Associated beacon-API / builder-specs still TBD; minimal changes done to compile in similar style to previous spec, but not standardized yet.

`BlobSidecar` is no longer signed, instead use Merkle proof to link
blobs with block.

- ethereum/consensus-specs#3531

Associated beacon-API / builder-specs still TBD; minimal changes done
to compile in similar style to previous spec, but not standardized yet.

- ethereum/beacon-APIs#369
- ethereum/builder-specs#90
Copy link

github-actions bot commented Nov 5, 2023

Unit Test Results

         9 files  ±  0    1 098 suites  ±0   31m 45s ⏱️ + 4m 35s
  3 945 tests +52    3 598 ✔️ +2  347 💤 +50  0 ±0 
16 048 runs  +72  15 650 ✔️  - 3  398 💤 +75  0 ±0 

Results for commit d88f1d3. ± Comparison against base commit e4dacc3.

♻️ This comment has been updated with latest results.

Comment on lines +435 to +436
# [REJECT] The sidecar's blob is valid as verified by `verify_blob_kzg_proof(
# blob_sidecar.blob, blob_sidecar.kzg_commitment, blob_sidecar.kzg_proof)`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double-checked with @mratsim , the proposer sig check is the faster one, so doing it before the KZG check: "pairings are 2 levels deep in KZG and 1 deep in BLS signature (just after deserialization and sanity checks that points are on curve and non-infinity)"

Batching of the two verifications would be clunky, likely not worth it.

Comment on lines +413 to +414
kzg_proofs: blobsBundle.proofs,
blobs: blobsBundle.blobs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sidecars are no longer sent back and forth, as they no longer need a signature. Instead, just the kzg_proofs and blobs are sent.

kzg_proofs and blobs are downloaded by the VC and then submitted again to the BN to support use case where the block is produced by a different BN than the one who is broadcasting. The BN then converts the SignedDenebBlockContents to SignedBeaconBlock + BlobSidecar list

Comment on lines +97 to +99
KzgProofs* = List[KzgProof, Limit MAX_BLOB_COMMITMENTS_PER_BLOCK]
Blobs* = List[Blob, Limit MAX_BLOB_COMMITMENTS_PER_BLOCK]
BlobRoots* = List[Eth2Digest, Limit MAX_BLOB_COMMITMENTS_PER_BLOCK]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where to put them and what to put as the correct limit. beacon-API uses 0 .. MAX_BLOBS_PER_BLOCK in informal metadata description, builder-API uses MAX_BLOB_COMMITMENTS_PER_BLOCK in a more explicit specification. In practice, it doesn't matter as the SSZ messages are not hash-tree-rooted in neither of the two protocols. If they were hash_tree_rooted, using the theoretical capacity (MAX_BLOB_COMMITMENTS_PER_BLOCK) makes more sense (that is also what they use for the KzgCommitments inside consensus, where HTR matters).

@@ -69,25 +69,18 @@ type

# https://github.com/ethereum/builder-specs/blob/534e4f81276b8346d785ed9aba12c4c74b927ec6/specs/deneb/builder.md#blindedblobsidecar
BlindedBlobSidecar* = object
block_root*: Eth2Digest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BlindedBlobSidecar may go away entirely, but the specs are still in flux:

For now, have done the minimal changes to it to make the flow work in theory, but the spec may optimize further and get rid of the BlindedBlobSidecar altogether.

@etan-status
Copy link
Contributor Author

etan-status commented Nov 5, 2023

This should be all that's needed to get us going with the new blob sidecar flavor, as an initial PR upgrading to v1.4.0-beta.4 test vectors.

  • Remove SignedBlobSidecar, and replace with new BlobSidecar conceptually
  • Change BlobSidecar, and replace with kzg_proofs + blobs lists conceptually
  • BN converts signed_block (kzg_commitments) + kzg_proofs + blobs lists to BlobSidecar and puts in cryptographic link via Merkle proof, instead of VC putting in a signature
  • Beacon-APIs updated for new flow
  • Builder-APIs flow updated minimally, it still uses BlindedBlobSidecar -> BlobSidecar flow; may become simpler in the future

Notably missing:

  • Final beacon-API and builder-API specs
  • Gossip per-chunk validation: per-chunk validation of libp2p req/resp chunk #5557
  • I think there could also be more sanity checking, but that's not a new problem. As in, any time one receives via REST a signed block, validate (minimally) that KZG proofs are correct, and that inclusion proofs for sidecars are correct. Helper functions are there. Not high prio as it only affects trusted REST API I think.
  • Same for VC, could check that proofs are valid before signing a block.

@etan-status etan-status marked this pull request as ready for review November 5, 2023 09:30
@etan-status etan-status merged commit d8a7f0d into unstable Nov 6, 2023
8 checks passed
@etan-status etan-status deleted the dev/etan/df-proof branch November 6, 2023 06:48
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