diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 444692c16..7ad305442 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -21,3 +21,4 @@ program](https://hackerone.com/tendermint). ### BUG FIXES: +- [blockchain/v1] [\#5701](https://github.com/tendermint/tendermint/pull/5701) Handle peers without blocks (@melekes) diff --git a/blockchain/v1/peer.go b/blockchain/v1/peer.go index 02b1b4fc1..8074c2c1b 100644 --- a/blockchain/v1/peer.go +++ b/blockchain/v1/peer.go @@ -30,6 +30,7 @@ type BpPeer struct { Height int64 // the peer reported height NumPendingBlockRequests int // number of requests still waiting for block responses blocks map[int64]*types.Block // blocks received or expected to be received from this peer + noBlocks map[int64]struct{} // heights for which the peer does not have blocks blockResponseTimer *time.Timer recvMonitor *flow.Monitor params *BpPeerParams // parameters for timer and monitor @@ -45,12 +46,13 @@ func NewBpPeer( params = BpPeerDefaultParams() } return &BpPeer{ - ID: peerID, - Height: height, - blocks: make(map[int64]*types.Block, maxRequestsPerPeer), - logger: log.NewNopLogger(), - onErr: onErr, - params: params, + ID: peerID, + Height: height, + blocks: make(map[int64]*types.Block, maxRequestsPerPeer), + noBlocks: make(map[int64]struct{}), + logger: log.NewNopLogger(), + onErr: onErr, + params: params, } } @@ -129,6 +131,19 @@ func (peer *BpPeer) RemoveBlock(height int64) { delete(peer.blocks, height) } +// SetNoBlock records that the peer does not have a block for height. +func (peer *BpPeer) SetNoBlock(height int64) { + peer.noBlocks[height] = struct{}{} +} + +// NoBlock returns true if the peer does not have a block for height. +func (peer *BpPeer) NoBlock(height int64) bool { + if _, ok := peer.noBlocks[height]; ok { + return true + } + return false +} + // RequestSent records that a request was sent, and starts the peer timer and monitor if needed. func (peer *BpPeer) RequestSent(height int64) { peer.blocks[height] = nil diff --git a/blockchain/v1/pool.go b/blockchain/v1/pool.go index be2edbc21..f0d2dbeb6 100644 --- a/blockchain/v1/pool.go +++ b/blockchain/v1/pool.go @@ -99,6 +99,18 @@ func (pool *BlockPool) UpdatePeer(peerID p2p.ID, height int64) error { return nil } +// SetNoBlock records that the peer does not have a block for height and +// schedules a new request for that height from another peer. +func (pool *BlockPool) SetNoBlock(peerID p2p.ID, height int64) { + peer := pool.peers[peerID] + if peer == nil { + return + } + peer.SetNoBlock(height) + + pool.rescheduleRequest(peerID, height) +} + // Cleans and deletes the peer. Recomputes the max peer height. func (pool *BlockPool) deletePeer(peer *BpPeer) { if peer == nil { @@ -213,7 +225,7 @@ func (pool *BlockPool) sendRequest(height int64) bool { if peer.NumPendingBlockRequests >= maxRequestsPerPeer { continue } - if peer.Height < height { + if peer.Height < height || peer.NoBlock(height) { continue } diff --git a/blockchain/v1/reactor_fsm.go b/blockchain/v1/reactor_fsm.go index 83305539c..f63d733f1 100644 --- a/blockchain/v1/reactor_fsm.go +++ b/blockchain/v1/reactor_fsm.go @@ -276,6 +276,7 @@ func init() { return waitForBlock, err case noBlockResponseEv: fsm.logger.Error("peer does not have requested block", "peer", data.peerID) + fsm.pool.SetNoBlock(data.peerID, data.height) return waitForBlock, nil case processedBlockEv: diff --git a/blockchain/v1/reactor_fsm_test.go b/blockchain/v1/reactor_fsm_test.go index 6f2a4dc88..c3f5e887b 100644 --- a/blockchain/v1/reactor_fsm_test.go +++ b/blockchain/v1/reactor_fsm_test.go @@ -100,6 +100,19 @@ func sProcessedBlockEv(current, expected string, reactorError error) fsmStepTest } } +func sNoBlockResponseEv(current, expected string, peerID p2p.ID, height int64, err error) fsmStepTestValues { + return fsmStepTestValues{ + currentState: current, + event: noBlockResponseEv, + data: bReactorEventData{ + peerID: peerID, + height: height, + }, + wantState: expected, + wantErr: err, + } +} + func sStatusEv(current, expected string, peerID p2p.ID, height int64, err error) fsmStepTestValues { return fsmStepTestValues{ currentState: current, @@ -352,6 +365,46 @@ func TestFSMBlockVerificationFailure(t *testing.T) { executeFSMTests(t, tests, false) } +func TestFSMNoBlockResponse(t *testing.T) { + tests := []testFields{ + { + name: "no block response", + startingHeight: 1, + maxRequestsPerPeer: 3, + steps: []fsmStepTestValues{ + sStartFSMEv(), + + // add P1 and get blocks 1-3 from it + sStatusEv("waitForPeer", "waitForBlock", "P1", 3, nil), + sMakeRequestsEv("waitForBlock", "waitForBlock", maxNumRequests), + sBlockRespEv("waitForBlock", "waitForBlock", "P1", 1, []int64{}), + sBlockRespEv("waitForBlock", "waitForBlock", "P1", 2, []int64{1}), + sBlockRespEv("waitForBlock", "waitForBlock", "P1", 3, []int64{1, 2}), + + // add P2 + sStatusEv("waitForBlock", "waitForBlock", "P2", 3, nil), + + // process block failure, should remove P1 and all blocks + sNoBlockResponseEv("waitForBlock", "waitForBlock", "P1", 1, nil), + sNoBlockResponseEv("waitForBlock", "waitForBlock", "P1", 2, nil), + sNoBlockResponseEv("waitForBlock", "waitForBlock", "P1", 3, nil), + + // get blocks 1-3 from P2 + sMakeRequestsEv("waitForBlock", "waitForBlock", maxNumRequests), + sBlockRespEv("waitForBlock", "waitForBlock", "P2", 1, []int64{}), + sBlockRespEv("waitForBlock", "waitForBlock", "P2", 2, []int64{1}), + sBlockRespEv("waitForBlock", "waitForBlock", "P2", 3, []int64{1, 2}), + + // finish after processing blocks 1 and 2 + sProcessedBlockEv("waitForBlock", "waitForBlock", nil), + sProcessedBlockEv("waitForBlock", "finished", nil), + }, + }, + } + + executeFSMTests(t, tests, false) +} + func TestFSMBadBlockFromPeer(t *testing.T) { tests := []testFields{ {