From 9a57ef9cbf671dbbeafa7aad268a44bade6117e6 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 21 Mar 2018 19:41:56 +0100 Subject: [PATCH 01/12] do not dial ourselves (ok, maybe just once) Refs #1275 --- p2p/node_info.go | 8 ++++++++ p2p/switch.go | 11 +++++++++++ 2 files changed, 19 insertions(+) diff --git a/p2p/node_info.go b/p2p/node_info.go index 6f44b49cd..46c9a1629 100644 --- a/p2p/node_info.go +++ b/p2p/node_info.go @@ -2,6 +2,7 @@ package p2p import ( "fmt" + "net" "strings" crypto "github.com/tendermint/go-crypto" @@ -107,10 +108,17 @@ OUTER_LOOP: return nil } +// ID returns node's ID. func (info NodeInfo) ID() ID { return PubKeyToID(info.PubKey) } +// IP returns node listen addr's IP. +func (info NodeInfo) IP() net.IP { + hostPort := strings.SplitN(info.ListenAddr, ":", 2) + return net.ParseIP(hostPort[0]) +} + // NetAddress returns a NetAddress derived from the NodeInfo - // it includes the authenticated peer ID and the self-reported // ListenAddr. Note that the ListenAddr is not authenticated and diff --git a/p2p/switch.go b/p2p/switch.go index fa037a9b8..f50cab802 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -58,6 +58,7 @@ type Switch struct { dialing *cmn.CMap nodeInfo NodeInfo // our node info nodeKey *NodeKey // our node privkey + nodeIP net.IP // our IP addrBook AddrBook filterConnByAddr func(net.Addr) error @@ -148,6 +149,7 @@ func (sw *Switch) IsListening() bool { // NOTE: Not goroutine safe. func (sw *Switch) SetNodeInfo(nodeInfo NodeInfo) { sw.nodeInfo = nodeInfo + sw.nodeIP = nodeInfo.IP() } // NodeInfo returns the switch's NodeInfo. @@ -381,6 +383,11 @@ func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent b // DialPeerWithAddress dials the given peer and runs sw.addPeer if it connects and authenticates successfully. // If `persistent == true`, the switch will always try to reconnect to this peer if the connection ever fails. func (sw *Switch) DialPeerWithAddress(addr *NetAddress, persistent bool) error { + // do not dial ourselves + if addr.IP == sw.nodeIP { + return ErrSwitchConnectToSelf + } + sw.dialing.Set(string(addr.ID), addr) defer sw.dialing.Delete(string(addr.ID)) return sw.addOutboundPeerWithConfig(addr, sw.peerConfig, persistent) @@ -522,6 +529,10 @@ func (sw *Switch) addPeer(pc peerConn) error { // Avoid self if sw.nodeKey.ID() == peerID { + // overwrite current IP to avoid dialing ourselves again + // it means original nodeIP was different from public one + sw.nodeIP = peerNodeInfo.IP() + return ErrSwitchConnectToSelf } From 5a2fa71b033c576177060845c49b995af820668a Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 21 Mar 2018 20:04:22 +0100 Subject: [PATCH 02/12] use combination of IP and port, not just IP --- p2p/node_info.go | 7 ------- p2p/switch.go | 34 +++++++++++++++++----------------- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/p2p/node_info.go b/p2p/node_info.go index 46c9a1629..346de37d3 100644 --- a/p2p/node_info.go +++ b/p2p/node_info.go @@ -2,7 +2,6 @@ package p2p import ( "fmt" - "net" "strings" crypto "github.com/tendermint/go-crypto" @@ -113,12 +112,6 @@ func (info NodeInfo) ID() ID { return PubKeyToID(info.PubKey) } -// IP returns node listen addr's IP. -func (info NodeInfo) IP() net.IP { - hostPort := strings.SplitN(info.ListenAddr, ":", 2) - return net.ParseIP(hostPort[0]) -} - // NetAddress returns a NetAddress derived from the NodeInfo - // it includes the authenticated peer ID and the self-reported // ListenAddr. Note that the ListenAddr is not authenticated and diff --git a/p2p/switch.go b/p2p/switch.go index f50cab802..d0e6df721 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -48,18 +48,18 @@ type AddrBook interface { type Switch struct { cmn.BaseService - config *cfg.P2PConfig - peerConfig *PeerConfig - listeners []Listener - reactors map[string]Reactor - chDescs []*conn.ChannelDescriptor - reactorsByCh map[byte]Reactor - peers *PeerSet - dialing *cmn.CMap - nodeInfo NodeInfo // our node info - nodeKey *NodeKey // our node privkey - nodeIP net.IP // our IP - addrBook AddrBook + config *cfg.P2PConfig + peerConfig *PeerConfig + listeners []Listener + reactors map[string]Reactor + chDescs []*conn.ChannelDescriptor + reactorsByCh map[byte]Reactor + peers *PeerSet + dialing *cmn.CMap + nodeInfo NodeInfo // our node info + nodeKey *NodeKey // our node privkey + nodePublicAddr *NetAddress + addrBook AddrBook filterConnByAddr func(net.Addr) error filterConnByID func(ID) error @@ -149,7 +149,7 @@ func (sw *Switch) IsListening() bool { // NOTE: Not goroutine safe. func (sw *Switch) SetNodeInfo(nodeInfo NodeInfo) { sw.nodeInfo = nodeInfo - sw.nodeIP = nodeInfo.IP() + sw.nodePublicAddr = nodeInfo.NetAddress() } // NodeInfo returns the switch's NodeInfo. @@ -384,7 +384,7 @@ func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent b // If `persistent == true`, the switch will always try to reconnect to this peer if the connection ever fails. func (sw *Switch) DialPeerWithAddress(addr *NetAddress, persistent bool) error { // do not dial ourselves - if addr.IP == sw.nodeIP { + if addr.Same(sw.nodePublicAddr) { return ErrSwitchConnectToSelf } @@ -529,9 +529,9 @@ func (sw *Switch) addPeer(pc peerConn) error { // Avoid self if sw.nodeKey.ID() == peerID { - // overwrite current IP to avoid dialing ourselves again - // it means original nodeIP was different from public one - sw.nodeIP = peerNodeInfo.IP() + // overwrite current addr to avoid dialing ourselves again + // it means original nodePublicAddr was different from public one + sw.nodePublicAddr = peerNodeInfo.NetAddress() return ErrSwitchConnectToSelf } From 4b8e342309e25c801b6295313bb234c2a9c356d2 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 21 Mar 2018 20:08:43 +0100 Subject: [PATCH 03/12] fix panic: lookup testing on 10.0.2.3:53: no such host --- p2p/switch.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/p2p/switch.go b/p2p/switch.go index d0e6df721..fec448265 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -58,7 +58,7 @@ type Switch struct { dialing *cmn.CMap nodeInfo NodeInfo // our node info nodeKey *NodeKey // our node privkey - nodePublicAddr *NetAddress + nodePublicAddr string addrBook AddrBook filterConnByAddr func(net.Addr) error @@ -149,7 +149,7 @@ func (sw *Switch) IsListening() bool { // NOTE: Not goroutine safe. func (sw *Switch) SetNodeInfo(nodeInfo NodeInfo) { sw.nodeInfo = nodeInfo - sw.nodePublicAddr = nodeInfo.NetAddress() + sw.nodePublicAddr = nodeInfo.ListenAddr } // NodeInfo returns the switch's NodeInfo. @@ -384,7 +384,7 @@ func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent b // If `persistent == true`, the switch will always try to reconnect to this peer if the connection ever fails. func (sw *Switch) DialPeerWithAddress(addr *NetAddress, persistent bool) error { // do not dial ourselves - if addr.Same(sw.nodePublicAddr) { + if addr.DialString() == sw.nodePublicAddr { return ErrSwitchConnectToSelf } @@ -531,7 +531,7 @@ func (sw *Switch) addPeer(pc peerConn) error { if sw.nodeKey.ID() == peerID { // overwrite current addr to avoid dialing ourselves again // it means original nodePublicAddr was different from public one - sw.nodePublicAddr = peerNodeInfo.NetAddress() + sw.nodePublicAddr = peerNodeInfo.ListenAddr return ErrSwitchConnectToSelf } From 3a672cb2a904a491a2a257d0eb0b3bcab834d5f7 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 21 Mar 2018 20:23:22 +0100 Subject: [PATCH 04/12] update changelog [ci skip] --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79e866a2c..69c98bf7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ IMPROVEMENTS: - [p2p] seeds respond with a bias towards good peers - [rpc] `/tx` and `/tx_search` responses now include the transaction hash - [rpc] include validator power in `/status` +- [p2p] do not try to connect to ourselves (ok, maybe only once) BUG FIXES: - [rpc] fix subscribing using an abci.ResponseDeliverTx tag From fc9ffee2e3c0bb650b424e77462fa572734f6b89 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 22 Mar 2018 09:54:32 +0100 Subject: [PATCH 05/12] remove unused tracking because it leads to memory leaks in tests see https://blog.cosmos.network/debugging-the-memory-leak-in-tendermint-210186711420 --- p2p/switch_test.go | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/p2p/switch_test.go b/p2p/switch_test.go index 06e8b642e..de0c8d204 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -39,8 +39,6 @@ type TestReactor struct { mtx sync.Mutex channels []*conn.ChannelDescriptor - peersAdded []Peer - peersRemoved []Peer logMessages bool msgsCounter int msgsReceived map[byte][]PeerMessage @@ -61,17 +59,9 @@ func (tr *TestReactor) GetChannels() []*conn.ChannelDescriptor { return tr.channels } -func (tr *TestReactor) AddPeer(peer Peer) { - tr.mtx.Lock() - defer tr.mtx.Unlock() - tr.peersAdded = append(tr.peersAdded, peer) -} +func (tr *TestReactor) AddPeer(peer Peer) {} -func (tr *TestReactor) RemovePeer(peer Peer, reason interface{}) { - tr.mtx.Lock() - defer tr.mtx.Unlock() - tr.peersRemoved = append(tr.peersRemoved, peer) -} +func (tr *TestReactor) RemovePeer(peer Peer, reason interface{}) {} func (tr *TestReactor) Receive(chID byte, peer Peer, msgBytes []byte) { if tr.logMessages { From 3284a13feed2ee3cda84e6ddbabc2366014449f7 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 22 Mar 2018 09:55:21 +0100 Subject: [PATCH 06/12] add test Refs #1275 --- p2p/switch_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/p2p/switch_test.go b/p2p/switch_test.go index de0c8d204..7ac72395c 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -175,6 +175,37 @@ func TestConnAddrFilter(t *testing.T) { assertNoPeersAfterTimeout(t, s2, 400*time.Millisecond) } +func TestSwitchFiltersOutItself(t *testing.T) { + s1 := MakeSwitch(config, 1, "127.0.0.2", "123.123.123", initSwitchFunc) + + // addr should be rejected immediately because of the same IP & port + addr := s1.NodeInfo().NetAddress() + err := s1.DialPeerWithAddress(addr, false) + if assert.Error(t, err) { + assert.Equal(t, ErrSwitchConnectToSelf, err) + } + + // simulate s1 having a public IP by creating a remote peer with the same ID + rp := &remotePeer{PrivKey: s1.nodeKey.PrivKey, Config: DefaultPeerConfig()} + rp.Start() + + // addr should be rejected in addPeer based on the same ID + err = s1.DialPeerWithAddress(rp.Addr(), false) + if assert.Error(t, err) { + assert.Equal(t, ErrSwitchConnectToSelf, err) + } + + // addr should be rejected immediately because during previous step we changed node's public IP + err = s1.DialPeerWithAddress(rp.Addr(), false) + if assert.Error(t, err) { + assert.Equal(t, ErrSwitchConnectToSelf, err) + } + + rp.Stop() + + assertNoPeersAfterTimeout(t, s1, 100*time.Millisecond) +} + func assertNoPeersAfterTimeout(t *testing.T, sw *Switch, timeout time.Duration) { time.Sleep(timeout) if sw.Peers().Size() != 0 { From 3b3f45d49b6ba255ed32152019864f95a7755e82 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 22 Mar 2018 16:58:57 +0100 Subject: [PATCH 07/12] use addrbook#AddOurAddress to store our address --- node/node.go | 9 ++++-- p2p/pex/addrbook.go | 17 +++++++++-- p2p/pex/pex_reactor_test.go | 57 +++++++++++++++++++++++++------------ p2p/switch.go | 34 +++++++++++----------- p2p/switch_test.go | 23 ++++++++++++++- 5 files changed, 99 insertions(+), 41 deletions(-) diff --git a/node/node.go b/node/node.go index 3a59d2f06..8b7ca1032 100644 --- a/node/node.go +++ b/node/node.go @@ -414,9 +414,14 @@ func (n *Node) OnStart() error { } n.Logger.Info("P2P Node ID", "ID", nodeKey.ID(), "file", n.config.NodeKeyFile()) - // Start the switch - n.sw.SetNodeInfo(n.makeNodeInfo(nodeKey.PubKey())) + nodeInfo := n.makeNodeInfo(nodeKey.PubKey()) + n.sw.SetNodeInfo(nodeInfo) n.sw.SetNodeKey(nodeKey) + + // Add ourselves to addrbook to prevent dialing ourselves + n.addrBook.AddOurAddress(nodeInfo.NetAddress()) + + // Start the switch err = n.sw.Start() if err != nil { return err diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index efb48f6da..adaba2fd6 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -34,6 +34,9 @@ type AddrBook interface { // Add our own addresses so we don't later add ourselves AddOurAddress(*p2p.NetAddress) + // Check if it is our address + OurAddress(*p2p.NetAddress) bool + // Add and remove an address AddAddress(addr *p2p.NetAddress, src *p2p.NetAddress) error RemoveAddress(addr *p2p.NetAddress) @@ -78,7 +81,7 @@ type addrBook struct { // accessed concurrently mtx sync.Mutex rand *rand.Rand - ourAddrs map[string]*p2p.NetAddress + ourAddrs map[string]struct{} addrLookup map[p2p.ID]*knownAddress // new & old bucketsOld []map[string]*knownAddress bucketsNew []map[string]*knownAddress @@ -93,7 +96,7 @@ type addrBook struct { func NewAddrBook(filePath string, routabilityStrict bool) *addrBook { am := &addrBook{ rand: rand.New(rand.NewSource(time.Now().UnixNano())), // TODO: seed from outside - ourAddrs: make(map[string]*p2p.NetAddress), + ourAddrs: make(map[string]struct{}), addrLookup: make(map[p2p.ID]*knownAddress), filePath: filePath, routabilityStrict: routabilityStrict, @@ -154,7 +157,15 @@ func (a *addrBook) AddOurAddress(addr *p2p.NetAddress) { a.mtx.Lock() defer a.mtx.Unlock() a.Logger.Info("Add our address to book", "addr", addr) - a.ourAddrs[addr.String()] = addr + a.ourAddrs[addr.String()] = struct{}{} +} + +// OurAddress returns true if it is our address. +func (a *addrBook) OurAddress(addr *p2p.NetAddress) bool { + a.mtx.Lock() + _, ok := a.ourAddrs[addr.String()] + a.mtx.Unlock() + return ok } // AddAddress implements AddrBook - adds the given address as received from the given source. diff --git a/p2p/pex/pex_reactor_test.go b/p2p/pex/pex_reactor_test.go index 20276ec86..329d17f37 100644 --- a/p2p/pex/pex_reactor_test.go +++ b/p2p/pex/pex_reactor_test.go @@ -63,35 +63,45 @@ func TestPEXReactorRunning(t *testing.T) { N := 3 switches := make([]*p2p.Switch, N) + // directory to store address books dir, err := ioutil.TempDir("", "pex_reactor") require.Nil(t, err) defer os.RemoveAll(dir) // nolint: errcheck - book := NewAddrBook(filepath.Join(dir, "addrbook.json"), false) - book.SetLogger(log.TestingLogger()) + + books := make([]*addrBook, N) + logger := log.TestingLogger() // create switches for i := 0; i < N; i++ { switches[i] = p2p.MakeSwitch(config, i, "127.0.0.1", "123.123.123", func(i int, sw *p2p.Switch) *p2p.Switch { - sw.SetLogger(log.TestingLogger().With("switch", i)) + books[i] = NewAddrBook(filepath.Join(dir, fmt.Sprintf("addrbook%d.json", i)), false) + books[i].SetLogger(logger.With("pex", i)) + sw.SetAddrBook(books[i]) - r := NewPEXReactor(book, &PEXReactorConfig{}) - r.SetLogger(log.TestingLogger()) + sw.SetLogger(logger.With("pex", i)) + + r := NewPEXReactor(books[i], &PEXReactorConfig{}) + r.SetLogger(logger.With("pex", i)) r.SetEnsurePeersPeriod(250 * time.Millisecond) sw.AddReactor("pex", r) + return sw }) } - // fill the address book and add listeners - for _, s := range switches { - addr := s.NodeInfo().NetAddress() - book.AddAddress(addr, addr) - s.AddListener(p2p.NewDefaultListener("tcp", s.NodeInfo().ListenAddr, true, log.TestingLogger())) + addOtherNodeAddrToAddrBook := func(switchIndex, otherSwitchIndex int) { + addr := switches[otherSwitchIndex].NodeInfo().NetAddress() + books[switchIndex].AddAddress(addr, addr) } - // start switches - for _, s := range switches { - err := s.Start() // start switch and reactors + addOtherNodeAddrToAddrBook(0, 1) + addOtherNodeAddrToAddrBook(1, 0) + addOtherNodeAddrToAddrBook(2, 1) + + for i, sw := range switches { + sw.AddListener(p2p.NewDefaultListener("tcp", sw.NodeInfo().ListenAddr, true, logger.With("pex", i))) + + err := sw.Start() // start switch and reactors require.Nil(t, err) } @@ -127,6 +137,7 @@ func TestPEXReactorRequestMessageAbuse(t *testing.T) { defer teardownReactor(book) sw := createSwitchAndAddReactors(r) + sw.SetAddrBook(book) peer := newMockPeer() p2p.AddPeerToSwitch(sw, peer) @@ -156,6 +167,7 @@ func TestPEXReactorAddrsMessageAbuse(t *testing.T) { defer teardownReactor(book) sw := createSwitchAndAddReactors(r) + sw.SetAddrBook(book) peer := newMockPeer() p2p.AddPeerToSwitch(sw, peer) @@ -182,13 +194,11 @@ func TestPEXReactorAddrsMessageAbuse(t *testing.T) { } func TestPEXReactorUsesSeedsIfNeeded(t *testing.T) { + // directory to store address books dir, err := ioutil.TempDir("", "pex_reactor") require.Nil(t, err) defer os.RemoveAll(dir) // nolint: errcheck - book := NewAddrBook(filepath.Join(dir, "addrbook.json"), false) - book.SetLogger(log.TestingLogger()) - // 1. create seed seed := p2p.MakeSwitch( config, @@ -196,6 +206,10 @@ func TestPEXReactorUsesSeedsIfNeeded(t *testing.T) { "127.0.0.1", "123.123.123", func(i int, sw *p2p.Switch) *p2p.Switch { + book := NewAddrBook(filepath.Join(dir, "addrbook0.json"), false) + book.SetLogger(log.TestingLogger()) + sw.SetAddrBook(book) + sw.SetLogger(log.TestingLogger()) r := NewPEXReactor(book, &PEXReactorConfig{}) @@ -222,6 +236,10 @@ func TestPEXReactorUsesSeedsIfNeeded(t *testing.T) { "127.0.0.1", "123.123.123", func(i int, sw *p2p.Switch) *p2p.Switch { + book := NewAddrBook(filepath.Join(dir, "addrbook1.json"), false) + book.SetLogger(log.TestingLogger()) + sw.SetAddrBook(book) + sw.SetLogger(log.TestingLogger()) r := NewPEXReactor( @@ -247,7 +265,8 @@ func TestPEXReactorCrawlStatus(t *testing.T) { defer teardownReactor(book) // Seed/Crawler mode uses data from the Switch - _ = createSwitchAndAddReactors(pexR) + sw := createSwitchAndAddReactors(pexR) + sw.SetAddrBook(book) // Create a peer, add it to the peer set and the addrbook. peer := p2p.CreateRandomPeer(false) @@ -291,7 +310,8 @@ func TestPEXReactorDialPeer(t *testing.T) { pexR, book := createReactor(&PEXReactorConfig{}) defer teardownReactor(book) - _ = createSwitchAndAddReactors(pexR) + sw := createSwitchAndAddReactors(pexR) + sw.SetAddrBook(book) peer := newMockPeer() addr := peer.NodeInfo().NetAddress() @@ -397,6 +417,7 @@ func assertPeersWithTimeout( } func createReactor(config *PEXReactorConfig) (r *PEXReactor, book *addrBook) { + // directory to store address book dir, err := ioutil.TempDir("", "pex_reactor") if err != nil { panic(err) diff --git a/p2p/switch.go b/p2p/switch.go index fec448265..a12052953 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -35,6 +35,8 @@ const ( type AddrBook interface { AddAddress(addr *NetAddress, src *NetAddress) error + AddOurAddress(*NetAddress) + OurAddress(*NetAddress) bool MarkGood(*NetAddress) Save() } @@ -48,18 +50,17 @@ type AddrBook interface { type Switch struct { cmn.BaseService - config *cfg.P2PConfig - peerConfig *PeerConfig - listeners []Listener - reactors map[string]Reactor - chDescs []*conn.ChannelDescriptor - reactorsByCh map[byte]Reactor - peers *PeerSet - dialing *cmn.CMap - nodeInfo NodeInfo // our node info - nodeKey *NodeKey // our node privkey - nodePublicAddr string - addrBook AddrBook + config *cfg.P2PConfig + peerConfig *PeerConfig + listeners []Listener + reactors map[string]Reactor + chDescs []*conn.ChannelDescriptor + reactorsByCh map[byte]Reactor + peers *PeerSet + dialing *cmn.CMap + nodeInfo NodeInfo // our node info + nodeKey *NodeKey // our node privkey + addrBook AddrBook filterConnByAddr func(net.Addr) error filterConnByID func(ID) error @@ -149,7 +150,6 @@ func (sw *Switch) IsListening() bool { // NOTE: Not goroutine safe. func (sw *Switch) SetNodeInfo(nodeInfo NodeInfo) { sw.nodeInfo = nodeInfo - sw.nodePublicAddr = nodeInfo.ListenAddr } // NodeInfo returns the switch's NodeInfo. @@ -384,7 +384,7 @@ func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent b // If `persistent == true`, the switch will always try to reconnect to this peer if the connection ever fails. func (sw *Switch) DialPeerWithAddress(addr *NetAddress, persistent bool) error { // do not dial ourselves - if addr.DialString() == sw.nodePublicAddr { + if sw.addrBook.OurAddress(addr) { return ErrSwitchConnectToSelf } @@ -529,9 +529,9 @@ func (sw *Switch) addPeer(pc peerConn) error { // Avoid self if sw.nodeKey.ID() == peerID { - // overwrite current addr to avoid dialing ourselves again - // it means original nodePublicAddr was different from public one - sw.nodePublicAddr = peerNodeInfo.ListenAddr + // add given address to the address book to avoid dialing ourselves again + // this is our public address + sw.addrBook.AddOurAddress(peerNodeInfo.NetAddress()) return ErrSwitchConnectToSelf } diff --git a/p2p/switch_test.go b/p2p/switch_test.go index 7ac72395c..4995ad3f2 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -90,6 +90,8 @@ func MakeSwitchPair(t testing.TB, initSwitch func(int, *Switch) *Switch) (*Switc } func initSwitchFunc(i int, sw *Switch) *Switch { + sw.SetAddrBook(&addrBookMock{ourAddrs: make(map[string]struct{})}) + // Make two reactors of two channels each sw.AddReactor("foo", NewTestReactor([]*conn.ChannelDescriptor{ {ID: byte(0x00), Priority: 10}, @@ -99,6 +101,7 @@ func initSwitchFunc(i int, sw *Switch) *Switch { {ID: byte(0x02), Priority: 10}, {ID: byte(0x03), Priority: 10}, }, true)) + return sw } @@ -177,9 +180,12 @@ func TestConnAddrFilter(t *testing.T) { func TestSwitchFiltersOutItself(t *testing.T) { s1 := MakeSwitch(config, 1, "127.0.0.2", "123.123.123", initSwitchFunc) + addr := s1.NodeInfo().NetAddress() + + // add ourselves like we do in node.go#427 + s1.addrBook.AddOurAddress(addr) // addr should be rejected immediately because of the same IP & port - addr := s1.NodeInfo().NetAddress() err := s1.DialPeerWithAddress(addr, false) if assert.Error(t, err) { assert.Equal(t, ErrSwitchConnectToSelf, err) @@ -371,3 +377,18 @@ func BenchmarkSwitchBroadcast(b *testing.B) { b.Logf("success: %v, failure: %v", numSuccess, numFailure) } + +type addrBookMock struct { + ourAddrs map[string]struct{} +} + +var _ AddrBook = (*addrBookMock)(nil) + +func (book *addrBookMock) AddAddress(addr *NetAddress, src *NetAddress) error { return nil } +func (book *addrBookMock) AddOurAddress(addr *NetAddress) { book.ourAddrs[addr.String()] = struct{}{} } +func (book *addrBookMock) OurAddress(addr *NetAddress) bool { + _, ok := book.ourAddrs[addr.String()] + return ok +} +func (book *addrBookMock) MarkGood(*NetAddress) {} +func (book *addrBookMock) Save() {} From 34b77fcad452cf0d797967a46359f05e4841c574 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 5 Apr 2018 15:19:15 +0200 Subject: [PATCH 08/12] log error when we fail to add new address --- p2p/pex/pex_reactor.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/p2p/pex/pex_reactor.go b/p2p/pex/pex_reactor.go index 8c6ad5a8b..1bcc493dd 100644 --- a/p2p/pex/pex_reactor.go +++ b/p2p/pex/pex_reactor.go @@ -164,7 +164,10 @@ func (r *PEXReactor) AddPeer(p Peer) { // peers when we need - we don't trust inbound peers as much. addr := p.NodeInfo().NetAddress() if !isAddrPrivate(addr, r.config.PrivatePeerIDs) { - r.book.AddAddress(addr, addr) + err := r.book.AddAddress(addr, addr) + if err != nil { + r.Logger.Error("Failed to add new address", "err", err) + } } } } @@ -265,7 +268,10 @@ func (r *PEXReactor) ReceiveAddrs(addrs []*p2p.NetAddress, src Peer) error { srcAddr := src.NodeInfo().NetAddress() for _, netAddr := range addrs { if netAddr != nil && !isAddrPrivate(netAddr, r.config.PrivatePeerIDs) { - r.book.AddAddress(netAddr, srcAddr) + err := r.book.AddAddress(netAddr, srcAddr) + if err != nil { + r.Logger.Error("Failed to add new address", "err", err) + } } } return nil From 7f6ee7a46b69cb86bebcacbcd7bd86a122130698 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 5 Apr 2018 15:20:53 +0200 Subject: [PATCH 09/12] add a comment for NewSwitch --- p2p/switch.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/p2p/switch.go b/p2p/switch.go index a12052953..1abc6b14e 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -33,6 +33,8 @@ const ( //----------------------------------------------------------------------------- +// An AddrBook represents an address book from the pex package, which is used +// to store peer addresses. type AddrBook interface { AddAddress(addr *NetAddress, src *NetAddress) error AddOurAddress(*NetAddress) @@ -43,7 +45,7 @@ type AddrBook interface { //----------------------------------------------------------------------------- -// `Switch` handles peer connections and exposes an API to receive incoming messages +// Switch handles peer connections and exposes an API to receive incoming messages // on `Reactors`. Each `Reactor` is responsible for handling incoming messages of one // or more `Channels`. So while sending outgoing messages is typically performed on the peer, // incoming messages are received on the reactor. @@ -68,6 +70,7 @@ type Switch struct { rng *rand.Rand // seed for randomizing dial times and orders } +// NewSwitch creates a new Switch with the given config. func NewSwitch(config *cfg.P2PConfig) *Switch { sw := &Switch{ config: config, From 6e39ec6e262790c9dc0080b043fa3c55ca76f364 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 5 Apr 2018 15:21:11 +0200 Subject: [PATCH 10/12] do not even try to dial ourselves also, remove address from the book (plus mark it as our address) and return an error if we fail to parse peers list --- p2p/pex/addrbook.go | 14 +++++++++++-- p2p/pex/addrbook_test.go | 16 +++++++++++++++ p2p/switch.go | 39 +++++++++++++++++++++++------------- p2p/switch_test.go | 43 ++++++++++++++++++++++------------------ 4 files changed, 77 insertions(+), 35 deletions(-) diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index adaba2fd6..a8462f375 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -33,13 +33,15 @@ type AddrBook interface { // Add our own addresses so we don't later add ourselves AddOurAddress(*p2p.NetAddress) - // Check if it is our address OurAddress(*p2p.NetAddress) bool // Add and remove an address AddAddress(addr *p2p.NetAddress, src *p2p.NetAddress) error - RemoveAddress(addr *p2p.NetAddress) + RemoveAddress(*p2p.NetAddress) + + // Check if the address is in the book + HasAddress(*p2p.NetAddress) bool // Do we need more peers? NeedMoreAddrs() bool @@ -196,6 +198,14 @@ func (a *addrBook) IsGood(addr *p2p.NetAddress) bool { return a.addrLookup[addr.ID].isOld() } +// HasAddress returns true if the address is in the book. +func (a *addrBook) HasAddress(addr *p2p.NetAddress) bool { + a.mtx.Lock() + defer a.mtx.Unlock() + ka := a.addrLookup[addr.ID] + return ka != nil +} + // NeedMoreAddrs implements AddrBook - returns true if there are not have enough addresses in the book. func (a *addrBook) NeedMoreAddrs() bool { return a.Size() < needAddressThreshold diff --git a/p2p/pex/addrbook_test.go b/p2p/pex/addrbook_test.go index 68edb188a..2e2604286 100644 --- a/p2p/pex/addrbook_test.go +++ b/p2p/pex/addrbook_test.go @@ -338,3 +338,19 @@ func TestAddrBookGetSelectionWithBias(t *testing.T) { t.Fatalf("expected more good peers (%% got: %d, %% expected: %d, number of good addrs: %d, total: %d)", got, expected, good, len(selection)) } } + +func TestAddrBookHasAddress(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) + + assert.True(t, book.HasAddress(addr)) + + book.RemoveAddress(addr) + + assert.False(t, book.HasAddress(addr)) +} diff --git a/p2p/switch.go b/p2p/switch.go index 1abc6b14e..9f65a85e0 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -40,6 +40,8 @@ type AddrBook interface { AddOurAddress(*NetAddress) OurAddress(*NetAddress) bool MarkGood(*NetAddress) + RemoveAddress(*NetAddress) + HasAddress(*NetAddress) bool Save() } @@ -351,17 +353,20 @@ func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent b for _, err := range errs { sw.Logger.Error("Error in peer's address", "err", err) } + if len(errs) > 0 { + return errors.New("Errors in peer addresses (see errors above)") + } + + ourAddr := sw.nodeInfo.NetAddress() + // TODO: move this out of here ? if addrBook != nil { // add peers to `addrBook` - ourAddr := sw.nodeInfo.NetAddress() for _, netAddr := range netAddrs { // do not add our address or ID - if netAddr.Same(ourAddr) { - continue + if !netAddr.Same(ourAddr) { + addrBook.AddAddress(netAddr, ourAddr) } - // TODO: move this out of here ? - addrBook.AddAddress(netAddr, ourAddr) } // Persist some peers to disk right away. // NOTE: integration tests depend on this @@ -372,8 +377,14 @@ func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent b perm := sw.rng.Perm(len(netAddrs)) for i := 0; i < len(perm); i++ { go func(i int) { - sw.randomSleep(0) j := perm[i] + + // do not dial ourselves + if netAddrs[j].Same(ourAddr) { + return + } + + sw.randomSleep(0) err := sw.DialPeerWithAddress(netAddrs[j], persistent) if err != nil { sw.Logger.Error("Error dialing peer", "err", err) @@ -386,11 +397,6 @@ func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent b // DialPeerWithAddress dials the given peer and runs sw.addPeer if it connects and authenticates successfully. // If `persistent == true`, the switch will always try to reconnect to this peer if the connection ever fails. func (sw *Switch) DialPeerWithAddress(addr *NetAddress, persistent bool) error { - // do not dial ourselves - if sw.addrBook.OurAddress(addr) { - return ErrSwitchConnectToSelf - } - sw.dialing.Set(string(addr.ID), addr) defer sw.dialing.Delete(string(addr.ID)) return sw.addOutboundPeerWithConfig(addr, sw.peerConfig, persistent) @@ -532,9 +538,14 @@ func (sw *Switch) addPeer(pc peerConn) error { // Avoid self if sw.nodeKey.ID() == peerID { - // add given address to the address book to avoid dialing ourselves again - // this is our public address - sw.addrBook.AddOurAddress(peerNodeInfo.NetAddress()) + addr := peerNodeInfo.NetAddress() + + // remove the given address from the address book if we're added it earlier + sw.addrBook.RemoveAddress(addr) + + // add the given address to the address book to avoid dialing ourselves + // again this is our public address + sw.addrBook.AddOurAddress(addr) return ErrSwitchConnectToSelf } diff --git a/p2p/switch_test.go b/p2p/switch_test.go index 4995ad3f2..8fec3e7ab 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -90,7 +90,9 @@ func MakeSwitchPair(t testing.TB, initSwitch func(int, *Switch) *Switch) (*Switc } func initSwitchFunc(i int, sw *Switch) *Switch { - sw.SetAddrBook(&addrBookMock{ourAddrs: make(map[string]struct{})}) + sw.SetAddrBook(&addrBookMock{ + addrs: make(map[string]struct{}, 0), + ourAddrs: make(map[string]struct{}, 0)}) // Make two reactors of two channels each sw.AddReactor("foo", NewTestReactor([]*conn.ChannelDescriptor{ @@ -180,32 +182,24 @@ func TestConnAddrFilter(t *testing.T) { func TestSwitchFiltersOutItself(t *testing.T) { s1 := MakeSwitch(config, 1, "127.0.0.2", "123.123.123", initSwitchFunc) - addr := s1.NodeInfo().NetAddress() + // addr := s1.NodeInfo().NetAddress() - // add ourselves like we do in node.go#427 - s1.addrBook.AddOurAddress(addr) - - // addr should be rejected immediately because of the same IP & port - err := s1.DialPeerWithAddress(addr, false) - if assert.Error(t, err) { - assert.Equal(t, ErrSwitchConnectToSelf, err) - } + // // add ourselves like we do in node.go#427 + // s1.addrBook.AddOurAddress(addr) // simulate s1 having a public IP by creating a remote peer with the same ID rp := &remotePeer{PrivKey: s1.nodeKey.PrivKey, Config: DefaultPeerConfig()} rp.Start() // addr should be rejected in addPeer based on the same ID - err = s1.DialPeerWithAddress(rp.Addr(), false) + err := s1.DialPeerWithAddress(rp.Addr(), false) if assert.Error(t, err) { assert.Equal(t, ErrSwitchConnectToSelf, err) } - // addr should be rejected immediately because during previous step we changed node's public IP - err = s1.DialPeerWithAddress(rp.Addr(), false) - if assert.Error(t, err) { - assert.Equal(t, ErrSwitchConnectToSelf, err) - } + assert.True(t, s1.addrBook.OurAddress(rp.Addr())) + + assert.False(t, s1.addrBook.HasAddress(rp.Addr())) rp.Stop() @@ -379,16 +373,27 @@ func BenchmarkSwitchBroadcast(b *testing.B) { } type addrBookMock struct { + addrs map[string]struct{} ourAddrs map[string]struct{} } var _ AddrBook = (*addrBookMock)(nil) -func (book *addrBookMock) AddAddress(addr *NetAddress, src *NetAddress) error { return nil } -func (book *addrBookMock) AddOurAddress(addr *NetAddress) { book.ourAddrs[addr.String()] = struct{}{} } +func (book *addrBookMock) AddAddress(addr *NetAddress, src *NetAddress) error { + book.addrs[addr.String()] = struct{}{} + return nil +} +func (book *addrBookMock) AddOurAddress(addr *NetAddress) { book.ourAddrs[addr.String()] = struct{}{} } func (book *addrBookMock) OurAddress(addr *NetAddress) bool { _, ok := book.ourAddrs[addr.String()] return ok } func (book *addrBookMock) MarkGood(*NetAddress) {} -func (book *addrBookMock) Save() {} +func (book *addrBookMock) HasAddress(addr *NetAddress) bool { + _, ok := book.addrs[addr.String()] + return ok +} +func (book *addrBookMock) RemoveAddress(addr *NetAddress) { + delete(book.addrs, addr.String()) +} +func (book *addrBookMock) Save() {} From 3233c318ea567d732c7905532ad77946690eb26e Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 6 Apr 2018 12:35:48 +0200 Subject: [PATCH 11/12] only log errors, dial correct addresses "this means if there are lookup errors or typos in the persistent_peers, tendermint will fail to start ? didn't some one ask for us not to do this previously ?" --- p2p/switch.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/p2p/switch.go b/p2p/switch.go index 9f65a85e0..e412d3cc9 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -350,12 +350,10 @@ func (sw *Switch) IsDialing(id ID) bool { // DialPeersAsync dials a list of peers asynchronously in random order (optionally, making them persistent). func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent bool) error { netAddrs, errs := NewNetAddressStrings(peers) + // only log errors, dial correct addresses for _, err := range errs { sw.Logger.Error("Error in peer's address", "err", err) } - if len(errs) > 0 { - return errors.New("Errors in peer addresses (see errors above)") - } ourAddr := sw.nodeInfo.NetAddress() From 3d32474da8fd687e9408a5a692a89c9d99afd173 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 6 Apr 2018 13:26:05 +0200 Subject: [PATCH 12/12] make linter happy --- p2p/switch_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/p2p/switch_test.go b/p2p/switch_test.go index 8fec3e7ab..c02eb26d3 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -91,8 +91,8 @@ func MakeSwitchPair(t testing.TB, initSwitch func(int, *Switch) *Switch) (*Switc func initSwitchFunc(i int, sw *Switch) *Switch { sw.SetAddrBook(&addrBookMock{ - addrs: make(map[string]struct{}, 0), - ourAddrs: make(map[string]struct{}, 0)}) + addrs: make(map[string]struct{}), + ourAddrs: make(map[string]struct{})}) // Make two reactors of two channels each sw.AddReactor("foo", NewTestReactor([]*conn.ChannelDescriptor{