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

feature: Add engine API forkchoice updated information in fixtures #256

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

spencer-tb
Copy link
Collaborator

@spencer-tb spencer-tb commented Aug 16, 2023

Similar to the following PR: #183 but for engine API forkchoice updated payload attriubes & version.

This information can be extracted and utilized in hive. Tagging ethereum/hive#830.

@spencer-tb spencer-tb force-pushed the forkchoice-in-fixtures branch from 8d00032 to 1dd0e4c Compare September 4, 2023 11:50
@spencer-tb spencer-tb marked this pull request as ready for review September 5, 2023 09:18
@spencer-tb
Copy link
Collaborator Author

Please note that this extra information is somewhat redundant as currently we only really need the forkchoice version within the pyspecs simulator, and at the moment this is equal to the EngineNewPayload version.

On the other hand ✋, it makes the EngineAPI information more complete and future proof. We may get more some use out of it moving forward for other simulators, i.e devp2p followed by engine_forkchoiceUpdated etc

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

A few comments but no full review yet.

src/ethereum_test_tools/common/types.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/spec/blockchain_test.py Outdated Show resolved Hide resolved
@spencer-tb
Copy link
Collaborator Author

Only keeping the fcu version for now as we don't have a definitive use for payload attributes at the moment.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

I think it's ok as is right now, but I would much rather keep this out of the filling logic, and keep it inside of the spec generator logic, because if we refactor the state tests, there is no place in the state test format for forkchoice updated or anything like that, so this would be something that only the blockchain test format would require.

src/ethereum_test_tools/filling/fill.py Outdated Show resolved Hide resolved
@spencer-tb spencer-tb requested a review from marioevz September 19, 2023 22:31
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

LGTM! :)

@spencer-tb spencer-tb merged commit eab85da into ethereum:main Sep 20, 2023
4 checks passed
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