optimistic block gossip validation (#3876)

beacon_chain/consensus_object_pools/block_pools_types.nim

@@ -255,9 +255,6 @@ type
     shuffled_active_validator_indices*: seq[ValidatorIndex]
     attester_dependent_root*: Eth2Digest
 
-    # enables more efficient merge block validation
-    merge_transition_complete*: bool
-
     # balances, as used in fork choice
     effective_balances_bytes*: seq[byte]
 

beacon_chain/consensus_object_pools/blockchain_dag.nim

@@ -179,8 +179,6 @@ func init*(
       shuffled_active_validator_indices:
         cache.get_shuffled_active_validator_indices(state, epoch),
       attester_dependent_root: attester_dependent_root,
-
-      merge_transition_complete: state.is_merge_transition_complete()
     )
     epochStart = epoch.start_slot()
 

beacon_chain/gossip_processing/block_processor.nim

@@ -450,6 +450,16 @@ proc is_optimistic_candidate_block(
       self.getBeaconTime().slotOrZero:
     return true
 
+  # Once merge is finalized, always true; in principle, should be caught by
+  # other checks, but sometimes blocks arrive out of order, triggering some
+  # spurious false negatives because the parent-block-check does not find a
+  # parent block. This can also occur under conditions where EL client RPCs
+  # cause processing delays. Either way, bound this risk to post-merge head
+  # and pre-merge finalization.
+  if not self.consensusManager.dag.loadExecutionBlockRoot(
+      self.consensusManager.dag.finalizedHead.blck).isZero:
+    return true
+
   let
     parentRoot = withBlck(blck): blck.message.parent_root
     parentBlck = self.consensusManager.dag.getBlockRef(parentRoot).valueOr:

beacon_chain/gossip_processing/gossip_validation.nim

@@ -175,51 +175,31 @@ template checkedReject(error: ValidationError): untyped =
   err(error)
 
 template validateBeaconBlockBellatrix(
-       signed_beacon_block: phase0.SignedBeaconBlock |
+    signed_beacon_block: phase0.SignedBeaconBlock | altair.SignedBeaconBlock,
-                            altair.SignedBeaconBlock,
+    parent: BlockRef): untyped =
-       parent: BlockRef): untyped =
   discard
 
-# https://github.com/ethereum/consensus-specs/blob/v1.1.7/specs/merge/p2p-interface.md#beacon_block
+# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.1/specs/bellatrix/p2p-interface.md#beacon_block
 template validateBeaconBlockBellatrix(
        signed_beacon_block: bellatrix.SignedBeaconBlock,
        parent: BlockRef): untyped =
   # If the execution is enabled for the block -- i.e.
   # is_execution_enabled(state, block.body) then validate the following:
-  let executionEnabled =
+  #
-    if signed_beacon_block.message.body.execution_payload !=
+  # `is_execution_enabled(state, block.body)` is
-        default(ExecutionPayload):
+  # `is_merge_transition_block(state, block.body) or is_merge_transition_complete(state)` is
-      true
+  # `(not is_merge_transition_complete(state) and block.body.execution_payload != ExecutionPayload()) or is_merge_transition_complete(state)` is
-    elif dag.getEpochRef(parent, parent.slot.epoch, true).expect(
+  # `is_merge_transition_complete(state) or block.body.execution_payload != ExecutionPayload()` is
-        "parent EpochRef doesn't fail").merge_transition_complete:
+  # `is_merge_transition_complete(state) or is_execution_block(block)`
-      # Should usually be inexpensive, but could require cache refilling - the
+  #
-      # parent block can be no older than the latest finalized block
+  # `is_merge_transition_complete(state)` tests for
-      true
+  # `state.latest_execution_payload_header != ExecutionPayloadHeader()`, while
-    else:
+  # https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.1/specs/bellatrix/beacon-chain.md#block-processing
-      # Somewhat more expensive fallback, with database I/O, but should be
+  # shows that `state.latest_execution_payload_header` being default or not is
-      # mostly relevant around merge transition epochs. It's possible that
+  # exactly equivalent to whether that block's execution payload is default or
-      # the previous block is phase 0 or Altair, if this is the transition
+  # not, so test cached block information rather than reconstructing a state.
-      # block itself.
+  if  signed_beacon_block.message.is_execution_block or
-      let blockData = dag.getForkedBlock(parent.bid)
+      not dag.loadExecutionBlockRoot(parent).isZero:
-      if blockData.isOk():
-        case blockData.get().kind:
-        of BeaconBlockFork.Phase0:
-          false
-        of BeaconBlockFork.Altair:
-          false
-        of BeaconBlockFork.Bellatrix:
-          # https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.1/specs/bellatrix/beacon-chain.md#process_execution_payload
-          # shows how this gets folded into the state each block; checking this
-          # is equivalent, without ever requiring state replay or any similarly
-          # expensive computation.
-          blockData.get().bellatrixData.message.body.execution_payload !=
-            default(ExecutionPayload)
-      else:
-        warn "Cannot load block parent, assuming execution is disabled",
-          parent = shortLog(parent)
-        false
-
-  if executionEnabled:
     # [REJECT] The block's execution payload timestamp is correct with respect
     # to the slot -- i.e. execution_payload.timestamp ==
     # compute_timestamp_at_slot(state, block.slot).
@@ -229,10 +209,17 @@ template validateBeaconBlockBellatrix(
     if not (signed_beacon_block.message.body.execution_payload.timestamp ==
         timestampAtSlot):
       quarantine[].addUnviable(signed_beacon_block.root)
-      return errReject("BeaconBlock: Mismatched execution payload timestamp")
+      return errReject("BeaconBlock: mismatched execution payload timestamp")
+
+    if signed_beacon_block.message.parent_root in dag.optimisticRoots:
+      # Definitely don't mark this as unviable.
+      # [REJECT] The block's parent (defined by `block.parent_root`) passes all
+      # validation (excluding execution node verification of the
+      # `block.body.execution_payload`).
+      return errReject("BeaconBlock: execution payload would build on optimistic parent")
 
 # https://github.com/ethereum/consensus-specs/blob/v1.1.9/specs/phase0/p2p-interface.md#beacon_block
-# https://github.com/ethereum/consensus-specs/blob/v1.1.8/specs/bellatrix/p2p-interface.md#beacon_block
+# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.1/specs/bellatrix/p2p-interface.md#beacon_block
 proc validateBeaconBlock*(
     dag: ChainDAGRef, quarantine: ref Quarantine,
     signed_beacon_block: phase0.SignedBeaconBlock | altair.SignedBeaconBlock |
@@ -306,12 +293,28 @@ proc validateBeaconBlock*(
   # (via both gossip and non-gossip sources) (a client MAY queue blocks for
   # processing once the parent block is retrieved).
   #
-  # And implicitly:
   # [REJECT] The block's parent (defined by block.parent_root) passes validation.
   let parent = dag.getBlockRef(signed_beacon_block.message.parent_root).valueOr:
     if signed_beacon_block.message.parent_root in quarantine[].unviable:
       quarantine[].addUnviable(signed_beacon_block.root)
-      return errReject("BeaconBlock: parent from unviable fork")
+
+      # https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.1/specs/bellatrix/p2p-interface.md#beacon_block
+      # `is_execution_enabled(state, block.body)` check, but unlike in
+      # validateBeaconBlockBellatrix() don't have parent BlockRef.
+      if  signed_beacon_block.message.is_execution_block or
+          not dag.loadExecutionBlockRoot(dag.finalizedHead.blck).isZero:
+        # Blocks with execution enabled will be permitted to propagate
+        # regardless of the validity of the execution payload. This prevents
+        # network segregation between optimistic and non-optimistic nodes.
+        #
+        # [IGNORE] The block's parent (defined by `block.parent_root`) passes all
+        # validation (including execution node verification of the
+        # `block.body.execution_payload`).
+        return errIgnore("BeaconBlock: ignored, parent from unviable fork")
+      else:
+        # [REJECT] The block's parent (defined by `block.parent_root`) passes
+        # validation.
+        return errReject("BeaconBlock: rejected, parent from unviable fork")
 
     # When the parent is missing, we can't validate the block - we'll queue it
     # in the quarantine for later processing
@@ -322,6 +325,11 @@ proc validateBeaconBlock*(
 
     return errIgnore("BeaconBlock: Parent not found")
 
+  # Continues block parent validity checking in optimistic case, where it does
+  # appear as a `BlockRef` (and not handled above) but isn't usable for gossip
+  # validation.
+  validateBeaconBlockBellatrix(signed_beacon_block, parent)
+
   # [REJECT] The block is from a higher slot than its parent.
   if not (signed_beacon_block.message.slot > parent.bid.slot):
     return errReject("BeaconBlock: block not from higher slot than its parent")
@@ -343,7 +351,6 @@ proc validateBeaconBlock*(
       finalized_checkpoint.root == ancestor.root or
       finalized_checkpoint.root.isZero):
     quarantine[].addUnviable(signed_beacon_block.root)
-
     return errReject("BeaconBlock: Finalized checkpoint not an ancestor")
 
   # [REJECT] The block is proposed by the expected proposer_index for the
@@ -361,7 +368,6 @@ proc validateBeaconBlock*(
 
   if uint64(proposer.get()) != signed_beacon_block.message.proposer_index:
     quarantine[].addUnviable(signed_beacon_block.root)
-
     return errReject("BeaconBlock: Unexpected proposer proposer")
 
   # [REJECT] The proposer signature, signed_beacon_block.signature, is valid
@@ -377,8 +383,6 @@ proc validateBeaconBlock*(
 
     return errReject("BeaconBlock: Invalid proposer signature")
 
-  validateBeaconBlockBellatrix(signed_beacon_block, parent)
-
   ok()
 
 # https://github.com/ethereum/consensus-specs/blob/v1.1.9/specs/phase0/p2p-interface.md#beacon_attestation_subnet_id