fix bug in #872 fix/msg-hash-zero (#880)

* Update ecdsa.rs

* fmt and update cells usage

---------

Co-authored-by: Zhuo Zhang <mycinbrin@gmail.com>

Author: zhenfeizhang

zkevm-circuits/src/sig_circuit/ecdsa.rs

@@ -35,6 +35,7 @@ pub(crate) fn ecdsa_verify_no_pubkey_check<F: PrimeField, CF: PrimeField, SF: Pr
 where
     GA: CurveAffineExt<Base = CF, ScalarExt = SF>,
 {
+    let ecc_chip = EccChip::<F, FpConfig<F, CF>>::construct(base_chip.clone());
     let scalar_chip = FpConfig::<F, SF>::construct(
         base_chip.range.clone(),
         base_chip.limb_bits,
@@ -85,7 +86,6 @@ where
     let u3_mul = {
         let generator = GA::generator();
         let neg_generator = -generator;
-        let ecc_chip = EccChip::<F, FpConfig<F, CF>>::construct(base_chip.clone());
         let generator = ecc_chip.assign_constant_point(ctx, generator);
         let neg_generator = ecc_chip.assign_constant_point(ctx, neg_generator);
         ecc_chip.select(ctx, &neg_generator, &generator, &u1_is_one)
@@ -99,21 +99,50 @@ where
     // check
     // - (u1 + u3) * G
     // - u2 * pubkey + u3 * G
-    // are not negatives and not equal
+    // are 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
     // 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, &u1u3_mul.x, &u2_pk_u3_g.x);
-    let u1_u2_not_neg = base_chip.range.gate().not(ctx, Existing(u1_u2_x_eq));
+    let u1_u2_x_eq = ecc_chip.is_equal(ctx, &u1u3_mul, &u2_pk_u3_g);
+    let u1_u2_not_eq = base_chip.range.gate().not(ctx, Existing(u1_u2_x_eq));
+
+    // check u1*G and u2*pubkey are not negate of each other
+    // that means the sum of
+    // - (u1 + u3) * G
+    // - u2 * pubkey + u3 * G
+    // should not equal to 2u3 * G
+    let u1_u2_not_neg = {
+        // again we get 2u3*G from constant to avoid scalar_multiply
+        let two_generator = GA::generator();
+        let neg_two_generator = -two_generator;
+        let two_generator = ecc_chip.assign_constant_point(ctx, two_generator);
+        let neg_two_generator = ecc_chip.assign_constant_point(ctx, neg_two_generator);
+        let two_u3_g = ecc_chip.select(ctx, &neg_two_generator, &two_generator, &u1_is_one);
+
+        base_chip.enforce_less_than_p(ctx, u1u3_mul.x());
+        base_chip.enforce_less_than_p(ctx, u2_pk_u3_g.x());
+        // safe: we have already checked (u1 + u3) * G != u2 * pubkey + u3 * G
+        let sum = ec_add_unequal(base_chip, ctx, &u1u3_mul, &u2_pk_u3_g, false);
+
+        let is_equal = ecc_chip.is_equal(ctx, &sum, &two_u3_g);
+        base_chip.range.gate().not(ctx, Existing(is_equal))
+    };
 
     // compute (x1, y1) = u1 * G + u2 * pubkey and check (r mod n) == x1 as integers
     // which is basically u1u3_mul + u2_mul - u3_mul
     // 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, u1u3_mul.x());
+    //
+    // WARNING: this may be trigger errors if
+    //  u1u3_mul == u2_mul
+    // if r is sampled truly from random then this will not happen
+    // to completely ensure the correctness we may need to sample u3 from random, but it is quite
+    // costly.
     let sum = ec_add_unequal(base_chip, ctx, &u1u3_mul, &u2_mul, false);
+    // safe: we have already checked u1G + u2 pk != 0
+    // so u1G + u3G + u2pk != u3G
     let sum = ec_sub_unequal(base_chip, ctx, &sum, &u3_mul, false);
     let equal_check = base_chip.is_equal(ctx, &sum.x, r);
 
@@ -151,12 +180,16 @@ where
     let res4 = base_chip
         .range
         .gate()
-        .and(ctx, Existing(res3), Existing(u1_u2_not_neg));
+        .and(ctx, Existing(res3), Existing(u1_u2_not_eq));
     let res5 = base_chip
         .range
         .gate()
-        .and(ctx, Existing(res4), Existing(equal_check));
-    (res5, sum.y)
+        .and(ctx, Existing(res4), Existing(u1_u2_not_neg));
+    let res6 = base_chip
+        .range
+        .gate()
+        .and(ctx, Existing(res5), Existing(equal_check));
+    (res6, sum.y)
 }
 
 fn scalar_field_element_is_one<F: PrimeField, SF: PrimeField>(

zkevm-circuits/src/sig_circuit/utils.rs

@@ -13,8 +13,8 @@ use halo2_proofs::{
 // Hard coded parameters.
 // FIXME: allow for a configurable param.
 pub(super) const MAX_NUM_SIG: usize = 128;
-// Each ecdsa signature requires 460456 cells
-pub(super) const CELLS_PER_SIG: usize = 460456;
+// Each ecdsa signature requires 462274 cells
+pub(super) const CELLS_PER_SIG: usize = 462274;
 // Each ecdsa signature requires 62994 lookup cells
 pub(super) const LOOKUP_CELLS_PER_SIG: usize = 62994;
 // Total number of rows allocated for ecdsa chip