[NOD-1181] Mark banned peers in address manager and persist bans to disk (#826)

* [NOD-1079] Fix block rejects over "Already have block" (#783)

* [NOD-1079] Return regular error instead of ruleError on already-have-block in ProcessBlock.

* [NOD-1079] Fix bad implementation of IsSelectedTipKnown.

* [NOD-1079] In shouldQueryPeerSelectedTips use selected DAG tip timestamp instead of past median time.

* [NOD-1079] Remove redundant (and possibly buggy) clearing of sm.requestedBlocks.

* [NOD-684] change simnet block rate to block per ms (#782)

* [NOD-684] Get rid of dag.targetTimePerBlock and use finality duration in dag params

* [NOD-684] Fix regtest genesis block

* [NOD-684] Set simnet's TargetTimePerBlock to 1ms

* [NOD-684] Shorten simnet finality duration

* [NOD-684] Change isDAGCurrentMaxDiff to be written as number of blocks

* [NOD-684] Fix NextBlockMinimumTime to be add one millisecond after past median time

* [NOD-1004] Make AddrManager.getAddress use only 1 loop to check all address chances and pick one of them (#741)

* [NOD-1004] Remove code duplication in Good().

* [NOD-1004] Remove some more code duplication in Good().

* [NOD-1004] Remove some more code duplication in Good().

* [NOD-1004] Remove code duplication in GetAddress().

* [NOD-1004] Remove code duplication in updateAddress.

* [NOD-1004] Remove some more code duplication in updateAddress.

* [NOD-1004] Remove redundant check in expireNew.

* [NOD-1004] Remove superfluous existence check from updateAddress.

* [NOD-1004] Make triedBucket use a slice instead of a list.

* [NOD-1004] Remove code duplication in getAddress.

* [NOD-1004] Remove infinite loops out of getAddress.

* [NOD-1004] Made impossible branch panic.

* [NOD-1004] Remove a mystery comment.

* [NOD-1004] Remove an unnecessary sort.

* [NOD-1004] Make AddressKey a type alias.

* [NOD-1004] Added comment for AddressKey

* [NOD-1004] Fix merge errors.

* [NOD-1004] Fix merge errors.

* [NOD-1004] Do some renaming.

* [NOD-1004] Do some more renaming.

* [NOD-1004] Rename AddrManager to AddressManager.

* [NOD-1004] Rename AddrManager to AddressManager.

* [NOD-1004] Do some more renaming.

* [NOD-1004] Rename bucket to addressBucketArray.

* [NOD-1004] Fix a comment.

* [NOD-1004] Rename na to netAddress.

* [NOD-1004] Bring back an existence check.

* [NOD-1004] Fix an error message.

* [NOD-1004] Fix a comment.

* [NOD-1004] Use a boolean instead of -1.

* [NOD-1004] Use a boolean instead of -1 in another place.

Co-authored-by: Mike Zak <feanorr@gmail.com>

* Fix merge errors.

* [NOD-1181] Move isBanned logic into addressManager.

* [NOD-1181] Persist bans to disk.

* [NOD-1181] Add comments.

* [NOD-1181] Add an additional exit condition to the connection loop.

* [NOD-1181] Add a TODO.

* [NOD-1181] Wrap not-found errors in addressManager.

* [NOD-1181] Fix a comment.

* [NOD-1181] Rename banned to isBanned.

* [NOD-1181] Fix bad error handling in routerInitializer.

* [NOD-1181] Remove a TODO.

Co-authored-by: Ori Newman <orinewman1@gmail.com>
Co-authored-by: Mike Zak <feanorr@gmail.com>

addressmanager/addressmanager.go

@@ -70,6 +70,8 @@ type serializedKnownAddress struct {
 	TimeStamp     int64
 	LastAttempt   int64
 	LastSuccess   int64
+	IsBanned      bool
+	BannedTime    int64
 	// no refcount or tried, that is available from context.
 }
 
@@ -185,6 +187,10 @@ const (
 	serializationVersion = 1
 )
 
+// ErrAddressNotFound is an error returned from some functions when a
+// given address is not found in the address manager
+var ErrAddressNotFound = errors.New("address not found")
+
 // New returns a new Kaspa address manager.
 func New(cfg *config.Config, databaseContext *dbaccess.DatabaseContext) *AddressManager {
 	addressManager := AddressManager{
@@ -485,6 +491,8 @@ func (am *AddressManager) PeersStateForSerialization() (*PeersStateForSerializat
 		serializedAddress.Attempts = knownAddress.attempts
 		serializedAddress.LastAttempt = knownAddress.lastAttempt.UnixMilliseconds()
 		serializedAddress.LastSuccess = knownAddress.lastSuccess.UnixMilliseconds()
+		serializedAddress.IsBanned = knownAddress.isBanned
+		serializedAddress.BannedTime = knownAddress.bannedTime.UnixMilliseconds()
 		// Tried and referenceCount are implicit in the rest of the structure
 		// and will be worked out from context on unserialisation.
 		peersState.Addresses[i] = serializedAddress
@@ -604,6 +612,8 @@ func (am *AddressManager) deserializePeersState(serializedPeerState []byte) erro
 		knownAddress.attempts = serializedKnownAddress.Attempts
 		knownAddress.lastAttempt = mstime.UnixMilliseconds(serializedKnownAddress.LastAttempt)
 		knownAddress.lastSuccess = mstime.UnixMilliseconds(serializedKnownAddress.LastSuccess)
+		knownAddress.isBanned = serializedKnownAddress.IsBanned
+		knownAddress.bannedTime = mstime.UnixMilliseconds(serializedKnownAddress.BannedTime)
 		am.addressIndex[NetAddressKey(knownAddress.netAddress)] = knownAddress
 	}
 
@@ -1372,3 +1382,40 @@ func (am *AddressManager) GetBestLocalAddress(remoteAddress *wire.NetAddress) *w
 
 	return bestAddress
 }
+
+// Ban marks the given address as banned
+func (am *AddressManager) Ban(address *wire.NetAddress) error {
+	return am.setBanned(address, true, mstime.Now())
+}
+
+// Unban marks the given address as not banned
+func (am *AddressManager) Unban(address *wire.NetAddress) error {
+	return am.setBanned(address, false, mstime.Time{})
+}
+
+func (am *AddressManager) setBanned(address *wire.NetAddress, isBanned bool, bannedTime mstime.Time) error {
+	am.localAddressesLock.Lock()
+	defer am.localAddressesLock.Unlock()
+
+	knownAddress := am.knownAddress(address)
+	if knownAddress == nil {
+		return errors.Wrapf(ErrAddressNotFound, "address %s "+
+			"is not registered with the address manager", address.TCPAddress())
+	}
+	knownAddress.isBanned = isBanned
+	knownAddress.bannedTime = bannedTime
+	return nil
+}
+
+// IsBanned returns whether the given address is banned
+func (am *AddressManager) IsBanned(address *wire.NetAddress) (bool, error) {
+	am.localAddressesLock.Lock()
+	defer am.localAddressesLock.Unlock()
+
+	knownAddress := am.knownAddress(address)
+	if knownAddress == nil {
+		return false, errors.Wrapf(ErrAddressNotFound, "address %s "+
+			"is not registered with the address manager", address.TCPAddress())
+	}
+	return knownAddress.isBanned, nil
+}

addressmanager/knownaddress.go

@@ -24,6 +24,8 @@ type KnownAddress struct {
 	tried          bool
 	referenceCount int // reference count of new buckets
 	subnetworkID   *subnetworkid.SubnetworkID
+	isBanned       bool
+	bannedTime     mstime.Time
 }
 
 // NetAddress returns the underlying wire.NetAddress associated with the

connmanager/connmanager.go

@@ -33,9 +33,6 @@ type ConnectionManager struct {
 	activeIncoming   map[string]struct{}
 	maxIncoming      int
 
-	bannedAddresses     map[string]struct{}
-	bannedAddressesLock sync.RWMutex
-
 	stop                   uint32
 	connectionRequestsLock sync.Mutex
 
@@ -53,7 +50,6 @@ func New(cfg *config.Config, netAdapter *netadapter.NetAdapter, addressManager *
 		pendingRequested: map[string]*connectionRequest{},
 		activeOutgoing:   map[string]struct{}{},
 		activeIncoming:   map[string]struct{}{},
-		bannedAddresses:  map[string]struct{}{},
 		resetLoopChan:    make(chan struct{}),
 		loopTicker:       time.NewTicker(connectionsLoopInterval),
 	}
@@ -129,30 +125,13 @@ func (c *ConnectionManager) ConnectionCount() int {
 }
 
 // Ban marks the given netConnection as banned
-func (c *ConnectionManager) Ban(netConnection *netadapter.NetConnection) {
-	c.banIP(netConnection.IP())
+func (c *ConnectionManager) Ban(netConnection *netadapter.NetConnection) error {
+	return c.addressManager.Ban(netConnection.NetAddress())
 }
 
 // IsBanned returns whether the given netConnection is banned
-func (c *ConnectionManager) IsBanned(netConnection *netadapter.NetConnection) bool {
-	return c.isIPBanned(netConnection.IP())
-}
-
-// banIP marks the given IP as banned
-func (c *ConnectionManager) banIP(ip string) {
-	c.bannedAddressesLock.Lock()
-	defer c.bannedAddressesLock.Unlock()
-
-	c.bannedAddresses[ip] = struct{}{}
-}
-
-// isIPBanned returns whether the given IP is banned
-func (c *ConnectionManager) isIPBanned(ip string) bool {
-	c.bannedAddressesLock.RLock()
-	defer c.bannedAddressesLock.RUnlock()
-
-	_, ok := c.bannedAddresses[ip]
-	return ok
+func (c *ConnectionManager) IsBanned(netConnection *netadapter.NetConnection) (bool, error) {
+	return c.addressManager.IsBanned(netConnection.NetAddress())
 }
 
 func (c *ConnectionManager) waitTillNextIteration() {

connmanager/outgoing_connections.go

@@ -23,7 +23,13 @@ func (c *ConnectionManager) checkOutgoingConnections(connSet connectionSet) {
 		liveConnections, c.targetOutgoing, c.targetOutgoing-liveConnections)
 
 	connectionsNeededCount := c.targetOutgoing - len(c.activeOutgoing)
-	for i := 0; i < connectionsNeededCount; i++ {
+	connectionAttempts := connectionsNeededCount * 2
+	for i := 0; i < connectionAttempts; i++ {
+		// Return in case we've already reached or surpassed our target
+		if len(c.activeOutgoing) >= c.targetOutgoing {
+			return
+		}
+
 		address := c.addressManager.GetAddress()
 		if address == nil {
 			log.Warnf("No more addresses available")
@@ -32,13 +38,18 @@ func (c *ConnectionManager) checkOutgoingConnections(connSet connectionSet) {
 
 		netAddress := address.NetAddress()
 		tcpAddress := netAddress.TCPAddress()
-		if c.isIPBanned(tcpAddress.IP.String()) {
+		addressString := tcpAddress.String()
+		isBanned, err := c.addressManager.IsBanned(netAddress)
+		if err != nil {
+			log.Infof("Couldn't resolve whether %s is banned: %s", addressString, err)
+			continue
+		}
+		if isBanned {
 			continue
 		}
 
 		c.addressManager.Attempt(netAddress)
-		addressString := tcpAddress.String()
-		err := c.initiateConnection(addressString)
+		err = c.initiateConnection(addressString)
 		if err != nil {
 			log.Infof("Couldn't connect to %s: %s", addressString, err)
 			continue

netadapter/netconnection.go

@@ -2,6 +2,7 @@ package netadapter
 
 import (
 	"fmt"
+	"github.com/kaspanet/kaspad/wire"
 
 	"github.com/kaspanet/kaspad/netadapter/id"
 	"github.com/kaspanet/kaspad/netadapter/server"
@@ -43,9 +44,9 @@ func (c *NetConnection) IsOutbound() bool {
 	return c.connection.IsOutbound()
 }
 
-// IP returns the IP address associated with this connection
-func (c *NetConnection) IP() string {
-	return c.connection.Address().IP.String()
+// NetAddress returns the NetAddress associated with this connection
+func (c *NetConnection) NetAddress() *wire.NetAddress {
+	return wire.NewNetAddress(c.connection.Address(), 0)
 }
 
 // SetOnInvalidMessageHandler sets a handler function

protocol/protocol.go

@@ -2,6 +2,7 @@ package protocol
 
 import (
 	"fmt"
+	"github.com/kaspanet/kaspad/addressmanager"
 	"github.com/kaspanet/kaspad/netadapter"
 	"github.com/kaspanet/kaspad/protocol/flows/ibd/selectedtip"
 	"sync/atomic"
@@ -24,7 +25,11 @@ import (
 func (m *Manager) routerInitializer(netConnection *netadapter.NetConnection) *routerpkg.Router {
 	router := routerpkg.NewRouter()
 	spawn("newRouterInitializer-startFlows", func() {
-		if m.context.ConnectionManager().IsBanned(netConnection) {
+		isBanned, err := m.context.ConnectionManager().IsBanned(netConnection)
+		if err != nil && !errors.Is(err, addressmanager.ErrAddressNotFound) {
+			panic(err)
+		}
+		if isBanned {
 			err := m.context.NetAdapter().Disconnect(netConnection)
 			if err != nil {
 				panic(err)
@@ -32,11 +37,14 @@ func (m *Manager) routerInitializer(netConnection *netadapter.NetConnection) *ro
 			return
 		}
 
-		err := m.startFlows(netConnection, router)
+		err = m.startFlows(netConnection, router)
 		if err != nil {
 			if protocolErr := &(protocolerrors.ProtocolError{}); errors.As(err, &protocolErr) {
 				if protocolErr.ShouldBan {
-					m.context.ConnectionManager().Ban(netConnection)
+					err := m.context.ConnectionManager().Ban(netConnection)
+					if err != nil && !errors.Is(err, addressmanager.ErrAddressNotFound) {
+						panic(err)
+					}
 				}
 				err = m.context.NetAdapter().Disconnect(netConnection)
 				if err != nil {

rpc/handle_get_peer_addresses.go

@@ -31,6 +31,8 @@ func handleGetPeerAddresses(s *Server, cmd interface{}, closeChan <-chan struct{
 			TimeStamp:    addr.TimeStamp,
 			LastAttempt:  addr.LastAttempt,
 			LastSuccess:  addr.LastSuccess,
+			IsBanned:     addr.IsBanned,
+			BannedTime:   addr.BannedTime,
 		}
 	}
 

rpc/model/rpc_results.go

@@ -237,6 +237,8 @@ type GetPeerAddressesKnownAddressResult struct {
 	TimeStamp    int64
 	LastAttempt  int64
 	LastSuccess  int64
+	IsBanned     bool
+	BannedTime   int64
 }
 
 // GetPeerAddressesNewBucketResult models a GetPeerAddressesResult new bucket.

rpc/rpcserverhelp.go

@@ -402,6 +402,8 @@ var helpDescsEnUS = map[string]string{
 	"getPeerAddressesKnownAddressResult-timeStamp":    "Time the address was added",
 	"getPeerAddressesKnownAddressResult-lastAttempt":  "Last attempt to connect to the address",
 	"getPeerAddressesKnownAddressResult-lastSuccess":  "Last successful attempt to connect to the address",
+	"getPeerAddressesKnownAddressResult-isBanned":     "Whether the address was banned",
+	"getPeerAddressesKnownAddressResult-bannedTime":   "Time the address was banned",
 
 	// GetPeerAddressesCmd help.
 	"getPeerAddresses--synopsis": "Returns the peers state.",