Finalize #334

npwardberkeley · 2020-12-31T05:44:05Z

Initial attempt to resolve #333

weikengchen · 2020-12-31T07:46:43Z

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

is it 1.48.0?
is it stable?

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

weikengchen · 2020-12-31T07:47:53Z

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

Pratyush · 2020-12-31T08:13:33Z

relations/src/r1cs/constraint_system.rs

@@ -149,6 +165,18 @@ impl<F: Field> ConstraintSystem<F> {
        self.mode == SynthesisMode::Setup
    }

+    /// Check whether this constraint system aims to optimize weight or
+    /// number of constraints.
+    pub fn get_optimization_goal(&self) -> OptimizationGoal {


Suggested change

pub fn get_optimization_goal(&self) -> OptimizationGoal {

pub fn optimization_goal(&self) -> OptimizationGoal {

Pratyush · 2020-12-31T08:14:21Z

relations/src/r1cs/constraint_system.rs

+    fn reduce_constraint_weight(&mut self) {
        self.outline_lcs();
    }


We can remove this now, right?

Wasn't the idea to add other weight-reducing optimizations, apart from just outlining? (That's what the comment here says.) In which case we'd still want this as a separate function

Pratyush · 2020-12-31T08:14:47Z

relations/src/r1cs/constraint_system.rs

+    pub fn get_optimization_goal(&self) -> OptimizationGoal {
+        self.inner().map_or(OptimizationGoal::Constraints, |cs| {
+            cs.borrow().get_optimization_goal()
+        })
+    }


Suggested change

pub fn get_optimization_goal(&self) -> OptimizationGoal {

self.inner().map_or(OptimizationGoal::Constraints, |cs| {

cs.borrow().get_optimization_goal()

})

}

pub fn optimization_goal(&self) -> OptimizationGoal {

self.inner().map_or(OptimizationGoal::Constraints, |cs| {

cs.borrow().optimization_goal()

})

}

ValarDragon · 2020-12-31T16:40:45Z

relations/src/r1cs/constraint_system.rs

@@ -91,6 +95,16 @@ pub enum SynthesisMode {
    },
 }

+/// Defines the parameter to optimize for a `ConstraintSystem`.
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd)]
+pub enum OptimizationGoal {


Should we have a None here, which will do no optimization passes?

That makes sense. Do you think None, or Constraints, should be the default?

I think constraints should be the default

What are the semantics of calling finalize with None? In the end we have to get rid of all SymbolicLC before converting to matrices

I think None would still need to inline Symbolic LC (self.inline_all_lcs(), without which to_matrices would fail.

Dev, what do you think? Basically, at this moment, None would be the same as Constraints, and it is uncertain if in the future we will treat them differently.

I think given that no specific use cases pop up for None, and we currently do not have a good idea of faster inlining (without optimization to reduce constraints), maybe we can omit None and add it later. Dev, what do you think?

Sounds good to me!

Actually, I do think None is still useful, in that it could just make far fewer symbolic LC's for memory reduction. E.g. when multiplying by a constant, we don't need to make a new symbolic LC if we don't care about optimizations

And we could have drop semantics to symbolic LC's to remove intermediate variables from the map as soon as they're dropped from memory. It is a significant engineering effort though, so maybe not worth having it now. (Its non breaking to add it later)

That makes sense. Then let us keep None and, for now, let None also do inline_all_lcs. More changes may be coming in the future.

ValarDragon · 2020-12-31T16:41:35Z

relations/src/r1cs/constraint_system.rs

-    ///     (usize, Option<Vec<F>>)
+    /// The transformer function is given a references of this constraint system
+    /// (&self), number of times used, and a mutable reference of the linear
+    /// combination to be transformed.     (&ConstraintSystem<F>, usize,


Were these comments re-ordered by a linter? Its kinda weird imo that the tuple is split across a new line

weikengchen · 2021-01-03T04:35:59Z

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 · 2021-01-03T04:42:34Z

Yes, that makes sense!

weikengchen · 2021-01-03T04:51:10Z

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

Pratyush · 2021-01-03T04:56:04Z

I left a comment about None

relations/src/r1cs/constraint_system.rs

weikengchen · 2021-01-03T20:56:33Z

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 · 2021-01-03T21:25:59Z

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

weikengchen · 2021-01-03T21:34:58Z

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 · 2021-01-03T21:37:52Z

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)

weikengchen

The final changes have been applied. Going to merge and updating Marlin then.

npwardberkeley added 2 commits December 30, 2020 19:06

finalize method

27f8b80

setter/getter, and wrapprs in ConstraintSystemRef

31a16cd

npwardberkeley marked this pull request as draft December 31, 2020 05:44

npwardberkeley marked this pull request as ready for review December 31, 2020 05:44

style

4c4ad1c

Pratyush reviewed Dec 31, 2020

View reviewed changes

ValarDragon reviewed Dec 31, 2020

View reviewed changes

npwardberkeley added 2 commits December 31, 2020 11:19

incorporate comments

afd19bb

constraints as default optimization goal

974ef4f

remove reduce_constraint_weight

32e5206

ValarDragon reviewed Jan 3, 2021

View reviewed changes

relations/src/r1cs/constraint_system.rs Show resolved Hide resolved

weikengchen requested changes Jan 3, 2021

View reviewed changes

relations/src/r1cs/constraint_system.rs Outdated Show resolved Hide resolved

None still inlines

a31f000

weikengchen added 2 commits January 3, 2021 21:36

export OptimizationGoal; add assertion checks

7f9ef99

retouch the comments

b58393a

weikengchen approved these changes Jan 4, 2021

View reviewed changes

update the changelog

9de6b0c

weikengchen merged commit 8d9055d into arkworks-rs:master Jan 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalize #334

Finalize #334

npwardberkeley commented Dec 31, 2020

weikengchen commented Dec 31, 2020

weikengchen commented Dec 31, 2020

Pratyush Dec 31, 2020

Pratyush Dec 31, 2020

npwardberkeley Dec 31, 2020

Pratyush Dec 31, 2020

ValarDragon Dec 31, 2020

npwardberkeley Dec 31, 2020

ValarDragon Dec 31, 2020

Pratyush Jan 3, 2021

weikengchen Jan 3, 2021

weikengchen Jan 3, 2021

ValarDragon Jan 3, 2021 •

edited

Loading

ValarDragon Jan 3, 2021 •

edited

Loading

ValarDragon Jan 3, 2021

weikengchen Jan 3, 2021

ValarDragon Dec 31, 2020

weikengchen commented Jan 3, 2021

npwardberkeley commented Jan 3, 2021

weikengchen commented Jan 3, 2021

Pratyush commented Jan 3, 2021

weikengchen commented Jan 3, 2021

weikengchen commented Jan 3, 2021

weikengchen commented Jan 3, 2021

weikengchen commented Jan 3, 2021

weikengchen left a comment

	pub fn get_optimization_goal(&self) -> OptimizationGoal {
	pub fn optimization_goal(&self) -> OptimizationGoal {

Finalize #334

Finalize #334

Conversation

npwardberkeley commented Dec 31, 2020

weikengchen commented Dec 31, 2020

weikengchen commented Dec 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Jan 3, 2021 • edited Loading

Choose a reason for hiding this comment

ValarDragon Jan 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weikengchen commented Jan 3, 2021

npwardberkeley commented Jan 3, 2021

weikengchen commented Jan 3, 2021

Pratyush commented Jan 3, 2021

weikengchen commented Jan 3, 2021

weikengchen commented Jan 3, 2021

weikengchen commented Jan 3, 2021

weikengchen commented Jan 3, 2021

weikengchen left a comment

Choose a reason for hiding this comment

ValarDragon Jan 3, 2021 •

edited

Loading

ValarDragon Jan 3, 2021 •

edited

Loading