Merge pull request #4548 from tendermint/callum/p2p-blacklist

p2p: ban bad peers

p2p/pex/addrbook.go

@@ -59,9 +59,12 @@ type AddrBook interface {
 	// Mark address
 	MarkGood(p2p.ID)
 	MarkAttempt(*p2p.NetAddress)
-	MarkBad(*p2p.NetAddress)
+	MarkBad(*p2p.NetAddress, time.Duration) // Move peer to bad peers list
+	// Add bad peers back to addrBook
+	ReinstateBadPeers()
 
 	IsGood(*p2p.NetAddress) bool
+	IsBanned(*p2p.NetAddress) bool
 
 	// Send a selection of addresses to peers
 	GetSelection() []*p2p.NetAddress
@@ -87,6 +90,7 @@ type addrBook struct {
 	ourAddrs   map[string]struct{}
 	privateIDs map[p2p.ID]struct{}
 	addrLookup map[p2p.ID]*knownAddress // new & old
+	badPeers   map[p2p.ID]*knownAddress // blacklisted peers
 	bucketsOld []map[string]*knownAddress
 	bucketsNew []map[string]*knownAddress
 	nOld       int
@@ -108,6 +112,7 @@ func NewAddrBook(filePath string, routabilityStrict bool) AddrBook {
 		ourAddrs:          make(map[string]struct{}),
 		privateIDs:        make(map[p2p.ID]struct{}),
 		addrLookup:        make(map[p2p.ID]*knownAddress),
+		badPeers:          make(map[p2p.ID]*knownAddress),
 		filePath:          filePath,
 		routabilityStrict: routabilityStrict,
 	}
@@ -205,12 +210,7 @@ func (a *addrBook) RemoveAddress(addr *p2p.NetAddress) {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
 
-	ka := a.addrLookup[addr.ID]
-	if ka == nil {
-		return
-	}
-	a.Logger.Info("Remove address from book", "addr", addr)
-	a.removeFromAllBuckets(ka)
+	a.removeAddress(addr)
 }
 
 // IsGood returns true if peer was ever marked as good and haven't
@@ -222,6 +222,15 @@ func (a *addrBook) IsGood(addr *p2p.NetAddress) bool {
 	return a.addrLookup[addr.ID].isOld()
 }
 
+// IsBanned returns true if the peer is currently banned
+func (a *addrBook) IsBanned(addr *p2p.NetAddress) bool {
+	a.mtx.Lock()
+	_, ok := a.badPeers[addr.ID]
+	a.mtx.Unlock()
+
+	return ok
+}
+
 // HasAddress returns true if the address is in the book.
 func (a *addrBook) HasAddress(addr *p2p.NetAddress) bool {
 	a.mtx.Lock()
@@ -324,10 +333,28 @@ func (a *addrBook) MarkAttempt(addr *p2p.NetAddress) {
 	ka.markAttempt()
 }
 
-// MarkBad implements AddrBook. Currently it just ejects the address.
-// TODO: black list for some amount of time
-func (a *addrBook) MarkBad(addr *p2p.NetAddress) {
-	a.RemoveAddress(addr)
+// MarkBad implements AddrBook. Kicks address out from book, places
+// the address in the badPeers pool.
+func (a *addrBook) MarkBad(addr *p2p.NetAddress, banTime time.Duration) {
+	a.mtx.Lock()
+	defer a.mtx.Unlock()
+
+	if a.addBadPeer(addr, banTime) {
+		a.removeAddress(addr)
+	}
+}
+
+func (a *addrBook) ReinstateBadPeers() {
+	a.mtx.Lock()
+	defer a.mtx.Unlock()
+	for _, ka := range a.badPeers {
+		if !ka.isBanned() {
+			bucket := a.calcNewBucket(ka.Addr, ka.Src)
+			a.addToNewBucket(ka, bucket)
+			delete(a.badPeers, ka.ID())
+			a.Logger.Info("Reinstated address", "addr", ka.Addr)
+		}
+	}
 }
 
 // GetSelection implements AddrBook.
@@ -592,6 +619,10 @@ func (a *addrBook) addAddress(addr, src *p2p.NetAddress) error {
 		return ErrAddrBookInvalidAddr{Addr: addr, AddrErr: err}
 	}
 
+	if _, ok := a.badPeers[addr.ID]; ok {
+		return ErrAddressBanned{addr}
+	}
+
 	if _, ok := a.privateIDs[addr.ID]; ok {
 		return ErrAddrBookPrivate{addr}
 	}
@@ -725,6 +756,32 @@ func (a *addrBook) moveToOld(ka *knownAddress) {
 	}
 }
 
+func (a *addrBook) removeAddress(addr *p2p.NetAddress) {
+	ka := a.addrLookup[addr.ID]
+	if ka == nil {
+		return
+	}
+	a.Logger.Info("Remove address from book", "addr", addr)
+	a.removeFromAllBuckets(ka)
+}
+
+func (a *addrBook) addBadPeer(addr *p2p.NetAddress, banTime time.Duration) bool {
+	// check it exists in addrbook
+	ka := a.addrLookup[addr.ID]
+	// check address is not already there
+	if ka == nil {
+		return false
+	}
+
+	if _, alreadyBadPeer := a.badPeers[addr.ID]; !alreadyBadPeer {
+		// add to bad peer list
+		ka.ban(banTime)
+		a.badPeers[addr.ID] = ka
+		a.Logger.Info("Add address to blacklist", "addr", addr)
+	}
+	return true
+}
+
 //---------------------------------------------------------------------
 // calculate bucket placements
 

p2p/pex/addrbook_test.go

@@ -7,10 +7,10 @@ import (
 	"math"
 	"os"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
 	"github.com/tendermint/tendermint/libs/log"
 	tmmath "github.com/tendermint/tendermint/libs/math"
 	tmrand "github.com/tendermint/tendermint/libs/rand"
@@ -343,7 +343,7 @@ func TestAddrBookGetSelectionWithBias(t *testing.T) {
 		}
 	}
 
-	got, expected := int((float64(good)/float64(len(selection)))*100), (100 - biasTowardsNewAddrs)
+	got, expected := int((float64(good)/float64(len(selection)))*100), 100-biasTowardsNewAddrs
 
 	// compute some slack to protect against small differences due to rounding:
 	slack := int(math.Round(float64(100) / float64(len(selection))))
@@ -396,6 +396,33 @@ func testCreatePrivateAddrs(t *testing.T, numAddrs int) ([]*p2p.NetAddress, []st
 	return addrs, private
 }
 
+func TestBanBadPeers(t *testing.T) {
+	fname := createTempFileName("addrbook_test")
+	defer deleteTempFile(fname)
+
+	book := NewAddrBook(fname, true)
+	book.SetLogger(log.TestingLogger())
+
+	addr := randIPv4Address(t)
+	_ = book.AddAddress(addr, addr)
+
+	book.MarkBad(addr, 1*time.Second)
+	// addr should not reachable
+	assert.False(t, book.HasAddress(addr))
+	assert.True(t, book.IsBanned(addr))
+
+	err := book.AddAddress(addr, addr)
+	// book should not add address from the blacklist
+	assert.Error(t, err)
+
+	time.Sleep(1 * time.Second)
+	book.ReinstateBadPeers()
+	// address should be reinstated in the new bucket
+	assert.EqualValues(t, 1, book.Size())
+	assert.True(t, book.HasAddress(addr))
+	assert.False(t, book.IsGood(addr))
+}
+
 func TestAddrBookEmpty(t *testing.T) {
 	fname := createTempFileName("addrbook_test")
 	defer deleteTempFile(fname)

p2p/pex/errors.go

@@ -63,3 +63,12 @@ type ErrAddrBookInvalidAddr struct {
 func (err ErrAddrBookInvalidAddr) Error() string {
 	return fmt.Sprintf("Cannot add invalid address %v: %v", err.Addr, err.AddrErr)
 }
+
+// ErrAddressBanned is thrown when the address has been banned and therefore cannot be used
+type ErrAddressBanned struct {
+	Addr *p2p.NetAddress
+}
+
+func (err ErrAddressBanned) Error() string {
+	return fmt.Sprintf("Address: %v is currently banned", err.Addr)
+}

p2p/pex/known_address.go

@@ -16,6 +16,7 @@ type knownAddress struct {
 	BucketType  byte            `json:"bucket_type"`
 	LastAttempt time.Time       `json:"last_attempt"`
 	LastSuccess time.Time       `json:"last_success"`
+	LastBanTime time.Time       `json:"last_ban_time"`
 }
 
 func newKnownAddress(addr *p2p.NetAddress, src *p2p.NetAddress) *knownAddress {
@@ -54,6 +55,16 @@ func (ka *knownAddress) markGood() {
 	ka.LastSuccess = now
 }
 
+func (ka *knownAddress) ban(banTime time.Duration) {
+	if ka.LastBanTime.Before(time.Now().Add(banTime)) {
+		ka.LastBanTime = time.Now().Add(banTime)
+	}
+}
+
+func (ka *knownAddress) isBanned() bool {
+	return ka.LastBanTime.After(time.Now())
+}
+
 func (ka *knownAddress) addBucketRef(bucketIdx int) int {
 	for _, bucket := range ka.Buckets {
 		if bucket == bucketIdx {

p2p/pex/pex_reactor.go

@@ -8,7 +8,7 @@ import (
 
 	"github.com/pkg/errors"
 
-	amino "github.com/tendermint/go-amino"
+	"github.com/tendermint/go-amino"
 	"github.com/tendermint/tendermint/libs/cmap"
 	tmmath "github.com/tendermint/tendermint/libs/math"
 	"github.com/tendermint/tendermint/libs/rand"
@@ -50,6 +50,9 @@ const (
 	// Especially in the beginning, node should have more trusted peers than
 	// untrusted.
 	biasToSelectNewPeers = 30 // 70 to select good peers
+
+	// if a peer is marked bad, it will be banned for at least this time period
+	defaultBanTime = 24 * time.Hour
 )
 
 type errMaxAttemptsToDial struct {
@@ -494,6 +497,12 @@ func (r *Reactor) ensurePeers() {
 	}
 
 	if r.book.NeedMoreAddrs() {
+		// Check if banned nodes can be reinstated
+		r.book.ReinstateBadPeers()
+	}
+
+	if r.book.NeedMoreAddrs() {
+
 		// 1) Pick a random peer and ask for more.
 		peers := r.Switch.Peers().List()
 		peersCount := len(peers)
@@ -525,11 +534,7 @@ func (r *Reactor) dialAttemptsInfo(addr *p2p.NetAddress) (attempts int, lastDial
 func (r *Reactor) dialPeer(addr *p2p.NetAddress) error {
 	attempts, lastDialed := r.dialAttemptsInfo(addr)
 	if !r.Switch.IsPeerPersistent(addr) && attempts > maxAttemptsToDial {
-		// TODO(melekes): have a blacklist in the addrbook with peers whom we've
-		// failed to connect to. Then we can clean up attemptsToDial, which acts as
-		// a blacklist currently.
-		// https://github.com/tendermint/tendermint/issues/3572
-		r.book.MarkBad(addr)
+		r.book.MarkBad(addr, defaultBanTime)
 		return errMaxAttemptsToDial{}
 	}
 
@@ -741,7 +746,7 @@ func markAddrInBookBasedOnErr(addr *p2p.NetAddress, book AddrBook, err error) {
 	// TODO: detect more "bad peer" scenarios
 	switch err.(type) {
 	case p2p.ErrSwitchAuthenticationFailure:
-		book.MarkBad(addr)
+		book.MarkBad(addr, defaultBanTime)
 	default:
 		book.MarkAttempt(addr)
 	}