[Fix] Panic when dealing with identity point (#71)

* More ecdsa tests

* Update mod.rs

* Update tests.rs

* Update ecdsa.rs

* Update ecdsa.rs

* Update ecdsa.rs

* msm tests

* Update mod.rs

* Update msm_sum_infinity.rs

* fix: ec_sub_strict was panicing when output is identity

* affects the MSM functions: right now if the answer is identity, there
  will be a panic due to divide by 0 instead of just returning 0
* there could be a more optimal solution, but due to the traits for
  EccChip, we just generate a random point solely to avoid divide by 0
in the case of identity point

* Fix/fb msm zero (#77)

* fix: fixed_base scalar multiply for [-1]P

* feat: use `multi_scalar_multiply` instead of `scalar_multiply`

* to reduce code maintanence / redundancy

* fix: add back scalar_multiply using any_point

* feat: remove flag from variable base `scalar_multiply`

* feat: add scalar multiply tests for secp256k1

* fix: variable scalar_multiply last select

* Fix/msm tests output identity (#75)

* fixed base msm tests for output infinity

* fixed base msm tests for output infinity

---------

Co-authored-by: yulliakot <[email protected]>

* feat: add tests and update CI

---------

Co-authored-by: yuliakot <[email protected]>
Co-authored-by: yulliakot <[email protected]>

---------

Co-authored-by: yulliakot <[email protected]>
Co-authored-by: yuliakot <[email protected]>

.github/workflows/ci.yml

@@ -4,7 +4,7 @@ on:
   push:
     branches: ["main", "release-0.3.0"]
   pull_request:
-    branches: ["main"]
+    branches: ["main", "release-0.3.0"]
 
 env:
   CARGO_TERM_COLOR: always
@@ -27,11 +27,12 @@ jobs:
           cd halo2-ecc
           cargo test -- --test-threads=1 test_fp
           cargo test -- test_ecc
-          cargo test -- test_secp256k1_ecdsa
+          cargo test -- test_secp
           cargo test -- test_ecdsa
           cargo test -- test_ec_add
-          cargo test -- test_fixed_base_msm
+          cargo test -- test_fixed
           cargo test -- test_msm
+          cargo test -- test_fb
           cargo test -- test_pairing
           cd ..
       - name: Run halo2-ecc tests real prover
@@ -40,7 +41,10 @@ jobs:
           cargo test --release -- test_fp_assert_eq
           cargo test --release -- --nocapture bench_secp256k1_ecdsa
           cargo test --release -- --nocapture bench_ec_add
+          mv configs/bn254/bench_fixed_msm.t.config configs/bn254/bench_fixed_msm.config
           cargo test --release -- --nocapture bench_fixed_base_msm
+          mv configs/bn254/bench_msm.t.config configs/bn254/bench_msm.config
           cargo test --release -- --nocapture bench_msm
+          mv configs/bn254/bench_pairing.t.config configs/bn254/bench_pairing.config
           cargo test --release -- --nocapture bench_pairing
           cd ..

halo2-ecc/configs/bn254/bench_fixed_msm.t.config

@@ -0,0 +1,5 @@
+{"strategy":"Simple","degree":17,"num_advice":83,"num_lookup_advice":9,"num_fixed":7,"lookup_bits":16,"limb_bits":88,"num_limbs":3,"batch_size":100,"radix":0,"clump_factor":4}
+{"strategy":"Simple","degree":18,"num_advice":42,"num_lookup_advice":5,"num_fixed":4,"lookup_bits":17,"limb_bits":88,"num_limbs":3,"batch_size":100,"radix":0,"clump_factor":4}
+{"strategy":"Simple","degree":19,"num_advice":20,"num_lookup_advice":2,"num_fixed":2,"lookup_bits":18,"limb_bits":90,"num_limbs":3,"batch_size":100,"radix":0,"clump_factor":4}
+{"strategy":"Simple","degree":19,"num_advice":6,"num_lookup_advice":1,"num_fixed":1,"lookup_bits":18,"limb_bits":88,"num_limbs":3,"batch_size":25,"radix":0,"clump_factor":4}
+{"strategy":"Simple","degree":20,"num_advice":6,"num_lookup_advice":1,"num_fixed":1,"lookup_bits":19,"limb_bits":88,"num_limbs":3,"batch_size":50,"radix":0,"clump_factor":4}

halo2-ecc/configs/bn254/bench_msm.t.config

@@ -0,0 +1,5 @@
+{"strategy":"Simple","degree":16,"num_advice":170,"num_lookup_advice":23,"num_fixed":1,"lookup_bits":15,"limb_bits":88,"num_limbs":3,"batch_size":100,"window_bits":4}
+{"strategy":"Simple","degree":17,"num_advice":84,"num_lookup_advice":11,"num_fixed":1,"lookup_bits":16,"limb_bits":88,"num_limbs":3,"batch_size":100,"window_bits":4}
+{"strategy":"Simple","degree":19,"num_advice":20,"num_lookup_advice":3,"num_fixed":1,"lookup_bits":18,"limb_bits":90,"num_limbs":3,"batch_size":100,"window_bits":4}
+{"strategy":"Simple","degree":19,"num_advice":6,"num_lookup_advice":1,"num_fixed":1,"lookup_bits":18,"limb_bits":88,"num_limbs":3,"batch_size":25,"window_bits":4}
+{"strategy":"Simple","degree":20,"num_advice":6,"num_lookup_advice":1,"num_fixed":1,"lookup_bits":19,"limb_bits":88,"num_limbs":3,"batch_size":50,"window_bits":4}

halo2-ecc/configs/bn254/bench_pairing.t.config

@@ -0,0 +1,5 @@
+{"strategy":"Simple","degree":15,"num_advice":105,"num_lookup_advice":14,"num_fixed":1,"lookup_bits":14,"limb_bits":90,"num_limbs":3}
+{"strategy":"Simple","degree":17,"num_advice":25,"num_lookup_advice":3,"num_fixed":1,"lookup_bits":16,"limb_bits":88,"num_limbs":3}
+{"strategy":"Simple","degree":18,"num_advice":13,"num_lookup_advice":2,"num_fixed":1,"lookup_bits":17,"limb_bits":88,"num_limbs":3}
+{"strategy":"Simple","degree":19,"num_advice":6,"num_lookup_advice":1,"num_fixed":1,"lookup_bits":18,"limb_bits":90,"num_limbs":3}
+{"strategy":"Simple","degree":20,"num_advice":3,"num_lookup_advice":1,"num_fixed":1,"lookup_bits":19,"limb_bits":88,"num_limbs":3}

halo2-ecc/src/bn254/tests/fixed_base_msm.rs

@@ -23,7 +23,7 @@ use itertools::Itertools;
 use rand_core::OsRng;
 
 #[derive(Clone, Copy, Debug, Serialize, Deserialize)]
-struct MSMCircuitParams {
+struct FixedMSMCircuitParams {
     strategy: FpStrategy,
     degree: u32,
     num_advice: usize,
@@ -39,7 +39,7 @@ struct MSMCircuitParams {
 
 fn fixed_base_msm_test(
     builder: &mut GateThreadBuilder<Fr>,
-    params: MSMCircuitParams,
+    params: FixedMSMCircuitParams,
     bases: Vec<G1Affine>,
     scalars: Vec<Fr>,
 ) {
@@ -68,7 +68,7 @@ fn fixed_base_msm_test(
 }
 
 fn random_fixed_base_msm_circuit(
-    params: MSMCircuitParams,
+    params: FixedMSMCircuitParams,
     bases: Vec<G1Affine>, // bases are fixed in vkey so don't randomly generate
     stage: CircuitBuilderStage,
     break_points: Option<MultiPhaseThreadBreakPoints>,
@@ -102,7 +102,7 @@ fn random_fixed_base_msm_circuit(
 #[test]
 fn test_fixed_base_msm() {
     let path = "configs/bn254/fixed_msm_circuit.config";
-    let params: MSMCircuitParams = serde_json::from_reader(
+    let params: FixedMSMCircuitParams = serde_json::from_reader(
         File::open(path).unwrap_or_else(|e| panic!("{path} does not exist: {e:?}")),
     )
     .unwrap();
@@ -112,6 +112,23 @@ fn test_fixed_base_msm() {
     MockProver::run(params.degree, &circuit, vec![]).unwrap().assert_satisfied();
 }
 
+#[test]
+fn test_fixed_msm_minus_1() {
+    let path = "configs/bn254/fixed_msm_circuit.config";
+    let params: FixedMSMCircuitParams = serde_json::from_reader(
+        File::open(path).unwrap_or_else(|e| panic!("{path} does not exist: {e:?}")),
+    )
+    .unwrap();
+    let base = G1Affine::random(OsRng);
+    let k = params.degree as usize;
+    let mut builder = GateThreadBuilder::mock();
+    fixed_base_msm_test(&mut builder, params, vec![base], vec![-Fr::one()]);
+
+    builder.config(k, Some(20));
+    let circuit = RangeCircuitBuilder::mock(builder);
+    MockProver::run(params.degree, &circuit, vec![]).unwrap().assert_satisfied();
+}
+
 #[test]
 fn bench_fixed_base_msm() -> Result<(), Box<dyn std::error::Error>> {
     let config_path = "configs/bn254/bench_fixed_msm.config";
@@ -126,7 +143,8 @@ fn bench_fixed_base_msm() -> Result<(), Box<dyn std::error::Error>> {
 
     let bench_params_reader = BufReader::new(bench_params_file);
     for line in bench_params_reader.lines() {
-        let bench_params: MSMCircuitParams = serde_json::from_str(line.unwrap().as_str()).unwrap();
+        let bench_params: FixedMSMCircuitParams =
+            serde_json::from_str(line.unwrap().as_str()).unwrap();
         let k = bench_params.degree;
         println!("---------------------- degree = {k} ------------------------------",);
         let rng = OsRng;

halo2-ecc/src/bn254/tests/mod.rs

@@ -1,20 +1,23 @@
 #![allow(non_snake_case)]
 use super::pairing::PairingChip;
 use super::*;
-use crate::halo2_proofs::{
-    dev::MockProver,
-    halo2curves::bn256::{pairing, Bn256, Fr, G1Affine},
-    plonk::*,
-    poly::commitment::ParamsProver,
-    poly::kzg::{
-        commitment::KZGCommitmentScheme,
-        multiopen::{ProverSHPLONK, VerifierSHPLONK},
-        strategy::SingleStrategy,
+use crate::{ecc::EccChip, fields::PrimeField};
+use crate::{
+    fields::FpStrategy,
+    halo2_proofs::{
+        dev::MockProver,
+        halo2curves::bn256::{pairing, Bn256, Fr, G1Affine},
+        plonk::*,
+        poly::commitment::ParamsProver,
+        poly::kzg::{
+            commitment::KZGCommitmentScheme,
+            multiopen::{ProverSHPLONK, VerifierSHPLONK},
+            strategy::SingleStrategy,
+        },
+        transcript::{Blake2bRead, Blake2bWrite, Challenge255},
+        transcript::{TranscriptReadBuffer, TranscriptWriterBuffer},
     },
-    transcript::{Blake2bRead, Blake2bWrite, Challenge255},
-    transcript::{TranscriptReadBuffer, TranscriptWriterBuffer},
 };
-use crate::{ecc::EccChip, fields::PrimeField};
 use ark_std::{end_timer, start_timer};
 use group::Curve;
 use halo2_base::utils::fe_to_biguint;
@@ -24,4 +27,20 @@ use std::io::Write;
 pub mod ec_add;
 pub mod fixed_base_msm;
 pub mod msm;
+pub mod msm_sum_infinity;
+pub mod msm_sum_infinity_fixed_base;
 pub mod pairing;
+
+#[derive(Clone, Copy, Debug, Serialize, Deserialize)]
+struct MSMCircuitParams {
+    strategy: FpStrategy,
+    degree: u32,
+    num_advice: usize,
+    num_lookup_advice: usize,
+    num_fixed: usize,
+    lookup_bits: usize,
+    limb_bits: usize,
+    num_limbs: usize,
+    batch_size: usize,
+    window_bits: usize,
+}

halo2-ecc/src/bn254/tests/msm_sum_infinity.rs

@@ -0,0 +1,183 @@
+use ff::PrimeField;
+use halo2_base::gates::{
+    builder::{
+        CircuitBuilderStage, GateThreadBuilder, MultiPhaseThreadBreakPoints, RangeCircuitBuilder,
+    },
+    RangeChip,
+};
+use rand_core::OsRng;
+use std::fs::File;
+
+use super::*;
+
+fn msm_test(
+    builder: &mut GateThreadBuilder<Fr>,
+    params: MSMCircuitParams,
+    bases: Vec<G1Affine>,
+    scalars: Vec<Fr>,
+    window_bits: usize,
+) {
+    std::env::set_var("LOOKUP_BITS", params.lookup_bits.to_string());
+    let range = RangeChip::<Fr>::default(params.lookup_bits);
+    let fp_chip = FpChip::<Fr>::new(&range, params.limb_bits, params.num_limbs);
+    let ecc_chip = EccChip::new(&fp_chip);
+
+    let ctx = builder.main(0);
+    let scalars_assigned =
+        scalars.iter().map(|scalar| vec![ctx.load_witness(*scalar)]).collect::<Vec<_>>();
+    let bases_assigned = bases
+        .iter()
+        .map(|base| ecc_chip.load_private_unchecked(ctx, (base.x, base.y)))
+        .collect::<Vec<_>>();
+
+    let msm = ecc_chip.variable_base_msm_in::<G1Affine>(
+        builder,
+        &bases_assigned,
+        scalars_assigned,
+        Fr::NUM_BITS as usize,
+        window_bits,
+        0,
+    );
+
+    let msm_answer = bases
+        .iter()
+        .zip(scalars.iter())
+        .map(|(base, scalar)| base * scalar)
+        .reduce(|a, b| a + b)
+        .unwrap()
+        .to_affine();
+
+    let msm_x = msm.x.value();
+    let msm_y = msm.y.value();
+    assert_eq!(msm_x, fe_to_biguint(&msm_answer.x));
+    assert_eq!(msm_y, fe_to_biguint(&msm_answer.y));
+}
+
+fn custom_msm_circuit(
+    params: MSMCircuitParams,
+    stage: CircuitBuilderStage,
+    break_points: Option<MultiPhaseThreadBreakPoints>,
+    bases: Vec<G1Affine>,
+    scalars: Vec<Fr>,
+) -> RangeCircuitBuilder<Fr> {
+    let k = params.degree as usize;
+    let mut builder = match stage {
+        CircuitBuilderStage::Mock => GateThreadBuilder::mock(),
+        CircuitBuilderStage::Prover => GateThreadBuilder::prover(),
+        CircuitBuilderStage::Keygen => GateThreadBuilder::keygen(),
+    };
+
+    let start0 = start_timer!(|| format!("Witness generation for circuit in {stage:?} stage"));
+    msm_test(&mut builder, params, bases, scalars, params.window_bits);
+
+    let circuit = match stage {
+        CircuitBuilderStage::Mock => {
+            builder.config(k, Some(20));
+            RangeCircuitBuilder::mock(builder)
+        }
+        CircuitBuilderStage::Keygen => {
+            builder.config(k, Some(20));
+            RangeCircuitBuilder::keygen(builder)
+        }
+        CircuitBuilderStage::Prover => RangeCircuitBuilder::prover(builder, break_points.unwrap()),
+    };
+    end_timer!(start0);
+    circuit
+}
+
+#[test]
+fn test_msm1() {
+    let path = "configs/bn254/msm_circuit.config";
+    let mut params: MSMCircuitParams = serde_json::from_reader(
+        File::open(path).unwrap_or_else(|e| panic!("{path} does not exist: {e:?}")),
+    )
+    .unwrap();
+    params.batch_size = 3;
+
+    let random_point = G1Affine::random(OsRng);
+    let bases = vec![random_point, random_point, random_point];
+    let scalars = vec![Fr::one(), Fr::one(), -Fr::one() - Fr::one()];
+
+    let circuit = custom_msm_circuit(params, CircuitBuilderStage::Mock, None, bases, scalars);
+    MockProver::run(params.degree, &circuit, vec![]).unwrap().assert_satisfied();
+}
+
+#[test]
+fn test_msm2() {
+    let path = "configs/bn254/msm_circuit.config";
+    let mut params: MSMCircuitParams = serde_json::from_reader(
+        File::open(path).unwrap_or_else(|e| panic!("{path} does not exist: {e:?}")),
+    )
+    .unwrap();
+    params.batch_size = 3;
+
+    let random_point = G1Affine::random(OsRng);
+    let bases = vec![random_point, random_point, (random_point + random_point).to_affine()];
+    let scalars = vec![Fr::one(), Fr::one(), -Fr::one()];
+
+    let circuit = custom_msm_circuit(params, CircuitBuilderStage::Mock, None, bases, scalars);
+    MockProver::run(params.degree, &circuit, vec![]).unwrap().assert_satisfied();
+}
+
+#[test]
+fn test_msm3() {
+    let path = "configs/bn254/msm_circuit.config";
+    let mut params: MSMCircuitParams = serde_json::from_reader(
+        File::open(path).unwrap_or_else(|e| panic!("{path} does not exist: {e:?}")),
+    )
+    .unwrap();
+    params.batch_size = 4;
+
+    let random_point = G1Affine::random(OsRng);
+    let bases = vec![
+        random_point,
+        random_point,
+        random_point,
+        (random_point + random_point + random_point).to_affine(),
+    ];
+    let scalars = vec![Fr::one(), Fr::one(), Fr::one(), -Fr::one()];
+
+    let circuit = custom_msm_circuit(params, CircuitBuilderStage::Mock, None, bases, scalars);
+    MockProver::run(params.degree, &circuit, vec![]).unwrap().assert_satisfied();
+}
+
+#[test]
+fn test_msm4() {
+    let path = "configs/bn254/msm_circuit.config";
+    let mut params: MSMCircuitParams = serde_json::from_reader(
+        File::open(path).unwrap_or_else(|e| panic!("{path} does not exist: {e:?}")),
+    )
+    .unwrap();
+    params.batch_size = 4;
+
+    let generator_point = G1Affine::generator();
+    let bases = vec![
+        generator_point,
+        generator_point,
+        generator_point,
+        (generator_point + generator_point + generator_point).to_affine(),
+    ];
+    let scalars = vec![Fr::one(), Fr::one(), Fr::one(), -Fr::one()];
+
+    let circuit = custom_msm_circuit(params, CircuitBuilderStage::Mock, None, bases, scalars);
+    MockProver::run(params.degree, &circuit, vec![]).unwrap().assert_satisfied();
+}
+
+#[test]
+fn test_msm5() {
+    // Very similar example that does not add to infinity. It works fine.
+    let path = "configs/bn254/msm_circuit.config";
+    let mut params: MSMCircuitParams = serde_json::from_reader(
+        File::open(path).unwrap_or_else(|e| panic!("{path} does not exist: {e:?}")),
+    )
+    .unwrap();
+    params.batch_size = 4;
+
+    let random_point = G1Affine::random(OsRng);
+    let bases =
+        vec![random_point, random_point, random_point, (random_point + random_point).to_affine()];
+    let scalars = vec![-Fr::one(), -Fr::one(), Fr::one(), Fr::one()];
+
+    let circuit = custom_msm_circuit(params, CircuitBuilderStage::Mock, None, bases, scalars);
+    MockProver::run(params.degree, &circuit, vec![]).unwrap().assert_satisfied();
+}