Deneb crypto helpers test coverage #3283
Conversation
|
What's up with |
|
@dankrad yes, it's fine to remove |
| #### `verify_sidecar_signature` | ||
|
|
||
| ```python | ||
| def verify_blob_sidecar_signature(state: BeaconState, signed_blob_sidecar: SignedBlobSidecar) -> bool: |
There was a problem hiding this comment.
should add description of when to use it
specs/deneb/validator.md
Outdated
| def get_blobs_and_kzg_commitments(payload_id: PayloadId) -> \ | ||
| Tuple[Sequence[BLSFieldElement], Sequence[KZGCommitment], Sequence[KZGProof]]: |
There was a problem hiding this comment.
[off this PR]
- By the function name, it looks like it should return
Sequence[Blob]instead ofSequence[BLSFieldElement]? - And this PR now adds
Sequence[KZGProof]... should it be renamed to signal it?
How about using the similar name of verify_blob_kzg_proof_batch:
| def get_blobs_and_kzg_commitments(payload_id: PayloadId) -> \ | |
| Tuple[Sequence[BLSFieldElement], Sequence[KZGCommitment], Sequence[KZGProof]]: | |
| def get_blob_kzg_proof_batch( | |
| payload_id: PayloadId | |
| ) -> Tuple[Sequence[Blob], Sequence[KZGCommitment], Sequence[KZGProof]]: |
There was a problem hiding this comment.
You're right on the signature, disagree on the naming though. It's not "batch" -- it's just an API to get all of them from the execution engine. There is no way to "not batch" this.
We could add proofs to the name, but I feel it's a bit long and unnecessary, the name does not need to provide information on everything the function is doing.
| @@ -66,13 +67,14 @@ use the `payload_id` to retrieve `blobs` and `blob_kzg_commitments` via `get_blo | |||
| ```python | |||
| def validate_blobs_and_kzg_commitments(execution_payload: ExecutionPayload, | |||
There was a problem hiding this comment.
how about using a similar name of verify_blob_kzg_proof_batch:
| def validate_blobs_and_kzg_commitments(execution_payload: ExecutionPayload, | |
| def validate_blob_kzg_proof_batch(execution_payload: ExecutionPayload, |
There was a problem hiding this comment.
Again I don't think it's a "batch" because it's just all of them. Batch suggests it's an arbitrary set which it is not.
| @@ -44,7 +44,8 @@ Note: This API is *unstable*. `get_blobs_and_kzg_commitments` and `get_payload` | |||
| Implementers may also retrieve blobs individually per transaction. | |||
There was a problem hiding this comment.
- Should we remove the "API unstable" warning?
- Please update ""The interface to retrieve blobs and corresponding kzg commitments." to note that it retrieves proofs too
There was a problem hiding this comment.
Ah and I checked, the engine api doesn't currently give you the proofs. so right now, adding the proofs to this would be not in sync with the engine api
That said abstracting it to magically show up in the validator spec does seem correct/safe so I'm okay leaving this as is
There was a problem hiding this comment.
We should change the engine to give the proofs, because they should be part of the BlobTransactionNetworkWrapper.
Which made me notice, it seems the EIP has not been updated to include this yet. Seems to be included in this PR: ethereum/EIPs#6610
There was a problem hiding this comment.
I see, let's go to engine api after eip is merged
Working on #3258