feat(test-cli): Add support for testing block building via simulator#2679
feat(test-cli): Add support for testing block building via simulator#2679fselmo wants to merge 3 commits intoethereum:forks/amsterdamfrom
Conversation
- Test build building via ``testing_buildBlockV1``, validating the built block, and re-consuming against the client
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2679 +/- ##
================================================
Coverage 86.25% 86.26%
================================================
Files 599 599
Lines 37032 37038 +6
Branches 3795 3795
================================================
+ Hits 31943 31949 +6
Misses 4525 4525
Partials 564 564
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Wire up slotnum for `testing_buildBlockV1` for `bal-devnet-3` branch We are experimenting testing block building through hive via EELS (PR [here](ethereum/execution-specs#2679)). This is the only change needed to test against `bal-devnet-3` - missing slotnum.
marioevz
left a comment
There was a problem hiding this comment.
Looks great! Just two comments to try to improve maintainability.
5852e2d to
795a4f9
Compare
|
@marioevz lmk if the last commit here is what you meant. I re-ran against geth |
| "-p", | ||
| "execution_testing.cli.pytest_commands.plugins.consume.simulators.build_block.conftest", | ||
| ] | ||
| ) |
There was a problem hiding this comment.
This is not important but maybe we could have something like this for the entire if-else comparison:
command_name = self.command_name
simulator_command_list = ["engine", "enginex", ...]
if command_name in simulator_command_list:
modified_args.extend(
[
"-p",
f"execution_testing.cli.pytest_commands.plugins.consume.simulators.{command_name}.conftest",
]
)
else:
raise ValueError(f"Unknown command name: {self.command_name}")i would like to continue on reviewing the remaining part of the PR!
There was a problem hiding this comment.
Thank you @fselmo for this feature! I leave some comment but they are all style-related issue.
For test_via_build.py, i think test_blockchain_via_build is too large as a single function and has too much nested logic. Could we split it into smaller helpers?
My proposal:
test_blockchain_via_build
-> bootstrap_engine_at_genesis: handle genesis block FCU, verify genesis hash
-> Extract valid payload and calls build_validate_and_advance for each payload
build_validate_and_advance
-> build block
-> validate block response and gas limit
-> proceed the chain with newPayload
-> update the chain with FCU
edit: two more things for the new command
- Could we add some docs for the new command?
- Should we PR to Hive for the new simulator?
| return PytestCommand( | ||
| config_file="pytest-consume.ini", | ||
| argument_processors=[ | ||
| HelpFlagsProcessor("build_block"), |
There was a problem hiding this comment.
| HelpFlagsProcessor("build_block"), | |
| HelpFlagsProcessor("consume"), |
The --help command is currently not working, this is the error message after i tried:
ERROR: usage: build-block [options] [file_or_dir] [file_or_dir] [...]
build-block: error: unrecognized arguments: --build_block-help
inifile: /Users/caijiacheng/Documents/Ethereum/eels-worktree/wt-eip-7778/packages/testing/src/execution_testing/cli/pytest_commands/pytest_ini_files/pytest-consume.ini
rootdir: /Users/caijiacheng/Documents/Ethereum/eels-worktree/wt-eip-7778
After we register build_block in HelpFlagsProcessor, it would be processed to --build_block-help command (see process_args in HelpFlagsProcessor).
But there is no such command registered under helper.py, only --consume-help, --fill-help and others.
We either register --build_block-help or update to HelpFlagsProcessor("consume").
| @click.command( | ||
| name="build-block", | ||
| context_settings={ | ||
| "help_option_names": ["-h", "--help"], |
There was a problem hiding this comment.
| "help_option_names": ["-h", "--help"], |
Do we need this? I checked common_pytest_options and found there is built-in help decorator:
# check common_pytest_options function
func = click.option(
"-h",
"--help",
"help_flag",
is_flag=True,
default=False,
expose_value=True,
help="Show help message.",
)(func)| if TYPE_CHECKING: | ||
| from execution_testing.rpc.rpc_types import PayloadAttributes | ||
|
|
There was a problem hiding this comment.
| if TYPE_CHECKING: | |
| from execution_testing.rpc.rpc_types import PayloadAttributes |
I think we dont need this? as you import PayloadAttributes in get_payload_attributes function, so there should be no effect of circular import.
| parent_beacon_block_root = ( | ||
| self.params[2] if len(self.params) >= 3 else None | ||
| ) |
There was a problem hiding this comment.
| parent_beacon_block_root = ( | |
| self.params[2] if len(self.params) >= 3 else None | |
| ) | |
| parent_beacon_block_root = ( | |
| self.params[2] if self.forkchoice_updated_version >= 3 else None | |
| ) |
I think this is much clear. So now we know that the parent_beacon_block_root only exists starting from version 3.
| max_attempts=30, | ||
| wait_fixed=1.0, |
There was a problem hiding this comment.
| max_attempts=30, | |
| wait_fixed=1.0, |
Removed since they are default value
| expected = genesis_header.block_hash | ||
| got = genesis_block["hash"] | ||
| logger.fail( | ||
| f"Genesis block hash mismatch. " | ||
| f"Expected: {expected}, Got: {got}" | ||
| ) |
There was a problem hiding this comment.
| expected = genesis_header.block_hash | |
| got = genesis_block["hash"] | |
| logger.fail( | |
| f"Genesis block hash mismatch. " | |
| f"Expected: {expected}, Got: {got}" | |
| ) |
I notice this comparison logic is covered in GenesisBlockMismatchExceptionError:
# inside GenesisBlockMismatchExceptionError
message = (
"Genesis block hash mismatch.\n\n"
f"Expected: {expected_header.block_hash}\n"
f" Got: {got_genesis_block['hash']}."
)
danceratopz
left a comment
There was a problem hiding this comment.
Really nice!! 🔥
Nice review by @LouisTsai-Csie. In addition, I added one comment below about adding execution request verification for forks >= Prague.
About the naming, imo, I don't think this merits a new top-level command namespace and it that this rather belongs under consume, please see the comment below.
Docs
Also, could you please add some docs for this? Assuming you rename under consume, we could add it to:
docs/running_tests/running.mddocs/consume/simulators.md?
If we don't rename, then it will be trickier to find a spot to document this, but I think this demonstrates that it belongs to the consume family? But it could go in a new section docs/running_tests/build_block/index.md. But I'm strongly in favor of not adding a new top-level entry-point for this functionality.
System Test
Last request, could you add an e2e test in https://github.com/ethereum/execution-specs/blob/c9d8be0101c6649e060950317e14fb4f515f70e9/.github/workflows/hive-consume.yaml ? It should be pretty quick!
| expected=expected_payload, | ||
| parent_gas_limit=parent_gas_limit, | ||
| ) | ||
|
|
There was a problem hiding this comment.
We should check the execution requests here for >= v4. I.e., that build_response.execution_requests == payload.params[3].
There was a problem hiding this comment.
Potential follow-up: It would also be really nice to test build_response.blobs_bundle, but we don't include commitments/proofs in any fixture format yet. We just have these tests that are execute mode only:
execution-specs/tests/osaka/eip7594_peerdas/test_get_blobs.py
Lines 352 to 367 in c9d8be0
| @click.command( | ||
| name="build-block", |
There was a problem hiding this comment.
This command does use the testing_buildBlockV1 endpoint, but I think it's rather close to a consume simulator (it still consumes a fixture) so I wondered if it should be a consume sub-command?
I.e., something like uv run consume build-block.
Then the simulator in hive would be eels/consume-build-block.
Imo, when we add a command to fill tests using the testing_buildBlockV1 in the future, that command should be the new build or build-block command. Wdyt?
🗒️ Description
Adds a hive-based simulator that verifies EL clients build correct blocks via
testing_buildBlockV1For each payload in a blockchain_test_engine fixture, the test sends the expected transactions and block attributes to the client's block building endpoint. The resulting block is validated against the fixture's expected output (
state_root,receipts_root, etc). The fixture block is then imported viaengine_newPayloadVXto advance the chain for subsequent blocks.gas_limitis validated a bit different for now, sincetesting_buildBlockV1doesn't accept a gas limit parameter. The client picks its own within the EIP-1559 adjustment range so we validate it is within that range until we can actually account for the gas limit. For this reason,block_hashis also excluded from comparison as it depends ongas_limit.Also looking for recs on the naming (
uv run build-block). We could go withbuildby itself but it feels vague to me. That said, I can be easily convinced here 😄.🔗 Related Issues or PRs
closes #2560
✅ Checklist
just statictype(scope):.Cute Animal Picture