Skip to content
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

Test EXTCALL creation gas charge #1025

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

Conversation

shemnon
Copy link
Collaborator

@shemnon shemnon commented Dec 16, 2024

πŸ—’οΈ Description

Test the creation gas charge of a contract when the contract was called and remained "empty."

πŸ”— Related Issues

βœ… Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

Test the creation gas charge of a contract when the contract was called
and remained "empty."

Signed-off-by: Danno Ferrin <danno@numisight.com>
Comment on lines 210 to 211
setup_code=Bytecode(None),
subject_code=opcode(address=empty_address) + Op.EXTCALL(address=empty_address, value=1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not 100% sure but this layout seems to not discriminate against where ACCOUNT_CREATION_GAS is actually charged - opcode or Op.EXTCALL.

Would an alternative where opcode is in the setup_code make sense? It would require some fiddling with warm/cold (because the baseline code would warm the address), but would be more "by-the-book" I guess

Copy link
Collaborator

Choose a reason for hiding this comment

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

2nd comment - the idea of gas_test would be to have the pushes in setup_code, decoupling the test from push costs. Note that the situation in the test just above is different

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opcode does not have value passed in as part of the call. so ACCOUNT_CREATION_GAS will not be added to that call.

Also, running an EXTCALL in setup is not side-effect free. A warm cost is embedded and winds up messing up the calculations.

Also, moving the pushes to the setup ruins the readability because you have to then know what values are in what slot, and extra messiness because EXTCALL has an extra argument for value=0 that would need to be conditionally handled.

And the fuzzing failure in Besu was the whole subject code: a call to an empty account that stayed empty, then a call to the empty account that fills it and gets charged the account creation cost.

Copy link
Collaborator

@pdobacz pdobacz Dec 18, 2024

Choose a reason for hiding this comment

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

This is all true. If those aren't easily fixable, I suggest not using gas_test at all. Just call the thing and compare gas to one calculated by hand (it is effectively what you're doing in the current version anyway). I don't think you're leveraging the gas_test abstraction at all, and in case the test fails for any reason, it will just be harder to debug.

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 would wind up using the key of the gas test, just that I am testing a string of opcodes and not just a single operation. I need "cold" and "warm" execution, as the account existing is what's different from the first an second call. Then I need to measure gas, which requires the whole legacy harness.

Perhaps we need to add to the harness a preamble code option that will be executed and not included in the measurement calculations. This will allow the initial call to the non-existent address to be warmed before the setup code is called.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then I need to measure gas, which requires the whole legacy harness

ah, I forgot state_test doesn't allow us to expect gas consumed by tx :(.

preamble_code sounds ok, just easy to be confused with setup_code, so pls make sure it's well documented. Maybe after we merge this, I'll spend some time rethinking and possibly we can make them back into one, I had issues with setup_code before too.

And I would move the second PUSHes to setup, optionality is easy to handle with an + if PUSH else NOOP or ideally - the test parameters, which is what is usually suggested here.

Signed-off-by: Danno Ferrin <danno@numisight.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants