Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clean up exchange configuration handling #4126

Merged
merged 3 commits into from
Sep 16, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 28 additions & 52 deletions beacon_chain/eth1/eth1_monitor.nim
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ type

exchangedConfiguration*: bool
terminalBlockHash*: Option[BlockHash]
terminalBlockNumber*: Option[Quantity]

runFut: Future[void]
stopFut: Future[void]
Expand Down Expand Up @@ -547,7 +546,6 @@ type
EtcStatus {.pure.} = enum
exchangeError
mismatch
localConfigurationUpdated
match

proc exchangeTransitionConfiguration*(p: Eth1Monitor): Future[EtcStatus] {.async.} =
Expand All @@ -561,67 +559,46 @@ proc exchangeTransitionConfiguration*(p: Eth1Monitor): Future[EtcStatus] {.async
if dataProvider.isNil:
return EtcStatus.exchangeError

# https://github.com/ethereum/execution-apis/blob/main/src/engine/specification.md#engine_exchangetransitionconfigurationv1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arnetheduck marked this conversation as resolved.
Show resolved Hide resolved
let consensusCfg = TransitionConfigurationV1(
terminalTotalDifficulty: p.depositsChain.cfg.TERMINAL_TOTAL_DIFFICULTY,
terminalBlockHash:
if p.terminalBlockHash.isSome:
p.terminalBlockHash.get
else:
(static default BlockHash),
terminalBlockNumber:
if p.terminalBlockNumber.isSome:
p.terminalBlockNumber.get
else:
(static default Quantity))
terminalBlockHash: p.depositsChain.cfg.TERMINAL_BLOCK_HASH,
Copy link
Contributor

@zah zah Sep 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code was not fully wrong here I think. We still need to consider the command-line override that the user may supply.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we consider the command line here:

metadata.cfg.TERMINAL_TOTAL_DIFFICULTY =

terminalBlockNumber: Quantity 0)
let executionCfg =
try:
awaitWithRetries(
dataProvider.web3.provider.engine_exchangeTransitionConfigurationV1(
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:
warn "Engine API configured with different terminal total difficulty",
engineAPI_value = executionCfg.terminalTotalDifficulty,
localValue = consensusCfg.terminalTotalDifficulty
return EtcStatus.mismatch

if not p.exchangedConfiguration:
# 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:
return
if consensusCfg.terminalTotalDifficulty != executionCfg.terminalTotalDifficulty:
error "Engine API configured with different terminal total difficulty",
engineAPI_value = executionCfg.terminalTotalDifficulty,
localValue = consensusCfg.terminalTotalDifficulty
EtcStatus.mismatch
elif consensusCfg.terminalBlockNumber != executionCfg.terminalBlockNumber:
warn "Engine API reporting different terminal block number",
engineAPI_value = executionCfg.terminalBlockNumber.uint64,
localValue = consensusCfg.terminalBlockNumber.uint64
res = EtcStatus.mismatch
if consensusCfg.terminalBlockHash != executionCfg.terminalBlockHash:
EtcStatus.mismatch
elif consensusCfg.terminalBlockHash != executionCfg.terminalBlockHash:
warn "Engine API reporting different terminal block hash",
engineAPI_value = executionCfg.terminalBlockHash,
localValue = consensusCfg.terminalBlockHash
res = EtcStatus.mismatch
return res
else:
if executionCfg.terminalBlockHash == default BlockHash:
# If TERMINAL_BLOCK_HASH is stubbed with
# 0x0000000000000000000000000000000000000000000000000000000000000000 then
# TERMINAL_BLOCK_HASH and TERMINAL_BLOCK_NUMBER parameters MUST NOT take
# an effect.
return EtcStatus.match

p.terminalBlockNumber = some executionCfg.terminalBlockNumber
p.terminalBlockHash = some executionCfg.terminalBlockHash
return EtcStatus.localConfigurationUpdated
EtcStatus.mismatch
else:
if not p.exchangedConfiguration:
# Log successful engine configuration exchange once at startup
p.exchangedConfiguration = true
info "Exchanged engine configuration",
terminalTotalDifficulty = executionCfg.terminalTotalDifficulty,
terminalBlockHash = executionCfg.terminalBlockHash,
terminalBlockNumber = executionCfg.terminalBlockNumber.uint64
EtcStatus.match

template readJsonField(j: JsonNode, fieldName: string, ValueType: type): untyped =
var res: ValueType
Expand Down Expand Up @@ -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:
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:
if 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, " &
Expand Down Expand Up @@ -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

Expand All @@ -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,
Expand Down
3 changes: 1 addition & 2 deletions beacon_chain/spec/presets.nim
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ type

# Transition
TERMINAL_TOTAL_DIFFICULTY*: UInt256
TERMINAL_BLOCK_HASH*: BlockHash
# TODO TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH*: Epoch
TERMINAL_BLOCK_HASH*: BlockHash # TODO use in eht1monitor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TERMINAL_BLOCK_HASH*: BlockHash # TODO use in eht1monitor
TERMINAL_BLOCK_HASH*: BlockHash # TODO use in eth1monitor


# Genesis
MIN_GENESIS_ACTIVE_VALIDATOR_COUNT*: uint64
Expand Down