-
Notifications
You must be signed in to change notification settings - Fork 162
feat(benchmark): add clz opcode test case #1845
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
base: main
Are you sure you want to change the base?
Conversation
sender=pre.fund_eoa(), | ||
) | ||
|
||
if (tx_gas_limit is None) or (tx_gas_limit > attack_gas_limit): |
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.
This if-statement is for EIP-7852 for transaction limit cap, more details could be found in this issue.
We should have a wrapper for all the benchmark test cases, but i will leave it in a separate PR.
code_seq = Bytecode() | ||
|
||
for i in range(2 * iteration): | ||
value = i if i % 2 else 2**256 - 1 - i |
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 second test case, I aim to add some randomness. Using a simple range from 1 to iteration results in many values being too similar.
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 get the idea but I would set this up differently. We have code_prefix
and code_suffix
so we know that we can add at most max_code_size - len(code_prefix) - len(code_suffix)
bytes. I'd keep incrementing some number here and then let CLZ work on 2^256 - 1 >> i % 256
. This i
increments each round and will thus right shift the values such that CLZ yields a different value. Note that for each CLZ you add you should check Op.CLZ(value) + Op.POP
if this length does not exceed the max size (else break
). This length is not constant because it will depend on how many bytes we are pushing (to push 2^256-1
need PUSH32
, and for 1
need PUSH1
)
b731387
to
561a50e
Compare
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.
Some comments π π
Need to also think on how to use these benchmarks, i.e. how to test these against clients and how to interpret the results. The end goal could be to determine the gas price of the opcode. For that we also need a baseline in order to interpret the results and thus also to price the opcode. This is non-trivial because these benchmarks use other opcodes as well, and we thus have to find a way to measure the costs for CLZ only (and not the cost of the other opcodes). Food for thought!
Left a comment on one of the tests. The 248 trick is beautiful π π
value = i if i % 2 else 2**256 - 1 - i | ||
code_seq += Op.CLZ(value) + Op.POP | ||
|
||
code_address = pre.deploy_contract(code=code_prefix + code_seq + code_suffix) |
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.
This will deploy a contract too large. Could you add the sanity check?
code_address = pre.deploy_contract(code=code_prefix + code_seq + code_suffix) | |
code_address = pre.deploy_contract(code=code_prefix + code_seq + code_suffix) | |
if len(attack_code) > max_contract_size: | |
raise ValueError( | |
f"Code size {len(attack_code)} exceeds maximum code size {max_contract_size}" | |
) |
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 might go with the following approach for consistency now, since the remaining benchmarks use it as well.
However, we could add this refactoring for all the assertions later in this issue
assert len(attack_code) <= max_code_size
code_seq = Bytecode() | ||
|
||
for i in range(2 * iteration): | ||
value = i if i % 2 else 2**256 - 1 - i |
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 get the idea but I would set this up differently. We have code_prefix
and code_suffix
so we know that we can add at most max_code_size - len(code_prefix) - len(code_suffix)
bytes. I'd keep incrementing some number here and then let CLZ work on 2^256 - 1 >> i % 256
. This i
increments each round and will thus right shift the values such that CLZ yields a different value. Note that for each CLZ you add you should check Op.CLZ(value) + Op.POP
if this length does not exceed the max size (else break
). This length is not constant because it will depend on how many bytes we are pushing (to push 2^256-1
need PUSH32
, and for 1
need PUSH1
)
561a50e
to
055ecf9
Compare
@jochem-brouwer Thanks for your review, I've updated accordingly! Please let me know if the change is good to you. |
055ecf9
to
72d22ab
Compare
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.
Amazing implementation, thanks!
I think this needs a careful rebase since I tried rebasing locally and the merge messed up the test, @LouisTsai-Csie could you try rebasing to latest main and see that it works? thanks! |
72d22ab
to
478c74f
Compare
@marioevz I did not see any conflict from my side, but I still rebase to the latest changes. Let's see if this works. |
ποΈ Description
Add benchmark test for CLZ opcode.
π Related Issues or PRs
Issue #1795
β 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.