Skip to content

Commit

Permalink
fix incorrect config validation regression from #5959 (#5966)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
etan-status committed Feb 26, 2024
1 parent 4e9bc7f commit c3fe055
Showing 1 changed file with 14 additions and 12 deletions.
26 changes: 14 additions & 12 deletions beacon_chain/validator_client/common.nim
Expand Up @@ -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,
Expand All @@ -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)
Expand Down

0 comments on commit c3fe055

Please sign in to comment.