refactor sig verify subcircuit (#538)

* xx

* fix: skip first 12 bytes for eth address

* chore: fmt

* feat: add sig table to evm circuit

* impl SubCircuit trait for TxCircuitTester

* fill r s v in test_util sig

* lint

* lint

* clean code

* chore: refactoring witness assignment

* [fix] soundness bug in ctr int (#543)

* [fix] soundness bug in ctr int

* [fix] clippy

* move assign function to make git diff easier

* Feat: assert v is odd (#546)

* [fix] soundness bug in ctr int

* [fix] clippy

* [feat] implement v_is_odd check

* [fix] extract v from affine coord

* [chore] cargo clippy; fmt

---------

Co-authored-by: Zhang Zhuo <[email protected]>

* address some comments

* fix instance col

* fix v permutation

* sync up changes with halo2-lib; remove lifetimers (#554)

* [fix] sync up api changes with halo2-lib: removing lifetimes

* [minor] update cargo lock

* [chore] clippy; update cargo lock

* Update sig_circuit.rs

---------

Co-authored-by: Zhang Zhuo <[email protected]>

* lint

* add lookup to sig table in tx circuit

* add lookup to sig table in tx_circuit

* remove get_num_rows_required

* Update tx_circuit.rs

* fix sig circuit assignment

* fix: sign_data of padding tx

* fmt

---------

Co-authored-by: Rohit Narurkar <[email protected]>
Co-authored-by: zhenfei <[email protected]>
Co-authored-by: kunxian xia <[email protected]>

Author: 4 people

Cargo.lock

@@ -7,7 +7,7 @@ use crate::{
     operation::{OperationContainer, RWCounter},
     Error,
 };
-use eth_types::{Address, Hash, ToWord, Word};
+use eth_types::{sign_types::SignData, Address, Hash, ToWord, Word};
 use std::collections::{BTreeMap, HashMap};
 
 /// Context of a [`Block`] which can mutate in a [`Transaction`].
@@ -153,6 +153,8 @@ pub struct Block {
     pub code: HashMap<Hash, Vec<u8>>,
     /// Inputs to the SHA3 opcode
     pub sha3_inputs: Vec<Vec<u8>>,
+    /// IO to/from the precompile Ecrecover calls.
+    pub ecrecover_events: Vec<SignData>,
     /// Block-wise steps
     pub block_steps: BlockSteps,
     /// Exponentiation events in the block.
@@ -246,4 +248,8 @@ impl Block {
     pub fn add_exp_event(&mut self, event: ExpEvent) {
         self.exp_events.push(event);
     }
+    /// Push an ecrecover event to the block.
+    pub fn add_ecrecover_event(&mut self, event: SignData) {
+        self.ecrecover_events.push(event);
+    }
 }

bus-mapping/src/circuit_input_builder/block.rs

@@ -24,7 +24,9 @@ mod tests {
     use rand::SeedableRng;
     use rand_chacha::ChaCha20Rng;
     use std::env::var;
-    use zkevm_circuits::{tx_circuit::TxCircuit, util::SubCircuit, witness::block_convert};
+    use zkevm_circuits::{
+        tx_circuit::TestTxCircuit as TxCircuit, util::SubCircuit, witness::block_convert,
+    };
 
     use bus_mapping::rpc::GethClient;
     use ethers::providers::Http;

circuit-benchmarks/src/tx_circuit.rs

@@ -333,7 +333,7 @@ impl Transaction {
             .try_into()
             .expect("hash length isn't 32 bytes");
         let v = self.tx_type.get_recovery_id(self.v);
-        let pk = recover_pk(v, &self.r, &self.s, &msg_hash)?;
+        let pk = recover_pk(v as u8, &self.r, &self.s, &msg_hash)?;
         // msg_hash = msg_hash % q
         let msg_hash = BigUint::from_bytes_be(msg_hash.as_slice());
         let msg_hash = msg_hash.mod_floor(&*SECP256K1_Q);
@@ -343,7 +343,7 @@ impl Transaction {
             libsecp256k1::Error::InvalidMessage,
         )?;
         Ok(SignData {
-            signature: (sig_r, sig_s),
+            signature: (sig_r, sig_s, v as u8),
             pk,
             msg,
             msg_hash,

eth-types/src/geth_types.rs

@@ -1,7 +1,10 @@
 //! secp256k1 signature types and helper functions.
 
-use crate::{ToBigEndian, Word};
-use ethers_core::types::Bytes;
+use crate::{ToBigEndian, ToWord, Word};
+use ethers_core::{
+    types::{Address, Bytes},
+    utils::keccak256,
+};
 use halo2_proofs::{
     arithmetic::{CurveAffine, FieldExt},
     halo2curves::{
@@ -23,32 +26,33 @@ pub fn sign(
     randomness: secp256k1::Fq,
     sk: secp256k1::Fq,
     msg_hash: secp256k1::Fq,
-) -> (secp256k1::Fq, secp256k1::Fq) {
+) -> (secp256k1::Fq, secp256k1::Fq, u8) {
     let randomness_inv =
         Option::<secp256k1::Fq>::from(randomness.invert()).expect("cannot invert randomness");
     let generator = Secp256k1Affine::generator();
     let sig_point = generator * randomness;
+    let sig_v: bool = sig_point.to_affine().y.is_odd().into();
+
     let x = *Option::<Coordinates<_>>::from(sig_point.to_affine().coordinates())
         .expect("point is the identity")
         .x();
 
-    let x_repr = &mut vec![0u8; 32];
-    x_repr.copy_from_slice(x.to_bytes().as_slice());
-
     let mut x_bytes = [0u8; 64];
-    x_bytes[..32].copy_from_slice(&x_repr[..]);
+    x_bytes[..32].copy_from_slice(&x.to_bytes());
 
     let sig_r = secp256k1::Fq::from_bytes_wide(&x_bytes); // get x cordinate (E::Base) on E::Scalar
+
     let sig_s = randomness_inv * (msg_hash + sig_r * sk);
-    (sig_r, sig_s)
+    (sig_r, sig_s, u8::from(sig_v))
 }
 
 /// Signature data required by the SignVerify Chip as input to verify a
 /// signature.
 #[derive(Clone, Debug)]
 pub struct SignData {
-    /// Secp256k1 signature point
-    pub signature: (secp256k1::Fq, secp256k1::Fq),
+    /// Secp256k1 signature point (r, s, v)
+    /// v must be 0 or 1
+    pub signature: (secp256k1::Fq, secp256k1::Fq, u8),
     /// Secp256k1 public key
     pub pk: Secp256k1Affine,
     /// Message being hashed before signing.
@@ -57,7 +61,20 @@ pub struct SignData {
     pub msg_hash: secp256k1::Fq,
 }
 
+impl SignData {
+    /// Recover address of the signature
+    pub fn get_addr(&self) -> Word {
+        let pk_le = pk_bytes_le(&self.pk);
+        let pk_be = pk_bytes_swap_endianness(&pk_le);
+        let pk_hash = keccak256(pk_be);
+        let mut addr_bytes = [0u8; 20];
+        addr_bytes.copy_from_slice(&pk_hash[12..]);
+        Address::from(addr_bytes).to_word()
+    }
+}
+
 lazy_static! {
+    // FIXME: use Transaction::dummy().sign_data() instead when we merged the develop branch
     static ref SIGN_DATA_DEFAULT: SignData = {
         let generator = Secp256k1Affine::generator();
         let sk = secp256k1::Fq::one();
@@ -80,10 +97,10 @@ lazy_static! {
             .expect("hash length isn't 32 bytes");
         let msg_hash = secp256k1::Fq::from_bytes(&msg_hash).unwrap();
         let randomness = secp256k1::Fq::one();
-        let (sig_r, sig_s) = sign(randomness, sk, msg_hash);
+        let (sig_r, sig_s, v) = sign(randomness, sk, msg_hash);
 
         SignData {
-            signature: (sig_r, sig_s),
+            signature: (sig_r, sig_s, v),
             pk,
             msg: msg.into(),
             msg_hash,
@@ -92,12 +109,12 @@ lazy_static! {
 }
 
 impl Default for SignData {
+    // Hardcoded valid signature corresponding to a hardcoded private key and
+    // message hash generated from "nothing up my sleeve" values to make the
+    // ECDSA chip pass the constraints, to be use for padding signature
+    // verifications (where the constraints pass, but we don't care about the
+    // message hash and public key).
     fn default() -> Self {
-        // Hardcoded valid signature corresponding to a hardcoded private key and
-        // message hash generated from "nothing up my sleeve" values to make the
-        // ECDSA chip pass the constraints, to be use for padding signature
-        // verifications (where the constraints pass, but we don't care about the
-        // message hash and public key).
         SIGN_DATA_DEFAULT.clone()
     }
 }

eth-types/src/sign_types.rs

@@ -15,7 +15,7 @@ use zkevm_circuits::{
     rlp_circuit_fsm::RlpCircuit,
     state_circuit::StateCircuit,
     super_circuit::SuperCircuit,
-    tx_circuit::TxCircuit,
+    tx_circuit::TestTxCircuit as TxCircuit,
     util::{Challenges, SubCircuit},
     witness,
     witness::Transaction,

integration-tests/tests/mainnet.rs

@@ -12,7 +12,7 @@ use ethers_core::{
 use ethers_signers::{LocalWallet, Signer};
 use lazy_static::lazy_static;
 use rand::SeedableRng;
-use rand_chacha::ChaCha20Rng;
+use rand_chacha::{rand_core::OsRng, ChaCha20Rng};
 
 lazy_static! {
     /// Collection of correctly hashed and signed Transactions which can be used to test circuits or opcodes that have to check integrity of the Tx itself.
@@ -171,7 +171,8 @@ impl Default for MockTransaction {
             block_hash: Hash::zero(),
             block_number: U64::zero(),
             transaction_index: U64::zero(),
-            from: AddrOrWallet::Addr(MOCK_ACCOUNTS[0]),
+            //from: AddrOrWallet::Addr(MOCK_ACCOUNTS[0]),
+            from: AddrOrWallet::random(&mut OsRng),
             to: None,
             value: Word::zero(),
             gas_price: *MOCK_GASPRICE,

mock/src/transaction.rs

@@ -29,17 +29,17 @@ keccak256 = { path = "../keccak256"}
 log = "0.4"
 env_logger = "0.9"
 
-halo2-base = { git = "https://github.com/scroll-tech/halo2-lib", branch = "halo2-ecc-snark-verifier-0323", default-features=false, features=["halo2-pse","display"] }
-halo2-ecc = { git = "https://github.com/scroll-tech/halo2-lib", branch = "halo2-ecc-snark-verifier-0323", default-features=false, features=["halo2-pse","display"] }
+halo2-base = { git = "https://github.com/scroll-tech/halo2-lib", branch = "develop", default-features=false, features=["halo2-pse","display"] }
+halo2-ecc = { git = "https://github.com/scroll-tech/halo2-lib", branch = "develop", default-features=false, features=["halo2-pse","display"] }
 
 maingate = { git = "https://github.com/privacy-scaling-explorations/halo2wrong", tag = "v2023_02_02" }
 
 libsecp256k1 = "0.7"
 num-bigint = { version = "0.4" }
 subtle = "2.4"
 rand_chacha = "0.3"
-snark-verifier = { git = "https://github.com/scroll-tech/snark-verifier", branch = "halo2-ecc-snark-verifier-0323" }
-snark-verifier-sdk = { git = "https://github.com/scroll-tech/snark-verifier", branch = "halo2-ecc-snark-verifier-0323", default-features=false, features = ["loader_halo2", "loader_evm", "halo2-pse"] }
+snark-verifier = { git = "https://github.com/scroll-tech/snark-verifier", branch = "develop" }
+snark-verifier-sdk = { git = "https://github.com/scroll-tech/snark-verifier", branch = "develop", default-features=false, features = ["loader_halo2", "loader_evm", "halo2-pse"] }
 hex = "0.4.3"
 rayon = "1.5"
 once_cell = "1.17.0"

zkevm-circuits/Cargo.toml

@@ -21,7 +21,8 @@ pub use crate::witness;
 use crate::{
     evm_circuit::param::{MAX_STEP_HEIGHT, STEP_STATE_HEIGHT},
     table::{
-        BlockTable, BytecodeTable, CopyTable, ExpTable, KeccakTable, LookupTable, RwTable, TxTable,
+        BlockTable, BytecodeTable, CopyTable, ExpTable, KeccakTable, LookupTable, RwTable,
+        SigTable, TxTable,
     },
     util::{SubCircuit, SubCircuitConfig},
 };
@@ -47,6 +48,7 @@ pub struct EvmCircuitConfig<F> {
     copy_table: CopyTable,
     keccak_table: KeccakTable,
     exp_table: ExpTable,
+    sig_table: SigTable,
 }
 
 /// Circuit configuration arguments
@@ -67,6 +69,8 @@ pub struct EvmCircuitConfigArgs<F: Field> {
     pub keccak_table: KeccakTable,
     /// ExpTable
     pub exp_table: ExpTable,
+    /// SigTable
+    pub sig_table: SigTable,
 }
 
 /// Circuit exported cells after synthesis, used for subcircuit
@@ -92,6 +96,7 @@ impl<F: Field> SubCircuitConfig<F> for EvmCircuitConfig<F> {
             copy_table,
             keccak_table,
             exp_table,
+            sig_table,
         }: Self::ConfigArgs,
     ) -> Self {
         let fixed_table = [(); 4].map(|_| meta.fixed_column());
@@ -108,6 +113,7 @@ impl<F: Field> SubCircuitConfig<F> for EvmCircuitConfig<F> {
             &copy_table,
             &keccak_table,
             &exp_table,
+            &sig_table,
         ));
 
         meta.annotate_lookup_any_column(byte_table[0], || "byte_range");
@@ -121,6 +127,7 @@ impl<F: Field> SubCircuitConfig<F> for EvmCircuitConfig<F> {
         copy_table.annotate_columns(meta);
         keccak_table.annotate_columns(meta);
         exp_table.annotate_columns(meta);
+        sig_table.annotate_columns(meta);
 
         Self {
             fixed_table,
@@ -133,6 +140,7 @@ impl<F: Field> SubCircuitConfig<F> for EvmCircuitConfig<F> {
             copy_table,
             keccak_table,
             exp_table,
+            sig_table,
         }
     }
 }
@@ -409,6 +417,7 @@ impl<F: Field> Circuit<F> for EvmCircuit<F> {
         let copy_table = CopyTable::construct(meta, q_copy_table);
         let keccak_table = KeccakTable::construct(meta);
         let exp_table = ExpTable::construct(meta);
+        let sig_table = SigTable::construct(meta);
         (
             EvmCircuitConfig::new(
                 meta,
@@ -421,6 +430,7 @@ impl<F: Field> Circuit<F> for EvmCircuit<F> {
                     copy_table,
                     keccak_table,
                     exp_table,
+                    sig_table,
                 },
             ),
             challenges,
@@ -465,6 +475,9 @@ impl<F: Field> Circuit<F> for EvmCircuit<F> {
             .keccak_table
             .dev_load(&mut layouter, &block.sha3_inputs, &challenges)?;
         config.exp_table.dev_load(&mut layouter, block)?;
+        config
+            .sig_table
+            .dev_load(&mut layouter, block, &challenges)?;
 
         self.synthesize_sub(&config, &challenges, &mut layouter)
     }
@@ -646,7 +659,9 @@ mod evm_circuit_stats {
             keccak_table,
             LOOKUP_CONFIG[6].1,
             exp_table,
-            LOOKUP_CONFIG[7].1
+            LOOKUP_CONFIG[7].1,
+            sig_table,
+            LOOKUP_CONFIG[8].1
         );
     }
 

zkevm-circuits/src/evm_circuit.rs

@@ -377,6 +377,7 @@ impl<F: Field> ExecutionConfig<F> {
         copy_table: &dyn LookupTable<F>,
         keccak_table: &dyn LookupTable<F>,
         exp_table: &dyn LookupTable<F>,
+        sig_table: &dyn LookupTable<F>,
     ) -> Self {
         let mut instrument = Instrument::default();
         let q_usable = meta.complex_selector();
@@ -650,6 +651,7 @@ impl<F: Field> ExecutionConfig<F> {
             copy_table,
             keccak_table,
             exp_table,
+            sig_table,
             &challenges,
             &cell_manager,
         );
@@ -905,6 +907,7 @@ impl<F: Field> ExecutionConfig<F> {
         copy_table: &dyn LookupTable<F>,
         keccak_table: &dyn LookupTable<F>,
         exp_table: &dyn LookupTable<F>,
+        sig_table: &dyn LookupTable<F>,
         challenges: &Challenges<Expression<F>>,
         cell_manager: &CellManager<F>,
     ) {
@@ -921,6 +924,7 @@ impl<F: Field> ExecutionConfig<F> {
                         Table::Copy => copy_table,
                         Table::Keccak => keccak_table,
                         Table::Exp => exp_table,
+                        Table::Sig => sig_table,
                     }
                     .table_exprs(meta);
                     vec![(

zkevm-circuits/src/evm_circuit/execution.rs

@@ -49,6 +49,7 @@ pub(crate) const LOOKUP_CONFIG: &[(Table, usize)] = &[
     (Table::Copy, COPY_TABLE_LOOKUPS),
     (Table::Keccak, KECCAK_TABLE_LOOKUPS),
     (Table::Exp, EXP_TABLE_LOOKUPS),
+    (Table::Sig, SIG_TABLE_LOOKUPS),
 ];
 
 /// Fixed Table lookups done in EVMCircuit
@@ -75,6 +76,9 @@ pub const KECCAK_TABLE_LOOKUPS: usize = 1;
 /// Exp Table lookups done in EVMCircuit
 pub const EXP_TABLE_LOOKUPS: usize = 1;
 
+/// Sig Table lookups done in EVMCircuit
+pub const SIG_TABLE_LOOKUPS: usize = 1;
+
 /// Maximum number of bytes that an integer can fit in field without wrapping
 /// around.
 pub(crate) const MAX_N_BYTES_INTEGER: usize = 31;