-
Notifications
You must be signed in to change notification settings - Fork 157
fix(tests): fixes EXTCODECOPY bench test #1865
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
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.
Good catch!
Just in case, I debugged a run of both the previous and new code in Geth, and can confirm the bug and fix.
@@ -182,7 +182,7 @@ def test_worst_bytecode_single_opcode( | |||
|
|||
attack_call = Bytecode() | |||
if opcode == Op.EXTCODECOPY: | |||
attack_call = Op.EXTCODECOPY(address=Op.SHA3(32 - 20 - 1, 85), dest_offset=85, size=1000) | |||
attack_call = Op.EXTCODECOPY(address=Op.SHA3(32 - 20 - 1, 85), dest_offset=96, size=1000) |
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.
Parallel suggestion: maybe something like this can be clearer for readers:
attack_call = Op.EXTCODECOPY(address=Op.SHA3(32 - 20 - 1, 85), dest_offset=96, size=1000) | |
create2_params_offset = 32 - 20 - 1 | |
create2_params_size = 85 | |
attack_call = Op.EXTCODECOPY(address=Op.SHA3(create2_params_offset, create2_params_size), dest_offset=create2_params_offset+create2_params_size, size=1000) |
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.
Yes good point, I'll follow up to somewhat take out these "magic numbers" π π
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, thanks for catching this!
I've verified through tracing analysis that the fix works.
Checking the coverage drop:
This is basically a memory expansion test case, but I think it's out of scope for this PR. |
ποΈ Description
In #1864 I realized a problem with the EXTCODECOPY test. It copies to memory to offset 85. This is correct if the CREATE2 hash alignment starts at 0. It however starts at byte 11 (the address starts at 12). This means that the memory region
[11,96)
is occupied by the bytes to hash to get the correct address. If we thus EXTCODECOPY to byte 85, we overwrite the bytes there (initcode hash is stored there). This means that all EXTCODECOPYs after the first one target incorrect addressess, i.e. they have empty code.I checked this file if there are more of these tests but this seem to be the only one. (This could be a problem with other tests also using CREATE2 and using operations which writes to memory. It should not overwrite the to-be-hashed data to retrieve the CREATE2 address created from the factory)
CC @jsign
π Related Issues or PRs
N/A.
β Checklist
tox
checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
type(scope):
.mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_from
marker.