Merge branch 'develop' into jm/basic-delay

op-node/node/node.go

@@ -49,6 +49,9 @@ type OpNode struct {
 	resourcesCtx   context.Context
 	resourcesClose context.CancelFunc
 
+	// Indicates when it's safe to close data sources used by the runtimeConfig bg loader
+	runtimeConfigReloaderDone chan struct{}
+
 	closed atomic.Bool
 }
 
@@ -197,9 +200,8 @@ func (n *OpNode) initRuntimeConfig(ctx context.Context, cfg *Config) error {
 			return l1Head, err
 		}
 
-		n.handleProtocolVersionsUpdate(ctx)
-
-		return l1Head, nil
+		err = n.handleProtocolVersionsUpdate(ctx)
+		return l1Head, err
 	}
 
 	// initialize the runtime config before unblocking
@@ -210,10 +212,10 @@ func (n *OpNode) initRuntimeConfig(ctx context.Context, cfg *Config) error {
 	}
 
 	// start a background loop, to keep reloading it at the configured reload interval
-	go func(ctx context.Context, reloadInterval time.Duration) {
+	reloader := func(ctx context.Context, reloadInterval time.Duration) bool {
 		if reloadInterval <= 0 {
 			n.log.Debug("not running runtime-config reloading background loop")
-			return
+			return false
 		}
 		ticker := time.NewTicker(reloadInterval)
 		defer ticker.Stop()
@@ -222,13 +224,30 @@ func (n *OpNode) initRuntimeConfig(ctx context.Context, cfg *Config) error {
 			case <-ticker.C:
 				// If the reload fails, we will try again the next interval.
 				// Missing a runtime-config update is not critical, and we do not want to overwhelm the L1 RPC.
-				if l1Head, err := reload(ctx); err != nil {
-					n.log.Warn("failed to reload runtime config", "err", err)
-				} else {
+				l1Head, err := reload(ctx)
+				switch err {
+				case errNodeHalt, nil:
 					n.log.Debug("reloaded runtime config", "l1_head", l1Head)
+					if err == errNodeHalt {
+						return true
+					}
+				default:
+					n.log.Warn("failed to reload runtime config", "err", err)
 				}
 			case <-ctx.Done():
-				return
+				return false
+			}
+		}
+	}
+
+	n.runtimeConfigReloaderDone = make(chan struct{})
+	// Manages the lifetime of reloader. In order to safely Close the OpNode
+	go func(ctx context.Context, reloadInterval time.Duration) {
+		halt := reloader(ctx, reloadInterval)
+		close(n.runtimeConfigReloaderDone)
+		if halt {
+			if err := n.Close(); err != nil {
+				n.log.Error("Failed to halt rollup", "err", err)
 			}
 		}
 	}(n.resourcesCtx, cfg.RuntimeConfigReloadInterval) // this keeps running after initialization
@@ -499,6 +518,11 @@ func (n *OpNode) Close() error {
 		}
 	}
 
+	// Wait for the runtime config loader to be done using the data sources before closing them
+	if n.runtimeConfigReloaderDone != nil {
+		<-n.runtimeConfigReloaderDone
+	}
+
 	// close L2 engine RPC client
 	if n.l2Source != nil {
 		n.l2Source.Close()

op-node/node/superchain.go

@@ -2,18 +2,21 @@ package node
 
 import (
 	"context"
+	"errors"
 
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum/go-ethereum/eth/catalyst"
 	"github.com/ethereum/go-ethereum/params"
 )
 
-func (n *OpNode) handleProtocolVersionsUpdate(ctx context.Context) {
+var errNodeHalt = errors.New("opted to halt, unprepared for protocol change")
+
+func (n *OpNode) handleProtocolVersionsUpdate(ctx context.Context) error {
 	recommended := n.runCfg.RecommendedProtocolVersion()
 	required := n.runCfg.RequiredProtocolVersion()
 	// if the protocol version sources are disabled we do not process them
 	if recommended == (params.ProtocolVersion{}) && required == (params.ProtocolVersion{}) {
-		return
+		return nil
 	}
 	local := rollup.OPStackSupport
 	// forward to execution engine, and get back the protocol version that op-geth supports
@@ -30,20 +33,20 @@ func (n *OpNode) handleProtocolVersionsUpdate(ctx context.Context) {
 	catalyst.LogProtocolVersionSupport(n.log.New("node", "engine"), local, required, "required")
 
 	// We may need to halt the node, if the user opted in to handling incompatible protocol-version signals
-	n.HaltMaybe()
+	return n.haltMaybe()
 }
 
-// HaltMaybe halts the rollup node if the runtime config indicates an incompatible required protocol change
+// haltMaybe returns errNodeHalt if the runtime config indicates an incompatible required protocol change
 // and the node is configured to opt-in to halting at this protocol-change level.
-func (n *OpNode) HaltMaybe() {
+func (n *OpNode) haltMaybe() error {
 	local := rollup.OPStackSupport
 	required := n.runCfg.RequiredProtocolVersion()
 	if haltMaybe(n.rollupHalt, local.Compare(required)) { // halt if we opted in to do so at this granularity
 		n.log.Error("Opted to halt, unprepared for protocol change", "required", required, "local", local)
-		if err := n.Close(); err != nil {
-			n.log.Error("Failed to halt rollup", "err", err)
-		}
+		// Avoid deadlocking the runtime config reloader by closing the OpNode elsewhere
+		return errNodeHalt
 	}
+	return nil
 }
 
 // haltMaybe returns true when we should halt, given the halt-option and required-version comparison

pnpm-lock.yaml

@@ -92,7 +92,7 @@ importers:
         version: 14.0.1
       markdownlint:
         specifier: ^0.31.0
-        version: 0.31.0
+        version: 0.31.1
       markdownlint-cli2:
         specifier: 0.4.0
         version: 0.4.0
@@ -10707,8 +10707,8 @@ packages:
       markdown-it: 12.3.2
     dev: true
 
-  /markdownlint@0.31.0:
-    resolution: {integrity: sha512-e+jZGRGZrz1s0T4wiPDFtyQafq7sKgdbf2sdL46gRT8zLEvDDihQmGeSCV6VXp9BsfkuZ0dPTAg9hf4j6NFgjg==}
+  /markdownlint@0.31.1:
+    resolution: {integrity: sha512-CKMR2hgcIBrYlIUccDCOvi966PZ0kJExDrUi1R+oF9PvqQmCrTqjOsgIvf2403OmJ+CWomuzDoylr6KbuMyvHA==}
     engines: {node: '>=16'}
     dependencies:
       markdown-it: 13.0.1
@@ -14693,7 +14693,7 @@ packages:
       - terser
     dev: true
 
-  /vite-node@0.34.4(@types/node@20.6.1):
+  /vite-node@0.34.4(@types/node@20.6.2):
     resolution: {integrity: sha512-ho8HtiLc+nsmbwZMw8SlghESEE3KxJNp04F/jPUCLVvaURwt0d+r9LxEqCX5hvrrOQ0GSyxbYr5ZfRYhQ0yVKQ==}
     engines: {node: '>=v14.18.0'}
     hasBin: true
@@ -14703,7 +14703,7 @@ packages:
       mlly: 1.4.0
       pathe: 1.1.1
       picocolors: 1.0.0
-      vite: 4.4.9(@types/node@20.6.1)
+      vite: 4.4.9(@types/node@20.6.2)
     transitivePeerDependencies:
       - '@types/node'
       - less
@@ -14787,6 +14787,42 @@ packages:
       fsevents: 2.3.2
     dev: true
 
+  /vite@4.4.9(@types/node@20.6.2):
+    resolution: {integrity: sha512-2mbUn2LlUmNASWwSCNSJ/EG2HuSRTnVNaydp6vMCm5VIqJsjMfbIWtbH2kDuwUVW5mMUKKZvGPX/rqeqVvv1XA==}
+    engines: {node: ^14.18.0 || >=16.0.0}
+    hasBin: true
+    peerDependencies:
+      '@types/node': '>= 14'
+      less: '*'
+      lightningcss: ^1.21.0
+      sass: '*'
+      stylus: '*'
+      sugarss: '*'
+      terser: ^5.4.0
+    peerDependenciesMeta:
+      '@types/node':
+        optional: true
+      less:
+        optional: true
+      lightningcss:
+        optional: true
+      sass:
+        optional: true
+      stylus:
+        optional: true
+      sugarss:
+        optional: true
+      terser:
+        optional: true
+    dependencies:
+      '@types/node': 20.6.2
+      esbuild: 0.18.15
+      postcss: 8.4.27
+      rollup: 3.28.0
+    optionalDependencies:
+      fsevents: 2.3.2
+    dev: true
+
   /vitest@0.34.1:
     resolution: {integrity: sha512-G1PzuBEq9A75XSU88yO5G4vPT20UovbC/2osB2KEuV/FisSIIsw7m5y2xMdB7RsAGHAfg2lPmp2qKr3KWliVlQ==}
     engines: {node: '>=v14.18.0'}
@@ -14951,7 +14987,7 @@ packages:
     dependencies:
       '@types/chai': 4.3.6
       '@types/chai-subset': 1.3.3
-      '@types/node': 20.6.1
+      '@types/node': 20.6.2
       '@vitest/expect': 0.34.4
       '@vitest/runner': 0.34.4
       '@vitest/snapshot': 0.34.4
@@ -14970,8 +15006,8 @@ packages:
       strip-literal: 1.0.1
       tinybench: 2.5.0
       tinypool: 0.7.0
-      vite: 4.4.9(@types/node@20.6.1)
-      vite-node: 0.34.4(@types/node@20.6.1)
+      vite: 4.4.9(@types/node@20.6.2)
+      vite-node: 0.34.4(@types/node@20.6.2)
       why-is-node-running: 2.2.2
     transitivePeerDependencies:
       - less