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

Finalize #334

Closed npwardberkeley closed 3 years ago

npwardberkeley commented 3 years ago

Initial attempt to resolve #333

weikengchen commented 3 years ago

This fmt issue may have something to do with the Rust version, aka

Because some other versions, and some nightly, have different cargo fmt preferences.

weikengchen commented 3 years ago

let me cc @Pratyush and @ValarDragon here for some feedback on the API.

weikengchen commented 3 years ago

It seems that all the comments have been resolved, with this one needing more discussion:

whether or not to keep this function

fn reduce_constraint_weight(&mut self) {

Since this function is now fn rather than pub fn, I think for now it is okay to remove this function. The function is only, previously used by Marlin (who needs to change accordingly).

Nick, what do you think?

npwardberkeley commented 3 years ago

Yes, that makes sense!

weikengchen commented 3 years ago

Let me cc @Pratyush and @ValarDragon for a final pass

Pratyush commented 3 years ago

I left a comment about None

weikengchen commented 3 years ago

Based on the discussion above, I think we will keep None for now and redirect it to self.inline_all_lcs(). This is the last change needed/ In the future, different treatments may occur, such as reducing the amount of symbolic LC.

weikengchen commented 3 years ago

Checks pass :) Pratyush and Dev, if it looks good, feel free to merge or do some final edits.

weikengchen commented 3 years ago

One more: we need to export OptimizationGoal here:

https://github.com/npwardberkeley/snark/blob/finalize/relations/src/r1cs/mod.rs#L21

Otherwise, Marlin could not find it.

weikengchen commented 3 years ago

add one to enforce that set_optimization_goal must check that no constraint/variable has been written.

        num_instance_variables: 1,
        num_witness_variables: 0,
        num_constraints: 0,
        num_linear_combinations: 0,

(sorry to bother Nick---I could add them if you add me as a PR repo's writer)