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

Add helper method to inline `mul_by_const` immediately under constraints optimization goal #339

Closed ValarDragon closed 3 years ago

ValarDragon commented 3 years ago

Description

This adds a method to constraint system that gadgets such as FpVar would call to do multiplication by a constant. It handles inlining immediately when under the Constraints Optimization goal, and doesn't create additional symbolic variables when the variable is Zero. This could cause speed losses to constraint-optimized circuits when doing two mul by consts in a row, but I think such a case isn't that likely.

This is intended to be used here: https://github.com/arkworks-rs/r1cs-std/blob/master/src/fields/fp/mod.rs#L192


Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why.

weikengchen commented 3 years ago

Corrected one small typo deffering -> deferring. (note: if you are using a Mac, you might be a victim of the bufferfly keyboard like me https://support.apple.com/keyboard-service-program-for-mac-notebooks)

Others look good.

Pratyush commented 3 years ago

Do we need to make a new API for this? As in, inside new_lc, we can detect if the incoming linear combination consists of just a single term, and can inline it directly if we're in the Constraints optimization goal.

(This might even be helpful under any optimization goal, when the variable in the single term is not SymbolicLc).

ValarDragon commented 3 years ago

That works, though it is unfortunate that we'd be doing wasted heap allocations then, versus producing the right lc. (One extra heap allocation for basically every operation) We don't really have empirical verification that thats a bottleneck though.

Pratyush commented 3 years ago

If we make LinearCombination to be a SmallVec of size 1 then we don't have to pay that overhead at all =)

ValarDragon commented 3 years ago

True! I'll close this in favor of going with that approach

ValarDragon commented 3 years ago

Actually, I'm realizing now that this actually increases memory overhead in the current design, since we're still storing each intermediate LC, and now those LC's are longer. (as would the new_lc approach) This type of approach would be useful if there were a drop-semantics style approach of removing old lc's from the lc_map.

Pratyush commented 3 years ago

I think there are two approaches to handle this:

ValarDragon commented 3 years ago

We had the same ideas =) #cref https://github.com/arkworks-rs/snark/issues/336#issuecomment-754852431

Either we have linear combinations in the variable directly, or in the lc_map, but not in both. This way, if we inline LCs, then when the variable goes out of scope, the memory for the LC is freed immediately.

We still need LC's getting saved to the lc_map at some point though, in order to do density saving optimizations. Doing that at constraint generation causes some awkward mutability problems though.

Pratyush commented 3 years ago

Let's consolidate in #336