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

Implements kzg tests #364

Merged
merged 27 commits into from
Apr 16, 2024
Merged

Implements kzg tests #364

merged 27 commits into from
Apr 16, 2024

Conversation

EchoAlice
Copy link
Contributor

Begins to fix #352

@EchoAlice EchoAlice changed the title Create boiler plate for kzg runner Creates kzg tests Mar 28, 2024
@EchoAlice EchoAlice changed the title Creates kzg tests Implements kzg tests Mar 28, 2024
Copy link
Owner

@ralexstokes ralexstokes 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! I'd follow the structure of the bls tests for how to move forward

Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

amazing! left some comments

rustc-ice-2024-03-29T15_01_52-72620.txt Outdated Show resolved Hide resolved
spec-tests/test_meta.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved

match (input_result, output_result) {
(Ok(blob), Ok(Some(expected))) => {
let kzg_commitment = blob_to_kzg_commitment(&blob, &kzg_settings).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

p sure we can just do this to simplify the code a bit

Suggested change
let kzg_commitment = blob_to_kzg_commitment(&blob, &kzg_settings).unwrap();
let kzg_commitment = blob_to_kzg_commitment(&blob, &kzg_settings)?;

Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

updates look good!

I would consider at this point moving each block of code in each handler to its own function so this doesn't become one huge function inside dispatch

so it would look something like

match meta.handler.0.as_str() {
  "blob_to_kzg_committment" => run_blob_to_kzg_committment_test(test, &kzg_settings),
  "compute_kzg_proof" => run_compute_kzg_proof_test(test, &kzg_settings),
   // etc...

where each "run_*_test" fn would be something like

fn run_*_test(test: &TestCase, kzg_settings: &KzgSettings) -> Result<(), Error> {
  // specific logic in here
}

@EchoAlice EchoAlice force-pushed the kzg-tests branch 2 times, most recently from 118293d to 46d7a6c Compare April 9, 2024 21:16
Copy link
Owner

@ralexstokes ralexstokes 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! left a few comments from a quick skim, getting there!!

ethereum-consensus/src/deneb/polynomial_commitments.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Show resolved Hide resolved
Copy link
Owner

@ralexstokes ralexstokes 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! we did compute some values an additional time when we don't need to

spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved
spec-tests/runners/kzg.rs Outdated Show resolved Hide resolved
Remove duplicate evaluations
@ralexstokes ralexstokes merged commit 88881d3 into ralexstokes:main Apr 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement KZG spec tests
2 participants