op-node: Introduce suitable constants to avoid magic numbers in PeerMonitor

op-node/p2p/host.go

@@ -32,6 +32,11 @@ import (
 	"github.com/ethereum-optimism/optimism/op-service/clock"
 )
 
+const (
+	staticPeerTag        = "static"
+	minAcceptedPeerScore = -100
+)
+
 type ExtraHostFeatures interface {
 	host.Host
 	ConnectionGater() gating.BlockingConnectionGater
@@ -67,7 +72,7 @@ func (e *extraHost) initStaticPeers() {
 		e.Peerstore().AddAddrs(addr.ID, addr.Addrs, time.Hour*24*7)
 		// We protect the peer, so the connection manager doesn't decide to prune it.
 		// We tag it with "static" so other protects/unprotects with different tags don't affect this protection.
-		e.connMgr.Protect(addr.ID, "static")
+		e.connMgr.Protect(addr.ID, staticPeerTag)
 		// Try to dial the node in the background
 		go func(addr *peer.AddrInfo) {
 			ctx, cancel := context.WithTimeout(context.Background(), time.Second*30)

op-node/p2p/monitor/peer_monitor.go

@@ -12,8 +12,9 @@ import (
 )
 
 const (
-	// Time delay between checking the score of each peer to avoid
+	// Time delay between checking the score of each peer to avoid activity spikes
 	checkInterval = 1 * time.Second
+	banDuration   = 1 * time.Hour
 )
 
 //go:generate mockery --name PeerManager --output mocks/ --with-expecter=true
@@ -29,13 +30,12 @@ type PeerManager interface {
 // When it finds bad peers, it disconnects and bans them.
 // A delay is introduced between each peer being checked to avoid spikes in system load.
 type PeerMonitor struct {
-	ctx         context.Context
-	cancelFn    context.CancelFunc
-	l           log.Logger
-	clock       clock.Clock
-	manager     PeerManager
-	minScore    float64
-	banDuration time.Duration
+	ctx      context.Context
+	cancelFn context.CancelFunc
+	l        log.Logger
+	clock    clock.Clock
+	manager  PeerManager
+	minScore float64
 
 	bgTasks sync.WaitGroup
 
@@ -44,16 +44,15 @@ type PeerMonitor struct {
 	nextPeerIdx int
 }
 
-func NewPeerMonitor(ctx context.Context, l log.Logger, clock clock.Clock, manager PeerManager, minScore float64, banDuration time.Duration) *PeerMonitor {
+func NewPeerMonitor(ctx context.Context, l log.Logger, clock clock.Clock, manager PeerManager, minScore float64) *PeerMonitor {
 	ctx, cancelFn := context.WithCancel(ctx)
 	return &PeerMonitor{
-		ctx:         ctx,
-		cancelFn:    cancelFn,
-		l:           l,
-		clock:       clock,
-		manager:     manager,
-		minScore:    minScore,
-		banDuration: banDuration,
+		ctx:      ctx,
+		cancelFn: cancelFn,
+		l:        l,
+		clock:    clock,
+		manager:  manager,
+		minScore: minScore,
 	}
 }
 func (k *PeerMonitor) Start() {
@@ -92,7 +91,7 @@ func (k *PeerMonitor) checkNextPeer() error {
 	if k.manager.IsStatic(id) {
 		return nil
 	}
-	if err := k.manager.BanPeer(id, k.clock.Now().Add(k.banDuration)); err != nil {
+	if err := k.manager.BanPeer(id, k.clock.Now().Add(banDuration)); err != nil {
 		return fmt.Errorf("banning peer %v: %w", id, err)
 	}
 

op-node/p2p/monitor/peer_monitor_test.go

@@ -15,13 +15,11 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-const monitorBanDuration = 10 * time.Minute
-
 func peerMonitorSetup(t *testing.T) (*PeerMonitor, *clock2.DeterministicClock, *mocks.PeerManager) {
 	l := testlog.Logger(t, log.LvlInfo)
 	clock := clock2.NewDeterministicClock(time.UnixMilli(10000))
 	manager := mocks.NewPeerManager(t)
-	monitor := NewPeerMonitor(context.Background(), l, clock, manager, -100, monitorBanDuration)
+	monitor := NewPeerMonitor(context.Background(), l, clock, manager, -100)
 	return monitor, clock, manager
 }
 
@@ -97,7 +95,7 @@ func TestCheckNextPeer(t *testing.T) {
 		manager.EXPECT().Peers().Return(peerIDs).Once()
 		manager.EXPECT().GetPeerScore(id).Return(-101, nil).Once()
 		manager.EXPECT().IsProtected(id).Return(false).Once()
-		manager.EXPECT().BanPeer(id, clock.Now().Add(monitorBanDuration)).Return(nil).Once()
+		manager.EXPECT().BanPeer(id, clock.Now().Add(banDuration)).Return(nil).Once()
 
 		require.NoError(t, monitor.checkNextPeer())
 	})

op-node/p2p/node.go

@@ -126,9 +126,7 @@ func (n *NodeP2P) init(resourcesCtx context.Context, rollupCfg *rollup.Config, l
 				n.scorer.OnDisconnect(conn.RemotePeer())
 			},
 		})
-		// TODO: minScore shouldn't just be hard coded here
-		// TODO: Ban duration needs to be set sensibly and probably shouldn't be a magic number here
-		n.peerMonitor = monitor.NewPeerMonitor(resourcesCtx, log, clock.SystemClock, n, -100, 1*time.Hour)
+		n.peerMonitor = monitor.NewPeerMonitor(resourcesCtx, log, clock.SystemClock, n, minAcceptedPeerScore)
 		// notify of any new connections/streams/etc.
 		n.host.Network().Notify(NewNetworkNotifier(log, metrics))
 		// note: the IDDelta functionality was removed from libP2P, and no longer needs to be explicitly disabled.
@@ -214,8 +212,7 @@ func (n *NodeP2P) GetPeerScore(id peer.ID) (float64, error) {
 }
 
 func (n *NodeP2P) IsStatic(id peer.ID) bool {
-	// TODO: "static" constant should be shared with host layer rather than hard-coded here
-	return n.connMgr != nil && n.connMgr.IsProtected(id, "static")
+	return n.connMgr != nil && n.connMgr.IsProtected(id, staticPeerTag)
 }
 
 func (n *NodeP2P) BanPeer(id peer.ID, expiration time.Time) error {