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

IPA Solidity unit-test generator #341

Merged
merged 4 commits into from
Mar 4, 2024
Merged

IPA Solidity unit-test generator #341

merged 4 commits into from
Mar 4, 2024

Conversation

storojs72
Copy link
Contributor

This PR is a first step to solve verifier's Rust <-> Solidity compatibility problem (#337)

With a help of Handlebars templating engine this PR adds IPA unit-test which dynamically generates exact ipa.t.sol from our Solidity verifier (checked manually by running forge tests in Solidity with Solidity file generated by Rust).

How to run?

In order to create ipa.t.sol directly from Rust (Arecibo) environment I used following commands:

cargo test test_solidity_compatibility_ipa --release -- --ignored --nocapture > ipa.t.sol
cat ipa.t.sol | sed '1,3d' | sed -n -e :a -e '1,4!{P;N;D;};N;ba' > tmp.file && mv tmp.file ipa.t.sol

The ignored test test_solidity_compatibility_ipa is invoked and its stdout is redirected to the ipa.t.sol file with a little subsequent truncations of irrelevant lines.

Sidenote

It is possible to vary number of variables and test_solidity_compatibility_ipa can produce correspondent IPA test-vector which is useful for detecting upper-bound gas cost experiments.

P.S. As it is mentioned in #337, IPA now is a first building block that supports dynamic IO generation directly from Rust, and we can experiment with CI enforcements using it as an example in order to derive optimal compatibility rules for every stakeholder.
cc @samuelburnham

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

It would be great to add this to the code base, but the current setup is quite invasive to the existing source files.

What about the following organization instead?

src/
└── provider/
    ├── mod.rs                # The main module file for `provider`
    └── tests/
        ├── mod.rs            # The main module file for test utilities within `provider`
        └── ipa_pc.rs         # Test file for IPA polynomial commitment solidity tests within `provider`

This isn't meant to replace the current test organization (which is often useful because it can access private fields), but is a complement to the existing one.

@@ -33,7 +33,7 @@ where
// this is a hack; we just assume the size of the element.
// Look for the static assertions in provider macros for a justification
#[abomonate_with(Vec<[u64; 8]>)]
ck: Vec<<E::GE as PrimeCurve>::Affine>,
pub(crate) ck: Vec<<E::GE as PrimeCurve>::Affine>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think pub (in crate::provider) would suffice.

Cargo.toml Outdated
Comment on lines 50 to 51
handlebars = "5.1.0"
serde_json = "1.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a dev-dependency instead?

@storojs72 storojs72 requested a review from adr1anh February 22, 2024 19:22
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Back to your queue for the file hierarchy changes requested in #341 (review)

@storojs72
Copy link
Contributor Author

storojs72 commented Feb 26, 2024

Back to your queue for the file hierarchy changes requested in #341 (review)

In this case, we will need to make even more fields public (ck_v, ck_s of VerifierKey and L_vec, R_vec, a_hat of InnerProductArgument) in order to access them from different module. Is that acceptable?

@huitseeker
Copy link
Contributor

In this case, we will need to make even more fields public (ck_v, ck_s of VerifierKey and L_vec, R_vec, a_hat of InnerProductArgument) in order to access them from different module. Is that acceptable?

Rust supports pub (in crate::provider), which is fine, and pub (crate) if you must.

Copy link
Contributor

@tchataigner tchataigner left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

🙏

@storojs72 storojs72 added this pull request to the merge queue Mar 4, 2024
Merged via the queue into dev with commit 59caba3 Mar 4, 2024
9 checks passed
@storojs72 storojs72 deleted the ipa_templating branch March 4, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants