MPT review

[CeciliaZ030]

MPT review #39

[Brechtpd]

Generally looks good! Just some suggestions and a missing is_long in the lookup table that needs some explanation/implementation to make it work.

[Brechtpd]

Could you do this a bit more like in the witness generation with a match statement for example. Will be quite a bit shorter, less duplicated code (like setting the tag to the same value for each case) and that also uses the RLP constants like RLP_SHORT etc... so fewer magic values.

[Brechtpd]

is_long seems to be missing? I think that's actually smart to only do 2 out of 3 in the lookup table because it saves a cell and you can deduce one of the three values e.g. is_long is 1 - is_short - is_very_long, but I don't see that being done so not sure what's up? :)

[Brechtpd]

I think you got the correct idea for the state machine selectors, but seems like the implementation is more complex than needed.

A table so it's easier to talk about:

row	is_start	is_branch	is_account	is_storage
0	1	0	0	0
1	0	0	0	0
2	0	1	0	0
3	0	0	0	0
4-23	0	0	0	0
24	0	0	1	0
...	...	...	...	....

So this is the current state of the circuit. The selectors for each node are enabled on each row correctly by the witness, but the circuit doesn't have any checks that enforces this.

So I think the only thing that really needs to be checked is that for e.g. the first row with a start node (which has StartRowType::Count rows) has zero's for all the state selectors on the next row, but on the 2nd row there is exactly one selector enabled again (can be any type, doesn't matter).

And because on each row we do check that the selectors are boolean, we can pretty much just brute-force this check

• For all state selectors in the following rows up to RowType::Count, basically just sum all those selectors togethers and force the sum to be 0.
• On the ``RowType::Countth, sum the selectors just on that row and check that it's 1`.

Easy solution, and pretty efficient.

The EVM circuit instead of doing the brute force method does do something similar you're trying to do here with counting down, so e.g.

row	is_start	is_branch	is_account	is_storage	num_data_rows_left
0	1	0	0	0	0
1	0	0	0	0	StartRowType::Count - 1
2	0	1	0	0	0
3	0	0	0	0	BranchRowType::Count - 1

Where the number of data only rows are written into num_data_rows_left and then there's 2 cases:

• num_data_rows_left == 0 -> A state selector needs to be enabled on the current row
• num_data_rows_left != 0 -> All state selectors need to be disabled on the current row, and num_data_rows_left.cur() - 1 => num_data_rows_left.next().

[CeciliaZ030]

I removed the unnecessary lookups as we discussed 😊 and added checks according to your table.
One thing is that I decided to leave Start -> Branch* -> Account -> Branch* -> Storage with q_row and q_node constraints because the current is_below_branch only make sure no account is beneath account. I feel like is better to have a set of holistic state machine constraints.

[Brechtpd]

One thing is that I decided to leave Start -> Branch* -> Account -> Branch* -> Storage with q_row and q_node constraints because the current is_below_branch only make sure no account is beneath account. I feel like is better to have a set of holistic state machine constraints.

We'll see. ;) I also think that generally you do indeed want to add these, but in this case it's a bit tricky to get right. Like Account -> Branch -> Storage is not the only option I believe, it's also possible to go directly Account -> Storage when there's only a single storage leaf. Same with the accounts I think.

These are all the valid transitions I think:

- Start -> Branch
- Start -> Account
- Start -> Start (for padding purposes)

- Branch -> Branch
- Branch -> Account
- Branch -> Storage
- Branch -> Start (doesn't make much sense, but because Start resets everything we can allow it)
- 
- Account -> Storage
- Account -> Start
- Account -> Branch

- Storage -> Start
- Storage -> X (could be made valid if we reset the necessary state as well)

So most transitions are actually allowed by the state machine itself, but we need checks elsewhere where we have more context what's going on to actually check if the transition make sense.

[Brechtpd]

Went over things pretty quickly but generally looking good!

Not sure why you kept the non-CachedRegion assign functions for now, but you can remove if you're done converting everything to the CachedRegion ones.

[Brechtpd]

Isn't some kind of stack more convenient? So you can just push and pop contexts as needed without having to worry about parents and stuff.

[Brechtpd]

Suggested change

	let mut cb = MPTConstraintBuilder::new(33 + 10, None, power_of_randomness.clone());
	let mut cb = MPTConstraintBuilder::new(9, None, power_of_randomness.clone());

This should be a good test so see if the expression splitting (and CachedRegion) is working correctly! (you may need to allocate some extra columns to the cell manager because this may need quite a few extra cells to store intermediate results).

[Brechtpd]

This is EVM circuit specific so to generalize things don't use these anymore. Instead of MAX_DEGREE use max_degree as stored in the ConstraintBuilder. IMPLICIT_DEGREE you can drop completely, we shouldn't need it anymore.

[CeciliaZ030]

Not sure why you kept the non-CachedRegion assign functions for now, but you can remove if you're done converting everything to the CachedRegion ones.

I tried to code in a way that if I break something I can use old things :) And I changed some of the names (like _CellManager with the dash) so I can change all occurance easily.