diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index dbba71345..c9c372638 100644 --- a/p2p/pex/addrbook.go +++ b/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 diff --git a/p2p/pex/addrbook_test.go b/p2p/pex/addrbook_test.go index 363958c44..e5a226daf 100644 --- a/p2p/pex/addrbook_test.go +++ b/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) diff --git a/p2p/pex/errors.go b/p2p/pex/errors.go index 911389a9e..1fc54ea50 100644 --- a/p2p/pex/errors.go +++ b/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) +} diff --git a/p2p/pex/known_address.go b/p2p/pex/known_address.go index af40d6ff0..e98a9e97e 100644 --- a/p2p/pex/known_address.go +++ b/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 { diff --git a/p2p/pex/pex_reactor.go b/p2p/pex/pex_reactor.go index 3a3d2d7de..b0a1c02bb 100644 --- a/p2p/pex/pex_reactor.go +++ b/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) }