fix(avm)!: last_child id constraints#16568
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
IlyasRidhuan
requested review from
Maddiaa0,
fcarreiro and
jeanmon
as code owners
dbanks12
left a comment
dbanks12
left a comment
There was a problem hiding this comment.
Looks mostly good, but a couple concerns
barretenberg/cpp/pil/vm2/context.pil
Outdated
| #[ENTER_CALL_LAST_CHILD_ID] | ||
| NOT_LAST_EXEC * (sel_enter_call + sel_error) * last_child_id' = 0; |
There was a problem hiding this comment.
Why the + sel_error? sel_enter_call implies sel_error == 0
There was a problem hiding this comment.
Yeah so last_child_id of the next row should be 0 if we either enter a new call context or if we have an exceptional halt. Thinking about this some more i think there might be an issue with the other returndatas as well
- It might need to only be for nested exceptional halt, in a top level halt we do nothing anyways
- We zero out rd_size and rd_addr so that the parent context can't access a failed child context's memory via returndata. However, in our current execution flow we don't zero it out.
I'll think abit more about it
There was a problem hiding this comment.
We no longer zero out last_child_id if we exit a failing call
There was a problem hiding this comment.
In the TS simulator, I think if a nested call exceptionally halts, its empty returndata becomes the parents "last child's returndata".
IlyasRidhuan
changed the base branch from
ir/08-19-fix_avm_handle_zero_copy_size
to
graphite-base/16568
IlyasRidhuan
force-pushed
the
ir/08-25-fix_avm_last_child_id_constraints
branch
from
95932af to
9093797
Compare
IlyasRidhuan
force-pushed
the
graphite-base/16568
branch
from
78de546 to
08e8cf0
Compare
IlyasRidhuan
changed the base branch from
graphite-base/16568
to
ir/08-19-fix_avm_handle_zero_copy_size
|
|
||
| check_relation<context>(trace); | ||
|
|
||
| // TODO: Migrate to check_interaction pattern once these lookups are added in a builder |
There was a problem hiding this comment.
Update to clean up this todo as well
| lookup_context_ctx_stack_return_settings>(trace); | ||
| } | ||
|
|
||
| TEST(ContextConstrainingTest, ContextSwitchingExceptionalHalt) |
There was a problem hiding this comment.
New test to check correct reset during an exceptional halt
IlyasRidhuan
force-pushed
the
ir/08-19-fix_avm_handle_zero_copy_size
branch
from
08e8cf0 to
9965aa8
Compare
IlyasRidhuan
force-pushed
the
ir/08-25-fix_avm_last_child_id_constraints
branch
from
9093797 to
b3ed32f
Compare
dbanks12
left a comment
dbanks12
left a comment
There was a problem hiding this comment.
Approving to unblock, but want to review for real later.
IlyasRidhuan
changed the base branch from
ir/08-19-fix_avm_handle_zero_copy_size
to
graphite-base/16568
IlyasRidhuan
force-pushed
the
ir/08-25-fix_avm_last_child_id_constraints
branch
from
b3ed32f to
8d6f1cd
Compare
IlyasRidhuan
force-pushed
the
graphite-base/16568
branch
from
9965aa8 to
d06ed9b
Compare
2 participants
Adds the constraints for the
last_child_idintroduced in #16531. It has similar behaviour to other return data information stored in the context