Rework ConstraintSystem API to improve ergonomics

[Pratyush]

Currently the API for writing constraints in r1cs-core is very generic: ConstraintSystem is a trait that downstream users can implement and customize, and it must be threaded throughout the gadget interfaces in r1cs-std and beyond. It also requires users to explicitly set namespaces, which, together with the previous point, makes constraint-generating code very verbose, and often obscures (relatively simple) logic.

In this (WIP) PR I'm trying out a different and hopefully more ergonomic approach. The high-level diff is as follows:
The ConstraintSystem trait is replaced with a single ConstraintSystem struct that is meant to be used for all purposes: generating constraints in a SNARK setup, generating witnesses during proving, debugging, etc. This leads to the following changes:

• We can store a (wrapper around) Rc<RefCell<ConstraintSystem<F>>> in gadgets that represent variables, such as FieldGadget. This means that we no longer have to pass &mut CS to every function that could possibly generate a constraint, and furthermore means that we can overload arithmetic operators for these gadgets.
• By default constraints are automatically given unique names, so that we don't have to call cs.ns(|| "blah") whenever we need to create a constraint. Note that if one wants to explicitly create a namespace, one can still invoke cs.enter_namespace() and then cs.leave_namespace().
• To generate a LinearCombination, users don't explicitly generate one themselves. Instead, they register the linear combination with a ConstraintSystem by doing cs.new_lc(my_lc), and get back a Variable::SymbolicLc in return.
• This latter design of ConstraintSystem enables a centralized implementation of (optional) optimizations such as
  • swapping the A and B matrices (useful for Groth16),
  • balancing the number of non-zero entries in A and B, (useful for Marlin and Fractal) and
  • "outlining" common linear combinations to reduce the number of non-zero entries in a constraint matrix (useful for Marlin and Fractal)

One concern is the debuggability of constraints generated under this new API because namespacing is automatic. Suggestions would be appreciated. Maybe compiler techniques would be a good starting point here.

Looking for feedback on the initial design. Note that the scope of the PR is massive, because we need to re-architect the r1cs-std, crypto-primitives and dpc crates =/.

CC @kobigurk @gakonst @paberr @ValarDragon

TODOs:

• Check equality of ConstraintSystemRefs when two different gadgets interact, and figure out the appropriate mechanism for checking equality (Rework ConstraintSystem API to improve ergonomics  #186 (comment))
• Improve debuggability: approach 1 vs inline a stacktrace or some such?

[kobigurk]

I love the direction, it's going to be great not having to namespace - and potentially using, e.g. AddAssign and MulAssign, traits!

Debuggability is an issue - I've been using that heavily when things fail. Maybe each gadget can define a namespace format as part of its type?

[burdges]

If gadgets interact, which sounds likely with ops traits, then you might want impl PartialEq for ConstraintSystemRef that compares .as_ptr() results, but maybe you're already doing this manually somewhere.

[Pratyush]

Debuggability is an issue - I've been using that heavily when things fail. Maybe each gadget can define a namespace format as part of its type?

One approach to solve this could be to have an environment variable (maybe R1CS_DEBUG) which, upon constraint_generation, and when running in "prover_mode", checks that constraints are satisfied as they are generated, so that you get an immediate error. This could also be controlled by a runtime variable that is false by default, but which users can set to be true when they want to debug.

[Pratyush]

If gadgets interact, which sounds likely with ops traits, then you might want impl PartialEq for ConstraintSystemRef that compares .as_ptr() results, but maybe you're already doing this manually somewhere.

Hmm good point. Is comparing via as_ptr() different from comparing via a derived PartialEq impl?

[kobigurk]

Yeah, that seems nice. Another thing I commonly use is extracting a variable to check for expected value, which I think this wouldn't solve, right?

…

On Sun, Apr 12, 2020, 23:56 Pratyush Mishra ***@***.***> wrote: Debuggability is an issue - I've been using that heavily when things fail. Maybe each gadget can define a namespace format as part of its type? One approach to solve this could be to have an environment variable (maybe R1CS_DEBUG) which, upon constraint_generation, and when running in "prover_mode", checks that constraints are satisfied as they are generated, so that you get an immediate error. This could also be controlled by a runtime variable that is false by default, but which users can set to be true when they want to debug. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#186 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA23MGFM6RJ5BMJVECYNVALRMITIPANCNFSM4MGRNWEQ> .

[burdges]

You want pointer comparisons here:

• A structural/derived equality could succeed incorrectly only if you've two identical ConstraintSystems, so maybe if doing multiple circuits together with an identical prefix using Make Groth16 efficient when ZK isn't needed #138
• I think pointer comparisons cannot succeed incorrectly because your ConstraintSystemRef still holds its RefCell, and they should run much faster too.

[Pratyush]

Yeah, that seems nice. Another thing I commonly use is extracting a variable to check for expected value, which I think this wouldn't solve, right?

It could be augmented to print out the value of the assignment during the assignment process, but that would only print out the value of the lowest-level assignments.

I think a better solution is as follows. Create macros new_witness, new_instance, new_lc, and enforce_constraint. In these macros, if R1CS_DEBUG=true, then we augment the namespace with file, module, and line number before invoking the corresponding ConstraintSystem methods.

Furthermore, we can also create a macro that works with the (new version of the) AllocGadget trait to do the same for higher-level variables.

[kobigurk]

k

Yeah, that seems nice. Another thing I commonly use is extracting a variable to check for expected value, which I think this wouldn't solve, right?

It could be augmented to print out the value of the assignment during the assignment process, but that would only print out the value of the lowest-level assignments.

I think a better solution is as follows. Create macros new_witness, new_instance, new_lc, and enforce_constraint. In these macros, if R1CS_DEBUG=true, then we augment the namespace with file, module, and line number before invoking the corresponding ConstraintSystem methods.

Furthermore, we can also create a macro that works with the (new version of the) AllocGadget trait to do the same for higher-level variables.

Love this, this should be enough and even better than the current situation. How about making it an option of TestConstraintSystem?

[paberr]

I like the idea of being able to get rid of the &mut cs and being able to use the traits like Add etc.
This could greatly improve the readability of code in circuits and gadgets. 👍

I think a better solution is as follows. Create macros new_witness, new_instance, new_lc, and enforce_constraint. In these macros, if R1CS_DEBUG=true, then we augment the namespace with file, module, and line number before invoking the corresponding ConstraintSystem methods.

Furthermore, we can also create a macro that works with the (new version of the) AllocGadget trait to do the same for higher-level variables.

I think this is a great idea and I agree with @kobigurk that this could even be better than the current situation.

[burdges]

Your impl Deref for ConstraintSystemRef never panics and is unsound.

Are all those pub member correct? Iffy..

I'd favor Cow<'static,str> over String when never mutated, except only test mode uses names. And current_namespace_path appears redundant.

I suspect prover time would take some small performance hit here, and prover code size would increase, due to namespace handling. These enter_namespace and leave_namespace methods exists, and take closure arguments, so that child namespaces inherit their root's namespace handling, which ignores namespaces outside tests. It's only r1cs_std::TestConstraintSystem that handles namespaces though, while we ignore name spaces in the four "real" root ConstraintSystems all gm17/groth16::ProvingAssignment/KeypairAssembly.

We might not care about prover time or code size here, but you'd suck all that complexity from four types across three distinct crates into r1cs_core. It's even worse if distributed trusted setup procedures might prefer variants on the KeypairAssemblys.

I'd think ConstraintSystem must remain a trait for these last two reasons.

[burdges]

As I understand it, you want gadgets that (a) interact with multiple variable etc, but whose (2) arguments consist solely of variables, without passing any separate &mut CS, so..

All variables must capture roughly a ConstraintSystemRef, aka Rc<RefCell<CS>> with CS: ConstraintSystem, which includes their namespace in test mode. It follows that all gadgets now have exactly as many namespaces as variable arguments. Yes? Also, we know variables outlive namespace declarations, so namespaces cannot be represented as a stack anymore.

We've two design choices here, either

1. all variables have some CS: ConstraintSystem type parameter so that variables contain Rc<RefCell<CS>>, or else
2. variables contain the trait object Rc<RefCell<dyn ConstraintSystem>> and we make ConstraintSystem object safe.

If Variable becomes Variable<CS: ConstraintSystem> then ideally F might become an associated type of ConstraintSystem, not a type parameter.

If we do dyn ConstraintSystem then we must eliminate all associated types and type parameters, meaning &dyn FnOnce() -> String/Result<F, SynthesisError> everywhere. It'll harm performance with allocations, and increase code size, but again maybe acceptable.

In either case, our namespace method ns must access this Rc<..> because TestConstraintSystem should spawn new Rc<..>s, which internally hold the root Rc<..>, while all real CS: ConstraintSystem simply propagate their one root Rc<..>.

We cannot however place an Option<Weak<RefCell<dyn ConstraintSystem>>> into each CS: ConstraintSystem due to memory leaks. We'd thus need some struct OwnRc<T>(NonNull<RcBox<T>>) that we transmute into a Weak<..>. Ugh!

In this case, I suppose the dyn ConstraintSystem<F> version resembles:

type Name = Cow<'static,str>;

pub trait ConstraintSystem<F: Field> {
    /// Return the "one" input variable
    fn one(&self) -> Variable;  
    // We now need &self and cannot provide a default body

    /// Allocate a private variable in the constraint system. The provided
    /// function is used to determine the assignment of the variable. The
    /// given `annotation` function is invoked in testing contexts in order
    /// to derive a unique name for this variable in the current namespace.
    fn alloc(
        &mut self, 
        annotation: &dyn FnOnce() -> Name,
        f: &dyn FnOnce() -> Result<F, SynthesisError>,
    ) -> Result<Variable, SynthesisError>;

    /// Allocate a public variable in the constraint system. The provided
    /// function is used to determine the assignment of the variable.
    fn alloc_input(
        &mut self, 
        annotation: &dyn FnOnce() -> Name,
        f: &dyn FnOnce() -> Result<F, SynthesisError>,
    ) -> Result<Variable, SynthesisError>;

    /// Enforce that `A` * `B` = `C`. The `annotation` function is invoked in
    /// testing contexts in order to derive a unique name for the constraint
    /// in the current namespace.
    fn enforce(
        &mut self,
        annotation: &dyn FnOnce() -> Name,
        a: &dyn FnOnce(LinearCombination<F>) -> LinearCombination<F>,
        b: &dyn FnOnce(LinearCombination<F>) -> LinearCombination<F>,
        c: &dyn FnOnce(LinearCombination<F>) -> LinearCombination<F>
    );

    /// Gets the "root" constraint system, bypassing the namespacing.
    /// Not intended for downstream use; use `namespace` instead.
    fn get_root(&mut self) -> ConstraintSystemRef<F: Field>;

    /// Begin a namespace for this constraint system.
    fn ns<'a, NR, N>(&'a mut self, name_fn: &dyn FnOnce() -> Name)
     -> ConstraintSystemRef<F: Field>;

    /// Output the number of constraints in the system.
    fn num_constraints(&self) -> usize;
}

In the #![feature(arbitrary_self_types)] discussion, there is interest in some MethodReciever trait that'd make self: Rc<Refcell<Self>> recievers object safe. At which point we've more convoluted schemes but that avoid unsafety.

We can simplify Variable<CS: ConstraintSystem> versions using #![feature(arbitrary_self_types)] too:

pub trait ConstraintSystem: Sized {
    type F: Field;

    /// Represents the type of the "root" of this constraint system
    /// so that nested namespaces can minimize indirection.
    type Root: ConstraintSystem<F = Self::F>;

    /// Return the "one" input variable, not usable directly, call `ConstraintSystemRef::one` instead.
    fn one(self: Rc<RefCell<Self>>) -> Variable<Self> { .. }

    .. alloc, alloc_input, enforce, get_root, and num_constraints unchanged ..

    /// Begin a namespace for this constraint system.
    fn ns<'a, NR, N>(self: Rc<RefCell<Self>>, name_fn: N) -> Namespace<'a, F, Self::Root>
    where
        NR: Into<String>,
        N: FnOnce() -> NR;
}

As a rule, trait objects are second class citizens in Rust, so you're almost surely better off avoiding them even if it requires adding where clauses about variables everywhere.

[burdges]

I'd think the simplest approach goes:

First, all CS: ConstraintSystem provide their own interior mutability, meaning all real ones look like pub struct CS { inner: Rc<RefCell<MyInnerCS>> }. And ConstraintSystemRef disappears.

Also Namespace disappears into r1cs_std::TestConstraintSystem, but our current namespace lives outside the interior mutability, so

pub struct TestConstraintSystem {
    current_namespace: Vec<String>,
    inner: Rc<RefCell<InnerTestConstraintSystem>>,
}

where InnerTestConstraintSystem resembles the current TestConstraintSystem minus current_namespace.

Second, your variables become pub struct Variable<CS: ConstraintSystem>(CS,Index);.

You'd still like &'a CS : ConstraintSystem so that Variables avoid always reference counting. It's doable, but complicates bounds when using Variables, so not sure. If so, we'd keep ConstraintSystem::Root for use in such bounds, ala

impl<'a, CS> ConstraintSystem for &'a CS where CS: ConstraintSystem {
    type Root = CS::Root;
    type F = CS::F;
    ...
}

impl<CS1,CS2> ::core::ops::AddAssign<Foo<CS2>> for Foo<CS1> 
where
    CS1: ConstraintSystem,
    CS2: ConstraintSystem,
    CS1::Root = CS2::Root,

[Pratyush]

@burdges (Man I wish GH had a threaded conversations feature)

Your impl Deref for ConstraintSystemRef never panics and is unsound.

Thanks for looking into this; maybe I'll just remove the Deref impl entirely.

Are all those pub member correct? Iffy..

You mean in ConstraintSystem or in ConstraintSystemRef?

I'd favor Cow<'static,str> over String when never mutated, except only test mode uses names. And current_namespace_path appears redundant.

I suspect prover time would take some small performance hit here, and prover code size would increase, due to namespace handling. These enter_namespace and leave_namespace methods exists, and take closure arguments, so that child namespaces inherit their root's namespace handling, which ignores namespaces outside tests. It's only r1cs_std::TestConstraintSystem that handles namespaces though, while we ignore name spaces in the four "real" root ConstraintSystems all gm17/groth16::ProvingAssignment/KeypairAssembly.

Maybe we can add a should_namespace: bool field in ConstraintSystem that decided whether to handle namespaces or not. Hopefully constant propagation can remove unused code branches in this case. It could also be a compile-time flag controlled by R1CS_DEBUG environment variable, in which case there would be no code to optimize out.

We might not care about prover time or code size here, but you'd suck all that complexity from four types across three distinct crates into r1cs_core. It's even worse if distributed trusted setup procedures might prefer variants on the KeypairAssemblys.

I'd think ConstraintSystem must remain a trait for these last two reasons.

From my experience with implementing these structs in gm17 and marlin, and from reviewing groth16, the ConstraintSystems in these structs are all essentially the same, with lots of duplicated logic (which leaves room for plenty of copy-paste errors). Also looking at distributed setup impl for Groth16 here, it seems that it directly uses the pre-existing struct from groth16. This indicates to me that a single centralized implementation wouldn't harm flexibility downstream. Also, avoiding a trait resolves the problems you encountered in later comments.

[burdges]

Are all those pub member correct? Iffy..
You mean in ConstraintSystem or in ConstraintSystemRef?

All the pub fields in ConstraintSystem. I suppose groth16, gm17, and marlin crates all manipulate them directly in different ways?

Maybe we can add a should_namespace: bool field in ConstraintSystem that decided whether to handle namespaces or not. Hopefully constant propagation can remove unused code branches in this case.

You could do either namespaces: Option<BTreeMap<String, NamedObject>> in ConstraintSystem or else add roughly Test { named_objects: BTreeMap<String, NamedObject> } in Mode probably. In this vein, your Mode could be expressed by two bools, yes?

    populate_variable: bool, 
    construct_matrices: bool

Alternatively, you could move instance_assignment and witness_assignment into Prove, yes?

It could also be a compile-time flag controlled by R1CS_DEBUG environment variable, in which case there would be no code to optimize out.

I'd sure this works but sounds worse for CI. It's also plausible some network would dynamically build constraint systems and do trusted setups, which makes this worse.

Also, you could keep current_namespace: Vec<String> inside ConstraintSystem of course. I still find enter_namespace and leave_namespace unnatural for Rust. You'll match RAII style better if current_namespace lives with referents not the ConstraintSystem type, so

pub struct ConstraintSystemRef<F: Field> {
    current_namespace: Vec<String>,
    inner: Rc<RefCell<ConstraintSystem<F>>>,
}
impl<F: Field> ConstraintSystemRef<F> {
    fn ns<NR, N>(&self, name_fn: N) -> ConstraintSystemRef<F>
    where
        NR: Into<String>,
        N: FnOnce() -> NR,
    {
        let mut current_namespace = Vec::new();
        if self.inner.borrow().namespaces.is_some() {
            current_namespace = self.current_namespace.clone();
            current_namespace.push(name_fn().into());
        }
        ConstraintSystemRef { current_namespace, inner: self.inner.clone() }
    }
}

Any gadget would invoke ns for all ConstraintSystemRefs passed in, which avoids touching current_namespace unless ConstraintSystem tracked namespaces of course.

From my experience with implementing these structs in gm17 and marlin, and from reviewing groth16, the ConstraintSystems in these structs are all essentially the same, with lots of duplicated logic (which leaves room for plenty of copy-paste errors).

I'd think that abstraction sounds worthwhile regardless :) but several options exist.

In #186 (comment) you've some InnerConstraintSystem type within which you could do that abstraction using smaller structs, traits, macros, etc.

Are there situations in which you build two related proofs concurrently? MPC in the Head? Could one do large RSA exponents with residue number systems run across several distinct curves over the same basefield?

Also looking at distributed setup impl for Groth16 here, it seems that it directly uses the pre-existing struct from groth16.

Cool

Also, avoiding a trait resolves the problems you encountered in later comments.

I agree the dyn ConstraintSystem sucked, but #186 (comment) should provide the desired interface fairly cleanly. It really depends upon the common ground between these different proving systems vs other lost opportunities.

[Pratyush]

Also, you could keep current_namespace: Vec<String> inside ConstraintSystem of course. I still find enter_namespace and leave_namespace unnatural for Rust. You'll match RAII style better if current_namespace lives with referents not the ConstraintSystem type, so

Ooh this is a very nice idea, to implement ns on ConstraintSystemRef directly! I avoided doing it on ConstraintSystem because then using the API from within gadgets would holding a mutable borrow for too long (which would cause panics from Rc), but this solution solves it perfectly!

[burdges]

One could add ConstraintSystemRef, and put the current namespace inside it, with ConstraintSystem being either a type or a trait. I think some trait version look similar:

pub struct ConstraintSystemRef<CS: ConstraintSystem> {
    current_namespace: Vec<String>,
    inner: Rc<RefCell<CS>>,
}
impl<CS: ConstraintSystem> ConstraintSystemRef<CS> {
    fn ns<NR, N>(&self, name_fn: N) -> ConstraintSystemRef<CS>
    where
        NR: Into<String>,
        N: FnOnce() -> NR,
    {
        let mut current_namespace = Vec::new();
        if CS::NAMESPACES {
            current_namespace = self.current_namespace.clone();
            current_namespace.push(name_fn().into());
        }
        ConstraintSystemRef { current_namespace, inner: self.inner.clone() }
    }
}

pub trait ConstraintSystem: Sized {
    const NAMESPACES : bool = false;
    type F: Field;
    ...
}

or

pub struct ConstraintSystemRef<CS: ConstraintSystem> {
    current_namespace: CurrentNamespace,
    inner: Rc<RefCell<CS>>,
}
impl<CS> ConstraintSystemRef<CS> 
where CS: ConstraintSystem<Name=()>
{
    fn ns<NR, N>(&self, _name_fn: N) -> ConstraintSystemRef<CS>
    where
        NR: Into<String>,
        N: FnOnce() -> NR,
    {
        ConstraintSystemRef { current_namespace: (), inner: self.inner.clone() }
    }
}
impl<CS> ConstraintSystemRef<CS> 
where CS: ConstraintSystem<Name=String>
{
    fn ns<NR, N>(&self, name_fn: N) -> ConstraintSystemRef<CS>
    where
        NR: Into<String>,
        N: FnOnce() -> NR,
    {
        let mut current_namespace = self.current_namespace.clone();
        current_namespace.extend(::std::iter::once(name_fn().into()));
        ConstraintSystemRef { current_namespace, inner: self.inner.clone() }
    }
}

pub trait ConstraintSystem: Sized {
    type F: Field;
    type Name = ();
    type CurrentNamespace: Clone+::std::iter::Extend<Name> = ()
    ...
}

I've some questions:

Why is F a type parameter of the current ConstraintSystem trait, not an associated type?

In this draft, ConstraintSystemRef only appears in AllocatedBit along side Variable. I suppose ConstraintSystemRef would always go alongside Variable, but maybe you've many Variabless for one ConstraintSystemRef, or handle with Variables with an implicit ConstraintSystemRef, which makes ConstraintSystemRef and Variable somewhat orthogonal? We expect gadgets with multiple Variables with distinct ConstraintSystemRefs though too, but presumably the gadget can figure out how it present such errors?

[burdges]

It's worth asking if this ConstraintSystem type works for DIZKs too.

[burdges]

If I understand, you're now passing the ConstraintSystem manually everywhere, not making variables, etc. track it for you?

[Pratyush]

Hmm the PR isn't anywhere near ready yet, but the idea is that high-level variables (eg: FieldGadgets, UInt8, etc.) carry a ConstraintSystemRef with them, and do operations locally on these.

[kobigurk]

Can we move this to the syntax that relies on Copy?

[Pratyush]

I need to figure out why it doesn't automatically find impl Add/Sub/Mul<Self> for Self

[ValarDragon]

Could the first two initial miscellaneous fix commits be cherry-picked into another PR, and added separately to master? (This PR should be rebaseable onto the new master then) That way the fixes are in master, and this PR has fewer different concepts to review.

(Also would it make sense to do the same for the bit iteration infrastructure?)

[weikengchen]

In the new API, it seems that computing the inverse of zero will still lead to an error. This would cause some trouble in writing the constraint system and indexing.

As discussed in this issue: #251

Can we change the implementation so that if the inverse does not exist, the inverse would be zero, so that the constraint would not satisfied? https://github.com/scipr-lab/zexe/blob/constraint-system-api/r1cs-std/src/fields/fp/mod.rs#L188

[weikengchen]

Saw a small typo on https://github.com/scipr-lab/zexe/blob/constraint-system-api/r1cs-std/src/fields/fp/mod.rs#L357

I think it is little-endian.