From ef0656a79d97cf7b31d012a31d5075ae024fd2fe Mon Sep 17 00:00:00 2001 From: z2trillion Date: Tue, 14 Nov 2023 08:06:12 +0800 Subject: [PATCH 1/4] Remove disallow dead code and remove it (#103) * deny warnings for clippy * don't allow dead code * fix clippy --------- Co-authored-by: Mason Liang --- Makefile | 2 +- src/constraint_builder/query.rs | 7 ------- src/gadgets/byte_representation.rs | 9 --------- src/gadgets/key_bit.rs | 9 +++------ src/gadgets/mpt_update.rs | 9 +-------- src/lib.rs | 1 - src/types.rs | 31 ------------------------------ src/util.rs | 17 +--------------- 8 files changed, 6 insertions(+), 79 deletions(-) diff --git a/Makefile b/Makefile index beb636f4..cf38b795 100644 --- a/Makefile +++ b/Makefile @@ -5,4 +5,4 @@ fmt: @cargo fmt clippy: - @cargo clippy --all-features + @cargo clippy --all-features -- -D warnings diff --git a/src/constraint_builder/query.rs b/src/constraint_builder/query.rs index c7b24c4c..11437fe1 100644 --- a/src/constraint_builder/query.rs +++ b/src/constraint_builder/query.rs @@ -6,13 +6,6 @@ use halo2_proofs::{ poly::Rotation, }; -#[derive(Clone, Copy)] -pub enum ColumnType { - Advice, - Fixed, - Challenge, -} - #[derive(Clone)] pub enum Query { Constant(F), diff --git a/src/gadgets/byte_representation.rs b/src/gadgets/byte_representation.rs index aa8b2904..f7ff1047 100644 --- a/src/gadgets/byte_representation.rs +++ b/src/gadgets/byte_representation.rs @@ -2,7 +2,6 @@ use super::{byte_bit::RangeCheck256Lookup, is_zero::IsZeroGadget, rlc_randomness use crate::constraint_builder::{ AdviceColumn, ConstraintBuilder, Query, SecondPhaseAdviceColumn, SelectorColumn, }; -use ethers_core::types::{Address, H256}; use halo2_proofs::{ arithmetic::FieldExt, circuit::{Region, Value}, @@ -156,14 +155,6 @@ fn u128_to_big_endian(x: &u128) -> Vec { x.to_be_bytes().to_vec() } -fn address_to_big_endian(x: &Address) -> Vec { - x.0.to_vec() -} - -fn h256_to_big_endian(x: &H256) -> Vec { - x.0.to_vec() -} - fn fr_to_big_endian(x: &Fr) -> Vec { let mut bytes = x.to_bytes(); bytes.reverse(); diff --git a/src/gadgets/key_bit.rs b/src/gadgets/key_bit.rs index 5d4b2581..abeece53 100644 --- a/src/gadgets/key_bit.rs +++ b/src/gadgets/key_bit.rs @@ -2,7 +2,7 @@ use super::{ byte_bit::{ByteBitLookup, RangeCheck256Lookup, RangeCheck8Lookup}, canonical_representation::CanonicalRepresentationLookup, }; -use crate::constraint_builder::{AdviceColumn, ConstraintBuilder, Query, SelectorColumn}; +use crate::constraint_builder::{AdviceColumn, ConstraintBuilder, Query}; use halo2_proofs::{ arithmetic::FieldExt, circuit::Region, halo2curves::bn256::Fr, plonk::ConstraintSystem, }; @@ -13,8 +13,6 @@ pub trait KeyBitLookup { #[derive(Clone)] pub struct KeyBitConfig { - selector: SelectorColumn, // always enabled selector for constraints we want always enabled. - // Lookup columns value: AdviceColumn, // We're proving value.bit(i) = bit in this gadget index: AdviceColumn, // 0 <= index < 256 @@ -35,8 +33,7 @@ impl KeyBitConfig { range_check_256: &impl RangeCheck256Lookup, byte_bit: &impl ByteBitLookup, ) -> Self { - let ([selector], [], [value, index, bit, index_div_8, index_mod_8, byte]) = - cb.build_columns(cs); + let ([], [], [value, index, bit, index_div_8, index_mod_8, byte]) = cb.build_columns(cs); cb.add_lookup( "0 <= index < 256", @@ -76,7 +73,6 @@ impl KeyBitConfig { ); Self { - selector, value, index, bit, @@ -134,6 +130,7 @@ mod test { rlc_randomness::RlcRandomness, }; use super::*; + use crate::constraint_builder::SelectorColumn; use halo2_proofs::{ circuit::{Layouter, SimpleFloorPlanner}, dev::MockProver, diff --git a/src/gadgets/mpt_update.rs b/src/gadgets/mpt_update.rs index cedd72c4..268da6cc 100644 --- a/src/gadgets/mpt_update.rs +++ b/src/gadgets/mpt_update.rs @@ -31,7 +31,7 @@ use ethers_core::types::Address; use halo2_proofs::{ arithmetic::{Field, FieldExt}, circuit::{Region, Value}, - halo2curves::{bn256::Fr, group::ff::PrimeField}, + halo2curves::bn256::Fr, plonk::ConstraintSystem, }; use itertools::izip; @@ -902,13 +902,6 @@ fn new_right(config: &MptUpdateConfig) -> Query { + (Query::one() - config.direction.current()) * config.sibling.current() } -fn address_to_fr(a: Address) -> Fr { - let mut bytes = [0u8; 32]; - bytes[32 - 20..].copy_from_slice(a.as_bytes()); - bytes.reverse(); - Fr::from_repr(bytes).unwrap() -} - fn configure_segment_transitions( cb: &mut ConstraintBuilder, segment: &OneHot, diff --git a/src/lib.rs b/src/lib.rs index aa76d409..4c353b6b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,3 @@ -#![allow(dead_code)] #![allow(clippy::too_many_arguments)] #![deny(unsafe_code)] diff --git a/src/types.rs b/src/types.rs index 47dbf8c6..0c9d3fca 100644 --- a/src/types.rs +++ b/src/types.rs @@ -185,7 +185,6 @@ pub struct Proof { pub struct EthAccount { pub nonce: u64, pub code_size: u64, - poseidon_codehash: Fr, pub balance: Fr, pub keccak_codehash: U256, pub storage_root: Fr, @@ -196,7 +195,6 @@ impl From for EthAccount { Self { nonce: account_data.nonce, code_size: account_data.code_size, - poseidon_codehash: fr_from_biguint(&account_data.poseidon_code_hash), balance: fr_from_biguint(&account_data.balance), keccak_codehash: u256_from_biguint(&account_data.code_hash), storage_root: Fr::zero(), // TODO: fixmeeee!!! @@ -957,39 +955,10 @@ fn check_hash_traces_new(traces: &[(bool, HashDomain, Fr, Fr, Fr, bool, bool)]) } } -fn bits(x: usize, len: usize) -> Vec { - let mut bits = vec![]; - let mut x = x; - while x != 0 { - bits.push(x % 2 == 1); - x /= 2; - } - bits.resize(len, false); - bits.reverse(); - bits -} - fn fr(x: HexBytes<32>) -> Fr { Fr::from_bytes(&x.0).unwrap() } -fn split_word(x: U256) -> (Fr, Fr) { - let mut bytes = [0; 32]; - x.to_big_endian(&mut bytes); - let high_bytes: [u8; 16] = bytes[..16].try_into().unwrap(); - let low_bytes: [u8; 16] = bytes[16..].try_into().unwrap(); - - let high = Fr::from_u128(u128::from_be_bytes(high_bytes)); - let low = Fr::from_u128(u128::from_be_bytes(low_bytes)); - (high, low) - - // TODO: what's wrong with this? - // let [limb_0, limb_1, limb_2, limb_3] = key.0; - // let key_high = Fr::from_u128(u128::from(limb_2) + u128::from(limb_3) << 64); - // let key_low = Fr::from_u128(u128::from(limb_0) + u128::from(limb_1) << 64); - // hash(key_high, key_low) -} - fn big_uint_to_fr(i: &BigUint) -> Fr { i.to_u64_digits() .iter() diff --git a/src/util.rs b/src/util.rs index 33fb3360..9a366b38 100644 --- a/src/util.rs +++ b/src/util.rs @@ -2,7 +2,7 @@ use crate::{constraint_builder::Query, serde::HexBytes, types::HashDomain}; use ethers_core::types::{Address, U256}; use halo2_proofs::{ arithmetic::{Field, FieldExt}, - halo2curves::{bn256::Fr, group::ff::PrimeField}, + halo2curves::bn256::Fr, }; use hash_circuit::hash::Hashable; use num_bigint::BigUint; @@ -50,15 +50,6 @@ pub(crate) fn split_word(x: U256) -> (Fr, Fr) { // hash(key_high, key_low) } -pub(crate) fn hi_lo(x: &BigUint) -> (Fr, Fr) { - let mut u64_digits = x.to_u64_digits(); - u64_digits.resize(4, 0); - ( - Fr::from_u128((u128::from(u64_digits[3]) << 64) + u128::from(u64_digits[2])), - Fr::from_u128((u128::from(u64_digits[1]) << 64) + u128::from(u64_digits[0])), - ) -} - pub(crate) fn u256_hi_lo(x: &U256) -> (u128, u128) { let u64_digits = x.0; ( @@ -86,12 +77,6 @@ pub fn u256_from_biguint(x: &BigUint) -> U256 { U256::from_big_endian(&x.to_bytes_be()) } -pub fn u256_to_fr(x: U256) -> Fr { - let mut bytes = [0u8; 32]; - x.to_little_endian(&mut bytes); - Fr::from_repr(bytes).unwrap() -} - pub fn u256_to_big_endian(x: &U256) -> Vec { let mut bytes = [0; 32]; x.to_big_endian(&mut bytes); From ee6e4a339414a6d75efa13068617c0fd9dd5a003 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Tue, 14 Nov 2023 08:06:59 +0800 Subject: [PATCH 2/4] Expand and correct comment explaining why account must already exist (#80) Co-authored-by: Mason Liang --- src/types.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/types.rs b/src/types.rs index 0c9d3fca..55c34b4d 100644 --- a/src/types.rs +++ b/src/types.rs @@ -260,7 +260,11 @@ impl From<(&MPTProofType, &SMTTrace)> for ClaimKind { match update { [None, None] => (), [Some(old), Some(new)] => { - // The account must exist, because only contracts with bytecode can modify their own storage slots. + // Accesses to the MPT happen in the order defined in the state (aka rw) circuit, which is not the + // same as the order they occur in the EVM. In the state circuit, nonce and balance modifications + // will precede storage modifications for a given address, which means that the MPT circuit only + // needs to handle storage modifications for existing accounts, even though this is not true in the + // EVM, where the storage of an account can be modified during its construction. if !(account_old == account_new || (account_old.is_none() && account_new == &Some(Default::default()))) { From 757675b77f77f2ad9e0227a38c844ddbbf90301c Mon Sep 17 00:00:00 2001 From: z2trillion Date: Tue, 14 Nov 2023 08:07:18 +0800 Subject: [PATCH 3/4] [ZK 37] only check path_type in configure_extension_old and configure_extension_new (#79) * Only check path_type in configure_extension_old and configure_extension_new * Remove always satisfied conditions for poseidon code hash updates * Remove always satisfied condition for code size and delete unreachable constraints * Remove duplicated constraint * fmt * Remove always satisfied condition * Check that code size and nonce are 0 for new account --------- Co-authored-by: Mason Liang Co-authored-by: z2trillion --- src/gadgets/mpt_update.rs | 111 ++++++++++++-------------------------- 1 file changed, 33 insertions(+), 78 deletions(-) diff --git a/src/gadgets/mpt_update.rs b/src/gadgets/mpt_update.rs index 268da6cc..9e1b014e 100644 --- a/src/gadgets/mpt_update.rs +++ b/src/gadgets/mpt_update.rs @@ -1127,10 +1127,15 @@ fn configure_extension_old( poseidon: &impl PoseidonLookup, ) { cb.assert( - "can only delete existing nodes for storage proofs", + "can only delete existing storage trie nodes for storage proofs", config .proof_type - .current_matches(&[MPTProofType::StorageChanged]), + .current_matches(&[MPTProofType::StorageChanged]) + .and( + config + .segment_type + .current_matches(&[SegmentType::StorageTrie, SegmentType::StorageLeaf0]), + ), ); cb.assert_zero( "new value is 0 when deleting node", @@ -1353,10 +1358,10 @@ fn configure_nonce( config.path_type.current_matches(&[PathType::ExtensionNew]), |cb| { cb.assert_equal( - "sibling is hash(0, hash(0, 0)) for nonce extension new at AccountLeaf2", - config.sibling.current(), - Query::from(*ZERO_STORAGE_ROOT_KECCAK_CODEHASH_HASH), - ); + "sibling is hash(0, hash(0, 0)) for nonce extension new at AccountLeaf2", + config.sibling.current(), + Query::from(*ZERO_STORAGE_ROOT_KECCAK_CODEHASH_HASH), + ); }, ); } @@ -1426,12 +1431,6 @@ fn configure_code_size( bytes: &impl BytesLookup, poseidon: &impl PoseidonLookup, ) { - cb.assert( - "new accounts have balance or nonce set first", - config - .path_type - .current_matches(&[PathType::Start, PathType::Common]), - ); for variant in SegmentType::iter() { let conditional_constraints = |cb: &mut ConstraintBuilder| match variant { SegmentType::Start | SegmentType::AccountTrie => { @@ -1602,19 +1601,21 @@ fn configure_balance( ); }, ); + cb.add_lookup( + "new balance is rlc(new_hash) and fits into 31 bytes", + [ + config.new_hash.current(), + Query::from(30), + config.new_value.current(), + ], + rlc.lookup(), + ); cb.condition( - config - .path_type - .current_matches(&[PathType::Common, PathType::ExtensionNew]), + config.path_type.current_matches(&[PathType::ExtensionNew]), |cb| { - cb.add_lookup( - "new balance is rlc(new_hash) and fits into 31 bytes", - [ - config.new_hash.current(), - Query::from(30), - config.new_value.current(), - ], - rlc.lookup(), + cb.assert_zero( + "sibling (code_size + nonce << 64) is 0 for new account)", + config.sibling.current(), ); }, ); @@ -1641,12 +1642,6 @@ fn configure_poseidon_code_hash( cb: &mut ConstraintBuilder, config: &MptUpdateConfig, ) { - cb.assert( - "new accounts have balance or nonce set first", - config - .path_type - .current_matches(&[PathType::Start, PathType::Common]), - ); for variant in SegmentType::iter() { let conditional_constraints = |cb: &mut ConstraintBuilder| match variant { SegmentType::AccountLeaf0 => { @@ -1654,29 +1649,15 @@ fn configure_poseidon_code_hash( } SegmentType::AccountLeaf1 => { cb.assert_equal("direction is 1", config.direction.current(), Query::one()); - cb.condition( - config - .path_type - .current_matches(&[PathType::Common, PathType::ExtensionOld]), - |cb| { - cb.assert_equal( - "old_hash is old poseidon code hash", - config.old_value.current(), - config.old_hash.current(), - ); - }, + cb.assert_equal( + "old_hash is old poseidon code hash", + config.old_value.current(), + config.old_hash.current(), ); - cb.condition( - config - .path_type - .current_matches(&[PathType::Common, PathType::ExtensionNew]), - |cb| { - cb.assert_equal( - "new_hash is new poseidon code hash", - config.new_value.current(), - config.new_hash.current(), - ); - }, + cb.assert_equal( + "new_hash is new poseidon code hash", + config.new_value.current(), + config.new_hash.current(), ); } _ => {} @@ -1696,12 +1677,6 @@ fn configure_keccak_code_hash( rlc: &impl RlcLookup, randomness: Query, ) { - cb.assert( - "new accounts have balance or nonce set first", - config - .path_type - .current_matches(&[PathType::Start, PathType::Common]), - ); for variant in SegmentType::iter() { let conditional_constraints = |cb: &mut ConstraintBuilder| match variant { SegmentType::Start | SegmentType::AccountTrie => { @@ -1797,10 +1772,6 @@ fn configure_storage( cb.assert_equal("direction is 1", config.direction.current(), Query::one()); } SegmentType::AccountLeaf3 => { - cb.assert( - "storage modifications must be on an existing account", - config.path_type.current_matches(&[PathType::Common]), - ); cb.assert_zero("direction is 0", config.direction.current()); let [key_high, key_low, ..] = config.intermediate_values; let [rlc_key_high, rlc_key_low, ..] = config.second_phase_intermediate_values; @@ -1820,11 +1791,8 @@ fn configure_storage( let [old_high, old_low, new_high, new_low, ..] = config.intermediate_values; let [rlc_old_high, rlc_old_low, rlc_new_high, rlc_new_low, ..] = config.second_phase_intermediate_values; - cb.condition( - config - .path_type - .current_matches(&[PathType::Common, PathType::ExtensionOld]), + config.path_type.current_matches(&[PathType::Common]), |cb| { configure_word_rlc( cb, @@ -1835,13 +1803,6 @@ fn configure_storage( rlc, randomness.clone(), ); - }, - ); - cb.condition( - config - .path_type - .current_matches(&[PathType::Common, PathType::ExtensionNew]), - |cb| { configure_word_rlc( cb, [config.new_hash, new_high, new_low], @@ -1970,12 +1931,6 @@ fn configure_empty_account( config: &MptUpdateConfig, poseidon: &impl PoseidonLookup, ) { - cb.assert( - "path type is start or common for empty account proof", - config - .path_type - .current_matches(&[PathType::Start, PathType::Common]), - ); cb.assert_zero("old value is 0", config.old_value.current()); cb.assert_zero("new value is 0", config.new_value.current()); cb.assert_equal( From 803c349dcb10f701fddd403d8d28ccc9f5b6130d Mon Sep 17 00:00:00 2001 From: z2trillion Date: Tue, 14 Nov 2023 09:00:27 +0800 Subject: [PATCH 4/4] Add fixed column to lookup_exprs for mpt circuit (#102) Co-authored-by: Mason Liang --- src/gadgets/mpt_update.rs | 7 ++++--- src/mpt.rs | 10 +++++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/gadgets/mpt_update.rs b/src/gadgets/mpt_update.rs index 9e1b014e..3689104d 100644 --- a/src/gadgets/mpt_update.rs +++ b/src/gadgets/mpt_update.rs @@ -45,7 +45,7 @@ lazy_static! { } pub trait MptUpdateLookup { - fn lookup(&self) -> [Query; 8]; + fn lookup(&self) -> [Query; 7]; } #[derive(Clone)] @@ -77,8 +77,10 @@ pub struct MptUpdateConfig { } impl MptUpdateLookup for MptUpdateConfig { - fn lookup(&self) -> [Query; 8] { + fn lookup(&self) -> [Query; 7] { let is_start = || self.segment_type.current_matches(&[SegmentType::Start]); + // Note that one non-start rows, all 7 queries will be 0. This corresponds to a valid + // mpt proof in that in an empty trie, the zero address has nonce = 0. let old_root_rlc = self.second_phase_intermediate_values[0].current() * is_start(); let new_root_rlc = self.second_phase_intermediate_values[1].current() * is_start(); let proof_type = self.proof_type.current() * is_start(); @@ -90,7 +92,6 @@ impl MptUpdateLookup for MptUpdateConfig { * is_start(); let storage_key_rlc = self.storage_key_rlc.current() * is_start(); [ - is_start().into(), address, storage_key_rlc, proof_type, diff --git a/src/mpt.rs b/src/mpt.rs index 12ce206c..da923762 100644 --- a/src/mpt.rs +++ b/src/mpt.rs @@ -1,5 +1,5 @@ use crate::{ - constraint_builder::{ConstraintBuilder, SelectorColumn}, + constraint_builder::{ConstraintBuilder, Query, SelectorColumn}, gadgets::{ byte_bit::ByteBitGadget, byte_representation::ByteRepresentationConfig, @@ -76,7 +76,6 @@ impl MptCircuitConfig { // exist in an mpt with root = 0 (i.e. the mpt is empty). let is_final_row = SelectorColumn(cs.fixed_column()); let padding_row_expressions = [ - 1.into(), 0.into(), 0.into(), (MPTProofType::AccountDoesNotExist as u64).into(), @@ -173,7 +172,12 @@ impl MptCircuitConfig { } pub fn lookup_exprs(&self, meta: &mut VirtualCells<'_, F>) -> [Expression; 8] { - self.mpt_update.lookup().map(|q| q.run(meta)) + std::iter::once(Query::from(self.selector.current())) + .chain(self.mpt_update.lookup().into_iter()) + .map(|q| q.run(meta)) + .collect::>() + .try_into() + .unwrap() } /// The number of minimum number of rows required for the mpt circuit.