Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add context pruning #112
Add context pruning #112
Changes from 2 commits
02ba230
d7fe960
13eeeb8
6c01ea3
fea4185
3c5f3df
f5a5769
91c02d6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 do you need to add these changes?
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.
codecopy_within_bounds
callscodecopy_after
instead ofwcopy_after
. I agree that's more or less code duplication, but I didn't see an easy way to do it more cleanly.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.
My problem is that I can't see where are you changing the context here.
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.
From my understanding, the problem with codecopy etc. is that they do not change contexts, but write code in another context. And the idea here is to still be able to prune that extra context when needed by setting the context in codecopy_after.
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.
Ahhh, and why
src_ctx
will be never accessed again?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.
In the case of
extcodecopy
andextcodesize
, we generatesrc_ctx
withnext_context_id
which increments@GLOBAL_METADATA_LARGEST_CONTEXT
, meaning that any other created context will have a different id.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.
Ahhh ok, I think see now. It's because you just need the
new_ctx
for writing the code there (and check the hash etc) but after you copy the code you can forget about it?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.
DQ: why do we expect this to be the content of the stack here? And why only for
EXTCODESIZE
but not for other tests?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.
That's because
extcodesize
(notsys_extcodesize
) now returnscode_size, codesize_ctx
. I added the returned stack in a comment in the ASM.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.
For my education, what is the case when
get_full_memory
isNone
? What does that mean thatGenerationState
use the default impl and returnsNone
?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.
When you're generating the proof (with the full trace), you don't need this memory. The
Memory
trace will be generated from the list ofMemOps
instead.