implement serde for interpreter #1909
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rakita
reviewed
Dec 12, 2024
rakita
reviewed
Dec 12, 2024
| } = ExtBytecodeSerde::deserialize(deserializer)?; | ||
|
|
||
| let mut bytecode = Self::new(base); | ||
| bytecode.absolute_jump(program_counter); |
There was a problem hiding this comment.
Would put a small sanity check above this line, if program_counter >= bytecode.bytecode_len() { panic!("serde pc: {program_counter} is less than bytecode len");
There was a problem hiding this comment.
added!
i believe you meant the message to be " is greater than or equal to bytecode len"
There was a problem hiding this comment.
i believe you meant the message to be " is greater than or equal to bytecode len"
Yeah, i edited my msg after i have reread it :D
rakita
approved these changes
Dec 12, 2024
FredCoen
force-pushed
the
Reimplement-serde-for-Interpreter-#1890
branch
from
a8d268e to
8999776
Compare
|
ah great to hear :) i might dare a second one this weekend :) |
royvardhan
pushed a commit
to royvardhan/revm
that referenced
this pull request
Dec 20, 2024
* implement serde for interpreter * add bytecode check * remove bytecode check * remove defaults generic params * add sanity check + cargo fmt * modify panic message
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I tried to tackle #1890
I am not sure this is correct. To my understanding the instruction_pointer is just a pointer and so i have replaced it with the program counter for serialization which i know is not the same but i also wonder what the value of that pointer is in a different context. To my understanding doing an absolute jump back to that offset should give us the correct instruction_pointer again when deserializing.
I probably need some guidance here, if this is too far from expecatiations please let me know i can also just let it go :)
In case this is correct i probably have to add a test where the instruction_pointer changes i.e a running execution. What is the best way to mock the host, do i have to implement a transaction, block etc. from scratch? @rakita thanks!