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

Executable EIP-4844 specs (PR #2901 squashed and rebased) #2937

Merged
merged 12 commits into from
Jul 16, 2022

Conversation

asn-d6
Copy link
Contributor

@asn-d6 asn-d6 commented Jul 13, 2022

Here is a squashed and rebased version of PR #2901 which makes EIP-4844 executable.

Squashing was non-trivial because of the various Merge branch 'dev' into eip4844-exe commits that were included in #2901, but I think this is a correct attempt.

@hwwhww I would appreciate a review here. The git diff between this branch and the branch from PR #2901 should be all the changes in dev that happened inbetween and were orthogonal to #2901.

asn-d6 and others added 4 commits July 13, 2022 13:12
- Move more code into polynomial-commitments.md
- Implement aggregated sidecar verification logic from PR ethereum#2915
- Rename `kzgs` to `kzg_commitments`

Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
- Move constants around
- Implement missing functions to make the spec executable

Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
- Implement all the required glue code to make things executable
- Implement a dummy KZG trusted setup

Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Thanks for the rebase! LGTM! 🎉

I also reviewed git diff pr2937..eip4844-exe.

We can close #2901 now.

@asn-d6 asn-d6 marked this pull request as ready for review July 13, 2022 12:31
@hwwhww hwwhww requested a review from djrtwo July 13, 2022 12:32
@hwwhww hwwhww added the Deneb was called: eip-4844 label Jul 13, 2022
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

looks great! a few minor nits

all_versioned_hashes = []
for tx in transactions:
if tx[0] == BLOB_TX_TYPE:
all_versioned_hashes.extend(tx_peek_blob_versioned_hashes(tx))
Copy link
Contributor

Choose a reason for hiding this comment

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

extend is a bit python. I had to look it up! so most non-pythonista readers of speak might need to

@@ -117,12 +106,14 @@ Alias `sidecar = signed_blobs_sidecar.message`.
- _[REJECT]_ the `sidecar.blobs` are all well formatted, i.e. the `BLSFieldElement` in valid range (`x < BLS_MODULUS`).
- _[REJECT]_ The KZG proof is a correctly encoded compressed BLS G1 Point -- i.e. `bls.KeyValidate(blobs_sidecar.kzg_aggregated_proof)
- _[REJECT]_ the beacon proposer signature, `signed_blobs_sidecar.signature`, is valid -- i.e.
```python

```
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
```
```python

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a silly workaround for not including this code snippet to pyspec. 😅

Alternatively, we can use "```py". Still not perfect though.

Copy link
Contributor

@hwwhww hwwhww Jul 15, 2022

Choose a reason for hiding this comment

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

I changed this code snippet into listed multi-steps: ec980da

specs/eip4844/validator.md Show resolved Hide resolved

After retrieving the execution payload from the execution engine as specified in Bellatrix,
the blobs are retrieved and processed:

```python
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this code snippet expected to be run by the validator?

I would make "are retrieved and processed" more as a direction rather than just a passive statement that it happens. "execute the following" or something

return [VersionedHash(opaque_tx[x:x+32]) for x in range(blob_versioned_hashes_offset, len(opaque_tx), 32)]
blob_versioned_hashes_offset = (
message_offset
+ uint32.decode_bytes(opaque_tx[(message_offset + 156):(message_offset + 160)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this math previously incorrect? (that it previously didn't add in message_offset)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, it was incorrect.

presets/mainnet/eip4844.yaml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@hwwhww hwwhww force-pushed the eip4844-exe-squashed-and-rebased branch from 90edfb9 to 8776c05 Compare July 15, 2022 16:54
hwwhww added 2 commits July 16, 2022 01:13
…ig_fork_epoch_overrides`

`config_fork_epoch_overrides`: since Capella and EIP4844 are in parallel, need to check if the field exists

Update `compute_fork_version`
@hwwhww hwwhww force-pushed the eip4844-exe-squashed-and-rebased branch from 8776c05 to ec980da Compare July 15, 2022 17:13
@hwwhww
Copy link
Contributor

hwwhww commented Jul 15, 2022

Updated

@asn-d6 asn-d6 merged commit 9e4f386 into ethereum:dev Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants