op-node: Make ban threshold and duration configurable.

op-node/flags/p2p_flags.go

@@ -51,6 +51,20 @@ var (
 		Required: false,
 		EnvVar:   p2pEnv("PEER_BANNING"),
 	}
+	BanningThreshold = cli.Float64Flag{
+		Name:     "p2p.ban.threshold",
+		Usage:    "The minimum score below which peers are disconnected and banned.",
+		Required: false,
+		Value:    -100,
+		EnvVar:   p2pEnv("PEER_BANNING_THRESHOLD"),
+	}
+	BanningDuration = cli.DurationFlag{
+		Name:     "p2p.ban.duration",
+		Usage:    "The duration that peers are banned for.",
+		Required: false,
+		Value:    1 * time.Hour,
+		EnvVar:   p2pEnv("PEER_BANNING_DURATION"),
+	}
 
 	TopicScoring = cli.StringFlag{
 		Name: "p2p.scoring.topics",
@@ -294,6 +308,8 @@ var p2pFlags = []cli.Flag{
 	PeerScoring,
 	PeerScoreBands,
 	Banning,
+	BanningThreshold,
+	BanningDuration,
 	TopicScoring,
 	ListenIP,
 	ListenTCPPort,

op-node/p2p/cli/load_config.go

@@ -62,7 +62,7 @@ func NewConfig(ctx *cli.Context, blockTime uint64) (*p2p.Config, error) {
 		return nil, fmt.Errorf("failed to load p2p peer score bands: %w", err)
 	}
 
-	if err := loadBanningOption(conf, ctx); err != nil {
+	if err := loadBanningOptions(conf, ctx); err != nil {
 		return nil, fmt.Errorf("failed to load banning option: %w", err)
 	}
 
@@ -135,10 +135,11 @@ func loadPeerScoreBands(conf *p2p.Config, ctx *cli.Context) error {
 	return nil
 }
 
-// loadBanningOption loads whether or not to ban peers from the CLI context.
-func loadBanningOption(conf *p2p.Config, ctx *cli.Context) error {
-	ban := ctx.GlobalBool(flags.Banning.Name)
-	conf.BanningEnabled = ban
+// loadBanningOptions loads whether or not to ban peers from the CLI context.
+func loadBanningOptions(conf *p2p.Config, ctx *cli.Context) error {
+	conf.BanningEnabled = ctx.GlobalBool(flags.Banning.Name)
+	conf.BanningThreshold = ctx.GlobalFloat64(flags.BanningThreshold.Name)
+	conf.BanningDuration = ctx.GlobalDuration(flags.BanningDuration.Name)
 	return nil
 }
 

op-node/p2p/config.go

@@ -44,6 +44,10 @@ type SetupP2P interface {
 	// Discovery creates a disc-v5 service. Returns nil, nil, nil if discovery is disabled.
 	Discovery(log log.Logger, rollupCfg *rollup.Config, tcpPort uint16) (*enode.LocalNode, *discover.UDPv5, error)
 	TargetPeers() uint
+	BanPeers() bool
+	BanThreshold() float64
+	BanDuration() time.Duration
+	PeerBandScorer() *BandScoreThresholds
 	GossipSetupConfigurables
 	ReqRespSyncEnabled() bool
 }
@@ -66,8 +70,11 @@ type Config struct {
 	// Peer Score Band Thresholds
 	BandScoreThresholds BandScoreThresholds
 
-	// Whether to ban peers based on their [PeerScoring] score.
+	// Whether to ban peers based on their [PeerScoring] score. Should be negative.
 	BanningEnabled bool
+	// Minimum score before peers are disconnected and banned
+	BanningThreshold float64
+	BanningDuration  time.Duration
 
 	ListenIP      net.IP
 	ListenTCPPort uint16
@@ -143,6 +150,14 @@ func (conf *Config) BanPeers() bool {
 	return conf.BanningEnabled
 }
 
+func (conf *Config) BanThreshold() float64 {
+	return conf.BanningThreshold
+}
+
+func (conf *Config) BanDuration() time.Duration {
+	return conf.BanningDuration
+}
+
 func (conf *Config) TopicScoringParams() *pubsub.TopicScoreParams {
 	return &conf.TopicScoring
 }

op-node/p2p/gossip.go

@@ -53,10 +53,8 @@ var MessageDomainValidSnappy = [4]byte{1, 0, 0, 0}
 type GossipSetupConfigurables interface {
 	PeerScoringParams() *pubsub.PeerScoreParams
 	TopicScoringParams() *pubsub.TopicScoreParams
-	BanPeers() bool
 	// ConfigureGossip creates configuration options to apply to the GossipSub setup
 	ConfigureGossip(rollupCfg *rollup.Config) []pubsub.Option
-	PeerBandScorer() *BandScoreThresholds
 }
 
 type GossipRuntimeConfig interface {

op-node/p2p/host.go

@@ -33,8 +33,7 @@ import (
 )
 
 const (
-	staticPeerTag        = "static"
-	minAcceptedPeerScore = -100
+	staticPeerTag = "static"
 )
 
 type ExtraHostFeatures interface {

op-node/p2p/monitor/peer_monitor.go

@@ -14,7 +14,6 @@ import (
 const (
 	// 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
@@ -30,12 +29,13 @@ 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
+	ctx         context.Context
+	cancelFn    context.CancelFunc
+	l           log.Logger
+	clock       clock.Clock
+	manager     PeerManager
+	minScore    float64
+	banDuration time.Duration
 
 	bgTasks sync.WaitGroup
 
@@ -44,54 +44,55 @@ type PeerMonitor struct {
 	nextPeerIdx int
 }
 
-func NewPeerMonitor(ctx context.Context, l log.Logger, clock clock.Clock, manager PeerManager, minScore float64) *PeerMonitor {
+func NewPeerMonitor(ctx context.Context, l log.Logger, clock clock.Clock, manager PeerManager, minScore float64, banDuration time.Duration) *PeerMonitor {
 	ctx, cancelFn := context.WithCancel(ctx)
 	return &PeerMonitor{
-		ctx:      ctx,
-		cancelFn: cancelFn,
-		l:        l,
-		clock:    clock,
-		manager:  manager,
-		minScore: minScore,
+		ctx:         ctx,
+		cancelFn:    cancelFn,
+		l:           l,
+		clock:       clock,
+		manager:     manager,
+		minScore:    minScore,
+		banDuration: banDuration,
 	}
 }
-func (k *PeerMonitor) Start() {
-	k.bgTasks.Add(1)
-	go k.background(k.checkNextPeer)
+func (p *PeerMonitor) Start() {
+	p.bgTasks.Add(1)
+	go p.background(p.checkNextPeer)
 }
 
-func (k *PeerMonitor) Stop() {
-	k.cancelFn()
-	k.bgTasks.Wait()
+func (p *PeerMonitor) Stop() {
+	p.cancelFn()
+	p.bgTasks.Wait()
 }
 
 // checkNextPeer checks the next peer and disconnects and bans it if its score is too low and its not protected.
 // The first call gets the list of current peers and checks the first one, then each subsequent call checks the next
 // peer in the list.  When the end of the list is reached, an updated list of connected peers is retrieved and the process
 // starts again.
-func (k *PeerMonitor) checkNextPeer() error {
+func (p *PeerMonitor) checkNextPeer() error {
 	// Get a new list of peers to check if we've checked all peers in the previous list
-	if k.nextPeerIdx >= len(k.peerList) {
-		k.peerList = k.manager.Peers()
-		k.nextPeerIdx = 0
+	if p.nextPeerIdx >= len(p.peerList) {
+		p.peerList = p.manager.Peers()
+		p.nextPeerIdx = 0
 	}
-	if len(k.peerList) == 0 {
+	if len(p.peerList) == 0 {
 		// No peers to check
 		return nil
 	}
-	id := k.peerList[k.nextPeerIdx]
-	k.nextPeerIdx++
-	score, err := k.manager.GetPeerScore(id)
+	id := p.peerList[p.nextPeerIdx]
+	p.nextPeerIdx++
+	score, err := p.manager.GetPeerScore(id)
 	if err != nil {
 		return fmt.Errorf("retrieve score for peer %v: %w", id, err)
 	}
-	if score > k.minScore {
+	if score > p.minScore {
 		return nil
 	}
-	if k.manager.IsStatic(id) {
+	if p.manager.IsStatic(id) {
 		return nil
 	}
-	if err := k.manager.BanPeer(id, k.clock.Now().Add(banDuration)); err != nil {
+	if err := p.manager.BanPeer(id, p.clock.Now().Add(p.banDuration)); err != nil {
 		return fmt.Errorf("banning peer %v: %w", id, err)
 	}
 
@@ -100,17 +101,17 @@ func (k *PeerMonitor) checkNextPeer() error {
 
 // background is intended to run as a separate go routine. It will call the supplied action function every checkInterval
 // until the context is done.
-func (k *PeerMonitor) background(action func() error) {
-	defer k.bgTasks.Done()
-	ticker := k.clock.NewTicker(checkInterval)
+func (p *PeerMonitor) background(action func() error) {
+	defer p.bgTasks.Done()
+	ticker := p.clock.NewTicker(checkInterval)
 	defer ticker.Stop()
 	for {
 		select {
-		case <-k.ctx.Done():
+		case <-p.ctx.Done():
 			return
 		case <-ticker.Ch():
 			if err := action(); err != nil {
-				k.l.Warn("Error while checking connected peer score", "err", err)
+				p.l.Warn("Error while checking connected peer score", "err", err)
 			}
 		}
 	}

op-node/p2p/monitor/peer_monitor_test.go

@@ -15,11 +15,13 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+const testBanDuration = 2 * time.Hour
+
 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)
+	monitor := NewPeerMonitor(context.Background(), l, clock, manager, -100, testBanDuration)
 	return monitor, clock, manager
 }
 
@@ -95,7 +97,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(banDuration)).Return(nil).Once()
+		manager.EXPECT().BanPeer(id, clock.Now().Add(testBanDuration)).Return(nil).Once()
 
 		require.NoError(t, monitor.checkNextPeer())
 	})

op-node/p2p/node.go

@@ -155,7 +155,7 @@ func (n *NodeP2P) init(resourcesCtx context.Context, rollupCfg *rollup.Config, l
 		}
 
 		if setup.BanPeers() {
-			n.peerMonitor = monitor.NewPeerMonitor(resourcesCtx, log, clock.SystemClock, n, minAcceptedPeerScore)
+			n.peerMonitor = monitor.NewPeerMonitor(resourcesCtx, log, clock.SystemClock, n, setup.BanThreshold(), setup.BanDuration())
 			n.peerMonitor.Start()
 		}
 	}

op-node/p2p/prepared.go

@@ -3,6 +3,7 @@ package p2p
 import (
 	"errors"
 	"fmt"
+	"time"
 
 	pubsub "github.com/libp2p/go-libp2p-pubsub"
 	"github.com/libp2p/go-libp2p/core/host"
@@ -80,6 +81,14 @@ func (p *Prepared) BanPeers() bool {
 	return false
 }
 
+func (p *Prepared) BanThreshold() float64 {
+	return -100
+}
+
+func (p *Prepared) BanDuration() time.Duration {
+	return 1 * time.Hour
+}
+
 func (p *Prepared) TopicScoringParams() *pubsub.TopicScoreParams {
 	return nil
 }