arkworks-rs / snark

Interfaces for Relations and SNARKs for these relations
https://www.arkworks.rs
Apache License 2.0
769 stars 203 forks source link

Rename `ConstraintSystemRef` to `R1CSBuilder` #343

Open Pratyush opened 3 years ago

Pratyush commented 3 years ago

Summary

Workflow:

let mut builder = R1CSBuilder::new();
R1CSDriver::synthesize(&mut builder);
let r1cs_triple: R1CS = R1CSBuilder.finish();

What do you think?


For Admin Use

weikengchen commented 3 years ago

I feel the Ref part is still meaningful, as it conveys a meaning that this is just a reference so feel free to clone. "Builder" seems to be something that should not be cloned everywhere. Would "R1CSRef" suffice?

Regarding "Driver", the situation appears since an R1CS likely would have many drivers. How about the ancient name "R1CSGadget"?

Pratyush commented 3 years ago

I feel the Ref part is still meaningful, as it conveys a meaning that this is just a reference so feel free to clone. "Builder" seems to be something that should not be cloned everywhere. Would "R1CSRef" suffice?

Hmm R1CSRef would imply it’s a reference for R1CS, which it isn’t. The reason to call it Builder is because you use it incrementally build up the R1CS matrices and variable assignments. Maybe we can just say in documentation that it is cheap to clone.

Regarding "Driver", the situation appears since an R1CS likely would have many drivers. How about the ancient name "R1CSGadget"?

I suggested Driver because it “drives” the building of the Builder. Maybe we can call it R1CSContext instead, as it contains the the Context to start the building?

alxiong commented 3 years ago

sorry to chime in here,

The reason to call it Builder is because you use it incrementally build up the R1CS matrices and variable assignments.

imo, "builder" has the implicit association with the traditional "builder pattern", (e.g. clap.rs API), however the process of CS building has lots of sequential dependency (e.g. builder.enforce_constraint(a, b, c) depends on the let a/b/c = builder.new_witness_var()), thus it's hard to create a clean builder workflow, not in traditional sense at least.

// traditional Builder workflow
let cs: R1CS = R1CSBuilder::new().add_xxx_constraint()
                                 .add_multi_exp_constraint()
                                 .add_xxx_constraint()
                                 .build();

// in reality, what we might end up with:
let mut builder = R1CSBuilder::new();
let a_var = builder.new_witness_var(a);
let b_var = builder.new_witness_var(b);
builder.enforce_add(a_var, b_var);
builder.finalize();

Question: The rationale behind having a separate enum CSRef?

I can see that it's a signal for cloneable object, avoiding cloning a giant object like CS itself, but why can't we just have a CS::as_ref(self) -> Rc<RefCell<Self>> instead?

Regarding CSRef::None variant, why couldn't we consider this as a special case of CS instance with constant values added to transcript without any variable to be enforced. Also, where do we get to use this, any real example on why this variant might be useful?

Thanks