r1cs-std icon indicating copy to clipboard operation
r1cs-std copied to clipboard

conditionally_select and conditional_enforce_equal use condition inconsistently

Open ValarDragon opened this issue 4 years ago • 3 comments

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.

ValarDragon avatar Jan 15 '21 13:01 ValarDragon

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.

Pratyush avatar Jan 15 '21 18:01 Pratyush

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.

ValarDragon avatar Jan 15 '21 18:01 ValarDragon

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 avatar Jan 15 '21 18:01 Pratyush