Skip to content
This repository was archived by the owner on Apr 18, 2025. It is now read-only.

Commit 9a0ea5b

Browse files
zhenfeizhangzhenfeizhang
committed
[fix] ecdsa sig_is_valid bug
1 parent 1ea9b59 commit 9a0ea5b

File tree

1 file changed

+86
-100
lines changed

1 file changed

+86
-100
lines changed

‎zkevm-circuits/src/tx_circuit/sign_verify.rs

+86-100
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
//! Circuit to verify multiple ECDSA secp256k1 signatures.
2-
// This module uses two different types of chip configurations
3-
// - halo2-ecc's ecdsa chip, which is used
4-
// - to prove the correctness of secp signatures
5-
// - to compute the RLC in circuit
6-
// - halo2wrong's main gate chip: this is used for keccak lookup table
7-
//
82
//
3+
// This module uses halo2-ecc's ecdsa chip
4+
// - to prove the correctness of secp signatures
5+
// - to compute the RLC in circuit
6+
// - to perform keccak lookup table
97
//
108
// Naming notes:
119
// - *_be: Big-Endian bytes
@@ -34,12 +32,17 @@ use halo2_ecc::{
3432
FieldChip,
3533
},
3634
};
35+
#[cfg(feature = "onephase")]
36+
use halo2_proofs::plonk::FirstPhase;
37+
#[cfg(not(feature = "onephase"))]
38+
use halo2_proofs::plonk::SecondPhase;
3739
use halo2_proofs::{
3840
circuit::{Cell, Layouter, Value},
3941
halo2curves::secp256k1::{Fp, Fq, Secp256k1Affine},
40-
plonk::{ConstraintSystem, Error, Selector},
42+
plonk::{Advice, Column, ConstraintSystem, Error, Fixed, Selector},
4143
poly::Rotation,
4244
};
45+
4346
use itertools::Itertools;
4447
use keccak256::plain::Keccak;
4548
use log::error;
@@ -138,45 +141,43 @@ impl<F: Field> Default for SignVerifyChip<F> {
138141
/// SignVerify Configuration
139142
#[derive(Debug, Clone)]
140143
pub(crate) struct SignVerifyConfig<F: Field> {
141-
// ECDSA
144+
/// ECDSA
142145
ecdsa_config: FpChip<F>,
143-
// main_gate_config: MainGateConfig,
144-
// Keccak
146+
/// A fixed column to store F::one
147+
// FIXME: it is pretty wasteful to allocate a new fixed column just
148+
// to store a constant value F::one.
149+
fixed_column: Column<Fixed>,
150+
/// An advice column to store RLC witnesses
151+
rlc_column: Column<Advice>,
152+
/// selector for keccak lookup table
145153
q_keccak: Selector,
146154
keccak_table: KeccakTable,
147155
}
148156

149157
impl<F: Field> SignVerifyConfig<F> {
150158
pub(crate) fn new(meta: &mut ConstraintSystem<F>, keccak_table: KeccakTable) -> Self {
159+
#[cfg(feature = "onephase")]
160+
let num_advice = [calc_required_advices(MAX_NUM_SIG)];
161+
#[cfg(not(feature = "onephase"))]
162+
// need an additional phase 2 column/basic gate to hold the witnesses during RLC
163+
// computations
164+
let num_advice = [calc_required_advices(MAX_NUM_SIG), 1];
165+
166+
#[cfg(feature = "onephase")]
167+
log::info!("configuring ECDSA chip with single phase");
168+
#[cfg(not(feature = "onephase"))]
169+
log::info!("configuring ECDSA chip with multiple phases");
170+
151171
// halo2-ecc's ECDSA config
152172
//
153-
// Create a new FpConfig chip for the following parameters
154-
// {"strategy":"Simple","degree":14,"num_advice":36,"num_lookup_advice":6,"
155-
// num_fixed":1," lookup_bits":13,"limb_bits":88,"num_limbs":3}
156-
//
157173
// - num_advice: 36
158-
// - num_lookup_advice: 6
174+
// - num_lookup_advice: 17
159175
// - num_fixed: 1
160176
// - lookup_bits: 13
161177
// - limb_bits: 88
162178
// - num_limbs: 3
163179
//
164180
// TODO: make those parameters tunable from a config file
165-
166-
#[cfg(feature = "onephase")]
167-
// need one extra column to host the lookup cells and RLC cells
168-
let num_advice = [calc_required_advices(MAX_NUM_SIG) + 1];
169-
#[cfg(not(feature = "onephase"))]
170-
// need two phase 2 columns:
171-
// - one to hold the witnesses for RLC computations
172-
// - one to hold the actual results of RLCs, i.e., rlc_column
173-
let num_advice = [calc_required_advices(MAX_NUM_SIG), 2];
174-
175-
#[cfg(feature = "onephase")]
176-
log::info!("configuring ECDSA chip with single phase");
177-
#[cfg(not(feature = "onephase"))]
178-
log::info!("configuring ECDSA chip with multiple phases");
179-
180181
let ecdsa_config = FpConfig::configure(
181182
meta,
182183
FpStrategy::Simple,
@@ -191,14 +192,18 @@ impl<F: Field> SignVerifyConfig<F> {
191192
TOTAL_NUM_ROWS, // maximum k of the chip
192193
);
193194

194-
// halo2wrong's main gate config
195+
// we are not really using this instance column
195196
let instance = meta.instance_column();
196197
meta.enable_equality(instance);
197-
198-
#[cfg(not(feature = "onephase"))]
199-
let rlc_column = ecdsa_config.range.gate.basic_gates[1].last().unwrap().value;
198+
// we will need one fixed column to check if ecdsa is valid
199+
let fixed_column = meta.fixed_column();
200+
meta.enable_equality(fixed_column);
201+
// we need one phase 2 column to store RLC results
200202
#[cfg(feature = "onephase")]
201-
let rlc_column = ecdsa_config.range.gate.basic_gates[0].last().unwrap().value;
203+
let rlc_column = meta.advice_column_in(FirstPhase);
204+
#[cfg(not(feature = "onephase"))]
205+
let rlc_column = meta.advice_column_in(SecondPhase);
206+
meta.enable_equality(rlc_column);
202207

203208
// Ref. spec SignVerifyChip 1. Verify that keccak(pub_key_bytes) = pub_key_hash
204209
// by keccak table lookup, where pub_key_bytes is built from the pub_key
@@ -210,7 +215,7 @@ impl<F: Field> SignVerifyConfig<F> {
210215
// msg_hash and signature which is not constrained to match msg_hash_rlc nor
211216
// the address.
212217
// Layout:
213-
// | q_keccak | rlc |
218+
// | q_keccak | rlc |
214219
// | -------- | --------------- |
215220
// | 1 | is_address_zero |
216221
// | | pk_rlc |
@@ -241,6 +246,8 @@ impl<F: Field> SignVerifyConfig<F> {
241246
ecdsa_config,
242247
keccak_table,
243248
q_keccak,
249+
fixed_column,
250+
rlc_column,
244251
}
245252
}
246253
}
@@ -366,7 +373,6 @@ impl<F: Field> SignVerifyChip<F> {
366373
// build ecc chip from Fp chip
367374
let ecc_chip = EccChip::<F, FpChip<F>>::construct(ecdsa_chip.clone());
368375
// build Fq chip from Fp chip
369-
// TODO: pass the parameters
370376
let fq_chip = FqChip::construct(ecdsa_chip.range.clone(), 88, 3, modulus::<Fq>());
371377

372378
log::trace!("r: {:?}", sig_r);
@@ -425,38 +431,29 @@ impl<F: Field> SignVerifyChip<F> {
425431
// | | pk_hash_rlc |
426432
config.q_keccak.enable(&mut ctx.region, *offset)?;
427433

428-
// this is a phase 2 column if "onephase" is not enabled
429-
#[cfg(not(feature = "onephase"))]
430-
let rlc_column = config.ecdsa_config.range.gate.basic_gates[1]
431-
.last()
432-
.unwrap()
433-
.value;
434-
#[cfg(feature = "onephase")]
435-
let rlc_column = config.ecdsa_config.range.gate.basic_gates[0]
436-
.last()
437-
.unwrap()
438-
.value;
439-
440434
// is_address_zero
441435
let tmp_cell = ctx.region.assign_advice(
442436
|| "is_address_zero",
443-
rlc_column,
437+
config.rlc_column,
444438
*offset,
445439
|| is_address_zero.value,
446440
)?;
447441
ctx.region
448442
.constrain_equal(is_address_zero.cell, tmp_cell.cell())?;
449443

450444
// pk_rlc
451-
let tmp_cell =
452-
ctx.region
453-
.assign_advice(|| "pk_rlc", rlc_column, *offset + 1, || pk_rlc.value)?;
445+
let tmp_cell = ctx.region.assign_advice(
446+
|| "pk_rlc",
447+
config.rlc_column,
448+
*offset + 1,
449+
|| pk_rlc.value,
450+
)?;
454451
ctx.region.constrain_equal(pk_rlc.cell, tmp_cell.cell())?;
455452

456453
// pk_hash_rlc
457454
let tmp_cell = ctx.region.assign_advice(
458455
|| "pk_hash_rlc",
459-
rlc_column,
456+
config.rlc_column,
460457
*offset + 2,
461458
|| pk_hash_rlc.value,
462459
)?;
@@ -719,12 +716,12 @@ impl<F: Field> SignVerifyChip<F> {
719716
let mut first_pass = SKIP_FIRST_PASS;
720717
let ecdsa_chip = &config.ecdsa_config;
721718

722-
let (deferred_keccak_check, assigned_sig_verifs) = layouter.assign_region(
719+
let assigned_sig_verifs = layouter.assign_region(
723720
|| "ecdsa chip verification",
724721
|region| {
725722
if first_pass {
726723
first_pass = false;
727-
return Ok((vec![], vec![]));
724+
return Ok(vec![]);
728725
}
729726

730727
let mut ctx = ecdsa_chip.new_context(region);
@@ -784,42 +781,17 @@ impl<F: Field> SignVerifyChip<F> {
784781
&e.sig_is_valid,
785782
)?;
786783
assigned_sig_verifs.push(assigned_sig_verif);
787-
deferred_keccak_check.push([
788-
AssignedValueNoTimer::from(to_be_keccak_checked[0].clone()),
789-
AssignedValueNoTimer::from(to_be_keccak_checked[1].clone()),
790-
AssignedValueNoTimer::from(to_be_keccak_checked[2].clone()),
791-
]);
792-
}
793-
794-
// IMPORTANT: this assigns all constants to the fixed columns
795-
// IMPORTANT: this copies cells to the lookup advice column to perform range
796-
// check lookups
797-
// This is not optional.
798-
let lookup_cells = ecdsa_chip.finalize(&mut ctx);
799-
log::info!("total number of lookup cells: {}", lookup_cells);
800-
801-
for sig_verif in assigned_sig_verifs.iter() {
802-
config.ecdsa_config.range.gate.assert_equal(
803-
&mut ctx,
804-
QuantumCell::Existing(&sig_verif.sig_is_valid.clone().into()),
805-
QuantumCell::Constant(F::one()),
784+
deferred_keccak_check.push(
785+
to_be_keccak_checked
806786
);
807787
}
808-
ctx.print_stats(&["Range"]);
809-
Ok((deferred_keccak_check, assigned_sig_verifs))
810-
},
811-
)?;
812788

813-
layouter.assign_region(
814-
|| "keccak lookup",
815-
|region| {
816-
let mut ctx = ecdsa_chip.new_context(region);
789+
// ================================================
790+
// step 4: deferred keccak checks
791+
// ================================================
817792
let mut offset = 0;
818793
for e in deferred_keccak_check.iter() {
819794
let [is_address_zero, pk_rlc, pk_hash_rlc] = e;
820-
let is_address_zero = AssignedValue::from(is_address_zero);
821-
let pk_rlc = AssignedValue::from(pk_rlc);
822-
let pk_hash_rlc = AssignedValue::from(pk_hash_rlc);
823795
self.enable_keccak_lookup(
824796
config,
825797
&mut ctx,
@@ -829,7 +801,16 @@ impl<F: Field> SignVerifyChip<F> {
829801
&pk_hash_rlc,
830802
)?;
831803
}
832-
Ok(())
804+
805+
// IMPORTANT: this assigns all constants to the fixed columns
806+
// IMPORTANT: this copies cells to the lookup advice column to perform range
807+
// check lookups
808+
// This is not optional.
809+
let lookup_cells = ecdsa_chip.finalize(&mut ctx);
810+
log::info!("total number of lookup cells: {}", lookup_cells);
811+
812+
ctx.print_stats(&["Range"]);
813+
Ok(assigned_sig_verifs)
833814
},
834815
)?;
835816

@@ -944,24 +925,29 @@ impl<F: Field> SignVerifyChip<F> {
944925

945926
pub(crate) fn assert_sig_is_valid(
946927
&self,
947-
_config: &SignVerifyConfig<F>,
928+
config: &SignVerifyConfig<F>,
948929
layouter: &mut impl Layouter<F>,
949930
sig_verifs: &[AssignedSignatureVerify<F>],
950931
) -> Result<(), Error> {
951-
// let flex_gate_chip = &config.ecdsa_config.range.gate;
952-
for (i, s) in sig_verifs.iter().enumerate() {
953-
log::trace!(
954-
"checking {}-th signature is valid: {:?}",
955-
i,
956-
s.sig_is_valid.value
957-
);
958-
}
959-
960932
layouter.assign_region(
961933
|| "ecdsa chip verification",
962-
|_region| {
963-
// not doing anything as the validity is already been checked
964-
// within assign() function
934+
|mut region| {
935+
let one = region.assign_fixed(
936+
|| "one",
937+
config.fixed_column,
938+
0,
939+
|| Value::known(F::one()),
940+
)?;
941+
942+
for (i, sig_verif) in sig_verifs.iter().enumerate() {
943+
log::trace!(
944+
"checking {}-th signature is valid: {:?}",
945+
i,
946+
sig_verif.sig_is_valid.value
947+
);
948+
949+
region.constrain_equal(sig_verif.sig_is_valid.cell, one.cell())?;
950+
}
965951

966952
Ok(())
967953
},

0 commit comments

Comments
 (0)