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

Rename ConstraintSystemRef to R1CSBuilder #343

Open
4 tasks
Pratyush opened this issue Mar 17, 2021 · 3 comments
Open
4 tasks

Rename ConstraintSystemRef to R1CSBuilder #343

Pratyush opened this issue Mar 17, 2021 · 3 comments

Comments

@Pratyush
Copy link
Member

Pratyush commented Mar 17, 2021

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
@weikengchen
Copy link
Member

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
Copy link
Member Author

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
Copy link

alxiong commented Apr 19, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants