Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LouisTsai-Csie
Copy link
Collaborator

@LouisTsai-Csie LouisTsai-Csie commented Jul 2, 2025

πŸ—’οΈ Description

Add benchmark test for CLZ opcode.

πŸ”— Related Issues or PRs

Issue #1795

βœ… Checklist

  • All: Ran fast 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
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

attack_gas_limit = env.gas_limit
tx_gas_limit = fork.transaction_gas_limit_cap()

magic_value = 248 # CLZ(248) = 248
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test case, each CLZ operation uses the same input. I discovered that CLZ(248) == 248, which makes it more efficient than the approach where we first push a value and then DUP it before calling CLZ.

To verify, you could try this script:

# This function is from https://github.com/ethereum/EIPs/blob/master/EIPS/eip-7939.md
def clz(x):
    """Returns the number of zeros preceding the most significant one bit."""
    if x < 0:
        raise ValueError("clz is undefined for negative numbers")
    if x > 2**256 - 1:
        raise ValueError("clz is undefined for numbers larger than 2**256 - 1")
    if x == 0:
        return 256
    # Convert to binary string and remove any '0b' prefix.
    bin_str = bin(x).replace('0b', '')
    return 256 - len(bin_str)

bits = 0
magic_value = 248
stack = [magic_value]

for i in range(100):
    x = stack.pop()
    assert clz(x) == magic_value
    stack.append(clz(x))

sender=pre.fund_eoa(),
)

if (tx_gas_limit is None) or (tx_gas_limit > attack_gas_limit):
Copy link
Collaborator Author

@LouisTsai-Csie LouisTsai-Csie Jul 2, 2025

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
Copy link
Collaborator Author

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.

Copy link
Member

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)

@LouisTsai-Csie LouisTsai-Csie marked this pull request as ready for review July 3, 2025 12:26
@LouisTsai-Csie LouisTsai-Csie self-assigned this Jul 3, 2025
@LouisTsai-Csie LouisTsai-Csie marked this pull request as draft July 4, 2025 14:45
@LouisTsai-Csie LouisTsai-Csie marked this pull request as ready for review July 4, 2025 14:45
Copy link
Member

@jochem-brouwer jochem-brouwer left a 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 πŸ˜„ πŸ‘

attack_gas_limit = env.gas_limit
tx_gas_limit = fork.transaction_gas_limit_cap()

magic_value = 248 # CLZ(248) = 248
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, this is a nice trick.

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)
Copy link
Member

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?

Suggested change
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}"
)

Copy link
Collaborator Author

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
Copy link
Member

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)

@LouisTsai-Csie
Copy link
Collaborator Author

@jochem-brouwer Thanks for your review, I've updated accordingly! Please let me know if the change is good to you.

Comment on lines +2378 to +2393
code_prefix = Op.JUMPDEST
code_suffix = Op.PUSH0 + Op.JUMP

available_code_size = max_code_size - len(code_prefix) - len(code_suffix)

code_seq = Bytecode()

for i in range(available_code_size):
value = (2**256 - 1) >> (i % 256)
clz_op = Op.CLZ(value) + Op.POP
if len(code_seq) + len(clz_op) > available_code_size:
break
code_seq += clz_op

attack_code = code_prefix + code_seq + code_suffix
assert len(attack_code) <= max_code_size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this whole logic is very generalizable, you set up the stack, setup a jumpdest then do 24k - prefix - suffix times / inner code include the operations then add the suffix.
I think this could be refactored out into a helper cc @marioevz e.g.

func createBenchLoop(prefix, code, suffix []byte) []byte{
    result := []byte
    result := append(result, prefix)
    dest := len(result)
    result := append(result, JUMPDEST)
    suffix := append(suffix, PUSH32)
    suffix := append(suffix, extend32(dest)
    suffixLen := len(suffix)
    
    available_code_size := max_contract_size - len(result) - suffixLen
    
    for i := range available_code_size / len(code)
         result := append(result, code)
    
    result := append(result, suffix)
    return result

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of our refactoring plan, we will create new test types: benchmark_test, and benchmark_state_test to handle the repetitive code, the transaction gas limit cap, and more. You could check more information in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants