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

feat(fw): add optional verify_sync flag to hive blockchain tests #431

Merged

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Feb 8, 2024

🗒️ Description

Adds an optional verify_sync flag for hive (or blockchain Engine API tests) to pick up and test a second client syncing to the first client.

Syncing is tricky to some clients and we actually need to send a full header via engine_newPayloadVN for some of them to start the sync process.

So this change also appends an empty block on top of the latest head of the test to use as syncing head.

🔗 Related Issues

None.

✅ 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: 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.

@marioevz marioevz added type:feat type: Feature scope:consume Scope: Consume command suite scope:fw Scope: Framework (evm|tools|forks|pytest) labels Feb 8, 2024
@spencer-tb
Copy link
Collaborator

spencer-tb commented Feb 9, 2024

I like this a lot! 💯

Are you thinking that syncing should be specific to certain tests, after processing and setting the head post complex scenario, or applied to the end of all tests? Or even simple specific sync tests for each fork?

I tried running this modification for some the blob_txs tests, using your hive branch and it worked well! Personally it seems reasonable to do this for all tests.

One question arises for me: instead of creating a sync payload could we not use the last engine new payload in the test?

If we use a sync payload, when we integrate consume fully within EEST it should be straight forward enough to generate this payload on the fly.

@marioevz
Copy link
Member Author

marioevz commented Feb 9, 2024

One question arises for me: instead of creating a sync payload could we not use the last engine new payload in the test?

The problem is that if the last engine payload is the one that triggers some special scenario you'd like to test under sync, and you provide this payload via the engine API, then we are not really testing syncing because the payload arrives in full via the engine api. Even in cases where we have more than one payload, we won't be testing syncing for the last one.

@marioevz marioevz changed the title [WIP] feat(fw): add optional verify_sync flag to hive blockchain tests feat(fw): add optional verify_sync flag to hive blockchain tests Mar 8, 2024
@marioevz marioevz marked this pull request as ready for review March 8, 2024 20:06
@marioevz marioevz requested a review from spencer-tb March 8, 2024 20:07
@spencer-tb spencer-tb force-pushed the add-sync-flag-to-blockchain-tests branch from 4834a2b to 31ee9ad Compare March 9, 2024 02:34
Copy link
Collaborator

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

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

LGTM - added the changelog.

@spencer-tb spencer-tb merged commit c8e0caa into ethereum:main Mar 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:consume Scope: Consume command suite scope:fw Scope: Framework (evm|tools|forks|pytest) type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants