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

fix(blobs): Validate sidecar KzgCommitments and disallow sidecar omission #2291

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

shotes
Copy link
Contributor

@shotes shotes commented Dec 18, 2024

This PR fixes a few things:

  • Ensures that the number of commitments in the BeaconBlock and the number of BlobSidecars are equal.
  • Ensures that BlobSidecars do not contain duplicate BlobSidecars
  • (this implicitly verifies that the KzgCommitments from the BeaconBlock are 1 to 1 with BlobSidecars due to the inclusion proof)

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 80 lines in your changes missing coverage. Please review.

Project coverage is 30.77%. Comparing base (0a2b137) to head (c651059).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
beacon/blockchain/process_proposal.go 0.00% 64 Missing ⚠️
da/blob/verifier.go 0.00% 16 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2291      +/-   ##
==========================================
- Coverage   30.85%   30.77%   -0.08%     
==========================================
  Files         333      333              
  Lines       15237    15283      +46     
  Branches       20       20              
==========================================
+ Hits         4702     4704       +2     
- Misses      10210    10254      +44     
  Partials      325      325              
Files with missing lines Coverage Δ
da/types/sidecars.go 34.04% <ø> (+4.25%) ⬆️
da/blob/verifier.go 0.00% <0.00%> (ø)
beacon/blockchain/process_proposal.go 0.00% <0.00%> (ø)

Base automatically changed from blob-configurable-constants-cleanup to main December 18, 2024 09:29
@shotes shotes force-pushed the blob-duplicates-check branch from 6831d90 to f040e37 Compare December 18, 2024 16:23
Copy link
Contributor

@calbera calbera left a comment

Choose a reason for hiding this comment

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

makes sense to me

@shotes shotes marked this pull request as ready for review December 19, 2024 16:34
@shotes shotes changed the title fix(blobs): validate blobsidecar kzgcommitments and disallow sidecar omission fix(blobs): Validate sidecar KzgCommitments and disallow sidecar omission Dec 19, 2024
// VerifyIncomingBlock).
// Make sure we have the right number of BlobSidecars
numCommitments := len(blk.GetBody().GetBlobKzgCommitments())
if numCommitments != sidecars.Len() {
Copy link
Contributor

Choose a reason for hiding this comment

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

although we already check before calling .Len() if sidecars.IsNil() I think we should prevent any sidecars function from ever panicking on its own

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. here Len, GetSidecars, Get should all check .IsNil before dereferencing the pointer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we do in line 81 right?

@calbera calbera self-requested a review December 20, 2024 13:31
Copy link
Collaborator

@abi87 abi87 left a comment

Choose a reason for hiding this comment

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

lgtm

@abi87 abi87 merged commit 763d7d8 into main Dec 20, 2024
16 checks passed
@abi87 abi87 deleted the blob-duplicates-check branch December 20, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants