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

conditionally_select and conditional_enforce_equal use condition inconsistently #41

Open
ValarDragon opened this issue Jan 15, 2021 · 3 comments
Labels
D-easy Difficulty: easy T-refactor Type: cleanup/refactor

Comments

@ValarDragon
Copy link
Member

ValarDragon commented Jan 15, 2021

Summary of Bug

CondSelectGadget's API is

    fn conditionally_select(
        cond: &Boolean<F>,
        true_val: &Self,
        false_val: &Self,
    )

whereas EqGadget's API is

    fn conditional_enforce_equal(
        &self,
        other: &Self,
        should_enforce: &Boolean<F>,
    ) -> Result<(), SynthesisError> {

I suggest we change EqGadget's conditional methods to be consistent with CondSelectGadget, and have the condition be the first parameter.

@Pratyush
Copy link
Member

Hmm I think the APIs are slightly different, in that in some sense for CondSelect, the boolean is really an active participant in the computation, whereas in EqGadget it's kinda different; the majority of the work involves the self and other objects, with the condition being kinda passive.

Of course, this is quite subjective, but IMO one can see the difference in the select method on Boolean: cond.select(&true_val, &false_val) makes sense, whereas cond.conditional_enforce_equal(&self, &other) makes (IMO) somewhat less sense.

@ValarDragon
Copy link
Member Author

I think conditional_enforce_equal(&self, should_enforce, other) is good, since the code looks like
a.conditional_enforce_equal(condition, b). And then code also looks consistent if you have a Type::conditionally_select nearby as well.

Agreed on having bool.conditional_enforce_equal doesn't make sense though.

Another option is to hide conditionally_select and not expose it to constraint writers, but I'm not really a fan of that.

@Pratyush
Copy link
Member

Pratyush commented Jan 15, 2021

Personally I think there's no scenario in which it makes sense to use Type::conditionally_select(cond, true_val, false_val), given that cond.select(true_val, false_val) is much cleaner.

Another option is to remove conditionally_enforce_equal all together, because I originally introduced it for use in larger primitives where you want to conditionally enforce verification, such as Groth16::conditionally_verify(&vk, &proof), but since then we have better APIs that just return the result of verification, and enforce that it's equal to the condition.

EDIT: from a quick grep across all the arkworks crates, the only places that conditional_enforce_equal is used is in implementations of conditional_enforce_equal lol

Pratyush pushed a commit that referenced this issue Aug 8, 2021
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
@Pratyush Pratyush added D-easy Difficulty: easy T-refactor Type: cleanup/refactor labels Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-easy Difficulty: easy T-refactor Type: cleanup/refactor
Projects
None yet
Development

No branches or pull requests

2 participants