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.

Sign up for GitHub

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

Deneb crypto helpers test coverage #3283

Merged
merged 17 commits into from
Mar 15, 2023
Merged

Deneb crypto helpers test coverage #3283

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ca8a51f
More CI tests for polynomial commitments
dankrad Mar 6, 2023
661cca5
Import `curve_order as BLS_MODULUS`
hwwhww Mar 7, 2023
81ab7de
Add unit tests for validate_kzg_g1 and bytes_to_bls_field
dankrad Mar 7, 2023
cce82b4
Remove spec. for getting BLS_MODULUS
dankrad Mar 7, 2023
b4c130a
Remove state from unit tests
dankrad Mar 7, 2023
08ba1f6
Merge branch 'dev' into deneb-crypto-coverage
dankrad Mar 7, 2023
a5333a1
Remove useless test_kzg.py
dankrad Mar 10, 2023
5e74c51
Tests for validate_blobs_and_kzg_commitments and fix
dankrad Mar 11, 2023
29b5309
Add proofs to validate_blobs_and_kzg_commitments
dankrad Mar 11, 2023
96ad61b
Add tests for blob sidecar signature
dankrad Mar 12, 2023
723e8a1
Remove unused imports/functions in test_validator.py
dankrad Mar 12, 2023
cc284b2
Toc
dankrad Mar 12, 2023
ff7a6c5
Add description when to use verify_sidecar_signature
dankrad Mar 14, 2023
2d4bfab
Correct signature of get_blobs_and_kzg_commitments
dankrad Mar 14, 2023
3e281e7
Alternative for linter
dankrad Mar 14, 2023
3141806
Merge branch 'dev' into deneb-crypto-coverage
dankrad Mar 14, 2023
5977f36
minor nits from code review
djrtwo Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion specs/deneb/p2p-interface.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ The following validations MUST pass before forwarding the `sidecar` on the netwo
- _[IGNORE]_ The sidecar's block's parent (defined by `sidecar.block_parent_root`) has been seen (via both gossip and non-gossip sources) (a client MAY queue sidecars for processing once the parent block is retrieved).
- _[REJECT]_ The sidecar's block's parent (defined by `sidecar.block_parent_root`) passes validation.
- _[REJECT]_ The sidecar is from a higher slot than the sidecar's block's parent (defined by `sidecar.block_parent_root`).
- _[REJECT]_ The proposer signature, `signed_blob_sidecar.signature`, is valid with respect to the `sidecar.proposer_index` pubkey.
- _[REJECT]_ The proposer signature, `signed_blob_sidecar.signature`, is valid as verified by `verify_sidecar_signature`
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
- _[IGNORE]_ The sidecar is the only sidecar with valid signature received for the tuple `(sidecar.block_root, sidecar.index)`.
- _[REJECT]_ The sidecar is proposed by the expected `proposer_index` for the block's slot in the context of the current shuffling (defined by `block_parent_root`/`slot`).
If the `proposer_index` cannot immediately be verified against the expected shuffling, the sidecar MAY be queued for later processing while proposers for the block's branch are calculated -- in such a case _do not_ `REJECT`, instead `IGNORE` this message.
Expand Down
5 changes: 3 additions & 2 deletions specs/deneb/validator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ Note: This API is *unstable*. `get_blobs_and_kzg_commitments` and `get_payload`
Implementers may also retrieve blobs individually per transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Should we remove the "API unstable" warning?
  2. Please update ""The interface to retrieve blobs and corresponding kzg commitments." to note that it retrieves proofs too

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@dankrad dankrad Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, let's go to engine api after eip is merged


```python
def get_blobs_and_kzg_commitments(payload_id: PayloadId) -> \
Tuple[Sequence[BLSFieldElement], Sequence[KZGCommitment], Sequence[KZGProof]]:
def get_blobs_and_kzg_commitments(
payload_id: PayloadId
) -> Tuple[Sequence[Blob], Sequence[KZGCommitment], Sequence[KZGProof]]:
# pylint: disable=unused-argument
...
```
Expand Down