From 4abb915334d5743f2806fac97ad75c83fe2384b7 Mon Sep 17 00:00:00 2001 From: Dustin Brody Date: Mon, 3 Oct 2022 07:19:36 +0000 Subject: [PATCH] move LVH handling to tests/; increase maximum fork choice retries --- .../consensus_object_pools/blockchain_dag.nim | 39 ------------------- .../consensus_manager.nim | 11 +----- .../gossip_processing/eth2_processor.nim | 2 +- .../light_client_processor.nim | 2 +- beacon_chain/spec/helpers.nim | 4 +- beacon_chain/spec/light_client_sync.nim | 4 +- beacon_chain/spec/state_transition.nim | 2 +- .../test_fixture_fork_choice.nim | 2 +- tests/testdbutil.nim | 39 +++++++++++++++++++ 9 files changed, 48 insertions(+), 57 deletions(-) diff --git a/beacon_chain/consensus_object_pools/blockchain_dag.nim b/beacon_chain/consensus_object_pools/blockchain_dag.nim index d4dd403a0e..81b71436a7 100644 --- a/beacon_chain/consensus_object_pools/blockchain_dag.nim +++ b/beacon_chain/consensus_object_pools/blockchain_dag.nim @@ -1897,45 +1897,6 @@ proc updateHead*( dag.finalizedHead.blck.root, stateRoot, dag.finalizedHead.slot.epoch) dag.onFinHappened(dag, data) -proc getEarliestInvalidBlockRoot*( - dag: ChainDAGRef, initialSearchRoot: Eth2Digest, - latestValidHash: Eth2Digest, defaultEarliestInvalidBlockRoot: Eth2Digest): - Eth2Digest = - # Earliest within a chain/fork in question, per LVH definition. Intended to - # be called with `initialRoot` as the parent of the block regarding which a - # newPayload or forkchoiceUpdated execution_status has been received as the - # tests effectively require being able to access this before the BlockRef's - # made. Therefore, to accommodate the EF consensus spec sync tests, and the - # possibilities that the LVH might be an immediate parent or a more distant - # ancestor special-case handling of an earliest invalid root as potentially - # not being from this function's search, but being provided as a default by - # the caller with access to the block. - var curBlck = dag.getBlockRef(initialSearchRoot).valueOr: - # Being asked to traverse a chain which the DAG doesn't know about -- but - # that'd imply the block's otherwise invalid for CL as well as EL. - return static(default(Eth2Digest)) - - # Only allow this special case outside loop; it's when the LVH is the direct - # parent of the reported invalid block - if curBlck.executionBlockRoot.isSome and - curBlck.executionBlockRoot.get == latestValidHash: - return defaultEarliestInvalidBlockRoot - - while true: - # This was supposed to have been either caught by the pre-loop check or the - # parent check. - if curBlck.executionBlockRoot.isSome and - curBlck.executionBlockRoot.get == latestValidHash: - doAssert false, "getEarliestInvalidBlockRoot: unexpected LVH in loop body" - - if (curBlck.parent.isNil) or - curBlck.parent.executionBlockRoot.get(latestValidHash) == - latestValidHash: - break - curBlck = curBlck.parent - - curBlck.root - proc isInitialized*(T: type ChainDAGRef, db: BeaconChainDB): Result[void, cstring] = # Lightweight check to see if we have the minimal information needed to # load up a database - we don't check head here - if something is wrong with diff --git a/beacon_chain/consensus_object_pools/consensus_manager.nim b/beacon_chain/consensus_object_pools/consensus_manager.nim index 6300a95310..86bd519877 100644 --- a/beacon_chain/consensus_object_pools/consensus_manager.nim +++ b/beacon_chain/consensus_object_pools/consensus_manager.nim @@ -238,15 +238,6 @@ proc updateExecutionClientHead( of PayloadExecutionStatus.valid: self.dag.markBlockVerified(self.quarantine[], newHead.blck.root) of PayloadExecutionStatus.invalid, PayloadExecutionStatus.invalid_block_hash: - # This is a CL root, not EL hash - let earliestKnownInvalidRoot = - if latestValidHash.isSome: - self.dag.getEarliestInvalidBlockRoot( - newHead.blck.root, latestValidHash.get.asEth2Digest, - newHead.blck.root) - else: - newHead.blck.root - self.attestationPool[].forkChoice.mark_root_invalid(newHead.blck.root) self.quarantine[].addUnviable(newHead.blck.root) return Opt.none(void) @@ -412,7 +403,7 @@ proc updateHeadWithExecution*( while (await self.updateExecutionClientHead(newHead)).isErr: # This proc is called on every new block; guarantee timely return inc attempts - const maxAttempts = 3 + const maxAttempts = 5 if attempts >= maxAttempts: warn "updateHeadWithExecution: too many attempts to recover from invalid payload", attempts, maxAttempts, newHead, initialNewHead diff --git a/beacon_chain/gossip_processing/eth2_processor.nim b/beacon_chain/gossip_processing/eth2_processor.nim index 9164204be5..a69d0e64d4 100644 --- a/beacon_chain/gossip_processing/eth2_processor.nim +++ b/beacon_chain/gossip_processing/eth2_processor.nim @@ -577,7 +577,7 @@ proc processLightClientFinalityUpdate*( self.lightClientPool[], self.dag, finality_update, wallTime) v -# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.3/specs/altair/light-client/sync-protocol.md#process_light_client_optimistic_update +# https://github.com/ethereum/consensus-specs/blob/v1.2.0/specs/altair/light-client/sync-protocol.md#process_light_client_optimistic_update proc processLightClientOptimisticUpdate*( self: var Eth2Processor, src: MsgSource, optimistic_update: altair.LightClientOptimisticUpdate diff --git a/beacon_chain/gossip_processing/light_client_processor.nim b/beacon_chain/gossip_processing/light_client_processor.nim index 7b90a5efbb..88bd12a44b 100644 --- a/beacon_chain/gossip_processing/light_client_processor.nim +++ b/beacon_chain/gossip_processing/light_client_processor.nim @@ -431,7 +431,7 @@ func toValidationError( # `attested_header.slot` was already forwarded on the network. errIgnore($r.error) -# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.3/specs/altair/light-client/sync-protocol.md#process_light_client_finality_update +# https://github.com/ethereum/consensus-specs/blob/v1.2.0/specs/altair/light-client/sync-protocol.md#process_light_client_finality_update proc processLightClientFinalityUpdate*( self: var LightClientProcessor, src: MsgSource, finality_update: altair.LightClientFinalityUpdate diff --git a/beacon_chain/spec/helpers.nim b/beacon_chain/spec/helpers.nim index 9fe75de0f3..4863a005ed 100644 --- a/beacon_chain/spec/helpers.nim +++ b/beacon_chain/spec/helpers.nim @@ -214,7 +214,7 @@ template is_finality_update*(update: SomeLightClientUpdate): bool = else: false -# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.3/specs/altair/light-client/sync-protocol.md#is_next_sync_committee_known +# https://github.com/ethereum/consensus-specs/blob/v1.2.0/specs/altair/light-client/sync-protocol.md#is_next_sync_committee_known template is_next_sync_committee_known*(store: LightClientStore): bool = not isZeroMemory(store.next_sync_committee) @@ -313,7 +313,7 @@ func is_merge_transition_complete*(state: bellatrix.BeaconState): bool = const defaultExecutionPayloadHeader = default(ExecutionPayloadHeader) state.latest_execution_payload_header != defaultExecutionPayloadHeader -# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.3/sync/optimistic.md#helpers +# https://github.com/ethereum/consensus-specs/blob/v1.2.0/sync/optimistic.md#helpers func is_execution_block*(blck: SomeForkyBeaconBlock): bool = when typeof(blck).toFork >= BeaconBlockFork.Bellatrix: const defaultExecutionPayload = diff --git a/beacon_chain/spec/light_client_sync.nim b/beacon_chain/spec/light_client_sync.nim index 938349a1ad..d35f704eea 100644 --- a/beacon_chain/spec/light_client_sync.nim +++ b/beacon_chain/spec/light_client_sync.nim @@ -18,7 +18,7 @@ import from ../consensus_object_pools/block_pools_types import BlockError export block_pools_types.BlockError -# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.3/specs/altair/light-client/sync-protocol.md#initialize_light_client_store +# https://github.com/ethereum/consensus-specs/blob/v1.2.0/specs/altair/light-client/sync-protocol.md#initialize_light_client_store func initialize_light_client_store*( trusted_block_root: Eth2Digest, bootstrap: altair.LightClientBootstrap @@ -39,7 +39,7 @@ func initialize_light_client_store*( current_sync_committee: bootstrap.current_sync_committee, optimistic_header: bootstrap.header)) -# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.3/specs/altair/light-client/sync-protocol.md#validate_light_client_update +# https://github.com/ethereum/consensus-specs/blob/v1.2.0/specs/altair/light-client/sync-protocol.md#validate_light_client_update proc validate_light_client_update*( store: LightClientStore, update: SomeLightClientUpdate, diff --git a/beacon_chain/spec/state_transition.nim b/beacon_chain/spec/state_transition.nim index 2dfe160260..3d31c1e6d2 100644 --- a/beacon_chain/spec/state_transition.nim +++ b/beacon_chain/spec/state_transition.nim @@ -364,7 +364,7 @@ proc makeBeaconBlock*( ok(blck) -# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.1/specs/altair/validator.md#preparing-a-beaconblock +# https://github.com/ethereum/consensus-specs/blob/v1.2.0/specs/altair/validator.md#preparing-a-beaconblock template partialBeaconBlock( cfg: RuntimeConfig, state: var altair.HashedBeaconState, diff --git a/tests/consensus_spec/test_fixture_fork_choice.nim b/tests/consensus_spec/test_fixture_fork_choice.nim index 077caa8bfd..25db1280b8 100644 --- a/tests/consensus_spec/test_fixture_fork_choice.nim +++ b/tests/consensus_spec/test_fixture_fork_choice.nim @@ -25,7 +25,7 @@ import # Third-party yaml, # Test - ../testutil, + ../testutil, ../testdbutil, ./fixtures_utils # Test format described at https://github.com/ethereum/consensus-specs/tree/v1.2.0-rc.1/tests/formats/fork_choice diff --git a/tests/testdbutil.nim b/tests/testdbutil.nim index 892788c2e3..7633007112 100644 --- a/tests/testdbutil.nim +++ b/tests/testdbutil.nim @@ -34,3 +34,42 @@ proc makeTestDB*(validators: Natural): BeaconChainDB = {skipBlsValidation})) genBlock = get_initial_beacon_block(genState[]) makeTestDB(genState[], genBlock) + +proc getEarliestInvalidBlockRoot*( + dag: ChainDAGRef, initialSearchRoot: Eth2Digest, + latestValidHash: Eth2Digest, defaultEarliestInvalidBlockRoot: Eth2Digest): + Eth2Digest = + # Earliest within a chain/fork in question, per LVH definition. Intended to + # be called with `initialRoot` as the parent of the block regarding which a + # newPayload or forkchoiceUpdated execution_status has been received as the + # tests effectively require being able to access this before the BlockRef's + # made. Therefore, to accommodate the EF consensus spec sync tests, and the + # possibilities that the LVH might be an immediate parent or a more distant + # ancestor special-case handling of an earliest invalid root as potentially + # not being from this function's search, but being provided as a default by + # the caller with access to the block. + var curBlck = dag.getBlockRef(initialSearchRoot).valueOr: + # Being asked to traverse a chain which the DAG doesn't know about -- but + # that'd imply the block's otherwise invalid for CL as well as EL. + return static(default(Eth2Digest)) + + # Only allow this special case outside loop; it's when the LVH is the direct + # parent of the reported invalid block + if curBlck.executionBlockRoot.isSome and + curBlck.executionBlockRoot.get == latestValidHash: + return defaultEarliestInvalidBlockRoot + + while true: + # This was supposed to have been either caught by the pre-loop check or the + # parent check. + if curBlck.executionBlockRoot.isSome and + curBlck.executionBlockRoot.get == latestValidHash: + doAssert false, "getEarliestInvalidBlockRoot: unexpected LVH in loop body" + + if (curBlck.parent.isNil) or + curBlck.parent.executionBlockRoot.get(latestValidHash) == + latestValidHash: + break + curBlck = curBlck.parent + + curBlck.root