fix(ecdsa): allow u1*G == u2*PK case (#36)

NOTE: current ecdsa requires `r, s` to be given as proper CRT integers

TODO: newtypes to guard this assumption

halo2-ecc/src/ecc/ecdsa.rs

@@ -1,20 +1,19 @@
+use halo2_base::{gates::GateInstructions, utils::CurveAffineExt, AssignedValue, Context};
+
 use crate::bigint::{big_less_than, CRTInteger};
 use crate::fields::{fp::FpChip, FieldChip, PrimeField};
-use halo2_base::{
-    gates::{GateInstructions, RangeInstructions},
-    utils::CurveAffineExt,
-    AssignedValue, Context,
-};
 
-use super::fixed_base;
-use super::{ec_add_unequal, scalar_multiply, EcPoint};
+use super::{fixed_base, EccChip};
+use super::{scalar_multiply, EcPoint};
 // CF is the coordinate field of GA
 // SF is the scalar field of GA
 // p = coordinate field modulus
 // n = scalar field modulus
 // Only valid when p is very close to n in size (e.g. for Secp256k1)
+// Assumes `r, s` are proper CRT integers
+/// **WARNING**: Only use this function if `1 / (p - n)` is very small (e.g., < 2<sup>-100</sup>)
 pub fn ecdsa_verify_no_pubkey_check<F: PrimeField, CF: PrimeField, SF: PrimeField, GA>(
-    base_chip: &FpChip<F, CF>,
+    chip: &EccChip<F, FpChip<F, CF>>,
     ctx: &mut Context<F>,
     pubkey: &EcPoint<F, <FpChip<F, CF> as FieldChip<F>>::FieldPoint>,
     r: &CRTInteger<F>,
@@ -26,6 +25,8 @@ pub fn ecdsa_verify_no_pubkey_check<F: PrimeField, CF: PrimeField, SF: PrimeFiel
 where
     GA: CurveAffineExt<Base = CF, ScalarExt = SF>,
 {
+    // Following https://en.wikipedia.org/wiki/Elliptic_Curve_Digital_Signature_Algorithm
+    let base_chip = chip.field_chip;
     let scalar_chip =
         FpChip::<F, SF>::new(base_chip.range, base_chip.limb_bits, base_chip.num_limbs);
     let n = scalar_chip.load_constant(ctx, scalar_chip.p.to_biguint().unwrap());
@@ -38,8 +39,6 @@ where
     let u1 = scalar_chip.divide(ctx, msghash, s);
     let u2 = scalar_chip.divide(ctx, r, s);
 
-    //let r_crt = scalar_chip.to_crt(ctx, r)?;
-
     // compute u1 * G and u2 * pubkey
     let u1_mul = fixed_base::scalar_multiply::<F, _, _>(
         base_chip,
@@ -58,21 +57,23 @@ where
         var_window_bits,
     );
 
-    // check u1 * G and u2 * pubkey are not negatives and not equal
-    //     TODO: Technically they could be equal for a valid signature, but this happens with vanishing probability
-    //           for an ECDSA signature constructed in a standard way
+    // check u1 * G != -(u2 * pubkey) but allow u1 * G == u2 * pubkey
+    // check (u1 * G).x != (u2 * pubkey).x or (u1 * G).y == (u2 * pubkey).y
     // coordinates of u1_mul and u2_mul are in proper bigint form, and lie in but are not constrained to [0, n)
     // we therefore need hard inequality here
-    let u1_u2_x_eq = base_chip.is_equal(ctx, &u1_mul.x, &u2_mul.x);
-    let u1_u2_not_neg = base_chip.range.gate().not(ctx, u1_u2_x_eq);
+    let x_eq = base_chip.is_equal(ctx, &u1_mul.x, &u2_mul.x);
+    let x_neq = base_chip.gate().not(ctx, x_eq);
+    let y_eq = base_chip.is_equal(ctx, &u1_mul.y, &u2_mul.y);
+    let u1g_u2pk_not_neg = base_chip.gate().or(ctx, x_neq, y_eq);
 
     // compute (x1, y1) = u1 * G + u2 * pubkey and check (r mod n) == x1 as integers
+    // because it is possible for u1 * G == u2 * pubkey, we must use `EccChip::sum`
+    let sum = chip.sum::<GA>(ctx, [u1_mul, u2_mul].iter());
     // WARNING: For optimization reasons, does not reduce x1 mod n, which is
     //          invalid unless p is very close to n in size.
-    base_chip.enforce_less_than_p(ctx, u1_mul.x());
-    base_chip.enforce_less_than_p(ctx, u2_mul.x());
-    let sum = ec_add_unequal(base_chip, ctx, &u1_mul, &u2_mul, false);
-    let equal_check = base_chip.is_equal(ctx, &sum.x, r);
+    // enforce x1 < n
+    scalar_chip.enforce_less_than_p(ctx, &sum.x);
+    let equal_check = scalar_chip.is_equal_unenforced(ctx, &sum.x, r);
 
     // TODO: maybe the big_less_than is optional?
     let u1_small = big_less_than::assign::<F>(
@@ -92,11 +93,11 @@ where
         base_chip.limb_bases[1],
     );
 
-    // check (r in [1, n - 1]) and (s in [1, n - 1]) and (u1_mul != - u2_mul) and (r == x1 mod n)
+    // check (r in [1, n - 1]) and (s in [1, n - 1]) and (u1 * G != - u2 * pubkey) and (r == x1 mod n)
     let res1 = base_chip.gate().and(ctx, r_valid, s_valid);
     let res2 = base_chip.gate().and(ctx, res1, u1_small);
     let res3 = base_chip.gate().and(ctx, res2, u2_small);
-    let res4 = base_chip.gate().and(ctx, res3, u1_u2_not_neg);
+    let res4 = base_chip.gate().and(ctx, res3, u1g_u2pk_not_neg);
     let res5 = base_chip.gate().and(ctx, res4, equal_check);
     res5
 }

halo2-ecc/src/fields/fp.rs

@@ -338,9 +338,9 @@ impl<'range, F: PrimeField, Fp: PrimeField> FieldChip<F> for FpChip<'range, F, F
         let (_, underflow) =
             sub::crt::<F>(self.range(), ctx, a, &p, self.limb_bits, self.limb_bases[1]);
         let is_underflow_zero = self.gate().is_zero(ctx, underflow);
-        let range_check = self.gate().not(ctx, is_underflow_zero);
+        let no_underflow = self.gate().not(ctx, is_underflow_zero);
 
-        self.gate().and(ctx, is_nonzero, range_check)
+        self.gate().and(ctx, is_nonzero, no_underflow)
     }
 
     // assuming `a` has been range checked to be a proper BigInt

halo2-ecc/src/secp256k1/tests/ecdsa.rs

@@ -69,7 +69,7 @@ fn ecdsa_test<F: PrimeField>(
     let pk = ecc_chip.load_private(ctx, (pk.x, pk.y));
     // test ECDSA
     let res = ecdsa_verify_no_pubkey_check::<F, Fp, Fq, Secp256k1Affine>(
-        &fp_chip, ctx, &pk, &r, &s, &m, 4, 4,
+        &ecc_chip, ctx, &pk, &r, &s, &m, 4, 4,
     );
     assert_eq!(res.value(), &F::one());
 }