From 7466f353451eae68183f86e042034b98a6755001 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Tue, 10 Mar 2020 18:13:45 +0100 Subject: [PATCH 01/15] mvp blacklist alg --- p2p/pex/addrbook.go | 44 +++++++++++++++++++++++++++++++++++----- p2p/pex/known_address.go | 12 +++++++++++ 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index dbba71345..3bf331172 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -22,8 +22,9 @@ import ( ) const ( - bucketTypeNew = 0x01 - bucketTypeOld = 0x02 + bucketTypeNew = 0x01 + bucketTypeOld = 0x02 + defaultBanTime = 24 * time.Hour ) // AddrBook is an address book used for tracking peers @@ -87,6 +88,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 +110,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, } @@ -233,6 +236,7 @@ func (a *addrBook) HasAddress(addr *p2p.NetAddress) bool { // NeedMoreAddrs implements AddrBook - returns true if there are not have enough addresses in the book. func (a *addrBook) NeedMoreAddrs() bool { + a.ReinstateBadPeers() return a.Size() < needAddressThreshold } @@ -324,10 +328,24 @@ 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 +// MarkBad implements AddrBook. Kicks address out from book, places +// the address in the badPeers pool. func (a *addrBook) MarkBad(addr *p2p.NetAddress) { - a.RemoveAddress(addr) + if a.addBadPeer(addr) { + a.RemoveAddress(addr) + } +} + +func (a *addrBook) ReinstateBadPeers() { + for _, ka := range a.badPeers { + if !ka.isBanned(defaultBanTime) { + a.mtx.Lock() + bucket := a.calcNewBucket(ka.Addr, ka.Src) + a.addToNewBucket(ka, bucket) + delete(a.badPeers, ka.ID()) + a.mtx.Unlock() + } + } } // GetSelection implements AddrBook. @@ -725,6 +743,22 @@ func (a *addrBook) moveToOld(ka *knownAddress) { } } +func (a *addrBook) addBadPeer(addr *p2p.NetAddress) bool { + a.mtx.Lock() + defer a.mtx.Unlock() + + // check it exists in addrbook + ka := a.addrLookup[addr.ID] + // check address is not already there + if _, alreadyBadPeer := a.badPeers[addr.ID]; ka != nil && !alreadyBadPeer { + // add to bad peer list + ka.ban() + a.badPeers[addr.ID] = ka + return true + } + return false +} + //--------------------------------------------------------------------- // calculate bucket placements diff --git a/p2p/pex/known_address.go b/p2p/pex/known_address.go index af40d6ff0..72729a5d5 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,17 @@ func (ka *knownAddress) markGood() { ka.LastSuccess = now } +func (ka *knownAddress) ban() { + ka.LastBanTime = time.Now() +} + +func (ka *knownAddress) isBanned(banTime time.Duration) bool { + if ka.LastBanTime.Add(banTime).Before(time.Now()) { + return false + } + return true +} + func (ka *knownAddress) addBucketRef(bucketIdx int) int { for _, bucket := range ka.Buckets { if bucket == bucketIdx { From 6ccd3324c2b58a0ff79c0085d2a6d1c87025d954 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Tue, 10 Mar 2020 19:01:56 +0100 Subject: [PATCH 02/15] move reinstatement after if statement in pex reactor --- p2p/pex/addrbook.go | 5 +++-- p2p/pex/pex_reactor.go | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 3bf331172..15019f5cd 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -60,7 +60,9 @@ type AddrBook interface { // Mark address MarkGood(p2p.ID) MarkAttempt(*p2p.NetAddress) - MarkBad(*p2p.NetAddress) + MarkBad(*p2p.NetAddress) // Move peer to bad peers list + // Add bad peers back to addrBook + ReinstateBadPeers() IsGood(*p2p.NetAddress) bool @@ -236,7 +238,6 @@ func (a *addrBook) HasAddress(addr *p2p.NetAddress) bool { // NeedMoreAddrs implements AddrBook - returns true if there are not have enough addresses in the book. func (a *addrBook) NeedMoreAddrs() bool { - a.ReinstateBadPeers() return a.Size() < needAddressThreshold } diff --git a/p2p/pex/pex_reactor.go b/p2p/pex/pex_reactor.go index 3a3d2d7de..66ec77503 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" @@ -494,6 +494,8 @@ func (r *Reactor) ensurePeers() { } if r.book.NeedMoreAddrs() { + // 0) Check if banned nodes can be reinstated + r.book.ReinstateBadPeers() // 1) Pick a random peer and ask for more. peers := r.Switch.Peers().List() peersCount := len(peers) From e18636ef1fcbee64f6df3751a79a7323089eaec9 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Tue, 10 Mar 2020 19:04:45 +0100 Subject: [PATCH 03/15] separate reinstatement of peers from discovery of new peers --- p2p/pex/pex_reactor.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/p2p/pex/pex_reactor.go b/p2p/pex/pex_reactor.go index 66ec77503..bce9b40ff 100644 --- a/p2p/pex/pex_reactor.go +++ b/p2p/pex/pex_reactor.go @@ -494,8 +494,12 @@ func (r *Reactor) ensurePeers() { } if r.book.NeedMoreAddrs() { - // 0) Check if banned nodes can be reinstated + // 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) From eeb0b0d1123358b3ead49d234840087fe22b9a17 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Tue, 10 Mar 2020 19:06:48 +0100 Subject: [PATCH 04/15] lint fix --- p2p/pex/known_address.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/p2p/pex/known_address.go b/p2p/pex/known_address.go index 72729a5d5..9d4d060a5 100644 --- a/p2p/pex/known_address.go +++ b/p2p/pex/known_address.go @@ -60,10 +60,7 @@ func (ka *knownAddress) ban() { } func (ka *knownAddress) isBanned(banTime time.Duration) bool { - if ka.LastBanTime.Add(banTime).Before(time.Now()) { - return false - } - return true + return ka.LastBanTime.Add(banTime).Before(time.Now()) } func (ka *knownAddress) addBucketRef(bucketIdx int) int { From 429febde801348f961d64d8ea82d67287e057e5c Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Wed, 11 Mar 2020 15:00:22 +0100 Subject: [PATCH 05/15] reconfigure mutexes --- p2p/pex/addrbook.go | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 15019f5cd..c99ea75e5 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -210,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 @@ -332,19 +327,22 @@ func (a *addrBook) MarkAttempt(addr *p2p.NetAddress) { // MarkBad implements AddrBook. Kicks address out from book, places // the address in the badPeers pool. func (a *addrBook) MarkBad(addr *p2p.NetAddress) { + a.mtx.Lock() + defer a.mtx.Unlock() + if a.addBadPeer(addr) { - a.RemoveAddress(addr) + a.removeAddress(addr) } } func (a *addrBook) ReinstateBadPeers() { + a.mtx.Lock() + defer a.mtx.Unlock() for _, ka := range a.badPeers { if !ka.isBanned(defaultBanTime) { - a.mtx.Lock() bucket := a.calcNewBucket(ka.Addr, ka.Src) a.addToNewBucket(ka, bucket) delete(a.badPeers, ka.ID()) - a.mtx.Unlock() } } } @@ -744,6 +742,15 @@ 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) bool { a.mtx.Lock() defer a.mtx.Unlock() @@ -751,10 +758,12 @@ func (a *addrBook) addBadPeer(addr *p2p.NetAddress) bool { // check it exists in addrbook ka := a.addrLookup[addr.ID] // check address is not already there - if _, alreadyBadPeer := a.badPeers[addr.ID]; ka != nil && !alreadyBadPeer { - // add to bad peer list - ka.ban() - a.badPeers[addr.ID] = ka + if ka != nil { + if _, alreadyBadPeer := a.badPeers[addr.ID]; !alreadyBadPeer { + // add to bad peer list + ka.ban() + a.badPeers[addr.ID] = ka + } return true } return false From 08ccbdcb43c214f2f349d6b2f2d6f6c271985836 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Wed, 11 Mar 2020 15:06:42 +0100 Subject: [PATCH 06/15] ban function requires a ban duration as an argument --- p2p/pex/addrbook.go | 4 ++-- p2p/pex/known_address.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index c99ea75e5..c3351a600 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -339,7 +339,7 @@ func (a *addrBook) ReinstateBadPeers() { a.mtx.Lock() defer a.mtx.Unlock() for _, ka := range a.badPeers { - if !ka.isBanned(defaultBanTime) { + if !ka.isBanned() { bucket := a.calcNewBucket(ka.Addr, ka.Src) a.addToNewBucket(ka, bucket) delete(a.badPeers, ka.ID()) @@ -761,7 +761,7 @@ func (a *addrBook) addBadPeer(addr *p2p.NetAddress) bool { if ka != nil { if _, alreadyBadPeer := a.badPeers[addr.ID]; !alreadyBadPeer { // add to bad peer list - ka.ban() + ka.ban(defaultBanTime) a.badPeers[addr.ID] = ka } return true diff --git a/p2p/pex/known_address.go b/p2p/pex/known_address.go index 9d4d060a5..fdd055a13 100644 --- a/p2p/pex/known_address.go +++ b/p2p/pex/known_address.go @@ -55,12 +55,12 @@ func (ka *knownAddress) markGood() { ka.LastSuccess = now } -func (ka *knownAddress) ban() { - ka.LastBanTime = time.Now() +func (ka *knownAddress) ban(banTime time.Duration) { + ka.LastBanTime = time.Now().Add(banTime) } -func (ka *knownAddress) isBanned(banTime time.Duration) bool { - return ka.LastBanTime.Add(banTime).Before(time.Now()) +func (ka *knownAddress) isBanned() bool { + return ka.LastBanTime.Before(time.Now()) } func (ka *knownAddress) addBucketRef(bucketIdx int) int { From 4110c252af6ba4202fe948d1a4fc7c35fa84e3e5 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Wed, 11 Mar 2020 15:44:22 +0100 Subject: [PATCH 07/15] make banTime an argument and set default in PEX reactor instead of AddrBook --- p2p/pex/addrbook.go | 15 +++++++-------- p2p/pex/addrbook_test.go | 28 ++++++++++++++++++++++++---- p2p/pex/pex_reactor.go | 7 +++++-- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index c3351a600..66a9118e3 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -22,9 +22,8 @@ import ( ) const ( - bucketTypeNew = 0x01 - bucketTypeOld = 0x02 - defaultBanTime = 24 * time.Hour + bucketTypeNew = 0x01 + bucketTypeOld = 0x02 ) // AddrBook is an address book used for tracking peers @@ -60,7 +59,7 @@ type AddrBook interface { // Mark address MarkGood(p2p.ID) MarkAttempt(*p2p.NetAddress) - MarkBad(*p2p.NetAddress) // Move peer to bad peers list + MarkBad(*p2p.NetAddress, time.Duration) // Move peer to bad peers list // Add bad peers back to addrBook ReinstateBadPeers() @@ -326,11 +325,11 @@ func (a *addrBook) MarkAttempt(addr *p2p.NetAddress) { // MarkBad implements AddrBook. Kicks address out from book, places // the address in the badPeers pool. -func (a *addrBook) MarkBad(addr *p2p.NetAddress) { +func (a *addrBook) MarkBad(addr *p2p.NetAddress, banTime time.Duration) { a.mtx.Lock() defer a.mtx.Unlock() - if a.addBadPeer(addr) { + if a.addBadPeer(addr, banTime) { a.removeAddress(addr) } } @@ -751,7 +750,7 @@ func (a *addrBook) removeAddress(addr *p2p.NetAddress) { a.removeFromAllBuckets(ka) } -func (a *addrBook) addBadPeer(addr *p2p.NetAddress) bool { +func (a *addrBook) addBadPeer(addr *p2p.NetAddress, banTime time.Duration) bool { a.mtx.Lock() defer a.mtx.Unlock() @@ -761,7 +760,7 @@ func (a *addrBook) addBadPeer(addr *p2p.NetAddress) bool { if ka != nil { if _, alreadyBadPeer := a.badPeers[addr.ID]; !alreadyBadPeer { // add to bad peer list - ka.ban(defaultBanTime) + ka.ban(banTime) a.badPeers[addr.ID] = ka } return true diff --git a/p2p/pex/addrbook_test.go b/p2p/pex/addrbook_test.go index 363958c44..eaa3280f2 100644 --- a/p2p/pex/addrbook_test.go +++ b/p2p/pex/addrbook_test.go @@ -3,14 +3,13 @@ package pex import ( "encoding/hex" "fmt" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "io/ioutil" "math" "os" "testing" - "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 +342,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)))) @@ -544,6 +543,27 @@ func TestMultipleAddrBookAddressSelection(t *testing.T) { } } +//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) +// +// assert.False(t, book.HasAddress(addr)) +// +// time.Sleep(1 * time.Second) +// +// book.ReinstateBadPeers() +// +// assert.True(t, book.HasAddress(addr)) +//} + func assertMOldAndNNewAddrsInSelection(t *testing.T, m, n int, addrs []*p2p.NetAddress, book *addrBook) { nOld, nNew := countOldAndNewAddrsInSelection(addrs, book) assert.Equal(t, m, nOld, "old addresses") diff --git a/p2p/pex/pex_reactor.go b/p2p/pex/pex_reactor.go index bce9b40ff..afe29e16e 100644 --- a/p2p/pex/pex_reactor.go +++ b/p2p/pex/pex_reactor.go @@ -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 { @@ -535,7 +538,7 @@ func (r *Reactor) dialPeer(addr *p2p.NetAddress) error { // 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{} } @@ -747,7 +750,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) } From 60d375eba62ef8b609cd12ed89fb6ece8cd12be4 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Wed, 11 Mar 2020 16:27:49 +0100 Subject: [PATCH 08/15] basic test for banning peers --- p2p/pex/addrbook.go | 3 --- p2p/pex/addrbook_test.go | 57 ++++++++++++++++++++++------------------ p2p/pex/known_address.go | 2 +- 3 files changed, 32 insertions(+), 30 deletions(-) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 66a9118e3..00835977e 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -751,9 +751,6 @@ func (a *addrBook) removeAddress(addr *p2p.NetAddress) { } func (a *addrBook) addBadPeer(addr *p2p.NetAddress, banTime time.Duration) bool { - a.mtx.Lock() - defer a.mtx.Unlock() - // check it exists in addrbook ka := a.addrLookup[addr.ID] // check address is not already there diff --git a/p2p/pex/addrbook_test.go b/p2p/pex/addrbook_test.go index eaa3280f2..454b0070f 100644 --- a/p2p/pex/addrbook_test.go +++ b/p2p/pex/addrbook_test.go @@ -5,15 +5,15 @@ import ( "fmt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "io/ioutil" - "math" - "os" - "testing" - "github.com/tendermint/tendermint/libs/log" tmmath "github.com/tendermint/tendermint/libs/math" tmrand "github.com/tendermint/tendermint/libs/rand" "github.com/tendermint/tendermint/p2p" + "io/ioutil" + "math" + "os" + "testing" + "time" ) // FIXME These tests should not rely on .(*addrBook) assertions @@ -395,6 +395,32 @@ 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)) + + 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) @@ -543,27 +569,6 @@ func TestMultipleAddrBookAddressSelection(t *testing.T) { } } -//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) -// -// assert.False(t, book.HasAddress(addr)) -// -// time.Sleep(1 * time.Second) -// -// book.ReinstateBadPeers() -// -// assert.True(t, book.HasAddress(addr)) -//} - func assertMOldAndNNewAddrsInSelection(t *testing.T, m, n int, addrs []*p2p.NetAddress, book *addrBook) { nOld, nNew := countOldAndNewAddrsInSelection(addrs, book) assert.Equal(t, m, nOld, "old addresses") diff --git a/p2p/pex/known_address.go b/p2p/pex/known_address.go index fdd055a13..674b66553 100644 --- a/p2p/pex/known_address.go +++ b/p2p/pex/known_address.go @@ -60,7 +60,7 @@ func (ka *knownAddress) ban(banTime time.Duration) { } func (ka *knownAddress) isBanned() bool { - return ka.LastBanTime.Before(time.Now()) + return ka.LastBanTime.After(time.Now()) } func (ka *knownAddress) addBucketRef(bucketIdx int) int { From 65d86bcad192574bb8fa0b359e935bff3965df83 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Wed, 11 Mar 2020 16:31:03 +0100 Subject: [PATCH 09/15] added banned address error --- p2p/pex/errors.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/p2p/pex/errors.go b/p2p/pex/errors.go index 911389a9e..1ea5a3b3e 100644 --- a/p2p/pex/errors.go +++ b/p2p/pex/errors.go @@ -63,3 +63,11 @@ type ErrAddrBookInvalidAddr struct { func (err ErrAddrBookInvalidAddr) Error() string { return fmt.Sprintf("Cannot add invalid address %v: %v", err.Addr, err.AddrErr) } + +type ErrAddressBanned struct { + Addr *p2p.NetAddress +} + +func (err ErrAddressBanned) Error() string { + return fmt.Sprintf("Address: %v is currently banned", err.Addr) +} From 2f2d62efed379bab2b2873e8c1d4ea3d7b1fa0e2 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Wed, 11 Mar 2020 16:33:45 +0100 Subject: [PATCH 10/15] banned addresses can't be added again --- p2p/pex/addrbook.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 00835977e..4913373ae 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -608,6 +608,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} } From 1e37a1f3e489eb97d132d6d92277e5f341e1ba24 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Wed, 11 Mar 2020 16:43:32 +0100 Subject: [PATCH 11/15] added isBanned check in addrbook --- p2p/pex/addrbook.go | 9 +++++++++ p2p/pex/addrbook_test.go | 1 + 2 files changed, 10 insertions(+) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 4913373ae..1637129c4 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -64,6 +64,7 @@ type AddrBook interface { ReinstateBadPeers() IsGood(*p2p.NetAddress) bool + IsBanned(*p2p.NetAddress) bool // Send a selection of addresses to peers GetSelection() []*p2p.NetAddress @@ -221,6 +222,14 @@ func (a *addrBook) IsGood(addr *p2p.NetAddress) bool { return a.addrLookup[addr.ID].isOld() } +func (a *addrBook) IsBanned(addr *p2p.NetAddress) bool { + a.mtx.Lock() + defer a.mtx.Unlock() + + _, ok := a.badPeers[addr.ID] + return ok +} + // HasAddress returns true if the address is in the book. func (a *addrBook) HasAddress(addr *p2p.NetAddress) bool { a.mtx.Lock() diff --git a/p2p/pex/addrbook_test.go b/p2p/pex/addrbook_test.go index 454b0070f..51e968693 100644 --- a/p2p/pex/addrbook_test.go +++ b/p2p/pex/addrbook_test.go @@ -408,6 +408,7 @@ func TestBanBadPeers(t *testing.T) { 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 From 5de6ec78e58544b7febb55da6ae523edab8bdac1 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Wed, 11 Mar 2020 16:48:06 +0100 Subject: [PATCH 12/15] added logs for more information --- p2p/pex/addrbook.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 1637129c4..207b6a1c3 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -351,6 +351,7 @@ func (a *addrBook) ReinstateBadPeers() { bucket := a.calcNewBucket(ka.Addr, ka.Src) a.addToNewBucket(ka, bucket) delete(a.badPeers, ka.ID()) + a.Logger.Info("Reinstated Address", "addr", ka.Addr) } } } @@ -772,6 +773,7 @@ func (a *addrBook) addBadPeer(addr *p2p.NetAddress, banTime time.Duration) bool // add to bad peer list ka.ban(banTime) a.badPeers[addr.ID] = ka + a.Logger.Info("Add address to blacklist", "addr", addr) } return true } From dbf02200dea000eb8d7fa18952569c305005a2cc Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Wed, 11 Mar 2020 17:16:46 +0100 Subject: [PATCH 13/15] lint fix --- p2p/pex/addrbook_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/p2p/pex/addrbook_test.go b/p2p/pex/addrbook_test.go index 51e968693..e5a226daf 100644 --- a/p2p/pex/addrbook_test.go +++ b/p2p/pex/addrbook_test.go @@ -3,17 +3,18 @@ package pex import ( "encoding/hex" "fmt" + "io/ioutil" + "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" "github.com/tendermint/tendermint/p2p" - "io/ioutil" - "math" - "os" - "testing" - "time" ) // FIXME These tests should not rely on .(*addrBook) assertions From c8bb1cc8b7d20573280528b61a1beb3efa80b531 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Thu, 12 Mar 2020 12:35:28 +0100 Subject: [PATCH 14/15] made suggested changes --- p2p/pex/addrbook.go | 7 ++++--- p2p/pex/errors.go | 1 + p2p/pex/pex_reactor.go | 4 ---- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 207b6a1c3..6e8397cd6 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -222,11 +222,12 @@ 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() - defer a.mtx.Unlock() - _, ok := a.badPeers[addr.ID] + a.mtx.Unlock() + return ok } @@ -351,7 +352,7 @@ func (a *addrBook) ReinstateBadPeers() { bucket := a.calcNewBucket(ka.Addr, ka.Src) a.addToNewBucket(ka, bucket) delete(a.badPeers, ka.ID()) - a.Logger.Info("Reinstated Address", "addr", ka.Addr) + a.Logger.Info("Reinstated address", "addr", ka.Addr) } } } diff --git a/p2p/pex/errors.go b/p2p/pex/errors.go index 1ea5a3b3e..1892b5fff 100644 --- a/p2p/pex/errors.go +++ b/p2p/pex/errors.go @@ -64,6 +64,7 @@ func (err ErrAddrBookInvalidAddr) Error() string { return fmt.Sprintf("Cannot add invalid address %v: %v", err.Addr, err.AddrErr) } +// Err is thrown when the address is banned and therefore cannot be used type ErrAddressBanned struct { Addr *p2p.NetAddress } diff --git a/p2p/pex/pex_reactor.go b/p2p/pex/pex_reactor.go index afe29e16e..b0a1c02bb 100644 --- a/p2p/pex/pex_reactor.go +++ b/p2p/pex/pex_reactor.go @@ -534,10 +534,6 @@ 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, defaultBanTime) return errMaxAttemptsToDial{} } From 7e6b1a87dae105c45d0efa644ec11a8fd6abebb0 Mon Sep 17 00:00:00 2001 From: Callum Michael Waters Date: Thu, 12 Mar 2020 15:29:31 +0100 Subject: [PATCH 15/15] cannot decrease ban time --- p2p/pex/addrbook.go | 19 ++++++++++--------- p2p/pex/errors.go | 2 +- p2p/pex/known_address.go | 4 +++- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 6e8397cd6..c9c372638 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -769,16 +769,17 @@ 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 { - 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 + 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 false + return true } //--------------------------------------------------------------------- diff --git a/p2p/pex/errors.go b/p2p/pex/errors.go index 1892b5fff..1fc54ea50 100644 --- a/p2p/pex/errors.go +++ b/p2p/pex/errors.go @@ -64,7 +64,7 @@ func (err ErrAddrBookInvalidAddr) Error() string { return fmt.Sprintf("Cannot add invalid address %v: %v", err.Addr, err.AddrErr) } -// Err is thrown when the address is banned and therefore cannot be used +// ErrAddressBanned is thrown when the address has been banned and therefore cannot be used type ErrAddressBanned struct { Addr *p2p.NetAddress } diff --git a/p2p/pex/known_address.go b/p2p/pex/known_address.go index 674b66553..e98a9e97e 100644 --- a/p2p/pex/known_address.go +++ b/p2p/pex/known_address.go @@ -56,7 +56,9 @@ func (ka *knownAddress) markGood() { } func (ka *knownAddress) ban(banTime time.Duration) { - ka.LastBanTime = time.Now().Add(banTime) + if ka.LastBanTime.Before(time.Now().Add(banTime)) { + ka.LastBanTime = time.Now().Add(banTime) + } } func (ka *knownAddress) isBanned() bool {