snark icon indicating copy to clipboard operation
snark copied to clipboard

Rename `ConstraintSystemRef` to `R1CSBuilder`

Open Pratyush opened this issue 3 years ago • 3 comments

Summary

  • New relation struct for R1CS.
  • ConstraintSystemRef -> R1CSBuilder.
  • ConstraintSynthesizer -> R1CSDriver (maybe something else?).

Workflow:

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

What do you think?


For Admin Use

  • [ ] Not duplicate issue
  • [ ] Appropriate labels applied
  • [ ] Appropriate contributors tagged
  • [ ] Contributor assigned/self-assigned

Pratyush avatar Mar 17 '21 20:03 Pratyush

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"?

weikengchen avatar Mar 18 '21 17:03 weikengchen

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?

Pratyush avatar Mar 18 '21 18:03 Pratyush

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

alxiong avatar Apr 19 '21 07:04 alxiong