Introduced Hopwood optimized vbSM gadget #126
Conversation
…eptional cases. Tests still not passing
| #[inline] | ||
| /// Compute 2 * self + other as (self + other) + self: this requires less constraints | ||
| /// than computing self.double().add(other). | ||
| /// Neither `self` nor `other` can be the neutral element, and other != ±self; |
There was a problem hiding this comment.
And (self + other) =! +/- self, which under our presumption that other != 0 is equivalent to 2*self + other != 0.
| // TODO: we must assert that bits.length() <= ScalarField::MODULUS.length(), | ||
| // otherwise the algorithm is not secure. |
There was a problem hiding this comment.
Alternatively, we may even want to restrict that the scalar is below the scalar field modulus.
| &result | ||
| ) | ||
| } | ||
|
|
||
| ///This will take [(4 + 1) * ceil(len(bits)/2)] constraints to put the x lookup constraint |
There was a problem hiding this comment.
Let us elaborate on this comment. It is not clear to me what it asserts.
Also, a web link to the implementation we refer to would be cool!
| @@ -392,12 +661,14 @@ for AffineGadget<P, ConstraintF, F> | |||
|
|
|||
| //Perform addition | |||
| let adder: Self = Self::new(x, y, Boolean::constant(false)); | |||
| result = result.add(cs.ns(||format!("Add_{}", i)), &adder)?; | |||
| //TODO: Can we use add unsafe ? | |||
There was a problem hiding this comment.
No we can't. Otherwise, a malicious prover may choose a crafted shift, which provocates exceptional cases in order to "exit" the curve. The random sampling of the shift is to allow an honest prover to efficiently find an execution path in which no exceptions are hit.
… short_weierstrass_projective.rs
| ///This will take [(4 + 1) * ceil(len(bits)/2)] constraints to put the x lookup constraint | ||
| ///into the addition formula. See coda/src/lib/snarky_curves/snarky_curves.ml "scale_known" | ||
| ///Note: `self` must be different from `result` due to SW incomplete addition. | ||
| #[inline] | ||
| fn mul_bits_fixed_base<'a, CS: ConstraintSystem<ConstraintF>>( |
There was a problem hiding this comment.
We should think about a variant of mul_bits_fixed_base, which has a separate function for the precompuation of the lookup table. This speeds up the witness computations in circuits that make use of many such scalar multiplication (with respect to the same base).
There was a problem hiding this comment.
I added more inline docu, extended the usage of the safe flag in double_and_add_internal(), and copied these changes also to the non-native gadget.
The Hopwood optimization of mul_bits() is not a general purpose scalar multiplication, in the sense that the leading bit must always be 1. Therefore it should be treated as a low-level function, using a different name + test for it (the generic tests cause troubles as we discussed offline). More importantly we cannot use this mul_bits() out of the box in our circuits, which work with scalar bits sequences which are not free of leading zeroes. For that we need to invest additional work.
One way to tackle the problem is by transforming a general scalar instance (not free of leading zeroes) it into an (almost) constant length scalar multiplication as follows.
- We add to the scalar the scalar field modulus
p(by simulating non-native, but non-modular arithmetics). This step assures that the length of the resulting scalar is eitherlen(p)orlen(p)+1and has it's leading one of its two most significant bits. - The first two steps of the double and add needs to be handled explicitly to take the 2 different possibilities for the leading 1 into account. This can be done by conditional selects which "reset"/corrects the state of
accto the proper one. - As now the modified scalar might exceed the length of the scalar field modulus by one bit, one also needs to treat the last iteration of the double and add explicitly to handle possible exceptions.
The non-native addition costs about ~1 constraint per scalar bit on top, yielding a general purpose mul_bits of ~7 constraints per bit.
For the non-native variant this increase is negligible, but for native arithmetic circuit we should investigate if there is a more efficient solution than the one I proposed.
Still needs to be tested on other instances than BN382/secp256k1.
Corrected non-native addition by padding the scalar. Docu on endianess in the used conversion functions.
…bits(). Changed from_bits_test() accordingly.
Changed inline docu for `mul_bits()`
This reverts commit e00a772.
Import Hopwood's optimized vbSM gadget as per zcash/zcash#3924 .
Currently performed with incomplete arithmetic and by dropping the requirement of the MSB of the scalar being 1.
Also partially addresses #125