-
Notifications
You must be signed in to change notification settings - Fork 153
feat(fill): implement address tag resolution for static test fillers #1781
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
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.
Excited about this! Just a few minor comments after a fleeting review. I can try it out and have a closer look tomorrow!
# Apply fixture format filtering for pre-allocation groups | ||
generate_pre_alloc = self.config.getoption("generate_pre_alloc_groups") | ||
use_pre_alloc = self.config.getoption("use_pre_alloc_groups") | ||
if generate_pre_alloc or use_pre_alloc: | ||
# When pre-allocation group flags are set, only generate | ||
# BlockchainEngineXFixture | ||
filtered_formats = [ | ||
format_item | ||
for format_item in fixture_formats | ||
if ( | ||
format_item is BlockchainEngineXFixture | ||
or ( | ||
isinstance(format_item, LabeledFixtureFormat) | ||
and format_item.format is BlockchainEngineXFixture | ||
) | ||
) | ||
] | ||
else: | ||
# Filter out BlockchainEngineXFixture if pre-allocation group | ||
# flags not set | ||
filtered_formats = [ | ||
format_item | ||
for format_item in fixture_formats | ||
if not ( | ||
format_item is BlockchainEngineXFixture | ||
or ( | ||
isinstance(format_item, LabeledFixtureFormat) | ||
and format_item.format is BlockchainEngineXFixture | ||
) | ||
) | ||
] | ||
|
||
for format_with_or_without_label in filtered_formats: |
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 think this was copied verbatim from filler.py
. Perhaps we can add a helper function, but perhaps it's not worth it?
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 can look into DRYing this up before we're ready to merge 👍🏼
77fbcde
to
b996b05
Compare
5a359c9
to
0af7ed8
Compare
I've managed to fine tune quite a few fillers that were not passing the tagging system. There are still edge cases I think can be cleaned up and I think some of the
My hope is there are more tests that can be turned on as well for tagging but examining all the tests and figuring out general approaches was my first attempt and I wanted to get this to a good first pass since it has taken up a lot of dev time. The list of currently incompatible tests is here. This could be converted to a list in the documentation somewhere once it's finalized for this first pass. |
- Use Prague.precompiles() for precompile addresses in convert_addresses.py
…agging. - Wrap up stTransaction tests fine-tuning - [fine-tuning] Replace 0 address in CALL code if in pre - Changes from comments on PR ethereum#1781: - Use Prague.precompiles() for precompile addresses in convert_addresses.py - Revamp script, simplify Claude code over-engineering. - fine tune by not tagging some addrs, add short name tag compat for selected tests
0f8ad98
to
a2c361d
Compare
OK, I made my last pass through the |
…agging. - Wrap up stTransaction tests fine-tuning - [fine-tuning] Replace 0 address in CALL code if in pre - Changes from comments on PR ethereum#1781: - Use Prague.precompiles() for precompile addresses in convert_addresses.py - Revamp script, simplify Claude code over-engineering. - fine tune by not tagging some addrs, add short name tag compat for selected tests
572ed90
to
9f56ecf
Compare
…ts for tagging. - Wrap up stTransaction tests fine-tuning - [fine-tuning] Replace 0 address in CALL code if in pre - Changes from comments on PR ethereum#1781: - Use Prague.precompiles() for precompile addresses in convert_addresses.py - Revamp script, simplify Claude code over-engineering. - fine tune by not tagging some addrs, add short name tag compat for selected tests
57a0f2f
to
d165e53
Compare
…ts for tagging. - Wrap up stTransaction tests fine-tuning - [fine-tuning] Replace 0 address in CALL code if in pre - Changes from comments on PR ethereum#1781: - Use Prague.precompiles() for precompile addresses in convert_addresses.py - Revamp script, simplify Claude code over-engineering. - fine tune by not tagging some addrs, add short name tag compat for selected tests
Hard-coded address conversion in yml and json fillers: - Add convert_addresses.py script to automate tag conversion - The correct way to run this is with the ``CONVERT_COINBASE`` flag set to ``False`` as this allows the same coinbase for all tests (just as python tests do). If we decide we want to handle the coinbase setting on the python side, we can turn this flag on and hard-code on the python side... but the currect approach seems correct. - Convert 1000+ static test YAML/JSON files to use address tags (Python) Generate deterministic addresses from tags coming from static test fillers: - Resolve tags to deterministic addresses in the same way python tests do - via pytest static filler plugin - Add ``BlockchainEngineXFixture`` support for pre-allocation groups This enables static tests to use symbolic address tags instead of hardcoded addresses, minimizing muddied context across tests when running via pre alloc sharing. ---- fix(tests/static): Fine tune addr tag script, turn on more static tests for tagging. - Wrap up stTransaction tests fine-tuning - [fine-tuning] Replace 0 address in CALL code if in pre - Changes from comments on PR ethereum#1781: - Use Prague.precompiles() for precompile addresses in convert_addresses.py - Revamp script, simplify Claude code over-engineering. - fine tune by not tagging some addrs, add short name tag compat for selected tests
c8d33ec
to
1736983
Compare
Hard-coded address conversion in yml and json fillers: - Add convert_addresses.py script to automate tag conversion - The correct way to run this is with the ``CONVERT_COINBASE`` flag set to ``False`` as this allows the same coinbase for all tests (just as python tests do). If we decide we want to handle the coinbase setting on the python side, we can turn this flag on and hard-code on the python side... but the currect approach seems correct. - Convert 1000+ static test YAML/JSON files to use address tags (Python) Generate deterministic addresses from tags coming from static test fillers: - Resolve tags to deterministic addresses in the same way python tests do - via pytest static filler plugin - Add ``BlockchainEngineXFixture`` support for pre-allocation groups This enables static tests to use symbolic address tags instead of hardcoded addresses, minimizing muddied context across tests when running via pre alloc sharing. ---- fix(tests/static): Fine tune addr tag script, turn on more static tests for tagging. - Wrap up stTransaction tests fine-tuning - [fine-tuning] Replace 0 address in CALL code if in pre - Changes from comments on PR ethereum#1781: - Use Prague.precompiles() for precompile addresses in convert_addresses.py - Revamp script, simplify Claude code over-engineering. - fine tune by not tagging some addrs, add short name tag compat for selected tests
1736983
to
a39720e
Compare
Hmm it looks like running with edit: I actually need to re-investigate the enginex flow altogether after the pydantic changes. It doesn't seem to be working as expected. |
4ec6097
to
87eb659
Compare
Ok @danceratopz @marioevz, there are a few things to consider here.
As of now, even before the last commit for the shared coinbase logic, I am getting timeout failures when running the tests with the shared pre alloc: |
87eb659
to
371032a
Compare
Hard-coded address conversion in yml and json fillers: - Add convert_addresses.py script to automate tag conversion - The correct way to run this is with the ``CONVERT_COINBASE`` flag set to ``False`` as this allows the same coinbase for all tests (just as python tests do). If we decide we want to handle the coinbase setting on the python side, we can turn this flag on and hard-code on the python side... but the currect approach seems correct. - Convert 1000+ static test YAML/JSON files to use address tags (Python) Generate deterministic addresses from tags coming from static test fillers: - Resolve tags to deterministic addresses in the same way python tests do - via pytest static filler plugin - Add ``BlockchainEngineXFixture`` support for pre-allocation groups This enables static tests to use symbolic address tags instead of hardcoded addresses, minimizing muddied context across tests when running via pre alloc sharing. ---- fix(tests/static): Fine tune addr tag script, turn on more static tests for tagging. - Wrap up stTransaction tests fine-tuning - [fine-tuning] Replace 0 address in CALL code if in pre - Changes from comments on PR ethereum#1781: - Use Prague.precompiles() for precompile addresses in convert_addresses.py - Revamp script, simplify Claude code over-engineering. - fine tune by not tagging some addrs, add short name tag compat for selected tests
* All pydantic simplifications * refactor(tests/static): rename sender:key -> eoa:sender * refactor: rename, use generics * fix: consider empty accounts * fix(tests): tests with empty accounts * fix(tests): addressOpcodesFiller.yml * feat: significantly improve test ids * fix: bugs in tag resolution * fix(tests): CREATE2_HighNonceDelegatecallFiller.yml * fix: types * Update src/ethereum_test_specs/static_state/account.py --- Co-authored-by: felipe <[email protected]> * fix: comment and generic tag regex * fix: Code raw code tag substitution
fix: fix when tabs are found in lines with spaces; fix shortnames fix: Resolve issues with label: / raw: parsing Turn off more tests, mostly related to create / `creation` addresses
- Add marker for tagged + untagged tests to make them easier to identify
371032a
to
a9842ec
Compare
🗒️ Description
Hard-coded address conversion in
.yml
and.json
static fillers:convert_addresses.py
script to automate tag conversionCONVERT_COINBASE
flag set toFalse
as this allows the same coinbase for all tests (just as python tests do). If we decide we want to handle the coinbase setting on the python side, we can turn this flag on and hard-code on the python side... but the current approach seems correct.DO_NOT_TAG_ADDRESSES
: Don't tag certain addresses crucial for some tests but only for addresses that were made sure to not appear in any other test.SHORT_NAME_FILLERS
: This finds and replaces short name for addresses (e.g.0x0000...000dead1
->0xdead1
). This turns on a few more fillers that were not working.NO_TAGS_IN_CODE
: Don't tag matches inside bytecode sections for these tests:data
,code
,storage
.Generate deterministic addresses from tags coming from static test fillers in python code:
BlockchainEngineXFixture
support for pre-allocation groupsThis enables static tests to use symbolic address tags instead of hardcoded addresses, minimizing muddied context across tests when running via pre-alloc sharing.
Note: This is a very large change with very many nuances. This is a first pass at a generalized approach. I think maybe we can add complexity and attempt at turning on more tests eventually.
🔗 Related Issues
related to #1750
✅ Checklist
mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.