diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 499b3a585..88ceeb5ac 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -31,7 +31,6 @@ var ( ) var ( - errLowTd = errors.New("peers TD is too low") errBusy = errors.New("busy") errUnknownPeer = errors.New("peer is unknown or unhealthy") errBadPeer = errors.New("action from bad peer ignored") @@ -94,8 +93,9 @@ type Downloader struct { dropPeer peerDropFn // Retrieved the TD of our own chain // Status - synchronising int32 - notified int32 + synchroniseMock func(id string, hash common.Hash) error // Replacement for synchronise during testing + synchronising int32 + notified int32 // Channels newPeerCh chan *peer @@ -202,7 +202,7 @@ func (d *Downloader) Synchronise(id string, head common.Hash) { case errBusy: glog.V(logger.Detail).Infof("Synchronisation already in progress") - case errTimeout, errBadPeer, errEmptyHashSet, errInvalidChain, errCrossCheckFailed: + case errTimeout, errBadPeer, errStallingPeer, errBannedHead, errEmptyHashSet, errPeersUnavailable, errInvalidChain, errCrossCheckFailed: glog.V(logger.Debug).Infof("Removing peer %v: %v", id, err) d.dropPeer(id) @@ -218,6 +218,10 @@ func (d *Downloader) Synchronise(id string, head common.Hash) { // it will use the best peer possible and synchronize if it's TD is higher than our own. If any of the // checks fail an error will be returned. This method is synchronous func (d *Downloader) synchronise(id string, hash common.Hash) error { + // Mock out the synchonisation if testing + if d.synchroniseMock != nil { + return d.synchroniseMock(id, hash) + } // Make sure only one goroutine is ever allowed past this point at once if !atomic.CompareAndSwapInt32(&d.synchronising, 0, 1) { return errBusy @@ -226,7 +230,7 @@ func (d *Downloader) synchronise(id string, hash common.Hash) error { // If the head hash is banned, terminate immediately if d.banned.Has(hash) { - return errInvalidChain + return errBannedHead } // Post a user notification of the sync (only once per session) if atomic.CompareAndSwapInt32(&d.notified, 0, 1) { diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go index 6269ed87c..c328e7d9a 100644 --- a/eth/downloader/downloader_test.go +++ b/eth/downloader/downloader_test.go @@ -2,6 +2,7 @@ package downloader import ( "encoding/binary" + "fmt" "math/big" "testing" "time" @@ -69,7 +70,7 @@ func newTester() *downloadTester { peerBlocks: make(map[string]map[common.Hash]*types.Block), } var mux event.TypeMux - downloader := New(&mux, tester.hasBlock, tester.getBlock, nil) + downloader := New(&mux, tester.hasBlock, tester.getBlock, tester.dropPeer) tester.downloader = downloader return tester @@ -130,6 +131,14 @@ func (dl *downloadTester) newPeer(id string, hashes []common.Hash, blocks map[co return err } +// dropPeer simulates a hard peer removal from the connection pool. +func (dl *downloadTester) dropPeer(id string) { + delete(dl.peerHashes, id) + delete(dl.peerBlocks, id) + + dl.downloader.UnregisterPeer(id) +} + // peerGetBlocksFn constructs a getHashes function associated with a particular // peer in the download tester. The returned function can be used to retrieve // batches of hashes from the particularly requested peer. @@ -544,14 +553,14 @@ func TestBannedChainStarvationAttack(t *testing.T) { for banned := tester.downloader.banned.Size(); ; { // Try to sync with the attacker, check hash chain failure if _, err := tester.syncTake("attack", hashes[0]); err != errInvalidChain { + if tester.downloader.banned.Has(hashes[0]) && err == errBannedHead { + break + } t.Fatalf("synchronisation error mismatch: have %v, want %v", err, errInvalidChain) } // Check that the ban list grew with at least 1 new item, or all banned bans := tester.downloader.banned.Size() if bans < banned+1 { - if tester.downloader.banned.Has(hashes[0]) { - break - } t.Fatalf("ban count mismatch: have %v, want %v+", bans, banned+1) } banned = bans @@ -606,3 +615,47 @@ func TestBannedChainMemoryExhaustionAttack(t *testing.T) { } } } + +// Tests that misbehaving peers are disconnected, whilst behaving ones are not. +func TestAttackerDropping(t *testing.T) { + // Define the disconnection requirement for individual errors + tests := []struct { + result error + drop bool + }{ + {nil, false}, // Sync succeeded, all is well + {errBusy, false}, // Sync is already in progress, no problem + {errUnknownPeer, false}, // Peer is unknown, was already dropped, don't double drop + {errBadPeer, true}, // Peer was deemed bad for some reason, drop it + {errStallingPeer, true}, // Peer was detected to be stalling, drop it + {errBannedHead, true}, // Peer's head hash is a known bad hash, drop it + {errNoPeers, false}, // No peers to download from, soft race, no issue + {errPendingQueue, false}, // There are blocks still cached, wait to exhaust, no issue + {errTimeout, true}, // No hashes received in due time, drop the peer + {errEmptyHashSet, true}, // No hashes were returned as a response, drop as it's a dead end + {errPeersUnavailable, true}, // Nobody had the advertised blocks, drop the advertiser + {errInvalidChain, true}, // Hash chain was detected as invalid, definitely drop + {errCrossCheckFailed, true}, // Hash-origin failed to pass a block cross check, drop + {errCancelHashFetch, false}, // Synchronisation was canceled, origin may be innocent, don't drop + {errCancelBlockFetch, false}, // Synchronisation was canceled, origin may be innocent, don't drop + } + // Run the tests and check disconnection status + tester := newTester() + for i, tt := range tests { + // Register a new peer and ensure it's presence + id := fmt.Sprintf("test %d", i) + if err := tester.newPeer(id, []common.Hash{knownHash}, nil); err != nil { + t.Fatalf("test %d: failed to register new peer: %v", i, err) + } + if _, ok := tester.peerHashes[id]; !ok { + t.Fatalf("test %d: registered peer not found", i) + } + // Simulate a synchronisation and check the required result + tester.downloader.synchroniseMock = func(string, common.Hash) error { return tt.result } + + tester.downloader.Synchronise(id, knownHash) + if _, ok := tester.peerHashes[id]; !ok != tt.drop { + t.Errorf("test %d: peer drop mismatch for %v: have %v, want %v", i, tt.result, !ok, tt.drop) + } + } +}