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

Use official KZG ceremony output trusted setups #3521

Merged
merged 17 commits into from
Oct 18, 2023
Merged

Use official KZG ceremony output trusted setups #3521

merged 17 commits into from
Oct 18, 2023

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Oct 12, 2023

This PR changed our testing_trusted_setups.json to the official KZG ceremony output trusted setups for both minimal and mainnet presets. Two files in trusted_setups folder:

  1. trusted_setup_4096.json is from https://github.com/CarlBeek/kzg-ceremony-verifier/blob/master/output_setups/trusted_setup_4096.json
    • g1_lagrange: KZG_SETUP_LAGRANGE
    • g2_monomial: KZG_SETUP_G2
    • file sha256 checksum: 0229b43f4fac9b17374809520eb621b5ee1a7f74547e7d36918e7d4b122e178d

2. trusted_setup_4096_roots_of_unity.json contains the roots of unity that pyspec needs for polynomial-commitments.md. Client teams don't need it if the KZG libraries compute it.
- roots_of_unity: ROOTS_OF_UNITY

This PR is based on the #3255 proposal, which changed the minimal preset FIELD_ELEMENTS_PER_BLOB to 4096.


Updated: now we use a compute_roots_of_unity(order: uint64) -> Sequence[BLSFieldElement] to compute the roots of unity in polynomial-commitments.md rather than defining a ROOTS_OF_UNITY preset.

@hwwhww hwwhww requested review from asn-d6 and CarlBeek October 12, 2023 12:14
@hwwhww hwwhww added the Deneb was called: eip-4844 label Oct 12, 2023
Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

I know this is just a draft, but I'd like to suggest we rename a few variables. Nit picky stuff.

specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@CarlBeek CarlBeek left a comment

Choose a reason for hiding this comment

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

Regarding the roots of unity, @hwwhww mentions that "Client teams don't need it if the KZG libraries compute it."

My understanding is that none of the KZG libraries are planning on loading in these points manually. (C-KZG is planning on calculating them too now.) It takes longer to parse the JSON and load in the
elements than it does to just calculate them. I think we should just specify the primary root and leave it at that.

It allows us to remove the scripts/gen_roots_of_unity.py in addition to all the *_roots_of_unity.json files (in the interests of keeping this repo small

@jtraglia
Copy link
Member

Yes, agreed. I think it would be best to have a method in the spec for this. Something like:

PRIMITIVE_ROOT = 7

def get_roots_of_unity() -> List[int]:
    """
    Return roots of unity of order ``FIELD_ELEMENTS_PER_BLOB``.
    """
    assert (BLS_MODULUS - 1) % FIELD_ELEMENTS_PER_BLOB == 0
    root_of_unity = pow(PRIMITIVE_ROOT, (BLS_MODULUS - 1) // FIELD_ELEMENTS_PER_BLOB, BLS_MODULUS)
    return compute_powers(root_of_unity, FIELD_ELEMENTS_PER_BLOB)

@hwwhww
Copy link
Contributor Author

hwwhww commented Oct 13, 2023

@jtraglia @CarlBeek thanks for the reviews!

1. For executable pyspec

For pyspec, ROOTS_OF_UNITY will be defined in the executable file mainnet.py. It seems the two approaches (loading json file v.s. runtime compile) have similar speeds in pyspec. Also, pyspec can add an LRU cache of it. I'm fine with either way.

2. roots_of_unity implementation:

@jtraglia thank you for the pseudo code. :)

@dankrad once implemented def roots_of_unity(order: uint64) -> List[BLSFieldElement] helper in
https://github.com/ethereum/consensus-specs/blob/dev/specs/_features/sharding/polynomial-commitments.md#roots_of_unity

The difference is that this helper is also used in low_degree_check helper with difference order argument values. I suppose it's good to make such helpers more compatible by having the parameter order: uint64?

def compute_roots_of_unity(order: uint64) -> Sequence[BLSFieldElement]:
    """
    Return roots of unity of ``order``.
    """
    PRIMITIVE_ROOT = 7
    assert (BLS_MODULUS - 1) % int(order) == 0
    root_of_unity = pow(PRIMITIVE_ROOT, (BLS_MODULUS - 1) // int(order), BLS_MODULUS)
    return compute_powers(root_of_unity, order)

/cc @asn-d6


Moreover, if clients don't need to download trusted_setup_4096.json from test vectors repo, we can move presets/[minimal, mainnet]/trusted_setups to presets/trusted_setups

@asn-d6
Copy link
Contributor

asn-d6 commented Oct 13, 2023

@jtraglia @CarlBeek thanks for the reviews!

1. For executable pyspec

For pyspec, ROOTS_OF_UNITY will be defined in the executable file mainnet.py. It seems the two approaches (loading json file v.s. runtime compile) have similar speeds in pyspec. Also, pyspec can add an LRU cache of it. I'm fine with either way.

2. roots_of_unity implementation:

@jtraglia thank you for the pseudo code. :)

@dankrad once implemented def roots_of_unity(order: uint64) -> List[BLSFieldElement] helper in https://github.com/ethereum/consensus-specs/blob/dev/specs/_features/sharding/polynomial-commitments.md#roots_of_unity

The difference is that this helper is also used in low_degree_check helper with difference order argument values. I suppose it's good to make such helpers more compatible by having the parameter order: uint64?

def compute_roots_of_unity(order: uint64) -> Sequence[BLSFieldElement]:
    """
    Return roots of unity of ``order``.
    """
    PRIMITIVE_ROOT = 7
    assert (BLS_MODULUS - 1) % int(order) == 0
    root_of_unity = pow(PRIMITIVE_ROOT, (BLS_MODULUS - 1) // int(order), BLS_MODULUS)
    return compute_powers(root_of_unity, order)

/cc @asn-d6

Hey. That code looks good to me. No strong opinion on whether the func should be generic (to also work with sharding) or more specific (for simplicity atm), but both functions look good to me and I cross-checked that they generate the same list.

Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

Other than the naming notes from @jtraglia above, this PR looks good to me!

@dankrad
Copy link
Contributor

dankrad commented Oct 13, 2023

I think it might make sense to go with the flexible roots of unity implementation. Since we are now looking into implementing some form of DAS much sooner, we will need it anyway so the very minor complication won't hurt.

@dankrad
Copy link
Contributor

dankrad commented Oct 13, 2023

(Also, I noticed that the spec is currently under-defined in this respect: While the group of roots of unity with a certain order is unique, the order is not. So we do really need to include the function to compute the actual list for it to be complete)

@mratsim
Copy link
Contributor

mratsim commented Oct 16, 2023

My understanding is that none of the KZG libraries are planning on loading in these points manually. (C-KZG is planning on calculating them too now.) It takes longer to parse the JSON and load in the
elements than it does to just calculate them. I think we should just specify the primary root and leave it at that.

By the way, I did a survey a couple months ago to assess if there was interest in creating an efficient serialization format for trusted setups:

The .json format will need to be changed to another format for C as parsing json from C is problematic from a security point-of-view.

@hwwhww
Copy link
Contributor Author

hwwhww commented Oct 16, 2023

@jtraglia @asn-d6
The kzg-4844 test vector tarball: kzg.tar.gz

@hwwhww hwwhww marked this pull request as ready for review October 16, 2023 14:22
@jtraglia
Copy link
Member

The kzg-4844 test vector tarball: kzg.tar.gz

These reference tests pass with the official trusted setup (converted to c-kzg's format) 👍

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

I think we can delete a few things now that we're computing the roots at runtime.

scripts/gen_roots_of_unity.py Outdated Show resolved Hide resolved
tests/core/pyspec/eth2spec/utils/kzg.py Outdated Show resolved Hide resolved
Copy link
Member

@jtraglia jtraglia 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! Thanks @hwwhww 🥇

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.

did a high level review, looks good

specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
Comment on lines -101 to -102
The trusted setup is part of the preset: during testing a `minimal` insecure variant may be used,
but reusing the `mainnet` settings in public networks is a critical security requirement.
Copy link
Contributor Author

@hwwhww hwwhww Oct 17, 2023

Choose a reason for hiding this comment

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

Should add some info/references for the KZG ceremony here? @CarlBeek's https://github.com/CarlBeek/kzg-ceremony-verifier repo makes sense to me; should we move that repo to @ethereum org?

@tvanepps
Copy link

not sure if it's too late to change but would it be possible to not use the "trusted setup" terminology? comes with more baggage from a time when all setup participants could be counted on two hands =)

maybe something like "kzg_setup" might be better?

@hwwhww
Copy link
Contributor Author

hwwhww commented Oct 18, 2023

Hi @tvanepps

Our team had a discussion about this topic yesterday.

The specs use trusted_setups as the folder name because they are defined in the "Trusted setup" section in polynomial-commitments.md. If we do want to sync all the names that include trusted_setup_4096.json file name, I'd say @CarlBeek's upstream https://github.com/CarlBeek/kzg-ceremony-verifier/blob/master/output_setups/trusted_setup_4096.json should be renamed since we copied it from upstream and it's better to make it clear they are identical files.

IIRC client software and KZG libraries have provided --trusted-setup-file flag. I agree with @dankrad that trusted setup is already a common technical term. Therefore, it may not be worth actively "replacing" them. For the specs, I'm fine with syncing the folder/file/presets naming, but it's better to add some more info in polynomial-commitments.md as I suggested in #3521 (comment).

I'm going to merge this PR for testgen now, but a post-merge (or post this release) clean-up PR is welcome. :)

@hwwhww hwwhww merged commit 0002f43 into dev Oct 18, 2023
13 checks passed
@hwwhww hwwhww deleted the official-kzg branch October 18, 2023 09:17
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.

9 participants