forked from ethereum/go-ethereum
-
Notifications
You must be signed in to change notification settings - Fork 4
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 KZG library according to latest consensus spec changes #49
Merged
Merged
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
bf19039
Replace kzg-related data_blob.go type methods Parse, ComputeCommitmen…
roberto-bayardo 7601916
Remove ComputeCommitments which is unused.
roberto-bayardo 594fcbb
Migrate remaining EIP-4844 consensus spec code into kzg_new,
roberto-bayardo 0cc40fe
Start implementing a bytes API with appropriate interfaces
roberto-bayardo 6cbb79e
pure bytes API for VerifyKZGProof per 3097, and move kzg_new back int…
roberto-bayardo bcfd80a
rename verifyBlobs validateBlobTransactionWrapper per updated spec, a…
roberto-bayardo 8891367
fix tests & type bug in kzg verification
roberto-bayardo b4e09c8
more consensus layer support in kzg package
roberto-bayardo 42a69e6
remove aggregated proof from blobs bundle
roberto-bayardo 6018bd6
propagate malformed blob errors when processing a sent transaction so…
roberto-bayardo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the specs, each Blob is defined as a flat sequence of bytes, so something like:
type Blob [BYTES_PER_FIELD_ELEMENT * FIELD_ELEMENTS_PER_BLOB]byte
Which concretely is:
type Blob [32 * 4096]byte
You could probably do
type Blob []byte
but i'm not familiar enough with how we will manage different blob sizes to suggest this with convinction.In either case, you could then remove BLSFieldElement and it would be upto the crypto to partition each blob into 32 byte segments. You would then have an extra check which fails if the number of bytes is not divisible by 32 in the cryptography code, since the cryptography API would take a
[]byte
slice since we ideally want it to be generic over the blob sizesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating this would be a bit out of scope for this change, which was to get the implementation of the shared library (kzg_bytes.go) into shape to support multiple clients which may choose different representations that do not necessarily match the spec. For example the eip-4844 devnet Prysm fork uses [][]byte on the client side for its Blob type.
I am not 100% sure why these types were chosen the way they are to be honest. We could update them in a later change if this seems better. Since kzg_bytes.go now expects an interface for Blob it allows us to pretty easily change the client side representation if we decide to do so. All one needs to do is provide the simple interface adapter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I should have added, I did try to use the flat byte array Blob type from the specification in the kzg lib side but Golang doesn't make it easy to convert among such types without a lot of cumbersome & costly copying, so opted for that interface approach instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, yeah it makes sense to use an interface for now then.
As to why []byte, the rationale was that we abstracted away as much detail as possible from client devs, so just passing a stream of bytes for a blob seemed like the most opaque thing to do, because then clients didn't even need to know that the blobs should be segmented into 32 bytes each.
If the copies are costly compared to, I guess using protobuf(?) to do it, then maybe we need to revisit this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes I get why the spec uses a single opaque byte slice. I wasn't sure why the geth implementation decided to use type Blob [params.FieldElementsPerBlob]BLSFieldElement instead. Perhaps that was what was used in an earlier revision of the spec? I think Prysm uses [][]byte because that was convenient with their protobuf based parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberto-bayardo fyi we'll be changing the blob representation in prysm to a flat byte slice (we've already done this in upstream prysm).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but yeah i agree that doing this is out of scope. We can follow up later