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

Port C++ merkle tests to Rust #243

Merged
merged 33 commits into from Aug 24, 2022
Merged

Port C++ merkle tests to Rust #243

merged 33 commits into from Aug 24, 2022

Conversation

tzerrell
Copy link
Member

These tests are arguably less elegant, and definitely more complex, in Rust. The core reason for this is that the C++ tests check specific lines for exceptions which they can catch and continue from. In the Rust tests, we need to handle panics from assertions failing, which happens at the level of the full test, not a specific line. While Rust does have std::panic::catch_unwind, the specific places we would need to use it do not appear to be unwind safe.

I also want to call attention to the change making verify::merkle a public module. This is necessary to get access to ReadIOP during the tests in prove::merkle.

@tzerrell tzerrell self-assigned this Aug 18, 2022
@tzerrell tzerrell marked this pull request as draft August 18, 2022 23:25
@tzerrell
Copy link
Member Author

Converting to draft while I fix merge errors

@tzerrell
Copy link
Member Author

Added a basic circuit (fibonacci) for testing purposes to ZKP to allow the instantiation of a HAL. I gated it behind the prove feature flag since it's used for the merkle tests in the prover module, but it could get its own feature flag if desired.

@tzerrell tzerrell marked this pull request as ready for review August 19, 2022 21:58
Comment on lines 28 to 29
#[cfg(feature = "prove")]
mod test_circuit;
Copy link
Member

Choose a reason for hiding this comment

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

One idea is to have a separate risc0-circuit-fib crate that is a dev-dependency of this crate. That way, in production, users don't need to have the fib circuit making libraries larger than necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I can't figure out how to make this work from a dependency graph perspective. Specifically, the test_circuit module from this PR depends on the adapter and taps modules of the risc0-zkp crate, and the prover module of the risc0-zkp crate depends on this test_circuit module (for tests). This works fine with test_circuit as a module of risc0-zkp, but if I were to pull it out into a separate risc0-circuit-fib crate, it seems like it would make risc0-zkp have a dev dependency of risc0-circuit-fib which would in turn have a dependency on risc0-zkp -- i.e. a circular dependency.

Copy link
Member

Choose a reason for hiding this comment

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

dev-dependencies are treated differently. You can think of it like:

  • risc0-zkp (dev-dep)
  • risc0-circuit-fib
  • risc0-zkp

Copy link
Member

Choose a reason for hiding this comment

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

IOW, a dev-dep is a different node in the graph. The test cfg of a crate is considered a different node in the dependency graph compared to its 'parent'.

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting, thank you! Will try to get this working then

}

#[test]
#[should_panic(expected = "assertion failed: idx < self.params.row_size")]
Copy link
Member

Choose a reason for hiding this comment

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

This is probably fine for now but I think in the future we should avoid using panics and instead use Result to do error reporting. Panic won't work well in some environments since a panic causes the entire thread to be terminated.

Copy link
Member Author

@tzerrell tzerrell Aug 22, 2022

Choose a reason for hiding this comment

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

These tests would be cleaner if this was Result-based error reporting than panics anyway. I'm tracking down an intermittent failure in this test the randomized version of this test anyway -- perhaps the most straightforward approach will be to rewrite the tested code to use Results.

Copy link
Member Author

Choose a reason for hiding this comment

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

fyi I switched to Result-based errors on the verifier side, but not here on the prover side. We should definitely do it eventually, but there is more to be done on the prover side and IMO it's less important on the prover side.

@flaub
Copy link
Member

flaub commented Aug 20, 2022

Interestingly, I also need to use the fib circuit in other repositories, so making it a separate (published) crate would actually be useful there.

@tzerrell
Copy link
Member Author

I saw an intermittent error on one of the randomized tests, marking as draft until I've investigated.

@tzerrell tzerrell marked this pull request as draft August 22, 2022 17:33
@tzerrell
Copy link
Member Author

Putting up my WIP on splitting out a risc0-circuit-fib crate -- if anyone has any ideas about why it can't find the PolyFp trait implementation for CircuitImpl I'm eager to hear them.

@tzerrell tzerrell marked this pull request as ready for review August 24, 2022 03:36
@tzerrell
Copy link
Member Author

I removed the fibonacci circuit to match the current HAL API. If we need it back for other purposes we can make a followup PR.

@tzerrell tzerrell merged commit 62db8c8 into main Aug 24, 2022
@tzerrell tzerrell deleted the tzerrell-port-merkle-tests branch August 24, 2022 21:48
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.

None yet

2 participants