diff --git a/beacon_chain/consensus_object_pools/block_pools_types.nim b/beacon_chain/consensus_object_pools/block_pools_types.nim index f052d9bd3..203d83653 100644 --- a/beacon_chain/consensus_object_pools/block_pools_types.nim +++ b/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] diff --git a/beacon_chain/consensus_object_pools/blockchain_dag.nim b/beacon_chain/consensus_object_pools/blockchain_dag.nim index 08b85d3b8..7b421c80f 100644 --- a/beacon_chain/consensus_object_pools/blockchain_dag.nim +++ b/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() diff --git a/beacon_chain/gossip_processing/block_processor.nim b/beacon_chain/gossip_processing/block_processor.nim index cb3d6cf9b..159540413 100644 --- a/beacon_chain/gossip_processing/block_processor.nim +++ b/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: diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim index 4985e9a93..defa41c9f 100644 --- a/beacon_chain/gossip_processing/gossip_validation.nim +++ b/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 | - altair.SignedBeaconBlock, - parent: BlockRef): untyped = + signed_beacon_block: phase0.SignedBeaconBlock | altair.SignedBeaconBlock, + 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 != - default(ExecutionPayload): - true - elif dag.getEpochRef(parent, parent.slot.epoch, true).expect( - "parent EpochRef doesn't fail").merge_transition_complete: - # Should usually be inexpensive, but could require cache refilling - the - # parent block can be no older than the latest finalized block - true - else: - # Somewhat more expensive fallback, with database I/O, but should be - # mostly relevant around merge transition epochs. It's possible that - # the previous block is phase 0 or Altair, if this is the transition - # block itself. - let blockData = dag.getForkedBlock(parent.bid) - 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: + # + # `is_execution_enabled(state, block.body)` is + # `is_merge_transition_block(state, block.body) or is_merge_transition_complete(state)` is + # `(not is_merge_transition_complete(state) and block.body.execution_payload != ExecutionPayload()) or is_merge_transition_complete(state)` is + # `is_merge_transition_complete(state) or block.body.execution_payload != ExecutionPayload()` is + # `is_merge_transition_complete(state) or is_execution_block(block)` + # + # `is_merge_transition_complete(state)` tests for + # `state.latest_execution_payload_header != ExecutionPayloadHeader()`, while + # https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.1/specs/bellatrix/beacon-chain.md#block-processing + # shows that `state.latest_execution_payload_header` being default or not is + # exactly equivalent to whether that block's execution payload is default or + # not, so test cached block information rather than reconstructing a state. + if signed_beacon_block.message.is_execution_block or + not dag.loadExecutionBlockRoot(parent).isZero: # [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