From 362ef77eb0c9a2e6180d4b1df07cd583e2af12e6 Mon Sep 17 00:00:00 2001 From: Alexander Simmerl Date: Fri, 12 Apr 2019 12:31:02 +0200 Subject: [PATCH 1/2] Bring back NodeInfo NetAddress form the dead (#3545) A prior change to address accidental DNS lookups introduced the SocketAddr on peer, which was then used to add it to the addressbook. Which in turn swallowed the self reported port of the peer, which is important on a reconnect. This change revives the NetAddress on NodeInfo which the Peer carries, but now returns an error to avoid nil dereferencing another issue observed in the past. Additionally we could potentially address #3532, yet the original problem statemenf of that issue stands. As a drive-by optimisation `MarkAsGood` now takes only a `p2p.ID` which makes it interface a bit stricter and leaner. (cherry picked from commit b5b3b85697a9c29adaab0e1ab5d1380459e2c5eb) Signed-off-by: Ismail Khoffi --- p2p/node_info.go | 20 +++++++------------- p2p/pex/addrbook.go | 6 +++--- p2p/pex/addrbook_test.go | 8 ++++---- p2p/pex/pex_reactor.go | 15 ++++++++++++--- p2p/pex/pex_reactor_test.go | 2 +- p2p/switch.go | 4 ++-- p2p/switch_test.go | 2 +- p2p/test_util.go | 2 +- 8 files changed, 31 insertions(+), 28 deletions(-) diff --git a/p2p/node_info.go b/p2p/node_info.go index e80f1e1b..8195471e 100644 --- a/p2p/node_info.go +++ b/p2p/node_info.go @@ -24,9 +24,14 @@ func MaxNodeInfoSize() int { // and determines if we're compatible. type NodeInfo interface { ID() ID + nodeInfoAddress nodeInfoTransport } +type nodeInfoAddress interface { + NetAddress() (*NetAddress, error) +} + // nodeInfoTransport validates a nodeInfo and checks // our compatibility with it. It's for use in the handshake. type nodeInfoTransport interface { @@ -209,20 +214,9 @@ OUTER_LOOP: // it includes the authenticated peer ID and the self-reported // ListenAddr. Note that the ListenAddr is not authenticated and // may not match that address actually dialed if its an outbound peer. -func (info DefaultNodeInfo) NetAddress() *NetAddress { +func (info DefaultNodeInfo) NetAddress() (*NetAddress, error) { idAddr := IDAddressString(info.ID(), info.ListenAddr) - netAddr, err := NewNetAddressString(idAddr) - if err != nil { - switch err.(type) { - case ErrNetAddressLookup: - // XXX If the peer provided a host name and the lookup fails here - // we're out of luck. - // TODO: use a NetAddress in DefaultNodeInfo - default: - panic(err) // everything should be well formed by now - } - } - return netAddr + return NewNetAddressString(idAddr) } //----------------------------------------------------------- diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 3cda9ac7..717c9dff 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -54,7 +54,7 @@ type AddrBook interface { PickAddress(biasTowardsNewAddrs int) *p2p.NetAddress // Mark address - MarkGood(*p2p.NetAddress) + MarkGood(p2p.ID) MarkAttempt(*p2p.NetAddress) MarkBad(*p2p.NetAddress) @@ -296,11 +296,11 @@ func (a *addrBook) PickAddress(biasTowardsNewAddrs int) *p2p.NetAddress { // MarkGood implements AddrBook - it marks the peer as good and // moves it into an "old" bucket. -func (a *addrBook) MarkGood(addr *p2p.NetAddress) { +func (a *addrBook) MarkGood(id p2p.ID) { a.mtx.Lock() defer a.mtx.Unlock() - ka := a.addrLookup[addr.ID] + ka := a.addrLookup[id] if ka == nil { return } diff --git a/p2p/pex/addrbook_test.go b/p2p/pex/addrbook_test.go index 9effa5d0..08f7fa23 100644 --- a/p2p/pex/addrbook_test.go +++ b/p2p/pex/addrbook_test.go @@ -41,7 +41,7 @@ func TestAddrBookPickAddress(t *testing.T) { assert.NotNil(t, addr, "expected an address") // pick an address when we only have old address - book.MarkGood(addrSrc.addr) + book.MarkGood(addrSrc.addr.ID) addr = book.PickAddress(0) assert.NotNil(t, addr, "expected an address") addr = book.PickAddress(50) @@ -126,7 +126,7 @@ func TestAddrBookPromoteToOld(t *testing.T) { // Promote half of them for i, addrSrc := range randAddrs { if i%2 == 0 { - book.MarkGood(addrSrc.addr) + book.MarkGood(addrSrc.addr.ID) } } @@ -330,7 +330,7 @@ func TestAddrBookGetSelectionWithBias(t *testing.T) { randAddrsLen := len(randAddrs) for i, addrSrc := range randAddrs { if int((float64(i)/float64(randAddrsLen))*100) >= 20 { - book.MarkGood(addrSrc.addr) + book.MarkGood(addrSrc.addr.ID) } } @@ -574,7 +574,7 @@ func createAddrBookWithMOldAndNNewAddrs(t *testing.T, nOld, nNew int) (book *add randAddrs := randNetAddressPairs(t, nOld) for _, addr := range randAddrs { book.AddAddress(addr.addr, addr.src) - book.MarkGood(addr.addr) + book.MarkGood(addr.addr.ID) } randAddrs = randNetAddressPairs(t, nNew) diff --git a/p2p/pex/pex_reactor.go b/p2p/pex/pex_reactor.go index 0ce11632..cf859cda 100644 --- a/p2p/pex/pex_reactor.go +++ b/p2p/pex/pex_reactor.go @@ -167,12 +167,18 @@ func (r *PEXReactor) AddPeer(p Peer) { } } else { // inbound peer is its own source - addr := p.SocketAddr() + addr, err := p.NodeInfo().NetAddress() + if err != nil { + r.Logger.Error("Failed to get peer NetAddress", "err", err, "peer", p) + return + } + + // Make it explicit that addr and src are the same for an inbound peer. src := addr // add to book. dont RequestAddrs right away because // we don't trust inbound as much - let ensurePeersRoutine handle it. - err := r.book.AddAddress(addr, src) + err = r.book.AddAddress(addr, src) r.logErrAddrBook(err) } } @@ -309,7 +315,10 @@ func (r *PEXReactor) ReceiveAddrs(addrs []*p2p.NetAddress, src Peer) error { } r.requestsSent.Delete(id) - srcAddr := src.SocketAddr() + srcAddr, err := src.NodeInfo().NetAddress() + if err != nil { + return err + } for _, netAddr := range addrs { // Validate netAddr. Disconnect from a peer if it sends us invalid data. if netAddr == nil { diff --git a/p2p/pex/pex_reactor_test.go b/p2p/pex/pex_reactor_test.go index be066970..74d9ef52 100644 --- a/p2p/pex/pex_reactor_test.go +++ b/p2p/pex/pex_reactor_test.go @@ -531,7 +531,7 @@ func testCreateSeed(dir string, id int, knownAddrs, srcAddrs []*p2p.NetAddress) book.SetLogger(log.TestingLogger()) for j := 0; j < len(knownAddrs); j++ { book.AddAddress(knownAddrs[j], srcAddrs[j]) - book.MarkGood(knownAddrs[j]) + book.MarkGood(knownAddrs[j].ID) } sw.SetAddrBook(book) diff --git a/p2p/switch.go b/p2p/switch.go index 2228ac07..9210e7c0 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -46,7 +46,7 @@ type AddrBook interface { AddAddress(addr *NetAddress, src *NetAddress) error AddOurAddress(*NetAddress) OurAddress(*NetAddress) bool - MarkGood(*NetAddress) + MarkGood(ID) RemoveAddress(*NetAddress) HasAddress(*NetAddress) bool Save() @@ -378,7 +378,7 @@ func (sw *Switch) SetAddrBook(addrBook AddrBook) { // like contributed to consensus. func (sw *Switch) MarkPeerAsGood(peer Peer) { if sw.addrBook != nil { - sw.addrBook.MarkGood(peer.SocketAddr()) + sw.addrBook.MarkGood(peer.ID()) } } diff --git a/p2p/switch_test.go b/p2p/switch_test.go index ed968d73..aa2c816a 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -581,7 +581,7 @@ func (book *addrBookMock) OurAddress(addr *NetAddress) bool { _, ok := book.ourAddrs[addr.String()] return ok } -func (book *addrBookMock) MarkGood(*NetAddress) {} +func (book *addrBookMock) MarkGood(ID) {} func (book *addrBookMock) HasAddress(addr *NetAddress) bool { _, ok := book.addrs[addr.String()] return ok diff --git a/p2p/test_util.go b/p2p/test_util.go index df60539b..f8020924 100644 --- a/p2p/test_util.go +++ b/p2p/test_util.go @@ -23,7 +23,7 @@ type mockNodeInfo struct { } func (ni mockNodeInfo) ID() ID { return ni.addr.ID } -func (ni mockNodeInfo) NetAddress() *NetAddress { return ni.addr } +func (ni mockNodeInfo) NetAddress() (*NetAddress, error) { return ni.addr, nil } func (ni mockNodeInfo) Validate() error { return nil } func (ni mockNodeInfo) CompatibleWith(other NodeInfo) error { return nil } From 62f90f0077893545db264c3a24c8f88a43fad997 Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Tue, 16 Apr 2019 14:30:58 +0200 Subject: [PATCH 2/2] Prep release: bump version & add changelog entry Signed-off-by: Ismail Khoffi --- CHANGELOG.md | 13 +++++++++++++ CHANGELOG_PENDING.md | 2 -- version/version.go | 2 +- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e1a9d26..f1c59707 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # Changelog +## v0.30.4 + +*April 16th, 2019* + +This release fixes a regression from v0.30.3 which used the peer's SocketAddr to add the peer to the address book. +This swallowed the peer's self-reported port which is important in case of reconnect. +It brings back NetAddress() to NodeInfo and uses it instead of SocketAddr for adding peers. + +### BUG FIXES: + +- [p2p] [\#3545](https://github.com/tendermint/tendermint/issues/3545) Add back `NetAddress()` to `NodeInfo` and use it +instead of peer's `SocketAddr()` when adding a peer to the `PEXReactor` (potential fix for [\#3532](https://github.com/tendermint/tendermint/issues/3532)) + ## v0.30.3 *April 1st, 2019* diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index a7548ee7..64098583 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -20,6 +20,4 @@ Special thanks to external contributors on this release: ### IMPROVEMENTS: -- [CircleCI] \#3497 Move release management to CircleCI - ### BUG FIXES: diff --git a/version/version.go b/version/version.go index ab9c4ab3..5e8bddfe 100644 --- a/version/version.go +++ b/version/version.go @@ -20,7 +20,7 @@ const ( // Must be a string because scripts like dist.sh read this file. // XXX: Don't change the name of this variable or you will break // automation :) - TMCoreSemVer = "0.30.3" + TMCoreSemVer = "0.30.4" // ABCISemVer is the semantic version of the ABCI library ABCISemVer = "0.15.0"