From 3b4d8e9d166e3ff8e0fa149718bdc8e8a51148ce Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Mon, 10 Oct 2022 17:36:09 +0200 Subject: [PATCH] Revert "security/p2p: prevent peers who errored being added to the peer_set (backport #9500) (#9516)" This reverts commit 430afb23e7816516a5555150fa8be2094ec481b9. --- CHANGELOG_PENDING.md | 2 +- blockchain/v2/reactor_test.go | 3 --- p2p/errors.go | 7 ------- p2p/mock/peer.go | 2 -- p2p/mocks/peer.go | 19 ------------------- p2p/peer.go | 14 -------------- p2p/peer_set.go | 9 --------- p2p/peer_set_test.go | 2 -- p2p/switch.go | 10 ---------- p2p/switch_test.go | 13 ------------- test/fuzz/p2p/pex/reactor_receive.go | 2 -- 11 files changed, 1 insertion(+), 82 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 151b15d0a..081fdc045 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -25,6 +25,6 @@ - [config] \#9483 Calling `tendermint init` would incorrectly leave out the new `[storage]` section delimiter in the generated configuration file - this has now been fixed -- [p2p] \#9500 prevent peers who have errored being added to the peer_set (@jmalicevic) - [indexer] \#9473 fix bug that caused the psql indexer to index empty blocks whenever one of the transactions returned a non zero code. The relevant deduplication logic has been moved within the kv indexer only (@cmwaters) - [blocksync] \#9518 handle the case when the sending queue is full: retry block request after a timeout + diff --git a/blockchain/v2/reactor_test.go b/blockchain/v2/reactor_test.go index 57a4a00b4..a135d68fd 100644 --- a/blockchain/v2/reactor_test.go +++ b/blockchain/v2/reactor_test.go @@ -59,9 +59,6 @@ func (mp mockPeer) TrySend(byte, []byte) bool { return true } func (mp mockPeer) Set(string, interface{}) {} func (mp mockPeer) Get(string) interface{} { return struct{}{} } -func (mp mockPeer) SetRemovalFailed() {} -func (mp mockPeer) GetRemovalFailed() bool { return false } - type mockBlockStore struct { blocks map[int64]*types.Block } diff --git a/p2p/errors.go b/p2p/errors.go index 4fc915292..3650a7a0a 100644 --- a/p2p/errors.go +++ b/p2p/errors.go @@ -145,13 +145,6 @@ func (e ErrTransportClosed) Error() string { return "transport has been closed" } -// ErrPeerRemoval is raised when attempting to remove a peer results in an error. -type ErrPeerRemoval struct{} - -func (e ErrPeerRemoval) Error() string { - return "peer removal failed" -} - //------------------------------------------------------------------- type ErrNetAddressNoID struct { diff --git a/p2p/mock/peer.go b/p2p/mock/peer.go index 10254c343..59f6e0f4a 100644 --- a/p2p/mock/peer.go +++ b/p2p/mock/peer.go @@ -68,5 +68,3 @@ func (mp *Peer) RemoteIP() net.IP { return mp.ip } func (mp *Peer) SocketAddr() *p2p.NetAddress { return mp.addr } func (mp *Peer) RemoteAddr() net.Addr { return &net.TCPAddr{IP: mp.ip, Port: 8800} } func (mp *Peer) CloseConn() error { return nil } -func (mp *Peer) SetRemovalFailed() {} -func (mp *Peer) GetRemovalFailed() bool { return false } diff --git a/p2p/mocks/peer.go b/p2p/mocks/peer.go index c4d89375c..f739a0b21 100644 --- a/p2p/mocks/peer.go +++ b/p2p/mocks/peer.go @@ -53,20 +53,6 @@ func (_m *Peer) Get(_a0 string) interface{} { return r0 } -// GetRemovalFailed provides a mock function with given fields: -func (_m *Peer) GetRemovalFailed() bool { - ret := _m.Called() - - var r0 bool - if rf, ok := ret.Get(0).(func() bool); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(bool) - } - - return r0 -} - // ID provides a mock function with given fields: func (_m *Peer) ID() p2p.ID { ret := _m.Called() @@ -258,11 +244,6 @@ func (_m *Peer) SetLogger(_a0 log.Logger) { _m.Called(_a0) } -// SetRemovalFailed provides a mock function with given fields: -func (_m *Peer) SetRemovalFailed() { - _m.Called() -} - // SocketAddr provides a mock function with given fields: func (_m *Peer) SocketAddr() *p2p.NetAddress { ret := _m.Called() diff --git a/p2p/peer.go b/p2p/peer.go index d8d61a7a0..751ca3cf2 100644 --- a/p2p/peer.go +++ b/p2p/peer.go @@ -39,9 +39,6 @@ type Peer interface { Set(string, interface{}) Get(string) interface{} - - SetRemovalFailed() - GetRemovalFailed() bool } //---------------------------------------------------------- @@ -120,9 +117,6 @@ type peer struct { metrics *Metrics metricsTicker *time.Ticker - - // When removal of a peer fails, we set this flag - removalAttemptFailed bool } type PeerOption func(*peer) @@ -322,14 +316,6 @@ func (p *peer) CloseConn() error { return p.peerConn.conn.Close() } -func (p *peer) SetRemovalFailed() { - p.removalAttemptFailed = true -} - -func (p *peer) GetRemovalFailed() bool { - return p.removalAttemptFailed -} - //--------------------------------------------------- // methods only used for testing // TODO: can we remove these? diff --git a/p2p/peer_set.go b/p2p/peer_set.go index 30bcc4d32..38dff7a9f 100644 --- a/p2p/peer_set.go +++ b/p2p/peer_set.go @@ -47,9 +47,6 @@ func (ps *PeerSet) Add(peer Peer) error { if ps.lookup[peer.ID()] != nil { return ErrSwitchDuplicatePeerID{peer.ID()} } - if peer.GetRemovalFailed() { - return ErrPeerRemoval{} - } index := len(ps.list) // Appending is safe even with other goroutines @@ -110,12 +107,6 @@ func (ps *PeerSet) Remove(peer Peer) bool { item := ps.lookup[peer.ID()] if item == nil { - // Removing the peer has failed so we set a flag to mark that a removal was attempted. - // This can happen when the peer add routine from the switch is running in - // parallel to the receive routine of MConn. - // There is an error within MConn but the switch has not actually added the peer to the peer set yet. - // Setting this flag will prevent a peer from being added to a node's peer set afterwards. - peer.SetRemovalFailed() return false } diff --git a/p2p/peer_set_test.go b/p2p/peer_set_test.go index db3d9261e..b61b43f10 100644 --- a/p2p/peer_set_test.go +++ b/p2p/peer_set_test.go @@ -32,8 +32,6 @@ func (mp *mockPeer) RemoteIP() net.IP { return mp.ip } func (mp *mockPeer) SocketAddr() *NetAddress { return nil } func (mp *mockPeer) RemoteAddr() net.Addr { return &net.TCPAddr{IP: mp.ip, Port: 8800} } func (mp *mockPeer) CloseConn() error { return nil } -func (mp *mockPeer) SetRemovalFailed() {} -func (mp *mockPeer) GetRemovalFailed() bool { return false } // Returns a mock peer func newMockPeer(ip net.IP) *mockPeer { diff --git a/p2p/switch.go b/p2p/switch.go index 884fd883e..3214de223 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -370,10 +370,6 @@ func (sw *Switch) stopAndRemovePeer(peer Peer, reason interface{}) { // https://github.com/tendermint/tendermint/issues/3338 if sw.peers.Remove(peer) { sw.metrics.Peers.Add(float64(-1)) - } else { - // Removal of the peer has failed. The function above sets a flag within the peer to mark this. - // We keep this message here as information to the developer. - sw.Logger.Debug("error on peer removal", ",", "peer", peer.ID()) } } @@ -828,12 +824,6 @@ func (sw *Switch) addPeer(p Peer) error { // so that if Receive errors, we will find the peer and remove it. // Add should not err since we already checked peers.Has(). if err := sw.peers.Add(p); err != nil { - switch err.(type) { - case ErrPeerRemoval: - sw.Logger.Error("Error starting peer ", - " err ", "Peer has already errored and removal was attempted.", - "peer", p.ID()) - } return err } sw.metrics.Peers.Add(float64(1)) diff --git a/p2p/switch_test.go b/p2p/switch_test.go index 11de0a442..36420d333 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -836,16 +836,3 @@ func BenchmarkSwitchBroadcast(b *testing.B) { b.Logf("success: %v, failure: %v", numSuccess, numFailure) } - -func TestSwitchRemovalErr(t *testing.T) { - - sw1, sw2 := MakeSwitchPair(t, func(i int, sw *Switch) *Switch { - return initSwitchFunc(i, sw) - }) - assert.Equal(t, len(sw1.Peers().List()), 1) - p := sw1.Peers().List()[0] - - sw2.StopPeerForError(p, fmt.Errorf("peer should error")) - - assert.Equal(t, sw2.peers.Add(p).Error(), ErrPeerRemoval{}.Error()) -} diff --git a/test/fuzz/p2p/pex/reactor_receive.go b/test/fuzz/p2p/pex/reactor_receive.go index 4d0ef6525..4ac06c892 100644 --- a/test/fuzz/p2p/pex/reactor_receive.go +++ b/test/fuzz/p2p/pex/reactor_receive.go @@ -84,5 +84,3 @@ func (fp *fuzzPeer) Send(byte, []byte) bool { return true } func (fp *fuzzPeer) TrySend(byte, []byte) bool { return true } func (fp *fuzzPeer) Set(key string, value interface{}) { fp.m[key] = value } func (fp *fuzzPeer) Get(key string) interface{} { return fp.m[key] } -func (fp *fuzzPeer) SetRemovalFailed() {} -func (fp *fuzzPeer) GetRemovalFailed() bool { return false }