refactor state circuit #339
Conversation
github-actions
Bot
added
T-opcode
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
Overall looks good to me! I've just added a few notes for discussion, but regardless of the outcome I think they should be non-blocking because there are still many changes to be made in the state circuit to bring it up to date with the latest spec :)
The only comment which should really be resolved is about the generate_lagrange_base_polynomial which I think has a mistake.
| } | ||
|
|
||
| impl RwMap { | ||
| /// These "sorted_xx" methods are used in state circuit |
There was a problem hiding this comment.
Currently we have sorting methods in the bus-mapping (in OperationContainer). I think we should have storing methods in one place. So either we
a. take the bus-mapping Operations, sort them and convert them to RwMap
b. take the bus-mapping Operations, convert them to RwMap, sort them
Since we only should sort once, I think having the sorting implementation in two places is redundant. I don't know which is the best place to do the sorting though.
|
|
||
| #[derive(Debug, Default)] | ||
| #[derive(Debug, Default, Clone)] | ||
| pub struct RwMap(pub HashMap<RwTableTag, Vec<Rw>>); |
There was a problem hiding this comment.
NOTE: This could be a fixed length array instead of a HashMap (considering that in most cases we're using all the tags (minus value 0 and 1, and we have just a few of them).
| pub struct RwRow<F: FieldExt> { | ||
| pub rw_counter: F, | ||
| pub is_write: F, | ||
| pub tag: F, | ||
| pub key2: F, | ||
| pub key3: F, | ||
| pub key4: F, | ||
| pub value: F, | ||
| pub value_prev: F, | ||
| pub aux1: F, | ||
| pub aux2: F, | ||
| } |
There was a problem hiding this comment.
Suggestion:
pub struct RwRow<F: FieldExt> {
pub rw_counter: F,
pub is_write: F,
pub keys: [F; 4]
pub value: F,
pub value_prev: F,
pub auxs: [F; 2],
}I think making groups makes it more readable, but this is just a suggestion!
|
|
||
| /// The rw table shared between evm circuit and state circuit | ||
| #[derive(Clone, Copy)] | ||
| pub struct RwTable { |
There was a problem hiding this comment.
The same suggestion applies here to merge fields (all keys, and all auxs)
miha-stopar
left a comment
miha-stopar
left a comment
There was a problem hiding this comment.
Looks cool to me, just a minor thing about generate_lagrange_base_polynomial that Edu mentioned is to be fixed.
|
I made two small changes (1) update generate_lagrange_base_polynomial (2) fix a comment typo Since a lot of updates needed for the new spec further, I think this PR can be merged first. Thank you! |
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the contribution :)
Thanks
I agree :) Let's wait for the tests to pass and we can merge this PR |
3 participants
This pr contains some useful modification of state circuit, as the first step to implement the new state spec privacy-ethereum/zkevm-specs#118
(x-1) * (x-3) * (x-4) / 12togenerate_lagrange_base_polynomial(x, 2, 1..=4)