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

Introduce halo2_proofs::circuit::Value #598

Merged
merged 6 commits into from
Jun 13, 2022
Merged

Introduce halo2_proofs::circuit::Value #598

merged 6 commits into from
Jun 13, 2022

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Jun 8, 2022

This replaces every usage of Option<V> for representing witnessed values that may not have a known value (e.g. advice cells during keygen).

This is a very big breaking change to the public API, but does not alter circuit compatibility.

Closes #509.

@str4d str4d added the A-rust-api Area: Public crate API label Jun 8, 2022
@str4d str4d force-pushed the circuit-value-type branch from 14c4d78 to b335623 Compare June 8, 2022 21:21
This is a more usable and type-safe replacement for `Option<V>` in
circuit synthesis.
@str4d str4d force-pushed the circuit-value-type branch 2 times, most recently from 29502a5 to a43b3df Compare June 8, 2022 22:45
@str4d str4d force-pushed the circuit-value-type branch from a43b3df to 5ed3d25 Compare June 8, 2022 23:31
@str4d
Copy link
Contributor Author

str4d commented Jun 8, 2022

Force-pushed to fill in the changelogs. The breaking changes to the public API aren't quite as bad as I thought! Still large, but enumerable.

@str4d
Copy link
Contributor Author

str4d commented Jun 9, 2022

zcash/orchard#336 has the migration of orchard to the new APIs.

Copy link
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Looks really good!! Just have some questions regarding the ops traits impls for Value.

Feels much better now avoiding a lot of unwraps and ok_or(Error::Synthesis). The readability will be improved quite a lot also IMO.
Thanks a lot for working on this @str4d 🥳

halo2_proofs/src/circuit/value.rs Outdated Show resolved Hide resolved
halo2_proofs/src/circuit/value.rs Outdated Show resolved Hide resolved
halo2_proofs/src/circuit/value.rs Show resolved Hide resolved
halo2_proofs/src/circuit/value.rs Show resolved Hide resolved
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK

Consider shortening uses of Value::known by importing it and using known(...).

@str4d str4d deleted the branch main June 9, 2022 23:48
@str4d str4d closed this Jun 9, 2022
@str4d str4d reopened this Jun 9, 2022
@str4d str4d changed the base branch from small-perf-improvements to main June 9, 2022 23:49
@str4d
Copy link
Contributor Author

str4d commented Jun 10, 2022

Consider shortening uses of Value::known by importing it and using known(...).

This doesn't actually work; Rust lets you import enum cases, but not struct methods. We'd need a standalone method instead, which would then be weird as it would lack context (halo2_proofs::circuit::known).

Copy link
Collaborator

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

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

utACK

halo2_proofs/src/circuit/value.rs Outdated Show resolved Hide resolved
Co-authored-by: ying tong <yingtong@z.cash>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust-api Area: Public crate API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error handling for AssignedCell value queries
4 participants