From 6e39ec6e262790c9dc0080b043fa3c55ca76f364 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 5 Apr 2018 15:21:11 +0200 Subject: [PATCH] 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() {}