From 26d0281e54febaed14abdb50957b4ce26e0bf54e Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Mon, 26 Feb 2024 13:32:01 +0100 Subject: [PATCH] fix incorrect config validation regression from #5959 (#5966) During refactoring of #5959, some implicit `return` were overlooked, resulting in spurious `err()` being returned without message. ``` {"lvl":"WRN","ts":"2024-02-26 10:12:20.469+00:00","msg":"Beacon nodes report different configuration values","reason":"","service":"fallback_service","node":"http://127.0.0.1:9303[Nimbus/v24.2.1-4e9bc7-stateofus]","node_index":0,"node_roles":"AGBSDT"} ``` Correcting the helpers to return explicit result in all exhaustive cases so that this cannot happen anymore by accident. --- beacon_chain/validator_client/common.nim | 26 +++++++++++++----------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/beacon_chain/validator_client/common.nim b/beacon_chain/validator_client/common.nim index db0cb1631..3fbb396e6 100644 --- a/beacon_chain/validator_client/common.nim +++ b/beacon_chain/validator_client/common.nim @@ -1440,23 +1440,24 @@ proc updateRuntimeConfig*(vc: ValidatorClientRef, localForkEpoch: Epoch, forkVersion: Opt[Version]): Result[void, string] = if localForkVersion.isNone(): - discard # Potentially discovered new fork, save it at end of function + ok() # Potentially discovered new fork, save it at end of function else: if forkVersion.isSome(): if forkVersion.get() == localForkVersion.get(): - discard # Already known + ok() # Already known else: - return err("Beacon node has conflicting " & - consensusFork.forkVersionConfigKey() & " value") + err("Beacon node has conflicting " & + consensusFork.forkVersionConfigKey() & " value") else: if wallEpoch < localForkEpoch: debug "Beacon node must be updated before fork activates", node = node, consensusFork, forkEpoch = localForkEpoch + ok() else: - return err("Beacon node must be updated and report correct " & - $consensusFork & " config value") + err("Beacon node must be updated and report correct " & + $consensusFork & " config value") ? ConsensusFork.Capella.validateForkVersionCompatibility( localForkConfig.capellaVersion, @@ -1468,23 +1469,24 @@ proc updateRuntimeConfig*(vc: ValidatorClientRef, localForkEpoch: Epoch, forkEpoch: Epoch): Result[void, string] = if localForkEpoch == FAR_FUTURE_EPOCH: - discard # Potentially discovered new fork, save it at end of function + ok() # Potentially discovered new fork, save it at end of function else: if forkEpoch != FAR_FUTURE_EPOCH: if forkEpoch == localForkEpoch: - discard # Already known + ok() # Already known else: - return err("Beacon node has conflicting " & - consensusFork.forkEpochConfigKey() & " value") + err("Beacon node has conflicting " & + consensusFork.forkEpochConfigKey() & " value") else: if wallEpoch < localForkEpoch: debug "Beacon node must be updated before fork activates", node = node, consensusFork, forkEpoch = localForkEpoch + ok() else: - return err("Beacon node must be updated and report correct " & - $consensusFork & " config value") + err("Beacon node must be updated and report correct " & + $consensusFork & " config value") ? ConsensusFork.Altair.validateForkEpochCompatibility( localForkConfig.altairEpoch, forkConfig.altairEpoch)