MPT initial #560

miha-stopar · 2022-06-07T16:15:27Z

MPT (from mpt2 branch) reduced to only have two chips (to simplify the reviewing process):

branch hash in parent (checking hash of the branch is at the proper position in the parent branch)
branch RLC (checking the branch RLC is computed properly - used for a keccak lookup for the first point)

Tests are included also.

adria0 · 2022-06-07T16:39:11Z

Since this is a really huge PR would you like to provide some kind of document to help and guide the reviewers?

miha-stopar · 2022-06-08T09:40:50Z

@miha-stopar WoW!! 🌟

Since this is a really huge PR would you like to provide some kind of document to help and guide the reviewers?

Oh no, that's the small one :). The purpose of this PR is to make mpt circuit easier to review. We reduced yesterday with @ChihChengLiang the original PR to only have two simple chips, so that the review process can start. But it will be reduced further and I agree - I will add more comments to the code to explain the internals. Otherwise, some initial docs are here.

adria0 · 2022-06-08T09:43:48Z

Oh no, that's the small one :)

LoL ;-)

ChihChengLiang · 2022-06-08T09:55:46Z

I think it'll improve more after another Clippy round and a coding style review. Then it would be easier to do a review on the content part.

ChihChengLiang

Some quick comments

mpt/Cargo.toml

mpt/src/branch_hash_in_parent.rs

mpt/src/mpt.rs

smtmfft · 2022-06-14T06:47:43Z

mpt/src/mpt.rs

+            q_not_first,
+            account_leaf.is_account_leaf_in_added_branch,
+            is_last_branch_child,
+            s_advices[IS_BRANCH_C_PLACEHOLDER_POS - LAYOUT_OFFSET],


Should it be c_advices??

s_advices is ok. s_advices and c_advices in branch children rows are used for children hashes (that's why 32 columns). However, there is a special row before branch children rows, named branch_init. This row contains some RLP specific data, but also some other information, for example which of the children is being changed (going from s to c) and whether the branch is a regular branch or extension node. This last is used in the code above. All this additional info is stored in s_advices simply because there is enough space there and it is something that is the same for s and c.

Got it. Thanks!

mpt/src/mpt.rs

adria0 · 2022-06-30T11:15:36Z

mpt/src/branch_rlc_init.rs

+            let three_rlp_bytes_c = meta.query_advice(s_main.bytes[1], Rotation::cur());
+
+            let one = Expression::Constant(F::one());
+            constraints.push((


use helpers::get_bool_constraint here?

adria0 · 2022-07-15T10:06:28Z

mpt/src/param.rs

@@ -0,0 +1,58 @@
+// Currently using 32 - each hash byte goes into its own cell, this might be
+// compressed for optimization purposes in the future.
+pub const HASH_WIDTH: usize = 32;


Missing documentation for these constants

Added in 9c0c058.

adria0 · 2022-07-15T10:07:35Z

mpt/src/mpt.rs

+}
+
+#[derive(Default)]
+struct ProofVariables<F> {


Undocumented constants

adria0 · 2022-07-15T15:52:31Z

@miha-stopar, added a bunch of comments for the initial review :)

adria0 · 2022-07-15T10:11:29Z

mpt/src/mpt.rs

+                        );
+                        let c_root_rlc = bytes_into_rlc(
+                            &row[l
+                                - 3 * HASH_WIDTH


I think that these constants should be hidden and kept defined or enumerated.

As an excercise I tryed to create a new struct called MptWitness and I think that makes code easier to read

#[derive(Eq, PartialEq, IntoPrimitive, TryFromPrimitive)] #[repr(u8)] pub enum MptWitnessRowType { InitBranch = 0, BranchChild = 1, StorageLeafSKey = 2, StorageLeafCKey = 3, HashToBeComputed = 5, AccountLeafKeyS = 6, AccountLeafKeyC = 4, AccountLeafNonceBalanceS = 7, AccountLeafNonceBalanceC = 8, AccountLeafRootCodehashS = 9, AccountLeafNeighbouringLeaf = 10, AccountLeafRootCodehashC = 11, StorageLeafSValue = 13, StorateLeafCValue = 14, NeighbouringStorageLeaf = 15, ExtensionNodeS = 16, ExtensionNodeC = 17, NonExistingProof = 18 } pub struct MptWitnessRow(pub Vec<u8>); impl MptWitnessRow { pub fn get_type(&self) -> MptWitnessRowType { MptWitnessRowType::try_from(self.row(1)).unwrap() } fn row(&self, rev_index: usize) -> u8 { self.0[self.0.len() - rev_index] } pub fn not_first_level(&self) -> u8 { self.row(NOT_FIRST_LEVEL_POS) } pub fn is_storage_mod(&self) -> u8 { self.row(IS_STORAGE_MOD_POS) } ... pub fn s_root_bytes(&self) -> &[u8] { &self.0[self.0.len() - 4 * HASH_WIDTH - COUNTER_MptWitness_LEN - IS_NON_EXISTING_ACCOUNT_POS .. self.0.len() - 4 * HASH_WIDTH - COUNTER_MptWitness_LEN - IS_NON_EXISTING_ACCOUNT_POS + HASH_WIDTH] } } impl Index<usize> for MptWitnessRow { type Output = u8; fn index<'a>(&'a self, i: usize) -> &'a u8 { &self.0[i] } } pub struct MptWitness(pub Vec<MptWitnessRow>); impl MptWitness { pub fn from(raw : &[Vec<u8>]) -> Self { let rows = raw.iter().map(|row| MptWitnessRow(row.clone())).collect(); MptWitness(rows) } } impl Index<usize> for MptWitness { type Output = MptWitnessRow; fn index<'a>(&'a self, i: usize) -> &'a MptWitnessRow { &self.0[i] } }

Nice!! Implemented in the main PR.

adria0 · 2022-07-15T11:07:19Z

mpt/src/mpt.rs

+                    // Note: filter out rows that are just to be hashed. We cannot simply use ind instead of offset
+                    // because hashing rows appear in the middle of other rows.
+                    for (ind, row) in witness.iter().filter(|r| r[r.len() - 1] != 5).enumerate() {
+                        if offset > 0 {


we check here that offset > 0 , but, since the operation to be done is clearing pv, and in offset==0 is already cleared, this condition seems irrelevant

It's needed because there is no previous row when offset = 0.

adria0 · 2022-07-15T11:38:33Z

mpt/src/mpt.rs

+                            )?;
+
+                            pv.node_index += 1;
+                        } else if row[row.len() - 1] != 5 {


previously you filter the rows with witness.iter().filter(|r| r[r.len() - 1] != 5).enumerate() , so this condition it will be always true?

Agree, removed. 👍

adria0 · 2022-07-15T11:41:46Z

mpt/src/mpt.rs

+                            // Branch (length 340) with three bytes of RLP meta data
+                            // [249,1,81,128,16,...
+
+                            pv.acc_s = F::from(row[BRANCH_0_S_START] as u64)


I think that all those row[BRANCH_0_S_start + x] can be removed if your create some variables over them like

let s_len = [0, 1, 2].map(|i| row[BRANCH_0_S_START + i] as u64);, then you can use s_len[0], s_len[1], s_len[2]

Agree, added in db1a700

adria0 · 2022-07-15T11:43:37Z

mpt/src/mpt.rs

+        Ok(())
+    }
+
+    fn assign_branch_init(


IMO is better to put all the specific code that relates to branch_init here, it's easy to pass here a pv: &mut ProofVariables

Agree :), I planned to refactor this when adding branch assignment in branch related files, but this the refactor you proposed now already in bd867f5

adria0 · 2022-07-18T10:55:59Z

mpt/src/mpt.rs

+        self.load_keccak_table(_layouter, to_be_hashed).ok();
+        self.load_fixed_table(_layouter).ok();


Suggested change

self.load_keccak_table(_layouter, to_be_hashed).ok();

self.load_fixed_table(_layouter).ok();

self.load_keccak_table(_layouter, to_be_hashed)?;

self.load_fixed_table(_layouter)?;

adria0 · 2022-07-18T14:32:54Z

mpt/src/branch_rlc.rs

+
+// BranchRLCChip verifies the random linear combination for the branch which is
+// then used to check the hash of a branch.
+impl<F: Field> BranchRLCConfig<F> {


Config or Chip? IMO makes more sense if is Chip

I think it suffices to have Config. Or do you think it's better to have Chips?

adria0 · 2022-07-19T11:17:25Z

mpt/src/branch_rlc.rs

+    _marker: PhantomData<F>,
+}
+
+// BranchRLCChip verifies the random linear combination for the branch which is


I see that branch_rlc_init works for S and C, but branch_rlc only works for one (S or C). Why this? I think that is going to be more clear to have a unique branch_rlc ( that are instantiated twice, one for S and other for C) that also includes init branch row.

adria0 · 2022-07-19T11:22:58Z

mpt/src/branch_rlc.rs

+        meta: &mut ConstraintSystem<F>,
+        q_enable: impl FnOnce(&mut VirtualCells<'_, F>) -> Expression<F>,
+        main: MainCols,
+        branch_acc: Column<Advice>,


this is called acc_s in branch_rlc_init.rs, what about renaming it to acc? (or renaming it in branch_rlc_init.rs to branch_acc_s ?)

adria0 · 2022-07-19T11:42:30Z

mpt/src/branch_rlc_init.rs

+
+        // Short RLP, meta data contains two bytes: 248, 81
+        // [1,0,1,0,248,81,0,248,81,0,3,0,0,0,...
+        // The length of RLP stream is: 81.


Why not to encode the C RLP length in the "C" section of the circuit (the second half of rows)?

All such information is stored in the S part in the branch init row (also for example whether the branch S or branch C are placeholders ...). Agree, it might be better to store C related info into C part, but then there is also information like drifted_pos which applies to both parts, so there would be some asymmetry in storing these fields in any case. But might be changed later, it should be easy.

miha-stopar · 2022-07-29T14:57:16Z

@miha-stopar, added a bunch of comments for the initial review :)

Thanks @adria0 ! I realised it's better to apply the modifications you propose in the original PR (otherwise the two PRs will go too far apart), will see later whether to proceed with this PR or not. I will do these changes in parallel with improving the specs - there will be quite some new structs introduced and I would like to properly document them in the specs.

miha-stopar · 2022-10-28T15:00:32Z

Closed in favour of #398.

) * refactor: update the default sign_data * clean keccak_inputs_tx_circuit * chore: clippy fix * add sanity check * fix --------- Co-authored-by: Rohit Narurkar <[email protected]>

miha-stopar added 3 commits June 7, 2022 17:46

part of assign function removed

29627dd

another part of assign function removed

67c214e

clippy / fmt

75c5bc4

miha-stopar requested a review from ChihChengLiang June 7, 2022 16:15

miha-stopar added 3 commits June 8, 2022 17:07

clippy

8c5aa63

AccountLeaf struct

9a69643

StorageLeaf struct

dd9f408

ChihChengLiang reviewed Jun 10, 2022

View reviewed changes

mpt/Cargo.toml Outdated Show resolved Hide resolved

mpt/src/branch_hash_in_parent.rs Outdated Show resolved Hide resolved

mpt/src/mpt.rs Outdated Show resolved Hide resolved

smtmfft reviewed Jun 14, 2022

View reviewed changes

miha-stopar added 8 commits June 14, 2022 14:28

using now eth_types::Field

3d9f3a1

Chips removed; only Configs left

0a47f08

byte range lookups for branch init

cd10057

assignment improved

d69ebf9

Branch struct

cd2cdc1

branch assignment fixed

92fb3b4

is_non_existing_account_row part of AccountLeaf struct

b6ad5ee

ProofType struct

8091156

ChihChengLiang reviewed Jun 15, 2022

View reviewed changes

mpt/src/mpt.rs Outdated Show resolved Hide resolved

miha-stopar added 2 commits June 16, 2022 11:43

ProofVariables uses Default now

2cd8d15

struct for s_rlp1, s_rlp2, s_advices

12e29cb

adria0 reviewed Jun 30, 2022

View reviewed changes

adria0 reviewed Jul 15, 2022

View reviewed changes

mpt/src/mpt.rs

}

#[derive(Default)]

struct ProofVariables<F> {

Copy link

Member

adria0 Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undocumented constants

adria0 reviewed Jul 19, 2022

View reviewed changes

description for constants in param.rs

9c0c058

ProofVariables description

bc0cb33

miha-stopar closed this Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPT initial #560

MPT initial #560

miha-stopar commented Jun 7, 2022 •

edited

Loading

adria0 commented Jun 7, 2022

miha-stopar commented Jun 8, 2022

adria0 commented Jun 8, 2022

ChihChengLiang commented Jun 8, 2022

ChihChengLiang left a comment

smtmfft Jun 14, 2022

miha-stopar Jun 14, 2022

smtmfft Jun 14, 2022

adria0 Jun 30, 2022

adria0 Jul 15, 2022

miha-stopar Jul 25, 2022

adria0 Jul 15, 2022

adria0 commented Jul 15, 2022

adria0 Jul 15, 2022

miha-stopar Aug 19, 2022

adria0 Jul 15, 2022

miha-stopar Aug 19, 2022

adria0 Jul 15, 2022

miha-stopar Aug 19, 2022

adria0 Jul 15, 2022

miha-stopar Aug 23, 2022

adria0 Jul 15, 2022

miha-stopar Aug 23, 2022

adria0 Jul 18, 2022

adria0 Jul 18, 2022

miha-stopar Aug 23, 2022

adria0 Jul 19, 2022

adria0 Jul 19, 2022

adria0 Jul 19, 2022

miha-stopar Aug 26, 2022

miha-stopar commented Jul 29, 2022

miha-stopar commented Oct 28, 2022

		self.load_keccak_table(_layouter, to_be_hashed).ok();
		self.load_fixed_table(_layouter).ok();

MPT initial #560

MPT initial #560

Conversation

miha-stopar commented Jun 7, 2022 • edited Loading

adria0 commented Jun 7, 2022

miha-stopar commented Jun 8, 2022

adria0 commented Jun 8, 2022

ChihChengLiang commented Jun 8, 2022

ChihChengLiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adria0 commented Jul 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-stopar commented Jul 29, 2022

miha-stopar commented Oct 28, 2022

miha-stopar commented Jun 7, 2022 •

edited

Loading