-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cell manager context #17
base: circuit-tools-wip
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I temporarily removed it from the MPT code because it's currently not used!
To test it out it'll have to be integrated with the macro's/constraint builder because otherwise the code it untested no?
height_limit: usize, | ||
|
||
// branch ctxs | ||
branch_ctxs: HashMap<String, CmContext<F, C>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it a hashmap and not a simple vector? Contexts will work like a stack no? Similarly to how we handle the condition in the constraint builder. So if you enter a context you just push a new context on the stack, if you exit a context you just pop it from the stack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brechtpd#9 If you scrolled down to one of my replies:
I thought of it and figured that a dynamic branching callstack is equivalent to a static upside-down tree:
|_if_| |_else_| |_if_| |_____if_______| |_else_| |__________________________|
A simple callstack structure won't work cuz we need to remember what happened in the past. Thus we have to save the entire tree.
In the condition case remember we had to use region_id
to hack around this problem, basically manually divide the branching callstack into states labeled with id:
|_if_| |_else_| |_if_|
|___ match____| |_match__|
|______________________________|
( START )( ACCOUNT ) ....
And one time i said ideally we can make store expressions work like CM ctx by changing the macros, but I failed 😭 and that could be something we can do for the next iteration.
@@ -274,16 +288,87 @@ impl<F: Field, C: CellType> CellManager<F, C> { | |||
expr: cells[0].expr(), | |||
cells, | |||
}); | |||
width += 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
width
is just columns.len()
? Why the extra variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably because in previous version there was a width
? I'll get rid of it :)
Ohh I forgot to add the macros back, will do it to see if it work. |
zkevm-circuits/pull/96