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.

Sign up for GitHub

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

Jump to bottom

Make closure of challenge phase FnOnce. #276

Merged
merged 1 commit into from
May 26, 2020
Merged

Make closure of challenge phase FnOnce. #276

merged 1 commit into from
May 26, 2020

Conversation

rubdos
Copy link
Contributor

@rubdos rubdos commented May 28, 2019 •
edited
Loading

It allows for the challenge phase to accept closures that take some more
context, e.g. moving non-Clone values in the challenge closure.

This works in stable since Rust 1.35; cfr.
rust-lang/rust#28796.

This closes issue #244.

In other news: I'm not yet happy with the 'static bound on the closures. Since I'm not yet worried about that kind of performance (just hacking around currently), I'm not going to fight the borrow checker there, yet. Cfr #323

@rubdos
Copy link
Contributor Author

rubdos commented May 28, 2019 •
edited
Loading

Seems like the CI fails on the Rust version (which should be 1.35 for this to work).

@rubdos rubdos force-pushed the 244 branch 2 times, most recently from 25cfd96 to 79a88ec Compare May 29, 2019 10:44
@rubdos
Copy link
Contributor Author

rubdos commented May 29, 2019

The new build failure is because of the bunch of recent deprecations in merlin.

@hdevalence
Copy link
Contributor

Hi, those deprecations were fixed in the main branch of this crate but those changes weren't merged into the develop branch. I can update the develop branch so that this PR can work again.

@rubdos
Copy link
Contributor Author

rubdos commented May 31, 2019

Is develop still being used then? Otherwise I'll rebase this on main.

@hdevalence
Copy link
Contributor

Yep, it's still being used, it's just that those changes were included as part of a PR that added additional validation to the rangeproof code, so they were put into the main branch. The develop branch is still for ongoing work on the R1CS implementation, which slowed a bit but will probably pick up again soon.

@rubdos
Copy link
Contributor Author

rubdos commented Jun 2, 2019

Ok, fair! So, would you mind porting those deprecation fixes to develop then, then I'll rebase.

@hdevalence
Copy link
Contributor

Currently in-progress with #277!

@rubdos
Copy link
Contributor Author

rubdos commented May 14, 2020

I've rebased this over develop. I'm looking into the 'static lifetime too now, so hold your horses for a few minutes :-)

@rubdos
Copy link
Contributor Author

rubdos commented May 14, 2020 •
edited
Loading

Looks like I managed to shorten the lifetime. I added a test that would fail before and succeed after the patch. It does however mean that all Provers and Verifiers, and RandomizableConstraintSystem all get some extra lifetimes (which technically would mean breaking the API). Since it's behind a yolo-feature, I assume that's not a big issue.

EDIT: feel free to cherry-pick whichever of the patches you like already!

@oleganza
Copy link
Collaborator

Does non-static lifetime work well with multiple calls to specify_randomized_constraints ? I'll try this out with zkvm and spacesuit next week.

@oleganza
Copy link
Collaborator

That's an exciting update, thanks!

@rubdos
Copy link
Contributor Author

rubdos commented May 15, 2020

Does non-static lifetime work well with multiple calls to specify_randomized_constraints ? I'll try this out with zkvm and spacesuit next week.

It should! I currently have no use for it myself, but I may soon have. If it doesn't work out, feel free to add a test case and I'll take a look.

rubdos
rubdos commented May 15, 2020
View reviewed changes
Copy link
Contributor Author

@rubdos rubdos left a comment

Choose a reason for hiding this comment

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

Some comments wrt. to the new lifetimes; I'd like some second opinions!

src/r1cs/constraint_system.rs Outdated Show resolved Hide resolved
tests/r1cs.rs Outdated Show resolved Hide resolved
src/r1cs/constraint_system.rs Outdated Show resolved Hide resolved
@rubdos rubdos mentioned this pull request May 20, 2020
Open
@rubdos
Copy link
Contributor Author

rubdos commented May 20, 2020

I dropped the non-'static stuff from this PR and created #323 in favour of that. I'll rebase this on develop now.

@rubdos
Make closure of challenge phase FnOnce.
977f987 
It allows for the challenge phase to accept closures that take some more
context, e.g. moving non-Clone values in the challenge closure.

This works in stable since Rust 1.35; cfr.
rust-lang/rust#28796.

This closes issue dalek-cryptography#244.
@oleganza oleganza merged commit 10a9d81 into dalek-cryptography:develop May 26, 2020
@rubdos rubdos deleted the 244 branch May 26, 2020 10:20
@rubdos rubdos mentioned this pull request May 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleganza oleganza oleganza left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rubdos @hdevalence @oleganza