move LVH handling to tests/; increase maximum fork choice retries

status-im · Oct 3, 2022 · 4abb915 · 4abb915
1 parent 8680006
commit 4abb915
Show file tree

Hide file tree

Showing 9 changed files with 48 additions and 57 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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