Changes from comments on PR ethereum#2007

- Use `pytest.mark.verify_sync` instead of the previous `verify_sync` flag on the blockchain test.
- Remove the syncing periodic checks as when the forkchoice_updated response is `VALID`, we can break.
- Update all files according to changes above.

Author: fselmo

docs/CHANGELOG.md

@@ -80,7 +80,7 @@ Users can select any of the artifacts depending on their testing needs for their
 - ✨ Generate unique addresses with Python for compatible static tests, instead of using hard-coded addresses from legacy static test fillers ([#1781](https://github.com/ethereum/execution-spec-tests/pull/1781)).
 - ✨ Added support for the `--benchmark-gas-values` flag in the `fill` command, allowing a single genesis file to be used across different gas limit settings when generating fixtures. ([#1895](https://github.com/ethereum/execution-spec-tests/pull/1895)).
 - ✨ Static tests can now specify a maximum fork where they should be filled for ([#1977](https://github.com/ethereum/execution-spec-tests/pull/1977)).
-- ✨ Add support for `BlockchainEngineSyncFixture` format for tests with `verify_sync=True` to enable client synchronization testing via `consume sync` command ([#2007](https://github.com/ethereum/execution-spec-tests/pull/2007)).
+- ✨ Add support for `BlockchainEngineSyncFixture` format for tests marked with `pytest.mark.verify_sync` to enable client synchronization testing via `consume sync` command ([#2007](https://github.com/ethereum/execution-spec-tests/pull/2007)).
 
 #### `consume`
 

docs/running_tests/test_formats/blockchain_test_sync.md

@@ -2,7 +2,7 @@
 
 The Blockchain Engine Sync Test fixture format tests are included in the fixtures subdirectory `blockchain_tests_sync`, and use Engine API directives to test client synchronization capabilities after fixtures are executed with valid payloads.
 
-These are produced by the `BlockchainTest` test spec when `verify_sync=True` is specified.
+These are produced by the `BlockchainTest` test spec when `pytest.mark.verify_sync` is used as a test marker.
 
 ## Description
 

src/ethereum_test_fixtures/blockchain.py

@@ -542,7 +542,6 @@ class BlockchainEngineFixture(BlockchainEngineFixtureCommon):
     genesis: FixtureHeader = Field(..., alias="genesisBlockHeader")
     post_state: Alloc | None = Field(None)
     payloads: List[FixtureEngineNewPayload] = Field(..., alias="engineNewPayloads")
-    sync_payload: FixtureEngineNewPayload | None = None
 
 
 @post_state_validator(alternate_field="post_state_diff")
@@ -566,11 +565,8 @@ class BlockchainEngineXFixture(BlockchainEngineFixtureCommon):
     payloads: List[FixtureEngineNewPayload] = Field(..., alias="engineNewPayloads")
     """Engine API payloads for blockchain execution."""
 
-    sync_payload: FixtureEngineNewPayload | None = None
-    """Optional sync payload for blockchain synchronization."""
-
 
-class BlockchainEngineSyncFixture(BlockchainEngineFixtureCommon):
+class BlockchainEngineSyncFixture(BlockchainEngineFixture):
     """
     Engine Sync specific test fixture information.
 
@@ -584,8 +580,4 @@ class BlockchainEngineSyncFixture(BlockchainEngineFixtureCommon):
     description: ClassVar[str] = (
         "Tests that generate a blockchain test fixture for Engine API testing with client sync."
     )
-    pre: Alloc
-    genesis: FixtureHeader = Field(..., alias="genesisBlockHeader")
-    post_state: Alloc | None = Field(None)
-    payloads: List[FixtureEngineNewPayload] = Field(..., alias="engineNewPayloads")
     sync_payload: FixtureEngineNewPayload | None = None

src/ethereum_test_specs/blockchain.py

@@ -404,9 +404,6 @@ class BlockchainTest(BaseTest):
     post: Alloc
     blocks: List[Block]
     genesis_environment: Environment = Field(default_factory=Environment)
-    # If set to True, generate a BlockchainEngineSyncFixture to
-    # verify syncing between clients.
-    verify_sync: bool = False
     chain_id: int = 1
     exclude_full_post_state_in_output: bool = False
     """
@@ -443,13 +440,14 @@ def discard_fixture_format_by_marks(
         """Discard a fixture format from filling if the appropriate marker is used."""
         marker_names = [m.name for m in markers]
 
-        if "blockchain_test_only" in marker_names:
-            return fixture_format != BlockchainFixture
-        if "blockchain_test_engine_only" in marker_names:
-            return fixture_format != BlockchainEngineFixture
-
-        # Exception tests cannot be used for sync testing
-        if "exception_test" in marker_names and fixture_format == BlockchainEngineSyncFixture:
+        if "blockchain_test_only" in marker_names and fixture_format != BlockchainFixture:
+            return True
+        if (
+            "blockchain_test_engine_only" in marker_names
+            and fixture_format != BlockchainEngineFixture
+        ):
+            return True
+        if fixture_format == BlockchainEngineSyncFixture and "verify_sync" not in marker_names:
             return True
 
         return False
@@ -833,10 +831,6 @@ def generate(
         fixture_format: FixtureFormat,
     ) -> BaseFixture:
         """Generate the BlockchainTest fixture."""
-        # Skip sync fixture generation if verify_sync is False
-        if fixture_format == BlockchainEngineSyncFixture and not self.verify_sync:
-            raise pytest.skip(f"Skipping sync fixture, verify_sync={self.verify_sync}")
-
         t8n.reset_traces()
         if fixture_format in [
             BlockchainEngineFixture,

src/pytest_plugins/consume/simulators/simulator_logic/test_via_sync.py

@@ -369,6 +369,12 @@ def test_blockchain_via_sync(
                 break
 
     if last_valid_payload:
+        last_valid_block_forkchoice_state = ForkchoiceState(
+            head_block_hash=last_valid_block_hash,
+            safe_block_hash=last_valid_block_hash,
+            finalized_block_hash=fixture.genesis.block_hash,
+        )
+
         try:
             version = last_valid_payload.new_payload_version  # log version used for debugging
             logger.info(f"Sending target payload via engine_newPayloadV{version}")
@@ -384,11 +390,7 @@ def test_blockchain_via_sync(
             # send forkchoice update pointing to latest block
             logger.info("Sending forkchoice update with last valid block to trigger sync...")
             sync_forkchoice_response = sync_engine_rpc.forkchoice_updated(
-                forkchoice_state=ForkchoiceState(
-                    head_block_hash=last_valid_block_hash,
-                    safe_block_hash=last_valid_block_hash,
-                    finalized_block_hash=fixture.genesis.block_hash,
-                ),
+                forkchoice_state=last_valid_block_forkchoice_state,
                 payload_attributes=None,
                 version=last_valid_payload.forkchoice_updated_version,
             )
@@ -437,9 +439,6 @@ def test_blockchain_via_sync(
 
         # Start monitoring sync progress
         sync_start_time = time.time()
-        last_sync_block_number = 0
-        sync_check_interval = 1.0
-        last_check_time = time.time()
         last_forkchoice_time = time.time()
         forkchoice_interval = 2.0  # Send forkchoice updates every 2 seconds
 
@@ -450,55 +449,19 @@ def test_blockchain_via_sync(
                     # Send forkchoice update to sync client to trigger/maintain sync
                     assert sync_engine_rpc is not None, "sync_engine_rpc is required"
                     sync_fc_response = sync_engine_rpc.forkchoice_updated(
-                        forkchoice_state=ForkchoiceState(
-                            head_block_hash=last_valid_block_hash,
-                            safe_block_hash=last_valid_block_hash,
-                            finalized_block_hash=fixture.genesis.block_hash,
-                        ),
+                        forkchoice_state=last_valid_block_forkchoice_state,
                         payload_attributes=None,
                         version=fixture.sync_payload.forkchoice_updated_version
                         if fixture.sync_payload
                         else fixture.payloads[-1].forkchoice_updated_version,
                     )
                     status = sync_fc_response.payload_status.status
                     logger.debug(f"Periodic forkchoice update status: {status}")
+                    if status.VALID:
+                        break
                     last_forkchoice_time = time.time()
                 except Exception as fc_err:
                     logger.debug(f"Periodic forkchoice update failed: {fc_err}")
-
-            # Check sync progress periodically
-            if time.time() - last_check_time >= sync_check_interval:
-                try:
-                    # Check current sync status
-                    assert sync_eth_rpc is not None, "sync_eth_rpc is required"
-                    sync_latest = sync_eth_rpc.get_block_by_number("latest")
-                    if sync_latest:
-                        sync_block_number = int(sync_latest["number"], 16)
-                        if sync_block_number > last_sync_block_number:
-                            hash_val = sync_latest["hash"]
-                            logger.info(
-                                f"Sync progress: block #{sync_block_number} (hash: {hash_val})"
-                            )
-                            last_sync_block_number = sync_block_number
-
-                        # Check if we've reached the target
-                        if sync_latest["hash"] == last_valid_block_hash:
-                            logger.info("Sync client successfully synchronized to target block!")
-                            break
-
-                    # try to get the target block directly, via hash
-                    sync_block = sync_eth_rpc.get_block_by_hash(last_valid_block_hash)
-                    if sync_block is not None:
-                        logger.info(
-                            f"Sync client has target block! Number: {sync_block['number']}"
-                        )
-                        break
-
-                except Exception as e:
-                    logger.debug(f"Error checking sync status: {e}")
-
-                last_check_time = time.time()
-
             time.sleep(0.5)
         else:
             raise LoggedError(