clean up exchange configuration handling (#4126)

Per spec, we should not be sending our detected terminal block to EL -
the EL configuration exchange should only look at values from
configuration and report mismatches.

beacon_chain/eth1/eth1_monitor.nim

@@ -136,7 +136,6 @@ type
 
     exchangedConfiguration*: bool
     terminalBlockHash*: Option[BlockHash]
-    terminalBlockNumber*: Option[Quantity]
 
     runFut: Future[void]
     stopFut: Future[void]
@@ -547,7 +546,6 @@ type
   EtcStatus {.pure.} = enum
     exchangeError
     mismatch
-    localConfigurationUpdated
     match
 
 proc exchangeTransitionConfiguration*(p: Eth1Monitor): Future[EtcStatus] {.async.} =
@@ -561,18 +559,11 @@ proc exchangeTransitionConfiguration*(p: Eth1Monitor): Future[EtcStatus] {.async
   if dataProvider.isNil:
     return EtcStatus.exchangeError
 
+  # https://github.com/ethereum/execution-apis/blob/v1.0.0-beta.1/src/engine/specification.md#engine_exchangetransitionconfigurationv1
   let consensusCfg = TransitionConfigurationV1(
     terminalTotalDifficulty: p.depositsChain.cfg.TERMINAL_TOTAL_DIFFICULTY,
-    terminalBlockHash:
+    terminalBlockHash: p.depositsChain.cfg.TERMINAL_BLOCK_HASH,
-      if p.terminalBlockHash.isSome:
+    terminalBlockNumber: Quantity 0)
-        p.terminalBlockHash.get
-      else:
-        (static default BlockHash),
-    terminalBlockNumber:
-      if p.terminalBlockNumber.isSome:
-        p.terminalBlockNumber.get
-      else:
-        (static default Quantity))
   let executionCfg =
     try:
       awaitWithRetries(
@@ -580,48 +571,34 @@ proc exchangeTransitionConfiguration*(p: Eth1Monitor): Future[EtcStatus] {.async
           consensusCfg),
         timeout = 1.seconds)
     except CatchableError as err:
-      error "Failed to exchange transition configuration", err = err.msg
+      warn "Failed to exchange transition configuration", err = err.msg
       return EtcStatus.exchangeError
 
-  if consensusCfg.terminalTotalDifficulty != executionCfg.terminalTotalDifficulty:
+  return
-    warn "Engine API configured with different terminal total difficulty",
+    if consensusCfg.terminalTotalDifficulty != executionCfg.terminalTotalDifficulty:
-         engineAPI_value = executionCfg.terminalTotalDifficulty,
+      error "Engine API configured with different terminal total difficulty",
-         localValue = consensusCfg.terminalTotalDifficulty
+          engineAPI_value = executionCfg.terminalTotalDifficulty,
-    return EtcStatus.mismatch
+          localValue = consensusCfg.terminalTotalDifficulty
-
+      EtcStatus.mismatch
-  if not p.exchangedConfiguration:
+    elif consensusCfg.terminalBlockNumber != executionCfg.terminalBlockNumber:
-    # Log successful engine configuration exchange once at startup
-    p.exchangedConfiguration = true
-    info "Exchanged engine configuration",
-      ttd = executionCfg.terminalTotalDifficulty,
-      terminalBlockHash = executionCfg.terminalBlockHash,
-      terminalBlockNumber = executionCfg.terminalBlockNumber.uint64
-
-  if p.terminalBlockNumber.isSome and p.terminalBlockHash.isSome:
-    var res = EtcStatus.match
-
-    if consensusCfg.terminalBlockNumber != executionCfg.terminalBlockNumber:
       warn "Engine API reporting different terminal block number",
             engineAPI_value = executionCfg.terminalBlockNumber.uint64,
             localValue = consensusCfg.terminalBlockNumber.uint64
-      res = EtcStatus.mismatch
+      EtcStatus.mismatch
-    if consensusCfg.terminalBlockHash != executionCfg.terminalBlockHash:
+    elif consensusCfg.terminalBlockHash != executionCfg.terminalBlockHash:
       warn "Engine API reporting different terminal block hash",
             engineAPI_value = executionCfg.terminalBlockHash,
             localValue = consensusCfg.terminalBlockHash
-      res = EtcStatus.mismatch
+      EtcStatus.mismatch
-    return res
+    else:
-  else:
+      if not p.exchangedConfiguration:
-    if executionCfg.terminalBlockHash == default BlockHash:
+        # Log successful engine configuration exchange once at startup
-      # If TERMINAL_BLOCK_HASH is stubbed with
+        p.exchangedConfiguration = true
-      # 0x0000000000000000000000000000000000000000000000000000000000000000 then
+        info "Exchanged engine configuration",
-      # TERMINAL_BLOCK_HASH and TERMINAL_BLOCK_NUMBER parameters MUST NOT take
+          terminalTotalDifficulty = executionCfg.terminalTotalDifficulty,
-      # an effect.
+          terminalBlockHash = executionCfg.terminalBlockHash,
-      return EtcStatus.match
+          terminalBlockNumber = executionCfg.terminalBlockNumber.uint64
-
+      EtcStatus.match
-    p.terminalBlockNumber = some executionCfg.terminalBlockNumber
-    p.terminalBlockHash = some executionCfg.terminalBlockHash
-    return EtcStatus.localConfigurationUpdated
 
 template readJsonField(j: JsonNode, fieldName: string, ValueType: type): untyped =
   var res: ValueType
@@ -1374,11 +1351,7 @@ proc startEth1Syncing(m: Eth1Monitor, delayBeforeStart: Duration) {.async.} =
 
   if m.currentEpoch >= m.cfg.BELLATRIX_FORK_EPOCH:
     let status = await m.exchangeTransitionConfiguration()
-    if status == EtcStatus.localConfigurationUpdated:
+    if status != EtcStatus.match and isFirstRun and m.requireEngineAPI:
-      info "Obtained terminal block from Engine API",
-           terminalBlockNumber = m.terminalBlockNumber.get.uint64,
-           terminalBlockHash = m.terminalBlockHash.get
-    elif status != EtcStatus.match and isFirstRun and m.requireEngineAPI:
       fatal "The Bellatrix hard fork requires the beacon node to be connected to a properly configured Engine API end-point. " &
             "See https://nimbus.guide/merge.html for more details. " &
             "If you want to temporarily continue operating Nimbus without configuring an Engine API end-point, " &
@@ -1531,6 +1504,10 @@ proc startEth1Syncing(m: Eth1Monitor, delayBeforeStart: Duration) {.async.} =
       doAssert m.latestEth1Block.isSome
       awaitWithRetries m.dataProvider.getBlockByHash(m.latestEth1Block.get.hash)
 
+    # TODO when a terminal block has is configured in cfg.TERMINAL_BLOCK_HASH,
+    #      we should try to fetch that block from the EL - this facility is not
+    #      in use on any current network, but should be implemented for full
+    #      compliance
     if m.currentEpoch >= m.cfg.BELLATRIX_FORK_EPOCH and m.terminalBlockHash.isNone:
       var terminalBlockCandidate = nextBlock
 
@@ -1551,7 +1528,6 @@ proc startEth1Syncing(m: Eth1Monitor, delayBeforeStart: Duration) {.async.} =
             break
           terminalBlockCandidate = parentBlock
         m.terminalBlockHash = some terminalBlockCandidate.hash
-        m.terminalBlockNumber = some terminalBlockCandidate.number
 
         debug "startEth1Syncing: found merge terminal block",
           currentEpoch = m.currentEpoch,

beacon_chain/spec/presets.nim

@@ -38,8 +38,7 @@ type
 
     # Transition
     TERMINAL_TOTAL_DIFFICULTY*: UInt256
-    TERMINAL_BLOCK_HASH*: BlockHash
+    TERMINAL_BLOCK_HASH*: BlockHash # TODO use in eht1monitor
-    # TODO TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH*: Epoch
 
     # Genesis
     MIN_GENESIS_ACTIVE_VALIDATOR_COUNT*: uint64