nimbus startup cleanups (#3806)

* shutdown both threads in case of init failure on one
* rename trusted-setup-file option to match eth2, make sure trusted
setup is loaded in main thread

Author: arnetheduck

execution_chain/conf.nim

@@ -116,10 +116,11 @@ type
       name: "import-key" .}: InputFile
 
     trustedSetupFile* {.
-      desc: "Load EIP-4844 trusted setup file"
+      hidden
+      desc: "Alternative EIP-4844 trusted setup file"
       defaultValue: none(string)
       defaultValueDesc: "Baked in trusted setup"
-      name: "trusted-setup-file" .}: Option[string]
+      name: "debug-trusted-setup-file" .}: Option[string]
 
     extraData* {.
       separator: "\pPAYLOAD BUILDING OPTIONS:"

execution_chain/nimbus.nim

@@ -147,6 +147,13 @@ type
       defaultValue: false
       name: "engine-api" .}: bool
 
+    trustedSetupFile* {.
+      hidden
+      desc: "Alternative EIP-4844 trusted setup file"
+      defaultValue: none(string)
+      defaultValueDesc: "Baked in trusted setup"
+      name: "debug-trusted-setup-file" .}: Option[string]
+
     case cmd* {.command, defaultValue: NStartUpCmd.nimbus.}: NStartUpCmd
     of nimbus:
       discard
@@ -201,8 +208,6 @@ proc runBeaconNode(p: BeaconThreadConfig) {.thread.} =
     stderr.writeLine error # Logging not yet set up
     quit QuitFailure
 
-  let rng = HmacDrbgContext.new()
-
   let engineUrl = EngineApiUrl.init(
     &"http://127.0.0.1:{defaultEngineApiPort}/", Opt.some(@(distinctBase(jwtKey)))
   )
@@ -218,45 +223,22 @@ proc runBeaconNode(p: BeaconThreadConfig) {.thread.} =
   config.tcpPort = p.tcpPort
   config.udpPort = p.udpPort
 
-  # TODO https://github.com/status-im/nim-taskpools/issues/6
-  #      share taskpool between bn and ec
-  let taskpool = setupTaskpool(config.numThreads)
-
   info "Launching beacon node",
     version = fullVersionStr,
     bls_backend = $BLS_BACKEND,
     const_preset,
     cmdParams = commandLineParams(),
-    config,
-    numThreads = taskpool.numThreads
-
-  config.createDumpDirs()
-
-  let metadata = config.loadEth2Network()
+    config
 
-  # Updating the config based on the metadata certainly is not beautiful but it
-  # works
-  for node in metadata.bootstrapNodes:
-    config.bootstrapNodes.add node
-
-  if config.syncHorizon.isNone:
-    config.syncHorizon = some(metadata.cfg.timeParams.defaultSyncHorizon)
-
-  block:
-    let res =
-      if config.trustedSetupFile.isNone:
-        bnconf.loadKzgTrustedSetup()
-      else:
-        bnconf.loadKzgTrustedSetup(config.trustedSetupFile.get)
-    if res.isErr():
-      raiseAssert res.error()
-
-  let stopper = p.tsp.justWait()
-
-  if stopper.finished():
-    return
-
-  let node = waitFor BeaconNode.init(rng, config, metadata, taskpool)
+  let
+    # TODO https://github.com/status-im/nim-taskpools/issues/6
+    #      share taskpool between bn and ec
+    taskpool = setupTaskpool(config.numThreads)
+    stopper = p.tsp.justWait()
+    rng = HmacDrbgContext.new()
+    node = (waitFor BeaconNode.init(rng, config, taskpool)).valueOr:
+      waitFor p.tsp.fire() # Stop the other thread as well..
+      return
 
   if stopper.finished():
     return
@@ -271,6 +253,9 @@ proc runBeaconNode(p: BeaconThreadConfig) {.thread.} =
     else:
       node.run(stopper)
 
+  # Stop the other thread as well, in case we're stopping early
+  waitFor p.tsp.fire()
+
 proc runExecutionClient(p: ExecutionThreadConfig) {.thread.} =
   var config = makeConfig(ignoreUnknown = true)
   config.metricsEnabled = false
@@ -285,15 +270,106 @@ proc runExecutionClient(p: ExecutionThreadConfig) {.thread.} =
 
   info "Launching execution client", version = FullVersionStr, config
 
-  # TODO https://github.com/status-im/nim-taskpools/issues/6
-  #      share taskpool between bn and ec
   let
+    # TODO https://github.com/status-im/nim-taskpools/issues/6
+    #      share taskpool between bn and ec
     taskpool = setupTaskpool(int config.numThreads)
     com = setupCommonRef(config, taskpool)
 
-  {.gcsafe.}:
-    dynamicLogScope(comp = "ec"):
-      nimbus_execution_client.runExeClient(config, com, p.tsp.justWait())
+  dynamicLogScope(comp = "ec"):
+    nimbus_execution_client.runExeClient(config, com, p.tsp.justWait())
+
+  # Stop the other thread as well, in case `runExeClient` stopped early
+  waitFor p.tsp.fire()
+
+proc runCombinedClient() =
+  # Make it harder to connect to the (internal) engine - this will of course
+  # go away
+  discard randomBytes(distinctBase(jwtKey))
+
+  const banner = "Nimbus v0.0.1"
+
+  var config = NimbusConf.loadWithBanners(banner, copyright, [specBanner], true).valueOr:
+    writePanicLine error # Logging not yet set up
+    quit QuitFailure
+
+  setupLogging(config.logLevel, config.logStdout, none OutFile)
+  setupFileLimits()
+
+  ProcessState.setupStopHandlers()
+
+  if not checkAndCreateDataDir(config.dataDir):
+    # We are unable to access/create data folder or data folder's
+    # permissions are insecure.
+    quit QuitFailure
+
+  let metricsServer = (waitFor config.initMetricsServer()).valueOr:
+    quit 1
+
+  # Nim GC metrics (for the main thread) will be collected in onSecond(), but
+  # we disable piggy-backing on other metrics here.
+  setSystemMetricsAutomaticUpdate(false)
+
+  if config.engineApiEnabled:
+    warn "Engine API is not available when running internal beacon node"
+
+  # Trusted setup is shared between threads, so it needs to be initalized
+  # from the main thread before anything else runs
+  if config.trustedSetupFile.isSome:
+    kzg.loadTrustedSetup(config.trustedSetupFile.get(), 0).isOkOr:
+      fatal "Cannot load Kzg trusted setup from file", msg = error
+      quit(QuitFailure)
+  else:
+    # Load eagerly to avoid race conditions - lazy kzg loading is not thread safe
+    loadTrustedSetupFromString(kzg.trustedSetup, 0).expect(
+      "Baked-in KZG setup is correct"
+    )
+
+  var bnThread: Thread[BeaconThreadConfig]
+  let bnStop = ThreadSignalPtr.new().expect("working ThreadSignalPtr")
+  createThread(
+    bnThread,
+    runBeaconNode,
+    BeaconThreadConfig(
+      tsp: bnStop,
+      tcpPort: config.beaconTcpPort.get(config.tcpPort.get(Port defaultEth2TcpPort)),
+      udpPort: config.beaconUdpPort.get(config.udpPort.get(Port defaultEth2TcpPort)),
+      elSync: config.elSync,
+    ),
+  )
+
+  var ecThread: Thread[ExecutionThreadConfig]
+  let ecStop = ThreadSignalPtr.new().expect("working ThreadSignalPtr")
+  createThread(
+    ecThread,
+    runExecutionClient,
+    ExecutionThreadConfig(
+      tsp: ecStop,
+      tcpPort:
+        # -1/+1 to make sure global default is respected but +1 is applied to --tcp-port
+        config.executionTcpPort.get(
+          Port(uint16(config.tcpPort.get(Port(defaultExecutionPort - 1))) + 1)
+        ),
+      udpPort:
+        if config.executionUdpPort.isSome:
+          config.executionUdpPort
+        elif config.udpPort.isSome:
+          some(Port(uint16(config.udpPort.get()) + 1))
+        else:
+          none(Port),
+    ),
+  )
+
+  while not ProcessState.stopIt(notice("Shutting down", reason = it)):
+    os.sleep(100)
+
+  waitFor bnStop.fire()
+  waitFor ecStop.fire()
+
+  joinThread(bnThread)
+  joinThread(ecThread)
+
+  waitFor metricsServer.stopMetricsServer()
 
 # noinline to keep it in stack traces
 proc main() {.noinline, raises: [CatchableError].} =
@@ -337,82 +413,7 @@ proc main() {.noinline, raises: [CatchableError].} =
   elif isEC:
     nimbus_execution_client.main()
   else:
-    # Make sure the default nim handlers don't run in any thread
-    ProcessState.setupStopHandlers()
-
-    # Make it harder to connect to the (internal) engine - this will of course
-    # go away
-    discard randomBytes(distinctBase(jwtKey))
-
-    const banner = "Nimbus v0.0.1"
-
-    var config = NimbusConf.loadWithBanners(banner, copyright, [specBanner], true).valueOr:
-      writePanicLine error # Logging not yet set up
-      quit QuitFailure
-
-    setupLogging(config.logLevel, config.logStdout, none OutFile)
-    setupFileLimits()
-
-    if not (checkAndCreateDataDir(config.dataDir)):
-      # We are unable to access/create data folder or data folder's
-      # permissions are insecure.
-      quit QuitFailure
-
-    let metricsServer = (waitFor config.initMetricsServer()).valueOr:
-      quit 1
-
-    # Nim GC metrics (for the main thread) will be collected in onSecond(), but
-    # we disable piggy-backing on other metrics here.
-    setSystemMetricsAutomaticUpdate(false)
-
-    if config.engineApiEnabled:
-      warn "Engine API is not available when running internal beacon node"
-
-    var bnThread: Thread[BeaconThreadConfig]
-    let bnStop = ThreadSignalPtr.new().expect("working ThreadSignalPtr")
-    createThread(
-      bnThread,
-      runBeaconNode,
-      BeaconThreadConfig(
-        tsp: bnStop,
-        tcpPort: config.beaconTcpPort.get(config.tcpPort.get(Port defaultEth2TcpPort)),
-        udpPort: config.beaconUdpPort.get(config.udpPort.get(Port defaultEth2TcpPort)),
-        elSync: config.elSync,
-      ),
-    )
-
-    var ecThread: Thread[ExecutionThreadConfig]
-    let ecStop = ThreadSignalPtr.new().expect("working ThreadSignalPtr")
-    createThread(
-      ecThread,
-      runExecutionClient,
-      ExecutionThreadConfig(
-        tsp: ecStop,
-        tcpPort:
-          # -1/+1 to make sure global default is respected but +1 is applied to --tcp-port
-          config.executionTcpPort.get(
-            Port(uint16(config.tcpPort.get(Port(defaultExecutionPort - 1))) + 1)
-          ),
-        udpPort:
-          if config.executionUdpPort.isSome:
-            config.executionUdpPort
-          elif config.udpPort.isSome:
-            some(Port(uint16(config.udpPort.get()) + 1))
-          else:
-            none(Port),
-      ),
-    )
-
-    while not ProcessState.stopIt(notice("Shutting down", reason = it)):
-      os.sleep(100)
-
-    waitFor bnStop.fire()
-    waitFor ecStop.fire()
-
-    joinThread(bnThread)
-    joinThread(ecThread)
-
-    waitFor metricsServer.stopMetricsServer()
+    runCombinedClient()
 
 when isMainModule:
   main()

execution_chain/nimbus_execution_client.nim

@@ -223,16 +223,6 @@ proc preventLoadingDataDirForTheWrongNetwork(db: CoreDbRef; config: ExecutionCli
     quit(QuitFailure)
 
 proc setupCommonRef*(config: ExecutionClientConf, taskpool: Taskpool): CommonRef =
-  # Trusted setup is needed for processing Cancun+ blocks
-  # If user not specify the trusted setup, baked in
-  # trusted setup will be loaded, lazily.
-  if config.trustedSetupFile.isSome:
-    let fileName = config.trustedSetupFile.get()
-    let res = kzg.loadTrustedSetup(fileName, 0)
-    if res.isErr:
-      fatal "Cannot load Kzg trusted setup from file", msg=res.error
-      quit(QuitFailure)
-
   let coreDB = AristoDbRocks.newCoreDbRef(
       config.dataDir,
       config.dbOptions(noKeyCache = config.cmd == NimbusCmd.`import`))
@@ -264,9 +254,6 @@ proc setupCommonRef*(config: ExecutionClientConf, taskpool: Taskpool): CommonRef
 
   com
 
-template displayLaunchingInfo(config: ExecutionClientConf) =
-  info "Launching execution client", version = FullVersionStr, config
-
 # ------------------------------------------------------------------------------
 # Public functions, `main()` API
 # ------------------------------------------------------------------------------
@@ -320,11 +307,10 @@ proc runExeClient*(
 proc main*(config = makeConfig(), nimbus = NimbusNode(nil)) {.noinline.} =
   # Set up logging before everything else
   setupLogging(config.logLevel, config.logStdout)
+  setupFileLimits()
 
   info "Launching execution client", version = FullVersionStr, config
 
-  setupFileLimits()
-
   ProcessState.setupStopHandlers()
 
   # TODO provide option for fixing / ignoring permission errors
@@ -333,6 +319,14 @@ proc main*(config = makeConfig(), nimbus = NimbusNode(nil)) {.noinline.} =
     # permissions are insecure.
     quit QuitFailure
 
+  # Trusted setup is needed for Cancun+ blocks and is shared between threads,
+  # so it needs to be initalized from the main thread before anything else tries
+  # to use it
+  if config.trustedSetupFile.isSome:
+    kzg.loadTrustedSetup(config.trustedSetupFile.get(), 0).isOkOr:
+      fatal "Cannot load KZG trusted setup from file", msg = error
+      quit(QuitFailure)
+
   # Metrics are useful not just when running node but also during import
   let metricsServer =
     try:

nimbus_verified_proxy/nimbus_verified_proxy.nim

@@ -160,7 +160,9 @@ proc run*(
 
     network = createEth2Node(
       rng, lcConfig, netKeys, cfg, forkDigests, getBeaconTime, genesis_validators_root
-    )
+    ).valueOr:
+      fatal "Failed to initialize node", err = error
+      quit QuitFailure
 
     # light client is set to optimistic finalization mode
     lightClient = createLightClient(

portal/bridge/beacon/portal_beacon_bridge.nim

@@ -395,7 +395,7 @@ proc runBeacon*(config: PortalBridgeConf) {.raises: [CatchableError].} =
       # is something that works.
 
       # Or basically `lightClientOptimisticUpdateSlotOffset`
-      await sleepAsync((SECONDS_PER_SLOT div INTERVALS_PER_SLOT).int.seconds)
+      await sleepAsync(cfg.timeParams.SLOT_DURATION div INTERVALS_PER_SLOT)
 
       lastOptimisticUpdateSlot = (
         await gossipLCOptimisticUpdate(restClient, portalRpcClient, cfg, forkDigests)

tests/config_file/basic.toml

@@ -3,7 +3,7 @@ era1-dir = "basic/era1/dir"
 era-dir  = "basic/era/dir"
 key-store = "basic/keystore"
 import-key = "basic_import_key"
-trusted-setup-file = "basic_trusted_setup_file"
+debug-trusted-setup-file = "basic_trusted_setup_file"
 extra-data = "basic_extra_data"
 gas-limit = 5678