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

chore: update the trusted setup to match the consensus-specs version #6117

Merged
merged 4 commits into from
Jul 18, 2024

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://raw.githubusercontent.com/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://raw.githubusercontent.com/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