From c4bfa733586eaa3702eea8c146afe4879c54356d Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Wed, 15 May 2019 10:04:29 -0400 Subject: [PATCH] Don't allow peers to update with lower height --- blockchain/pool.go | 37 ++++++++++------------------------ blockchain/pool_test.go | 25 ++++------------------- blockchain/reactor_fsm.go | 1 + blockchain/reactor_fsm_test.go | 2 +- 4 files changed, 17 insertions(+), 48 deletions(-) diff --git a/blockchain/pool.go b/blockchain/pool.go index 1bee2849..e60ef652 100644 --- a/blockchain/pool.go +++ b/blockchain/pool.go @@ -106,7 +106,7 @@ func (pool *blockPool) updateMaxPeerHeight() { } // Adds a new peer or updates an existing peer with a new height. -// If the peer is too short it is removed. +// If a new peer is too short it is not added. func (pool *blockPool) updatePeer(peerID p2p.ID, height int64) error { peer := pool.peers[peerID] oldHeight := int64(0) @@ -115,37 +115,22 @@ func (pool *blockPool) updatePeer(peerID p2p.ID, height int64) error { } pool.logger.Debug("updatePeer", "peerID", peerID, "height", height, "old_height", oldHeight) - if height < pool.height { - pool.logger.Info("Peer height too small", "peer", peerID, "height", height, "fsm_height", pool.height) - - // Don't add or update a peer that is not useful. - if peer != nil { - pool.logger.Info("remove short peer", "peer", peerID, "height", height, "fsm_height", pool.height) - pool.removePeer(peerID, errPeerTooShort) - } - return errPeerTooShort - } - if peer == nil { + if height < pool.height { + pool.logger.Info("Peer height too small", "peer", peerID, "height", height, "fsm_height", pool.height) + return errPeerTooShort + } // Add new peer. peer = newBPPeer(peerID, height, pool.toBcR.sendPeerError) peer.setLogger(pool.logger.With("peer", peerID)) pool.peers[peerID] = peer } else { - // Update existing peer. - // Remove any requests made for heights in range (height, peer.height]. - for h, block := range pool.peers[peerID].blocks { - if h <= height { - continue - } - // Reschedule the requests for all blocks waiting for the peer, or received and not processed yet. - if block == nil { - // Since block was not yet received it is counted in numPending, decrement. - pool.numPending-- - pool.peers[peerID].numPending-- - } - pool.rescheduleRequest(peerID, h) + // Check if peer is lowering its height. This is not allowed. + if height < peer.height { + pool.removePeer(peerID, errPeerLowersItsHeight) + return errPeerLowersItsHeight } + // Update existing peer. peer.height = height } @@ -171,7 +156,7 @@ func (pool *blockPool) deletePeer(peerID p2p.ID) { // Removes any blocks and requests associated with the peer and deletes the peer. // Also triggers new requests if blocks have been removed. func (pool *blockPool) removePeer(peerID p2p.ID, err error) { - pool.logger.Info("removing peer", "peerID", peerID) + pool.logger.Info("removing peer", "peerID", peerID, "error", err) peer := pool.peers[peerID] if peer == nil { diff --git a/blockchain/pool_test.go b/blockchain/pool_test.go index 21552134..9615d093 100644 --- a/blockchain/pool_test.go +++ b/blockchain/pool_test.go @@ -143,38 +143,21 @@ func TestBlockPoolUpdatePeer(t *testing.T) { poolWanted: makeBlockPool(testBcR, 100, []bpPeer{{id: "P1", height: 123}}, map[int64]tPBlocks{}), }, { - name: "decrease the height of P1 from 120 to 110 (still over current processing height)", + name: "decrease the height of P1 from 120 to 110", pool: makeBlockPool(testBcR, 100, []bpPeer{{id: "P1", height: 120}}, map[int64]tPBlocks{}), args: testPeer{"P1", 110}, - poolWanted: makeBlockPool(testBcR, 100, []bpPeer{{id: "P1", height: 110}}, map[int64]tPBlocks{}), - }, - { - name: "decrease the height of P1 from 120 to 90 (under current processing height)", - pool: makeBlockPool(testBcR, 100, []bpPeer{{id: "P1", height: 120}}, map[int64]tPBlocks{}), - args: testPeer{"P1", 90}, - errWanted: errPeerTooShort, + errWanted: errPeerLowersItsHeight, poolWanted: makeBlockPool(testBcR, 100, []bpPeer{}, map[int64]tPBlocks{}), }, { name: "decrease the height of P1 from 105 to 102 with blocks", pool: makeBlockPool(testBcR, 100, []bpPeer{{id: "P1", height: 105}}, - map[int64]tPBlocks{ - 100: {"P1", true}, 101: {"P1", true}, 102: {"P1", true}, - 103: {"P1", true}, 104: {"P1", false}}), - args: testPeer{"P1", 102}, - poolWanted: makeBlockPool(testBcR, 100, []bpPeer{{id: "P1", height: 102}}, map[int64]tPBlocks{ 100: {"P1", true}, 101: {"P1", true}, 102: {"P1", true}}), - }, - { - name: "decrease the height of P1 from 102 to 99 with blocks", - pool: makeBlockPool(testBcR, 100, []bpPeer{{id: "P1", height: 102}}, - map[int64]tPBlocks{ - 100: {"P1", true}, 101: {"P1", true}, 102: {"P1", true}}), - args: testPeer{"P1", 99}, + args: testPeer{"P1", 102}, + errWanted: errPeerLowersItsHeight, poolWanted: makeBlockPool(testBcR, 100, []bpPeer{}, map[int64]tPBlocks{}), - errWanted: errPeerTooShort, }, } diff --git a/blockchain/reactor_fsm.go b/blockchain/reactor_fsm.go index 5240de67..726fcfd0 100644 --- a/blockchain/reactor_fsm.go +++ b/blockchain/reactor_fsm.go @@ -150,6 +150,7 @@ var ( errNoErrorFinished = errors.New("FSM is finished") errInvalidEvent = errors.New("invalid event in current state") errNoTallerPeer = errors.New("FSM timed out on waiting for a peer taller than this node") + errPeerLowersItsHeight = errors.New("peer reports a height lower than previous") errNoPeerResponseForCurrentHeights = errors.New("FSM timed out on peer block response for current heights") errNoPeerResponse = errors.New("FSM timed out on peer block response") errBadDataFromPeer = errors.New("received from wrong peer or bad block") diff --git a/blockchain/reactor_fsm_test.go b/blockchain/reactor_fsm_test.go index d3691e17..3cf87203 100644 --- a/blockchain/reactor_fsm_test.go +++ b/blockchain/reactor_fsm_test.go @@ -802,7 +802,7 @@ func TestFSMPeerRelatedEvents(t *testing.T) { // statusResponseEv from P1 makeStepStatusEv("waitForPeer", "waitForBlock", "P1", 200, nil), // statusResponseEv from P1 - makeStepStatusEv("waitForBlock", "waitForPeer", "P1", 3, errPeerTooShort), + makeStepStatusEv("waitForBlock", "waitForPeer", "P1", 3, errPeerLowersItsHeight), }, }, {