Implement all needed KZG wrappers for peerDAS in the kzg package.
#15186
Conversation
This way, If we need to change the KZG backend, the only package to modify is the `kzg` package.
| @@ -1,4 +1,4102 @@ | |||
| { | |||
| "g1_monomial": [ | |||
There was a problem hiding this comment.
Is there a reference to this?
| @@ -0,0 +1,138 @@ | |||
| package kzg | |||
There was a problem hiding this comment.
Why is this package located in blockchain instead of a place like consensus-types? Curious if there’s any rationale or tradeoff behind that decision
There was a problem hiding this comment.
The new kzg.go file has been added here because the kzg package was located here.
Seems Potuz created this package here.
I'm not opposed to move it elsewhere. I don't have any strong opinion on that.
| } | ||
|
|
||
| // BlobToKZGCommitment computes a KZG commitment from a given blob. | ||
| func BlobToKZGCommitment(blob *Blob) (Commitment, error) { |
There was a problem hiding this comment.
I realized we dont have unit tests for functions in this file, should we have them?
There was a problem hiding this comment.
I asked myself the same question.
It turns out all these functions are only wrappers around the KZG backend.
For example:
// ComputeBlobKZGProof computes the blob KZG proof from a given blob and its commitment.
func ComputeBlobKZGProof(blob *Blob, commitment Commitment) (Proof, error) {
kzgBlob := kzg4844.Blob(*blob)
proof, err := kzg4844.ComputeBlobProof(&kzgBlob, kzg4844.Commitment(commitment))
if err != nil {
return [48]byte{}, err
}
return Proof(proof), nil
}does only:
- Cast a
*Blobinto akzg4844.Blob - Call
kzg4844.ComputeBlobProof - Return the error if error
- If no error, cast the result into
Proofand return it
Testing the wrapper itself does not add a lot of safety.
However:
- The KZG backend itself is very well tested (and will be externally audited in ~June)
- All functions using these wrappers are very well tested as well (as in PeerDAS: Implement core. #15192)
beacon-chain/blockchain/kzg/kzg.go
Outdated
|
|
||
| // BlobToKZGCommitment computes a KZG commitment from a given blob. | ||
| func BlobToKZGCommitment(blob *Blob) (Commitment, error) { | ||
| kzgBlob := kzg4844.Blob(*blob) |
There was a problem hiding this comment.
These cast directly from [BytesPerBlob]byte to the expected struct. If upstream changes the internal layout of ckzg4844.Blob, this could break. While currently safe (since it's just a byte array), a safer approach would be:
var kzgBlob kzg4844.Blob
copy(kzgBlob[:], blob[:])
There was a problem hiding this comment.
Same with (*ckzg4844.Blob)(blob)
beacon-chain/blockchain/kzg/kzg.go
Outdated
| return makeCellsAndProofs(ckzgCells[:], ckzgProofs[:]) | ||
| } | ||
|
|
||
| // VerifyBlobKZGProof verifies the KZG proofs for a given slice of commitments, cells indices, cells and proofs. |
There was a problem hiding this comment.
Comments should start with VerifyCellKZGProofBatch
beacon-chain/blockchain/kzg/kzg.go
Outdated
| var cells []Cell | ||
| var proofs []Proof |
There was a problem hiding this comment.
I think you can pre allocate these slices already
cells := make([]Cell, len(ckzgCells))
proofs := make([]Proof, len(ckzgProofs))
There was a problem hiding this comment.
Actually because we use append we should pre allocate the capacity but not the size.
…15186) * Implement all needed KZG wrappers for peerDAS in the `kzg` package. This way, If we need to change the KZG backend, the only package to modify is the `kzg` package. * `.bazelrc`: Add `build --compilation_mode=opt` * Remove --compilation_mode=opt, use supranational blst headers. * Fix Terence's comment. * Fix Terence`s comments. --------- Co-authored-by: Preston Van Loon <[email protected]>
…15186) * Implement all needed KZG wrappers for peerDAS in the `kzg` package. This way, If we need to change the KZG backend, the only package to modify is the `kzg` package. * `.bazelrc`: Add `build --compilation_mode=opt` * Remove --compilation_mode=opt, use supranational blst headers. * Fix Terence's comment. * Fix Terence`s comments. --------- Co-authored-by: Preston Van Loon <[email protected]>
…15186) * Implement all needed KZG wrappers for peerDAS in the `kzg` package. This way, If we need to change the KZG backend, the only package to modify is the `kzg` package. * `.bazelrc`: Add `build --compilation_mode=opt` * Remove --compilation_mode=opt, use supranational blst headers. * Fix Terence's comment. * Fix Terence`s comments. --------- Co-authored-by: Preston Van Loon <[email protected]>
…ffchainLabs#15186) * Implement all needed KZG wrappers for peerDAS in the `kzg` package. This way, If we need to change the KZG backend, the only package to modify is the `kzg` package. * `.bazelrc`: Add `build --compilation_mode=opt` * Remove --compilation_mode=opt, use supranational blst headers. * Fix Terence's comment. * Fix Terence`s comments. --------- Co-authored-by: Preston Van Loon <[email protected]>
…ffchainLabs#15186) * Implement all needed KZG wrappers for peerDAS in the `kzg` package. This way, If we need to change the KZG backend, the only package to modify is the `kzg` package. * `.bazelrc`: Add `build --compilation_mode=opt` * Remove --compilation_mode=opt, use supranational blst headers. * Fix Terence's comment. * Fix Terence`s comments. --------- Co-authored-by: Preston Van Loon <[email protected]>
Please read commit by commit.
This way, If we need to change the KZG backend, the only package to modify is the
kzgpackage.What type of PR is this?
Feature
What does this PR do? Why is it needed?
The whole picture is available here.
Other notes for review
Peerdas from scratch
Acknowledgements