op-node: update gating to first check perma-bans, then check temp-bans independently

op-node/p2p/gating/expiry.go

 package gating
 
 import (
-	"errors"
-	"net"
 	"time"
 
-	"github.com/ethereum-optimism/optimism/op-service/clock"
-	"github.com/ethereum/go-ethereum/log"
 	"github.com/libp2p/go-libp2p/core/network"
 	"github.com/libp2p/go-libp2p/core/peer"
 	"github.com/multiformats/go-multiaddr"
 	manet "github.com/multiformats/go-multiaddr/net"
+
+	"github.com/ethereum/go-ethereum/log"
+
+	"github.com/ethereum-optimism/optimism/op-node/p2p/store"
+	"github.com/ethereum-optimism/optimism/op-service/clock"
 )
 
 type UnbanMetrics interface {
@@ -18,12 +19,10 @@ type UnbanMetrics interface {
 	RecordIPUnban()
 }
 
-var UnknownExpiry = errors.New("unknown ban expiry")
-
 //go:generate mockery --name ExpiryStore --output mocks/ --with-expecter=true
 type ExpiryStore interface {
-	PeerBanExpiry(id peer.ID) (time.Time, error)
-	IPBanExpiry(ip net.IP) (time.Time, error)
+	store.IPBanStore
+	store.PeerBanStore
 }
 
 // ExpiryConnectionGater enhances a BlockingConnectionGater by implementing ban-expiration
@@ -47,11 +46,11 @@ func AddBanExpiry(gater BlockingConnectionGater, store ExpiryStore, log log.Logg
 
 func (g *ExpiryConnectionGater) peerBanExpiryCheck(p peer.ID) (allow bool) {
 	// if the peer is blocked, check if it's time to unblock
-	expiry, err := g.store.PeerBanExpiry(p)
+	expiry, err := g.store.GetPeerBanExpiration(p)
+	if err == store.UnknownBanErr {
+		return true // peer is allowed if it has not been banned
+	}
 	if err != nil {
-		if errors.Is(err, UnknownExpiry) {
-			return false // peer is permanently banned if no expiry time is set.
-		}
 		g.log.Warn("failed to load peer-ban expiry time", "peer_id", p, "err", err)
 		return false
 	}
@@ -59,7 +58,7 @@ func (g *ExpiryConnectionGater) peerBanExpiryCheck(p peer.ID) (allow bool) {
 		return false
 	}
 	g.log.Info("peer-ban expired, unbanning peer", "peer_id", p, "expiry", expiry)
-	if err := g.BlockingConnectionGater.UnblockPeer(p); err != nil {
+	if err := g.store.SetPeerBanExpiration(p, time.Time{}); err != nil {
 		g.log.Warn("failed to unban peer", "peer_id", p, "err", err)
 		return false // if we ignored the error, then the inner connection-gater would drop them
 	}
@@ -73,18 +72,12 @@ func (g *ExpiryConnectionGater) addrBanExpiryCheck(ma multiaddr.Multiaddr) (allo
 		g.log.Error("tried to check multi-addr with bad IP", "addr", ma)
 		return false
 	}
-	// Check if it's a subnet-wide ban first. Subnet-bans do not expire.
-	for _, ipnet := range g.BlockingConnectionGater.ListBlockedSubnets() {
-		if ipnet.Contains(ip) {
-			return false // peer is still in banned subnet
-		}
-	}
 	// if just the IP is blocked, check if it's time to unblock
-	expiry, err := g.store.IPBanExpiry(ip)
+	expiry, err := g.store.GetIPBanExpiration(ip)
+	if err == store.UnknownBanErr {
+		return true // IP is allowed if it has not been banned
+	}
 	if err != nil {
-		if errors.Is(err, UnknownExpiry) {
-			return false // IP is permanently banned if no expiry time is set.
-		}
 		g.log.Warn("failed to load IP-ban expiry time", "ip", ip, "err", err)
 		return false
 	}
@@ -92,7 +85,7 @@ func (g *ExpiryConnectionGater) addrBanExpiryCheck(ma multiaddr.Multiaddr) (allo
 		return false
 	}
 	g.log.Info("IP-ban expired, unbanning IP", "ip", ip, "expiry", expiry)
-	if err := g.BlockingConnectionGater.UnblockAddr(ip); err != nil {
+	if err := g.store.SetIPBanExpiration(ip, time.Time{}); err != nil {
 		g.log.Warn("failed to unban IP", "ip", ip, "err", err)
 		return false // if we ignored the error, then the inner connection-gater would drop them
 	}
@@ -101,31 +94,24 @@ func (g *ExpiryConnectionGater) addrBanExpiryCheck(ma multiaddr.Multiaddr) (allo
 }
 
 func (g *ExpiryConnectionGater) InterceptPeerDial(p peer.ID) (allow bool) {
-	// if not allowed, and not expired, then do not allow the dial
-	return g.BlockingConnectionGater.InterceptPeerDial(p) || g.peerBanExpiryCheck(p)
+	if !g.BlockingConnectionGater.InterceptPeerDial(p) {
+		return false
+	}
+	return g.peerBanExpiryCheck(p)
 }
 
 func (g *ExpiryConnectionGater) InterceptAddrDial(id peer.ID, ma multiaddr.Multiaddr) (allow bool) {
 	if !g.BlockingConnectionGater.InterceptAddrDial(id, ma) {
-		// Check if it was intercepted because of a peer ban
-		if !g.BlockingConnectionGater.InterceptPeerDial(id) {
-			if !g.peerBanExpiryCheck(id) {
-				return false // peer is still peer-banned
-			}
-			if g.BlockingConnectionGater.InterceptAddrDial(id, ma) { // allow dial if peer-ban was everything
-				return true
-			}
-		}
-		// intercepted because of addr ban still, check if it is expired
-		if !g.addrBanExpiryCheck(ma) {
-			return false // peer is still addr-banned
-		}
+		return false
 	}
-	return true
+	return g.peerBanExpiryCheck(id) && g.addrBanExpiryCheck(ma)
 }
 
 func (g *ExpiryConnectionGater) InterceptAccept(mas network.ConnMultiaddrs) (allow bool) {
-	return g.BlockingConnectionGater.InterceptAccept(mas) || g.addrBanExpiryCheck(mas.RemoteMultiaddr())
+	if !g.BlockingConnectionGater.InterceptAccept(mas) {
+		return false
+	}
+	return g.addrBanExpiryCheck(mas.RemoteMultiaddr())
 }
 
 func (g *ExpiryConnectionGater) InterceptSecured(direction network.Direction, id peer.ID, mas network.ConnMultiaddrs) (allow bool) {
@@ -133,7 +119,10 @@ func (g *ExpiryConnectionGater) InterceptSecured(direction network.Direction, id
 	if direction == network.DirOutbound {
 		return true
 	}
+	if !g.BlockingConnectionGater.InterceptSecured(direction, id, mas) {
+		return false
+	}
 	// InterceptSecured is called after InterceptAccept, we already checked the addrs.
 	// This leaves just the peer-ID expiry to check on inbound connections.
-	return g.BlockingConnectionGater.InterceptSecured(direction, id, mas) || g.peerBanExpiryCheck(id)
+	return g.peerBanExpiryCheck(id)
 }

op-node/p2p/gating/mocks/ExpiryStore.go

@@ -25,8 +25,8 @@ func (_m *ExpiryStore) EXPECT() *ExpiryStore_Expecter {
 	return &ExpiryStore_Expecter{mock: &_m.Mock}
 }
 
-// IPBanExpiry provides a mock function with given fields: ip
-func (_m *ExpiryStore) IPBanExpiry(ip net.IP) (time.Time, error) {
+// GetIPBanExpiration provides a mock function with given fields: ip
+func (_m *ExpiryStore) GetIPBanExpiration(ip net.IP) (time.Time, error) {
 	ret := _m.Called(ip)
 
 	var r0 time.Time
@@ -49,36 +49,36 @@ func (_m *ExpiryStore) IPBanExpiry(ip net.IP) (time.Time, error) {
 	return r0, r1
 }
 
-// ExpiryStore_IPBanExpiry_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'IPBanExpiry'
-type ExpiryStore_IPBanExpiry_Call struct {
+// ExpiryStore_GetIPBanExpiration_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetIPBanExpiration'
+type ExpiryStore_GetIPBanExpiration_Call struct {
 	*mock.Call
 }
 
-// IPBanExpiry is a helper method to define mock.On call
+// GetIPBanExpiration is a helper method to define mock.On call
 //   - ip net.IP
-func (_e *ExpiryStore_Expecter) IPBanExpiry(ip interface{}) *ExpiryStore_IPBanExpiry_Call {
-	return &ExpiryStore_IPBanExpiry_Call{Call: _e.mock.On("IPBanExpiry", ip)}
+func (_e *ExpiryStore_Expecter) GetIPBanExpiration(ip interface{}) *ExpiryStore_GetIPBanExpiration_Call {
+	return &ExpiryStore_GetIPBanExpiration_Call{Call: _e.mock.On("GetIPBanExpiration", ip)}
 }
 
-func (_c *ExpiryStore_IPBanExpiry_Call) Run(run func(ip net.IP)) *ExpiryStore_IPBanExpiry_Call {
+func (_c *ExpiryStore_GetIPBanExpiration_Call) Run(run func(ip net.IP)) *ExpiryStore_GetIPBanExpiration_Call {
 	_c.Call.Run(func(args mock.Arguments) {
 		run(args[0].(net.IP))
 	})
 	return _c
 }
 
-func (_c *ExpiryStore_IPBanExpiry_Call) Return(_a0 time.Time, _a1 error) *ExpiryStore_IPBanExpiry_Call {
+func (_c *ExpiryStore_GetIPBanExpiration_Call) Return(_a0 time.Time, _a1 error) *ExpiryStore_GetIPBanExpiration_Call {
 	_c.Call.Return(_a0, _a1)
 	return _c
 }
 
-func (_c *ExpiryStore_IPBanExpiry_Call) RunAndReturn(run func(net.IP) (time.Time, error)) *ExpiryStore_IPBanExpiry_Call {
+func (_c *ExpiryStore_GetIPBanExpiration_Call) RunAndReturn(run func(net.IP) (time.Time, error)) *ExpiryStore_GetIPBanExpiration_Call {
 	_c.Call.Return(run)
 	return _c
 }
 
-// PeerBanExpiry provides a mock function with given fields: id
-func (_m *ExpiryStore) PeerBanExpiry(id peer.ID) (time.Time, error) {
+// GetPeerBanExpiration provides a mock function with given fields: id
+func (_m *ExpiryStore) GetPeerBanExpiration(id peer.ID) (time.Time, error) {
 	ret := _m.Called(id)
 
 	var r0 time.Time
@@ -101,30 +101,116 @@ func (_m *ExpiryStore) PeerBanExpiry(id peer.ID) (time.Time, error) {
 	return r0, r1
 }
 
-// ExpiryStore_PeerBanExpiry_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'PeerBanExpiry'
-type ExpiryStore_PeerBanExpiry_Call struct {
+// ExpiryStore_GetPeerBanExpiration_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetPeerBanExpiration'
+type ExpiryStore_GetPeerBanExpiration_Call struct {
 	*mock.Call
 }
 
-// PeerBanExpiry is a helper method to define mock.On call
+// GetPeerBanExpiration is a helper method to define mock.On call
 //   - id peer.ID
-func (_e *ExpiryStore_Expecter) PeerBanExpiry(id interface{}) *ExpiryStore_PeerBanExpiry_Call {
-	return &ExpiryStore_PeerBanExpiry_Call{Call: _e.mock.On("PeerBanExpiry", id)}
+func (_e *ExpiryStore_Expecter) GetPeerBanExpiration(id interface{}) *ExpiryStore_GetPeerBanExpiration_Call {
+	return &ExpiryStore_GetPeerBanExpiration_Call{Call: _e.mock.On("GetPeerBanExpiration", id)}
 }
 
-func (_c *ExpiryStore_PeerBanExpiry_Call) Run(run func(id peer.ID)) *ExpiryStore_PeerBanExpiry_Call {
+func (_c *ExpiryStore_GetPeerBanExpiration_Call) Run(run func(id peer.ID)) *ExpiryStore_GetPeerBanExpiration_Call {
 	_c.Call.Run(func(args mock.Arguments) {
 		run(args[0].(peer.ID))
 	})
 	return _c
 }
 
-func (_c *ExpiryStore_PeerBanExpiry_Call) Return(_a0 time.Time, _a1 error) *ExpiryStore_PeerBanExpiry_Call {
+func (_c *ExpiryStore_GetPeerBanExpiration_Call) Return(_a0 time.Time, _a1 error) *ExpiryStore_GetPeerBanExpiration_Call {
 	_c.Call.Return(_a0, _a1)
 	return _c
 }
 
-func (_c *ExpiryStore_PeerBanExpiry_Call) RunAndReturn(run func(peer.ID) (time.Time, error)) *ExpiryStore_PeerBanExpiry_Call {
+func (_c *ExpiryStore_GetPeerBanExpiration_Call) RunAndReturn(run func(peer.ID) (time.Time, error)) *ExpiryStore_GetPeerBanExpiration_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// SetIPBanExpiration provides a mock function with given fields: ip, expiry
+func (_m *ExpiryStore) SetIPBanExpiration(ip net.IP, expiry time.Time) error {
+	ret := _m.Called(ip, expiry)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(net.IP, time.Time) error); ok {
+		r0 = rf(ip, expiry)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// ExpiryStore_SetIPBanExpiration_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'SetIPBanExpiration'
+type ExpiryStore_SetIPBanExpiration_Call struct {
+	*mock.Call
+}
+
+// SetIPBanExpiration is a helper method to define mock.On call
+//   - ip net.IP
+//   - expiry time.Time
+func (_e *ExpiryStore_Expecter) SetIPBanExpiration(ip interface{}, expiry interface{}) *ExpiryStore_SetIPBanExpiration_Call {
+	return &ExpiryStore_SetIPBanExpiration_Call{Call: _e.mock.On("SetIPBanExpiration", ip, expiry)}
+}
+
+func (_c *ExpiryStore_SetIPBanExpiration_Call) Run(run func(ip net.IP, expiry time.Time)) *ExpiryStore_SetIPBanExpiration_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(net.IP), args[1].(time.Time))
+	})
+	return _c
+}
+
+func (_c *ExpiryStore_SetIPBanExpiration_Call) Return(_a0 error) *ExpiryStore_SetIPBanExpiration_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *ExpiryStore_SetIPBanExpiration_Call) RunAndReturn(run func(net.IP, time.Time) error) *ExpiryStore_SetIPBanExpiration_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// SetPeerBanExpiration provides a mock function with given fields: id, expiry
+func (_m *ExpiryStore) SetPeerBanExpiration(id peer.ID, expiry time.Time) error {
+	ret := _m.Called(id, expiry)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(peer.ID, time.Time) error); ok {
+		r0 = rf(id, expiry)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// ExpiryStore_SetPeerBanExpiration_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'SetPeerBanExpiration'
+type ExpiryStore_SetPeerBanExpiration_Call struct {
+	*mock.Call
+}
+
+// SetPeerBanExpiration is a helper method to define mock.On call
+//   - id peer.ID
+//   - expiry time.Time
+func (_e *ExpiryStore_Expecter) SetPeerBanExpiration(id interface{}, expiry interface{}) *ExpiryStore_SetPeerBanExpiration_Call {
+	return &ExpiryStore_SetPeerBanExpiration_Call{Call: _e.mock.On("SetPeerBanExpiration", id, expiry)}
+}
+
+func (_c *ExpiryStore_SetPeerBanExpiration_Call) Run(run func(id peer.ID, expiry time.Time)) *ExpiryStore_SetPeerBanExpiration_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(peer.ID), args[1].(time.Time))
+	})
+	return _c
+}
+
+func (_c *ExpiryStore_SetPeerBanExpiration_Call) Return(_a0 error) *ExpiryStore_SetPeerBanExpiration_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *ExpiryStore_SetPeerBanExpiration_Call) RunAndReturn(run func(peer.ID, time.Time) error) *ExpiryStore_SetPeerBanExpiration_Call {
 	_c.Call.Return(run)
 	return _c
 }

op-node/p2p/node.go

@@ -195,9 +195,6 @@ func (n *NodeP2P) ConnectionManager() connmgr.ConnManager {
 }
 
 func (n *NodeP2P) BanPeer(id peer.ID, expiration time.Time) error {
-	if err := n.gater.BlockPeer(id); err != nil {
-		return fmt.Errorf("failed to block peer: %w", err)
-	}
 	if err := n.store.SetPeerBanExpiration(id, expiration); err != nil {
 		return fmt.Errorf("failed to set peer ban expiry: %w", err)
 	}
@@ -208,9 +205,6 @@ func (n *NodeP2P) BanPeer(id peer.ID, expiration time.Time) error {
 }
 
 func (n *NodeP2P) BanIP(ip net.IP, expiration time.Time) error {
-	if err := n.gater.BlockAddr(ip); err != nil {
-		return fmt.Errorf("failed to block IP: %w", err)
-	}
 	if err := n.store.SetIPBanExpiration(ip, expiration); err != nil {
 		return fmt.Errorf("failed to set IP ban expiry: %w", err)
 	}