Refactor mul_fixed_short API to copy in (magnitude, sign)
#145
Conversation
therealyingtong
force-pushed
the
refactor-short-scalar
branch
from
33c5ca7
to
b7a6858
Compare
Codecov Report
@@ Coverage Diff @@
## main #145 +/- ##
==========================================
- Coverage 86.60% 86.48% -0.13%
==========================================
Files 64 62 -2
Lines 6359 6413 +54
==========================================
+ Hits 5507 5546 +39
- Misses 852 867 +15
Continue to review full report at Codecov.
|
therealyingtong
changed the title
mul_fixed_short API to copy in (magnitude, sign)mul_fixed_short API to copy in (magnitude, sign)
|
For the purposes of Orchard, Edit: This is implemented in this PR 345832c. |
therealyingtong
force-pushed
the
refactor-short-scalar
branch
from
345832c
to
28ebdcb
Compare
This decomposes a field element into K-bit windows using a running sum. Each step of the running sum is range-constrained. In strict mode, the final output of the running sum is constrained to be zero. This helper asserts K <= 3.
Witness a scalar in the region where it is used for multiplication, instead of witnessing it separately and then copying it in.
In the Orchard circuit, the short signed scalar is v_old - v_new, which will be witnessed as two cells: a 64-bit magnitude, and a sign that is +/- 1.
Check that a magnitude larger than 64 bits results in a constraint failure. Check that a sign other than +/- 1 results in a constrain failure.
In the Orchard protocol, only the NullifierK fixed base in used in scalar multiplication with a base field element. The mul_fixed_base_field_elem() API does not have to accept fixed bases other than NullifierK; conversely, NullifierK does not have to work with the full-width mul_fixed() API.
therealyingtong
force-pushed
the
refactor-short-scalar
branch
from
28ebdcb
to
32f3068
Compare
| // Magnitude larger than 64 bits should fail | ||
| { | ||
| let circuit = MyCircuit { | ||
| magnitude: Some(pallas::Base::from_u128(1 << 64)), |
There was a problem hiding this comment.
Test this for 1<<66 and 1<<254 as well. (Maybe also do all the test cases for both signs.)
There was a problem hiding this comment.
I realise that 1 << 66 would give the same failure as 1 << 64, because the "last window check" is on the running sum, which will be nonzero (even if the three-bit window itself is zero).
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
| scalar: &Self::Var, | ||
| base: &Self::Point, | ||
| ) -> Result<Self::Point, Error>; | ||
| ) -> Result<(Self::Point, Self::ScalarVar), Error>; |
There was a problem hiding this comment.
I'm confused by this change. It looks like we are not witnessing scalar inside this instruction, but decomposing an existing witnessed scalar?
| scalar: &Self::ScalarFixed, | ||
| scalar: Option<C::Scalar>, | ||
| base: &Self::FixedPoints, | ||
| ) -> Result<Self::Point, Error>; | ||
| ) -> Result<(Self::Point, Self::ScalarFixed), Error>; |
There was a problem hiding this comment.
I'd personally prefer that we enable these same optimizations with something like zcash/halo2#334, but as discussed in a pairing I'm reasonably confident we could retrofit that API onto these changes without altering the circuit.
| @@ -264,7 +264,7 @@ impl Config { | |||
| self.overflow_config | |||
| .overflow_check(layouter.namespace(|| "overflow check"), alpha, &zs)?; | |||
|
|
|||
| Ok(result) | |||
| Ok((result, alpha)) | |||
There was a problem hiding this comment.
Okay, now I'm really confused. If variable-base scalar mul is just passing alpha through, why even alter those APIs?
There was a problem hiding this comment.
My motivation was to make the mul API resemble the mul_fixed_* APIs.
These are now provided as inputs to the witness_decompose() and copy_decompose() methods. This allows us to reuse the same config for different word/window lengths, avoiding a duplicate constraint creation. Co-authored-by: Jack Grigg <jack@electriccoin.co>
The decompose_running_sum gadget in strict mode already enforces this check. Co-authored-by: Jack Grigg <jack@electriccoin.co>
Selectors previously used in the witness_scalar_* APIs, such as q_scalar_fixed and q_scalar_fixed_short, are now removed. The remaining selectors have been renamed for clarity. The coordinates check for scalars decomposed using a running sum has been moved into the mul_fixed.rs file, instead of being duplicated in both mul_fixed::base_field_elem and mul_fixed::short. The decompose_scalar_fixed() method is now only used in mul_fixed::full_width, and has been moved there.
therealyingtong
force-pushed
the
refactor-short-scalar
branch
from
3ae18f8
to
90b59ba
Compare
Co-authored-by: Jack Grigg <jack@electriccoin.co>
The coordinate check for an element decomposed using a running sum is enforced by mul_fixed::Config::running_sum_coords_gate(). Co-authored-by: Jack Grigg <jack@electriccoin.co>
| @@ -69,14 +69,11 @@ impl Config { | |||
| .coords_check(meta, q_mul_fixed_running_sum, word) | |||
| }); | |||
|
|
|||
| // Check that we get z_85 = 0 as the final output of the three-bit decomposition running sum. | |||
| // Also check that the base field element is canonical. | |||
| // Check that the base field element is canonical. | |||
There was a problem hiding this comment.
https://zcash.github.io/orchard/design/circuit/gadgets/ecc/fixed-base-scalar-mul.html#fixed-base-scalar-multiplication-using-base-field-element includes the z_85 = 0 constraint as part of the canonicity check. Update the book to reflect the implementation.
There was a problem hiding this comment.
z_85 = 0 is now constrained by the self.running_sum_config.copy_decompose() call in base_field_elem::Config::assign(), with strict = true.
I'll add a comment to this effect + update the book.
There was a problem hiding this comment.
@daira z_85 is constrained because we now use strict checks during scalar decomposition.
There was a problem hiding this comment.
I don't see any uses of strict = false except in tests. Did I miss any? If not, why do we need the strict check to be optional?
There was a problem hiding this comment.
The updated book PR is at: #146.
There was a problem hiding this comment.
I chose to include the strict option for a more general helper API.
(Sidenote: For the lookup_range_check helper, we only ever use strict = false, so we could choose to not provide a strict = true option there.)
There was a problem hiding this comment.
I find the code more confusing to think about with the two options, and more work to review. I would prefer that we remove the ones we don't use in Orchard. (Does not have to be in this PR.)
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Documented in #146.
The value commitment integrity check in the Action circuit involves a fixed-base scalar mul
[v_old - v_new] ValueCommitV, wherev_net = v_old - v_newis a signed 64-bit value.v_netwill be witnessed in the circuit as two cells: amagnitudeandsign. These should be copied into themul_fixed_shortAPI. This means themagnitudecan now be decomposed using a running sum, instead of being witnessed directly as bits.This PR also inlines all
witness_scalar_*APIs. A scalar is now directly witnessed in the region where it is used for multiplication, whereas previously it was witnessed separately and then copied in.