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

EIP-7594: Add cryptography specs for sampling #3557

Merged
merged 29 commits into from Jan 15, 2024

Conversation

dankrad
Copy link
Contributor

@dankrad dankrad commented Dec 4, 2023

Add the spec for the cryptography that is required for sampling


Deneb change:

  • Add KZG_SETUP_G1_MONOMIAL to the trusted setup file

Known TODOs in other PRs:

  • Fix linter
  • Fix roots of unity preset
  • Accelerate the performance (in algorithm or use other binding)
  • Refactor
  • Add more test cases

specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved

For any KZG library extended to support DAS, functions flagged as "Public method" MUST be provided by the underlying KZG library as public functions. All other functions are private functions used internally by the KZG library.

Public functions MUST accept raw bytes as input and perform the required cryptographic normalization before invoking any internal functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like:

Public functions MUST accept raw bytes as input, serialized according to:

and perform the following checks:

  • Field element a:
    • in the range 0 <= a < r
  • Elliptic curve point P:
    • field coordinates (x, y) are in the range 0 <= x, y < p
    • Point is on the curve (G1: y² = x³ + b, G2: y² = x³ + ξb, with ξ the multiplicative twist)
    • Point is in the proper subgroup

Details are available in the IETF spec for KeyValidate, but identity point is allowed: https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#name-keyvalidate

specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md 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.

Looking great! Here are some things I noticed during my initial review.

Also, how would you feel about a more complex sample structure? For example:

class Sample(Container):
    data: Vector[BLSFieldElement, FIELD_ELEMENTS_PER_SAMPLE]
    proof: KZGProof
    row_index: uint64
    column_index: uint64

I got this idea from the aggregated sample verification proof of concept here. When I made the changes to my prototyping work, I found that it simplified a lot and made the code more human readable.

specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
### `recover_samples_impl`

```python
def recover_samples_impl(samples: Sequence[Tuple[int, Sequence[BLSFieldElement]]]) -> Polynomial:
Copy link
Member

Choose a reason for hiding this comment

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

This function would benefit from being broken up into smaller pieces 😅

specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
@hwwhww
Copy link
Contributor

hwwhww commented Dec 12, 2023

Discussed with @dankrad today.

Some TODO actions:

  • [@hwwhww] clean up cryptography specs:
    • move to _features/peerdas folder.
    • make it executable there.
  • [@hwwhww] sync the naming: use “cell” for the unit in matrix
  • Find cryptographers to help review the cryptography logic

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.

Awesome work here! Did an initial rudimentary review. I would like to spend some time next week to think how to break down recover_samples_impl() into more granular functions.

specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
@asn-d6
Copy link
Contributor

asn-d6 commented Dec 29, 2023

Awesome work here! Did an initial rudimentary review. I would like to spend some time next week to think how to break down recover_samples_impl() into more granular functions.

From an initial investigation (and reading the corresponding ethresearch post), I think we can split recover_samples_impl() into the following subroutines:

  • construct_vanishing_polynomial() which constructs the vanishing "subset" polynomial (i.e. up to the first round of asserts)
  • recover_shifted_data() (i.e. up to eval_shifted_reconstructed_poly)
  • recover_original_data() (i.e. up to the end of the func)

It might be possible to split recover_shifted_data() further into what Vitalik calls Q_1, Q_2, Q_3 in his post, but we can see about that.

I can take a stab at it next week.

draghtnight

This comment was marked as spam.

@ethereum ethereum deleted a comment from draghtnight Jan 2, 2024
@asn-d6
Copy link
Contributor

asn-d6 commented Jan 4, 2024

Awesome work here! Did an initial rudimentary review. I would like to spend some time next week to think how to break down recover_samples_impl() into more granular functions.

From an initial investigation (and reading the corresponding ethresearch post), I think we can split recover_samples_impl() into the following subroutines:

  • construct_vanishing_polynomial() which constructs the vanishing "subset" polynomial (i.e. up to the first round of asserts)
  • recover_shifted_data() (i.e. up to eval_shifted_reconstructed_poly)
  • recover_original_data() (i.e. up to the end of the func)

It might be possible to split recover_shifted_data() further into what Vitalik calls Q_1, Q_2, Q_3 in his post, but we can see about that.

I can take a stab at it next week.

FWIW, after discussion with @hwwhww we decided to do this sort of refactoring after the spec is executable and minimally tested, to avoid introducing bugs during the refactor.

@hwwhww
Copy link
Contributor

hwwhww commented Jan 5, 2024

Changelog:

  • Add KZG_SETUP_G1_MONOMIAL to trusted setup files (Thank @CarlBeek for updating upstream files!)
  • Move new docs to the new peerdas feature folder
  • Fix some helpers (03583b1)
  • Add basic verification tests
  • Add PeerDAS CI job

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.

@asn-d6 @dankrad

I added successful tests for verification and recover functions! @dankrad it was amazing that (I think) there was no tricky bug! 👏 👏

@asn-d6 feel free to go refactor the recover function. I will try to settle down the format of roots of unity in the meanwhile.

@hwwhww hwwhww marked this pull request as ready for review January 8, 2024 17:40
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.

Per the typing, there is still a mess around PolynomialCoeff. However, to make it mergeable, I skipped the linter check for PeerDAS feature. If anyone wants to add it back, just revert 09c2519

@asn-d6 and I suggest that we merge this PR as status-quo and add more iteration PRs later.

Known TODOs in other PRs:

  • Fix linter
  • Fix roots of unity preset
  • Accelerate the performance (in algorithm or use other binding)
  • Refactor
  • Add more test cases

specs/_features/peerdas/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
@hwwhww hwwhww added the general:enhancement New feature or request label Jan 10, 2024
@hwwhww hwwhww mentioned this pull request Jan 11, 2024
1 task
@hwwhww hwwhww changed the title Add cryptography specs for sampling EIP-7594: Add cryptography specs for sampling Jan 15, 2024
@hwwhww hwwhww merged commit 1509b22 into dev Jan 15, 2024
30 checks passed
@jtraglia jtraglia deleted the polynomial-commitments-sampling branch April 22, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants