-
Notifications
You must be signed in to change notification settings - Fork 177
feat(fill, execute): track execution & setup testing phase #2157
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?
feat(fill, execute): track execution & setup testing phase #2157
Conversation
ef9acbe
to
ec37e3a
Compare
Note: I have not yet tested against |
ec37e3a
to
9f85ffc
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.
Hey @LouisTsai-Csie. I don't have much to add on the implementation. This looks great. I'm going to test this early next week but just had some minor observations at first pass and left as comments.
I wanted to just get a bit of clarity on the scope on how the metadata of each phase will be used. I think we're capturing the data in a good way and I will continue my review next week, but I'd like to understand all the ways we currently plan to use this kind of metadata to get a better understanding for the design.
I think we mentioned being able to call execute
on a network with a setup
phase first, and later call it with the execution
phase. Is there any other case I'm missing so I can understand the end goal on capturing this split? Thanks!
cc: @marioevz
assert len(manager.setup_blocks) == 0 | ||
assert len(manager.execution_transactions) == 0 | ||
assert len(manager.execution_blocks) == 0 | ||
assert manager.get_current_phase() == TestPhase.EXECUTION |
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.
Perhaps we should have a test with two managers which tests that changing the phase of one does not impact the other (no context muddying). This will be the case for sure because the phase is set on the instance, but just to sanity check against point 2 in this comment in the old PR in case the phase logic ever changes.
It could be simple... something like entering a setup
context block with manager1
, checking that its context changed to setup
, and making sure manager2.get_current_phase()
is still execution
(default).
Thoughts?
block_access_list: Bytes | None = Field(None) | ||
"""EIP-7928: Block-level access lists (serialized).""" | ||
""" | ||
EIP-7928: Block-level access lists (serialized). |
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.
nit: Any reason this was changed? There's an empty space here but we can probably put it back to one line? 👀
🗒️ Description
Background for reviewers:
In some benchmark tests, there are not only benchmark transactions but also
setup
transactions that create the scenario for the benchmark target. For example, in theSSTORE
benchmark we first need asetup
transaction that deploys as many contracts as possible, and then a separate transaction to perform the storage updates. This feature helps gas-limit testing distinguish between thesetup
andexecution
phases.This PR extends transaction metadata, which labeling phases such as
setup
,testing
, andcleanup
. Currently, operations likedeploy_contract
orfund_eoa
are tagged assetup
phase, while transactions/blocks included instate_test
orblockchain_test
are tagged as testing. However, the current labeling is not always precise enough.Take
test_worst_blockhash
as an example. The first 256 blocks should be classified assetup
, while only the final block should count as the testing phase. Without this distinction, we might mistakenly include the setup blocks in benchmark accounting.The design introduces a
test_phase
attribute at both the block and transaction level (currently supportingexecution
andsetup
phases). This attribute is used during execution (seetransaction_post.py
for details), and the metadata is updated accordingly.🔗 Related Issues or PRs
This is the follow-up PR for #1945, more description could be found in issue #2137.
Related discussion: 1, 2
✅ 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.