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.

Sign up for GitHub

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

Jump to bottom

chore: Refactor cell recovery code #3781

Merged
merged 29 commits into from
Jun 11, 2024
Merged

chore: Refactor cell recovery code #3781

merged 29 commits into from
Jun 11, 2024

Conversation

kevaundray
Copy link
Contributor

This PR modularizes what was previously being called a "shift on a polynomial" to the coset_fft method.

Initially it was named "shift_fft_field" so that it is not confused with the cosets created by reverse_bit_ordering, but left it as this because this is the conventional name.

Changes made:

  • Remove shift_polynomial_coeff
  • Remove recover_shifted_data
  • Remove recover_original_data
  • Move zero_poly_eval_brp under sanity check comment as its only used for sanity checking

@kevaundray kevaundray marked this pull request as ready for review May 27, 2024 21:39
@kevaundray kevaundray changed the title chore: Refactor cell recovery chore: Refactor cell recovery code May 27, 2024
@hwwhww hwwhww added the EIP-7594 PeerDAS label May 28, 2024
jtraglia
jtraglia reviewed May 28, 2024
View reviewed changes
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 this refactor is cleaner, thanks!

specs/_features/eip7594/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/_features/eip7594/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/_features/eip7594/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
asn-d6
asn-d6 reviewed Jun 6, 2024
View reviewed changes
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.

Initial pass.

specs/_features/eip7594/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/_features/eip7594/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/_features/eip7594/polynomial-commitments-sampling.md Show resolved Hide resolved
b-wagn
b-wagn reviewed Jun 7, 2024
View reviewed changes
Copy link
Contributor

@b-wagn b-wagn left a comment

Choose a reason for hiding this comment

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

I left some minor comments.
In general, I think this change improves the structure a lot. 👍

...pyspec/eth2spec/test/eip7594/unittests/polynomial_commitments/test_polynomial_commitments.py Outdated Show resolved Hide resolved
...pyspec/eth2spec/test/eip7594/unittests/polynomial_commitments/test_polynomial_commitments.py Show resolved Hide resolved
...pyspec/eth2spec/test/eip7594/unittests/polynomial_commitments/test_polynomial_commitments.py Show resolved Hide resolved
specs/_features/eip7594/polynomial-commitments-sampling.md Show resolved Hide resolved
specs/_features/eip7594/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
kevaundray and others added 16 commits June 7, 2024 19:05
@kevaundray
multi:
7930128 
- Remove shift_polynomial_coeff
- Remove recover_shifted_data
- Remove recover_original_data
- Move `zero_poly_eval_brp ` under sanity check comment as its only used for sanity checking
@kevaundray
chore: remove sanity check -- this was doing a wasteful `compute_root…
61598ae 
…_of_unity` operation
@kevaundray
chore: add previous sanity check as a unit test
3382f84
@kevaundray
chore: copy values python was taking a reference, so it passes in our…
583adb4 
… regular codepaths but not in isolated test
@kevaundray
chore: add coset_fft test
01873d5
@kevaundray @jtraglia
Update specs/_features/eip7594/polynomial-commitments-sampling.md
6565122 
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
@kevaundray @jtraglia
Update specs/_features/eip7594/polynomial-commitments-sampling.md
7541930 
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
@kevaundray
chore: linter
cd17ff5
@kevaundray
chore: asn (switch to bls_modular_inverse)
83c1d20
@kevaundray
chore: (ben) rename func to test_construct_vanishing_polynomial
f049ea9
@kevaundray
chore: (ben) rename extended_evaluations_coeffs to `extended_evalua…
44031e0 
…tion_times_zero_coeffs`
@kevaundray
chore: compute roots_of_unity_extended in recover_data method
af3520a
@kevaundray
chore: add more comments explaining whats happening in recover_data
daa7497
@kevaundray
chore: compute_zero_poly_coeff in recover_data
6144e3d
@kevaundray
chore: make lint
1e6d91f
@kevaundray
chore: add doc comment to coset_fft_field
1593ede
asn-d6
asn-d6 requested changes Jun 10, 2024
View reviewed changes
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.

Thanks for this PR, @kevaundray ! I agree this is a better way at looking at (and implementing) the protocol.

I added some comments about the special-case vanishing poly code added in this ticket, that I would like to be discussed before merging this.

specs/_features/eip7594/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
...pyspec/eth2spec/test/eip7594/unittests/polynomial_commitments/test_polynomial_commitments.py Show resolved Hide resolved
specs/_features/eip7594/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
specs/_features/eip7594/polynomial-commitments-sampling.md Outdated Show resolved Hide resolved
@kevaundray kevaundray requested a review from asn-d6 June 11, 2024 10:38
jtraglia
jtraglia approved these changes Jun 11, 2024
View reviewed changes
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.

LGTM! Thanks for making those changes 😄

asn-d6
asn-d6 approved these changes Jun 11, 2024
View reviewed changes
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.

LGTM!

@asn-d6 asn-d6 merged commit 6efab19 into ethereum:dev Jun 11, 2024
26 checks passed
@hwwhww hwwhww mentioned this pull request Jun 14, 2024
Merged
This was referenced Jun 19, 2024
Closed
Closed
@asn-d6 asn-d6 mentioned this pull request Jul 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b-wagn b-wagn b-wagn left review comments

@jtraglia jtraglia jtraglia approved these changes

@asn-d6 asn-d6 asn-d6 approved these changes

Assignees
No one assigned
Labels
EIP-7594 PeerDAS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kevaundray @asn-d6 @jtraglia @b-wagn @hwwhww