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

feat: Forward ports to testnest v1.0.7 #99

Merged
merged 10 commits into from
Aug 1, 2024
Merged

feat: Forward ports to testnest v1.0.7 #99

merged 10 commits into from
Aug 1, 2024

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Jul 24, 2024

This ports us to parity with SP1 testnest v1.0.7

One leftover item to note (for @samuelburnham and I to take on): upstream tests now run on docker by default (see recursion/gnark-ffi/src/docker.rs). The default-ness of this behaviour, as well as adapting the pointers to Succinct-hosted docker images, have not been performed in this PR. See #100 for the follow-up task.

@huitseeker huitseeker force-pushed the forward_ports_40 branch 4 times, most recently from dcf3a89 to ba77a09 Compare July 25, 2024 18:11
@huitseeker huitseeker marked this pull request as ready for review July 25, 2024 18:11
@huitseeker huitseeker force-pushed the forward_ports_40 branch 2 times, most recently from b51cfd6 to c915c38 Compare July 25, 2024 21:28
Copy link
Contributor

@wwared wwared left a comment

Choose a reason for hiding this comment

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

A couple of nits below.

There will most likely need to be additional changes related to #38 (010f2af) in recursion/program/src/stark.rs and prover/src/verify.rs, specifically the checks I commented out/changed slightly (I think this PR reintroduces the checks and they're still broken and need to be removed), but I'll take care of those. I'm currently generating a couple plonk proofs with >1 shard now to check the recursive constraints and will report back after committing the necessary changes

We'll also need a companion PR updating sphinx-contracts and update the s3 bucket artifacts, I can look into that done once this is ready to merge

examples/Cargo.toml Outdated Show resolved Hide resolved
prover/Makefile Outdated Show resolved Hide resolved
prover/Makefile Outdated Show resolved Hide resolved
prover/proof-with-pis.json Outdated Show resolved Hide resolved
Copy link
Contributor

@wwared wwared left a comment

Choose a reason for hiding this comment

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

Looks like all of the changes from 010f2af were already integrated, so I just removed the commented out blocks of code and TODOs, as well as removing the now-unnecessary builder.assert_var_ne(index, C::N::from_canonical_usize(EMPTY)); constraint that was added

I should be able to upload the circuit artifacts and look into the sphinx-contracts PR later, but this PR is good to merge right now

Thanks a lot for all the hard work with the ports!

@huitseeker
Copy link
Contributor Author

Thanks a lot for the review and fixes @wwared !

@huitseeker huitseeker merged commit b2ce341 into dev Aug 1, 2024
7 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.

3 participants