From 9a0ea5b3e445a889a5586fa30faff5159cc98069 Mon Sep 17 00:00:00 2001 From: zhenfei Date: Thu, 20 Apr 2023 18:45:40 -0400 Subject: [PATCH 1/2] [fix] ecdsa sig_is_valid bug --- zkevm-circuits/src/tx_circuit/sign_verify.rs | 186 +++++++++---------- 1 file changed, 86 insertions(+), 100 deletions(-) diff --git a/zkevm-circuits/src/tx_circuit/sign_verify.rs b/zkevm-circuits/src/tx_circuit/sign_verify.rs index 07c11eb2e8..57e9dce725 100644 --- a/zkevm-circuits/src/tx_circuit/sign_verify.rs +++ b/zkevm-circuits/src/tx_circuit/sign_verify.rs @@ -1,11 +1,9 @@ //! Circuit to verify multiple ECDSA secp256k1 signatures. -// This module uses two different types of chip configurations -// - halo2-ecc's ecdsa chip, which is used -// - to prove the correctness of secp signatures -// - to compute the RLC in circuit -// - halo2wrong's main gate chip: this is used for keccak lookup table -// // +// This module uses halo2-ecc's ecdsa chip +// - to prove the correctness of secp signatures +// - to compute the RLC in circuit +// - to perform keccak lookup table // // Naming notes: // - *_be: Big-Endian bytes @@ -34,12 +32,17 @@ use halo2_ecc::{ FieldChip, }, }; +#[cfg(feature = "onephase")] +use halo2_proofs::plonk::FirstPhase; +#[cfg(not(feature = "onephase"))] +use halo2_proofs::plonk::SecondPhase; use halo2_proofs::{ circuit::{Cell, Layouter, Value}, halo2curves::secp256k1::{Fp, Fq, Secp256k1Affine}, - plonk::{ConstraintSystem, Error, Selector}, + plonk::{Advice, Column, ConstraintSystem, Error, Fixed, Selector}, poly::Rotation, }; + use itertools::Itertools; use keccak256::plain::Keccak; use log::error; @@ -138,45 +141,43 @@ impl Default for SignVerifyChip { /// SignVerify Configuration #[derive(Debug, Clone)] pub(crate) struct SignVerifyConfig { - // ECDSA + /// ECDSA ecdsa_config: FpChip, - // main_gate_config: MainGateConfig, - // Keccak + /// A fixed column to store F::one + // FIXME: it is pretty wasteful to allocate a new fixed column just + // to store a constant value F::one. + fixed_column: Column, + /// An advice column to store RLC witnesses + rlc_column: Column, + /// selector for keccak lookup table q_keccak: Selector, keccak_table: KeccakTable, } impl SignVerifyConfig { pub(crate) fn new(meta: &mut ConstraintSystem, keccak_table: KeccakTable) -> Self { + #[cfg(feature = "onephase")] + let num_advice = [calc_required_advices(MAX_NUM_SIG)]; + #[cfg(not(feature = "onephase"))] + // need an additional phase 2 column/basic gate to hold the witnesses during RLC + // computations + let num_advice = [calc_required_advices(MAX_NUM_SIG), 1]; + + #[cfg(feature = "onephase")] + log::info!("configuring ECDSA chip with single phase"); + #[cfg(not(feature = "onephase"))] + log::info!("configuring ECDSA chip with multiple phases"); + // halo2-ecc's ECDSA config // - // Create a new FpConfig chip for the following parameters - // {"strategy":"Simple","degree":14,"num_advice":36,"num_lookup_advice":6," - // num_fixed":1," lookup_bits":13,"limb_bits":88,"num_limbs":3} - // // - num_advice: 36 - // - num_lookup_advice: 6 + // - num_lookup_advice: 17 // - num_fixed: 1 // - lookup_bits: 13 // - limb_bits: 88 // - num_limbs: 3 // // TODO: make those parameters tunable from a config file - - #[cfg(feature = "onephase")] - // need one extra column to host the lookup cells and RLC cells - let num_advice = [calc_required_advices(MAX_NUM_SIG) + 1]; - #[cfg(not(feature = "onephase"))] - // need two phase 2 columns: - // - one to hold the witnesses for RLC computations - // - one to hold the actual results of RLCs, i.e., rlc_column - let num_advice = [calc_required_advices(MAX_NUM_SIG), 2]; - - #[cfg(feature = "onephase")] - log::info!("configuring ECDSA chip with single phase"); - #[cfg(not(feature = "onephase"))] - log::info!("configuring ECDSA chip with multiple phases"); - let ecdsa_config = FpConfig::configure( meta, FpStrategy::Simple, @@ -191,14 +192,18 @@ impl SignVerifyConfig { TOTAL_NUM_ROWS, // maximum k of the chip ); - // halo2wrong's main gate config + // we are not really using this instance column let instance = meta.instance_column(); meta.enable_equality(instance); - - #[cfg(not(feature = "onephase"))] - let rlc_column = ecdsa_config.range.gate.basic_gates[1].last().unwrap().value; + // we will need one fixed column to check if ecdsa is valid + let fixed_column = meta.fixed_column(); + meta.enable_equality(fixed_column); + // we need one phase 2 column to store RLC results #[cfg(feature = "onephase")] - let rlc_column = ecdsa_config.range.gate.basic_gates[0].last().unwrap().value; + let rlc_column = meta.advice_column_in(FirstPhase); + #[cfg(not(feature = "onephase"))] + let rlc_column = meta.advice_column_in(SecondPhase); + meta.enable_equality(rlc_column); // Ref. spec SignVerifyChip 1. Verify that keccak(pub_key_bytes) = pub_key_hash // by keccak table lookup, where pub_key_bytes is built from the pub_key @@ -210,7 +215,7 @@ impl SignVerifyConfig { // msg_hash and signature which is not constrained to match msg_hash_rlc nor // the address. // Layout: - // | q_keccak | rlc | + // | q_keccak | rlc | // | -------- | --------------- | // | 1 | is_address_zero | // | | pk_rlc | @@ -241,6 +246,8 @@ impl SignVerifyConfig { ecdsa_config, keccak_table, q_keccak, + fixed_column, + rlc_column, } } } @@ -366,7 +373,6 @@ impl SignVerifyChip { // build ecc chip from Fp chip let ecc_chip = EccChip::>::construct(ecdsa_chip.clone()); // build Fq chip from Fp chip - // TODO: pass the parameters let fq_chip = FqChip::construct(ecdsa_chip.range.clone(), 88, 3, modulus::()); log::trace!("r: {:?}", sig_r); @@ -425,22 +431,10 @@ impl SignVerifyChip { // | | pk_hash_rlc | config.q_keccak.enable(&mut ctx.region, *offset)?; - // this is a phase 2 column if "onephase" is not enabled - #[cfg(not(feature = "onephase"))] - let rlc_column = config.ecdsa_config.range.gate.basic_gates[1] - .last() - .unwrap() - .value; - #[cfg(feature = "onephase")] - let rlc_column = config.ecdsa_config.range.gate.basic_gates[0] - .last() - .unwrap() - .value; - // is_address_zero let tmp_cell = ctx.region.assign_advice( || "is_address_zero", - rlc_column, + config.rlc_column, *offset, || is_address_zero.value, )?; @@ -448,15 +442,18 @@ impl SignVerifyChip { .constrain_equal(is_address_zero.cell, tmp_cell.cell())?; // pk_rlc - let tmp_cell = - ctx.region - .assign_advice(|| "pk_rlc", rlc_column, *offset + 1, || pk_rlc.value)?; + let tmp_cell = ctx.region.assign_advice( + || "pk_rlc", + config.rlc_column, + *offset + 1, + || pk_rlc.value, + )?; ctx.region.constrain_equal(pk_rlc.cell, tmp_cell.cell())?; // pk_hash_rlc let tmp_cell = ctx.region.assign_advice( || "pk_hash_rlc", - rlc_column, + config.rlc_column, *offset + 2, || pk_hash_rlc.value, )?; @@ -719,12 +716,12 @@ impl SignVerifyChip { let mut first_pass = SKIP_FIRST_PASS; let ecdsa_chip = &config.ecdsa_config; - let (deferred_keccak_check, assigned_sig_verifs) = layouter.assign_region( + let assigned_sig_verifs = layouter.assign_region( || "ecdsa chip verification", |region| { if first_pass { first_pass = false; - return Ok((vec![], vec![])); + return Ok(vec![]); } let mut ctx = ecdsa_chip.new_context(region); @@ -784,42 +781,17 @@ impl SignVerifyChip { &e.sig_is_valid, )?; assigned_sig_verifs.push(assigned_sig_verif); - deferred_keccak_check.push([ - AssignedValueNoTimer::from(to_be_keccak_checked[0].clone()), - AssignedValueNoTimer::from(to_be_keccak_checked[1].clone()), - AssignedValueNoTimer::from(to_be_keccak_checked[2].clone()), - ]); - } - - // IMPORTANT: this assigns all constants to the fixed columns - // IMPORTANT: this copies cells to the lookup advice column to perform range - // check lookups - // This is not optional. - let lookup_cells = ecdsa_chip.finalize(&mut ctx); - log::info!("total number of lookup cells: {}", lookup_cells); - - for sig_verif in assigned_sig_verifs.iter() { - config.ecdsa_config.range.gate.assert_equal( - &mut ctx, - QuantumCell::Existing(&sig_verif.sig_is_valid.clone().into()), - QuantumCell::Constant(F::one()), + deferred_keccak_check.push( + to_be_keccak_checked ); } - ctx.print_stats(&["Range"]); - Ok((deferred_keccak_check, assigned_sig_verifs)) - }, - )?; - layouter.assign_region( - || "keccak lookup", - |region| { - let mut ctx = ecdsa_chip.new_context(region); + // ================================================ + // step 4: deferred keccak checks + // ================================================ let mut offset = 0; for e in deferred_keccak_check.iter() { let [is_address_zero, pk_rlc, pk_hash_rlc] = e; - let is_address_zero = AssignedValue::from(is_address_zero); - let pk_rlc = AssignedValue::from(pk_rlc); - let pk_hash_rlc = AssignedValue::from(pk_hash_rlc); self.enable_keccak_lookup( config, &mut ctx, @@ -829,7 +801,16 @@ impl SignVerifyChip { &pk_hash_rlc, )?; } - Ok(()) + + // IMPORTANT: this assigns all constants to the fixed columns + // IMPORTANT: this copies cells to the lookup advice column to perform range + // check lookups + // This is not optional. + let lookup_cells = ecdsa_chip.finalize(&mut ctx); + log::info!("total number of lookup cells: {}", lookup_cells); + + ctx.print_stats(&["Range"]); + Ok(assigned_sig_verifs) }, )?; @@ -944,24 +925,29 @@ impl SignVerifyChip { pub(crate) fn assert_sig_is_valid( &self, - _config: &SignVerifyConfig, + config: &SignVerifyConfig, layouter: &mut impl Layouter, sig_verifs: &[AssignedSignatureVerify], ) -> Result<(), Error> { - // let flex_gate_chip = &config.ecdsa_config.range.gate; - for (i, s) in sig_verifs.iter().enumerate() { - log::trace!( - "checking {}-th signature is valid: {:?}", - i, - s.sig_is_valid.value - ); - } - layouter.assign_region( || "ecdsa chip verification", - |_region| { - // not doing anything as the validity is already been checked - // within assign() function + |mut region| { + let one = region.assign_fixed( + || "one", + config.fixed_column, + 0, + || Value::known(F::one()), + )?; + + for (i, sig_verif) in sig_verifs.iter().enumerate() { + log::trace!( + "checking {}-th signature is valid: {:?}", + i, + sig_verif.sig_is_valid.value + ); + + region.constrain_equal(sig_verif.sig_is_valid.cell, one.cell())?; + } Ok(()) }, From a1f6bb5231abaf29b588d9ec35d0bb1f3d9024cc Mon Sep 17 00:00:00 2001 From: zhenfei Date: Fri, 21 Apr 2023 09:28:29 -0400 Subject: [PATCH 2/2] [chore] cargo fmt; fix linter --- zkevm-circuits/src/tx_circuit/sign_verify.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/zkevm-circuits/src/tx_circuit/sign_verify.rs b/zkevm-circuits/src/tx_circuit/sign_verify.rs index 57e9dce725..8abff4c9c9 100644 --- a/zkevm-circuits/src/tx_circuit/sign_verify.rs +++ b/zkevm-circuits/src/tx_circuit/sign_verify.rs @@ -781,9 +781,7 @@ impl SignVerifyChip { &e.sig_is_valid, )?; assigned_sig_verifs.push(assigned_sig_verif); - deferred_keccak_check.push( - to_be_keccak_checked - ); + deferred_keccak_check.push(to_be_keccak_checked); } // ================================================ @@ -796,9 +794,9 @@ impl SignVerifyChip { config, &mut ctx, &mut offset, - &is_address_zero, - &pk_rlc, - &pk_hash_rlc, + is_address_zero, + pk_rlc, + pk_hash_rlc, )?; }