From 746d137f86f34ecdb5f2a1d2b94a66913c1c9efe Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 18 Oct 2018 18:26:32 -0400 Subject: [PATCH] p2p: Restore OriginalAddr (#2668) * p2p: bring back OriginalAddr * p2p: set OriginalAddr * update changelog --- CHANGELOG_PENDING.md | 2 ++ blockchain/reactor_test.go | 1 + p2p/dummy/peer.go | 5 +++++ p2p/peer.go | 28 ++++++++++++++++++++++++++++ p2p/peer_set_test.go | 1 + p2p/peer_test.go | 2 +- p2p/pex/pex_reactor_test.go | 1 + p2p/switch.go | 9 ++++++--- p2p/test_util.go | 12 +++++++----- p2p/transport.go | 21 +++++++++++++-------- 10 files changed, 65 insertions(+), 17 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 99c389974..758bfeb2f 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -82,3 +82,5 @@ Proposal or timeoutProposal before entering prevote - [p2p] \#2555 fix p2p switch FlushThrottle value (@goolAdapter) - [libs/event] \#2518 fix event concurrency flaw (@goolAdapter) - [state] \#2616 Pass nil to NewValidatorSet() when genesis file's Validators field is nil +- [p2p] \#2668 Reconnect to originally dialed address (not self-reported + address) for persistent peers diff --git a/blockchain/reactor_test.go b/blockchain/reactor_test.go index 7fc7ffb77..fca063e0c 100644 --- a/blockchain/reactor_test.go +++ b/blockchain/reactor_test.go @@ -206,3 +206,4 @@ func (tp *bcrTestPeer) IsPersistent() bool { return true } func (tp *bcrTestPeer) Get(s string) interface{} { return s } func (tp *bcrTestPeer) Set(string, interface{}) {} func (tp *bcrTestPeer) RemoteIP() net.IP { return []byte{127, 0, 0, 1} } +func (tp *bcrTestPeer) OriginalAddr() *p2p.NetAddress { return nil } diff --git a/p2p/dummy/peer.go b/p2p/dummy/peer.go index 4871719d4..65ff65fb2 100644 --- a/p2p/dummy/peer.go +++ b/p2p/dummy/peer.go @@ -78,3 +78,8 @@ func (p *peer) Get(key string) interface{} { } return nil } + +// OriginalAddr always returns nil. +func (p *peer) OriginalAddr() *p2p.NetAddress { + return nil +} diff --git a/p2p/peer.go b/p2p/peer.go index 009313141..944174b0e 100644 --- a/p2p/peer.go +++ b/p2p/peer.go @@ -26,6 +26,7 @@ type Peer interface { NodeInfo() NodeInfo // peer's info Status() tmconn.ConnectionStatus + OriginalAddr() *NetAddress Send(byte, []byte) bool TrySend(byte, []byte) bool @@ -43,10 +44,28 @@ type peerConn struct { config *config.P2PConfig conn net.Conn // source connection + originalAddr *NetAddress // nil for inbound connections + // cached RemoteIP() ip net.IP } +func newPeerConn( + outbound, persistent bool, + config *config.P2PConfig, + conn net.Conn, + originalAddr *NetAddress, +) peerConn { + + return peerConn{ + outbound: outbound, + persistent: persistent, + config: config, + conn: conn, + originalAddr: originalAddr, + } +} + // ID only exists for SecretConnection. // NOTE: Will panic if conn is not *SecretConnection. func (pc peerConn) ID() ID { @@ -195,6 +214,15 @@ func (p *peer) NodeInfo() NodeInfo { return p.nodeInfo } +// OriginalAddr returns the original address, which was used to connect with +// the peer. Returns nil for inbound peers. +func (p *peer) OriginalAddr() *NetAddress { + if p.peerConn.outbound { + return p.peerConn.originalAddr + } + return nil +} + // Status returns the peer's ConnectionStatus. func (p *peer) Status() tmconn.ConnectionStatus { return p.mconn.Status() diff --git a/p2p/peer_set_test.go b/p2p/peer_set_test.go index c0ad80005..daa9b2c82 100644 --- a/p2p/peer_set_test.go +++ b/p2p/peer_set_test.go @@ -28,6 +28,7 @@ func (mp *mockPeer) IsPersistent() bool { return true } func (mp *mockPeer) Get(s string) interface{} { return s } func (mp *mockPeer) Set(string, interface{}) {} func (mp *mockPeer) RemoteIP() net.IP { return mp.ip } +func (mp *mockPeer) OriginalAddr() *NetAddress { return nil } // Returns a mock peer func newMockPeer(ip net.IP) *mockPeer { diff --git a/p2p/peer_test.go b/p2p/peer_test.go index 9c330ee52..02f1d2c0f 100644 --- a/p2p/peer_test.go +++ b/p2p/peer_test.go @@ -114,7 +114,7 @@ func testOutboundPeerConn( return peerConn{}, cmn.ErrorWrap(err, "Error creating peer") } - pc, err := testPeerConn(conn, config, true, persistent, ourNodePrivKey) + pc, err := testPeerConn(conn, config, true, persistent, ourNodePrivKey, addr) if err != nil { if cerr := conn.Close(); cerr != nil { return peerConn{}, cmn.ErrorWrap(err, cerr.Error()) diff --git a/p2p/pex/pex_reactor_test.go b/p2p/pex/pex_reactor_test.go index b0338c3c2..9d3f49bba 100644 --- a/p2p/pex/pex_reactor_test.go +++ b/p2p/pex/pex_reactor_test.go @@ -402,6 +402,7 @@ func (mockPeer) Send(byte, []byte) bool { return false } func (mockPeer) TrySend(byte, []byte) bool { return false } func (mockPeer) Set(string, interface{}) {} func (mockPeer) Get(string) interface{} { return nil } +func (mockPeer) OriginalAddr() *p2p.NetAddress { return nil } func assertPeersWithTimeout( t *testing.T, diff --git a/p2p/switch.go b/p2p/switch.go index 64e248fc3..b1406b9b0 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -280,9 +280,12 @@ func (sw *Switch) StopPeerForError(peer Peer, reason interface{}) { sw.stopAndRemovePeer(peer, reason) if peer.IsPersistent() { - // TODO: use the original address dialed, not the self reported one - // See #2618. - addr := peer.NodeInfo().NetAddress() + addr := peer.OriginalAddr() + if addr == nil { + // FIXME: persistent peers can't be inbound right now. + // self-reported address for inbound persistent peers + addr = peer.NodeInfo().NetAddress() + } go sw.reconnectToPeer(addr) } } diff --git a/p2p/test_util.go b/p2p/test_util.go index 4d43175bb..e1f7b5040 100644 --- a/p2p/test_util.go +++ b/p2p/test_util.go @@ -206,7 +206,7 @@ func testInboundPeerConn( config *config.P2PConfig, ourNodePrivKey crypto.PrivKey, ) (peerConn, error) { - return testPeerConn(conn, config, false, false, ourNodePrivKey) + return testPeerConn(conn, config, false, false, ourNodePrivKey, nil) } func testPeerConn( @@ -214,6 +214,7 @@ func testPeerConn( cfg *config.P2PConfig, outbound, persistent bool, ourNodePrivKey crypto.PrivKey, + originalAddr *NetAddress, ) (pc peerConn, err error) { conn := rawConn @@ -231,10 +232,11 @@ func testPeerConn( // Only the information we already have return peerConn{ - config: cfg, - outbound: outbound, - persistent: persistent, - conn: conn, + config: cfg, + outbound: outbound, + persistent: persistent, + conn: conn, + originalAddr: originalAddr, }, nil } diff --git a/p2p/transport.go b/p2p/transport.go index b20f32f3d..10565d8a9 100644 --- a/p2p/transport.go +++ b/p2p/transport.go @@ -171,7 +171,7 @@ func (mt *MultiplexTransport) Accept(cfg peerConfig) (Peer, error) { cfg.outbound = false - return mt.wrapPeer(a.conn, a.nodeInfo, cfg), nil + return mt.wrapPeer(a.conn, a.nodeInfo, cfg, nil), nil case <-mt.closec: return nil, &ErrTransportClosed{} } @@ -199,7 +199,7 @@ func (mt *MultiplexTransport) Dial( cfg.outbound = true - p := mt.wrapPeer(secretConn, nodeInfo, cfg) + p := mt.wrapPeer(secretConn, nodeInfo, cfg, &addr) return p, nil } @@ -399,14 +399,19 @@ func (mt *MultiplexTransport) wrapPeer( c net.Conn, ni NodeInfo, cfg peerConfig, + dialedAddr *NetAddress, ) Peer { + + peerConn := newPeerConn( + cfg.outbound, + cfg.persistent, + &mt.p2pConfig, + c, + dialedAddr, + ) + p := newPeer( - peerConn{ - conn: c, - config: &mt.p2pConfig, - outbound: cfg.outbound, - persistent: cfg.persistent, - }, + peerConn, mt.mConfig, ni, cfg.reactorsByCh,