-
Notifications
You must be signed in to change notification settings - Fork 112
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
feat(fw): allow adding verbatim bytes to Bytecode #1119
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.
LGTM. I pushed some minor fixes and will merge once the CI passes.
Regarding the inconsistencies, this feature should be handled with care anyway, and I think we should only use it when absolutely necessary, so I will keep an eye on usages during reviews to make sure it doesn't get out of control :P |
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.
I had some late comments
@@ -116,7 +116,7 @@ def test_rjump_truncated_rjump_2( | |||
): | |||
"""EOF1I4200_0002 (Invalid) EOF code containing truncated RJUMP.""" | |||
eof_test( | |||
data=Container.Code(Op.RJUMP + Op.STOP), | |||
data=Container.Code(Op.RJUMP + b"\x00"), |
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.
to address @marioevz 's concerns, maybe this should be Op.RJUMP + verbatim(b"\x00")
, and verbatim
produces some type which can be added to Bytecode
but isn't bytes
explicitly, possibly Bytecode
would be fine? This would make spotting abuse much easier.
Basically verbatim
could be what @chfast envisioned for the constructor Bytecode
.
* feat(fw): allow adding verbatim bytes to Bytecode * fix: linting, tests --------- Co-authored-by: Mario Vega <[email protected]>
🗒️ Description
This provides a basic requested feature to allow concatenating
Bytecode
with verbatimbytes
.This implementation overload the
__add__
and make the basic use case work.However, this is not very consistent. E.g.
code + b"\x00"
means something different thancode + Bytecode(b"\x00")
Bytecode() + b"\x00"
works in places whereBytecode()
is expected butb"\x00"
don't.The reason this is not a complete solution is that
Bytecode()
already has a constructor frombytes
but its purpose is different and unknown to me. We should consider changing the semantic of conversion from bytes toBytecode()
by just copying the bytes.There is one more option for handling truncated opcode immediate bytes: take verbatim bytes from
[]
, e.g.Op.PUSH3[b"\x00"]
. This currently compiles, butBytecode
also tries to be smart here and extends the provided bytes to the desired length. I'm not 100% sure what it does but I couldn't built a code with truncated immediates this way. However, there is one example that seems to be working:execution-spec-tests/tests/osaka/eip7692_eof_v1/eip4200_relative_jumps/test_rjumpv.py
Line 273 in ca91240
🔗 Related Issues
✅ Checklist
mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.