-
Notifications
You must be signed in to change notification settings - Fork 998
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
Conversation
|
||
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. |
There was a problem hiding this comment.
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:
- https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#name-bls12-381
- https://github.com/zkcrypto/pairing/blob/34aa52b0f7bef705917252ea63e5a13fa01af551/src/bls12_381/README.md#serialization
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
There was a problem hiding this 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.
### `recover_samples_impl` | ||
|
||
```python | ||
def recover_samples_impl(samples: Sequence[Tuple[int, Sequence[BLSFieldElement]]]) -> Polynomial: |
There was a problem hiding this comment.
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 😅
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
There was a problem hiding this 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.
From an initial investigation (and reading the corresponding ethresearch post), I think we can split
It might be possible to split 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. |
…ture. Add basic test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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
Add the spec for the cryptography that is required for sampling
Deneb change:
KZG_SETUP_G1_MONOMIAL
to the trusted setup fileKnown TODOs in other PRs: