improve documentation and move one test

blockpool/blockpool_test.go

@@ -9,6 +9,9 @@ import (
 	"github.com/ethereum/go-ethereum/core/types"
 )
 
+// using the mock framework in blockpool_util_test
+// we test various scenarios here
+
 func TestPeerWithKnownBlock(t *testing.T) {
 	test.LogInit()
 	_, blockPool, blockPoolTester := newTestBlockPool(t)
@@ -44,50 +47,6 @@ func TestPeerWithKnownParentBlock(t *testing.T) {
 	blockPoolTester.checkBlockChain(blockPoolTester.refBlockChain)
 }
 
-func TestPeerPromotionByOptionalTdOnBlock(t *testing.T) {
-	test.LogInit()
-	_, blockPool, blockPoolTester := newTestBlockPool(t)
-	blockPoolTester.blockChain[0] = nil
-	blockPoolTester.initRefBlockChain(4)
-	peer0 := blockPoolTester.newPeer("peer0", 2, 2)
-	peer1 := blockPoolTester.newPeer("peer1", 1, 1)
-	peer2 := blockPoolTester.newPeer("peer2", 4, 4)
-
-	blockPool.Start()
-	blockPoolTester.tds = make(map[int]int)
-	blockPoolTester.tds[3] = 3
-
-	// pool
-	peer0.AddPeer()
-	peer0.serveBlocks(1, 2)
-	best := peer1.AddPeer()
-	// this tests that peer1 is not promoted over peer0 yet
-	if best {
-		t.Errorf("peer1 (TD=1) should not be set as best")
-	}
-	best = peer2.AddPeer()
-	peer2.serveBlocks(3, 4)
-	peer2.serveBlockHashes(4, 3, 2, 1)
-	hashes := blockPoolTester.hashPool.IndexesToHashes([]int{2, 3})
-	peer1.waitBlocksRequests(3)
-	blockPool.AddBlock(&types.Block{
-		HeaderHash:       common.Hash(hashes[1]),
-		ParentHeaderHash: common.Hash(hashes[0]),
-		Td:               common.Big3,
-	}, "peer1")
-
-	blockPool.RemovePeer("peer2")
-	if blockPool.peers.best.id != "peer1" {
-		t.Errorf("peer1 (TD=3) should be set as best")
-	}
-	peer1.serveBlocks(0, 1, 2)
-
-	blockPool.Wait(waitTimeout)
-	blockPool.Stop()
-	blockPoolTester.refBlockChain[4] = []int{}
-	blockPoolTester.checkBlockChain(blockPoolTester.refBlockChain)
-}
-
 func TestSimpleChain(t *testing.T) {
 	test.LogInit()
 	_, blockPool, blockPoolTester := newTestBlockPool(t)
@@ -166,6 +125,7 @@ func TestNewBlocksOnPartialChain(t *testing.T) {
 	go peer1.serveBlocks(4, 5) // partially complete section
 	go peer1.serveBlockHashes(5, 4, 3)
 	peer1.serveBlocks(3, 4) // partially complete section
+
 	// peer1 found new blocks
 	peer1.td = 7
 	peer1.currentBlock = 7
@@ -176,7 +136,6 @@ func TestNewBlocksOnPartialChain(t *testing.T) {
 	go peer1.serveBlocks(5, 6)
 	go peer1.serveBlockHashes(3, 2, 1) // tests that hash request from known chain root is remembered
 	peer1.serveBlocks(0, 1, 2)
-	// blockPool.RemovePeer("peer1")
 
 	blockPool.Wait(waitTimeout)
 	blockPool.Stop()
@@ -468,8 +427,8 @@ func TestForkCompleteSectionSwitchBackByPeerSwitchBack(t *testing.T) {
 	peer1.AddPeer()
 	go peer1.serveBlocks(8, 9)
 	go peer1.serveBlockHashes(9, 8, 7)
-	peer1.serveBlocks(3, 7, 8) // make sure this section is complete
-	time.Sleep(1 * time.Second)
+	peer1.serveBlocks(3, 7, 8)               // make sure this section is complete
+	time.Sleep(1 * time.Second)              //
 	go peer1.serveBlockHashes(7, 3, 2)       // block 3/7 is section boundary
 	peer1.serveBlocks(2, 3)                  // partially complete sections block 2 missing
 	peer2.AddPeer()                          //
@@ -477,8 +436,7 @@ func TestForkCompleteSectionSwitchBackByPeerSwitchBack(t *testing.T) {
 	go peer2.serveBlockHashes(6, 5, 4, 3, 2) // peer2 forks on block 3
 	peer2.serveBlocks(2, 3, 4, 5)            // block 2 still missing.
 	blockPool.RemovePeer("peer2")            // peer2 disconnects, peer1 is promoted again as best peer
-	// peer1.serveBlockHashes(7, 3)             // tests that hash request from fork root is remembered even though section process completed
-	go peer1.serveBlockHashes(2, 1, 0) //
+	go peer1.serveBlockHashes(2, 1, 0)       //
 	peer1.serveBlocks(0, 1, 2)
 
 	blockPool.Wait(waitTimeout)

blockpool/blockpool_util_test.go

@@ -66,7 +66,8 @@ func (self *blockPoolTester) Errorf(format string, params ...interface{}) {
 }
 
 // blockPoolTester implements the 3 callbacks needed by the blockPool:
-// hasBlock, insetChain, verifyPoW
+// hasBlock, insetChain, verifyPoW as well as provides the eventer
+// to subscribe to head insertions
 func (self *blockPoolTester) hasBlock(block common.Hash) (ok bool) {
 	self.lock.RLock()
 	defer self.lock.RUnlock()
@@ -77,6 +78,7 @@ func (self *blockPoolTester) hasBlock(block common.Hash) (ok bool) {
 	return
 }
 
+// mock insertChain relies on refBlockChain to determine block validity
 func (self *blockPoolTester) insertChain(blocks types.Blocks) error {
 	self.lock.Lock()
 	defer self.lock.Unlock()
@@ -127,6 +129,7 @@ func (self *blockPoolTester) insertChain(blocks types.Blocks) error {
 	return nil
 }
 
+// mock soft block validation always succeeds
 func (self *blockPoolTester) verifyPoW(pblock pow.Block) bool {
 	return true
 }
@@ -152,24 +155,24 @@ func (self *blockPoolTester) checkBlockChain(blockChain map[int][]int) {
 	}
 }
 
-//
-
 // peerTester provides the peer callbacks for the blockPool
 // it registers actual callbacks so that the result can be compared to desired behaviour
 // provides helper functions to mock the protocol calls to the blockPool
 type peerTester struct {
+	// containers to record request and error callbacks
 	blockHashesRequests []int
 	blocksRequests      [][]int
 	blocksRequestsMap   map[int]bool
 	peerErrors          []int
-	blockPool           *BlockPool
-	hashPool            *test.TestHashPool
-	lock                sync.RWMutex
-	bt                  *blockPoolTester
-	id                  string
-	td                  int
-	currentBlock        int
-	t                   *testing.T
+
+	blockPool    *BlockPool
+	hashPool     *test.TestHashPool
+	lock         sync.RWMutex
+	bt           *blockPoolTester
+	id           string
+	td           int
+	currentBlock int
+	t            *testing.T
 }
 
 // peerTester constructor takes hashPool and blockPool from the blockPoolTester
@@ -222,6 +225,7 @@ func (self *peerTester) checkBlockHashesRequests(blocksHashesRequests ...int) {
 
 // waiter function used by peer.serveBlocks
 // blocking until requests appear
+// this mocks proper wire protocol behaviour
 // since block requests are sent to any random peers
 // block request map is shared between peers
 // times out after waitTimeout
@@ -254,6 +258,7 @@ func (self *peerTester) waitBlocksRequests(blocksRequest ...int) {
 
 // waiter function used by peer.serveBlockHashes
 // blocking until requests appear
+// this mocks proper wire protocol behaviour
 // times out after a period
 func (self *peerTester) waitBlockHashesRequests(blocksHashesRequest int) {
 	timeout := time.After(waitTimeout)
@@ -299,6 +304,7 @@ func (self *peerTester) serveBlockHashes(indexes ...int) {
 	self.sendBlockHashes(indexes...)
 }
 
+// peer sends blockhashes not waiting for request
 func (self *peerTester) sendBlockHashes(indexes ...int) {
 	// fmt.Printf("adding block hashes %v\n", indexes)
 	hashes := self.hashPool.IndexesToHashes(indexes)
@@ -315,13 +321,14 @@ func (self *peerTester) sendBlockHashes(indexes ...int) {
 }
 
 // peer sends blocks if and when there is a request
-// (in the shared request store, not necessarily to a person)
+// (in the shared request store, not necessarily to a specific peer)
 func (self *peerTester) serveBlocks(indexes ...int) {
 	// fmt.Printf("ready to serve blocks %v\n", indexes[1:])
 	self.waitBlocksRequests(indexes[1:]...)
 	self.sendBlocks(indexes...)
 }
 
+// peer sends blocks not waiting for request
 func (self *peerTester) sendBlocks(indexes ...int) {
 	// fmt.Printf("adding blocks %v \n", indexes)
 	hashes := self.hashPool.IndexesToHashes(indexes)
@@ -331,9 +338,10 @@ func (self *peerTester) sendBlocks(indexes ...int) {
 	}
 }
 
-// peer callbacks
-// -1 is special: not found (a hash never seen)
+// the 3 mock peer callbacks
+
 // records block hashes requests by the blockPool
+// -1 is special: not found (a hash never seen)
 func (self *peerTester) requestBlockHashes(hash common.Hash) error {
 	indexes := self.hashPool.HashesToIndexes([]common.Hash{hash})
 	// fmt.Printf("[%s] block hash request %v %x\n", self.id, indexes[0], hash[:4])

blockpool/config_test.go

@@ -23,12 +23,13 @@ func TestBlockPoolConfig(t *testing.T) {
 	test.CheckDuration("BlocksTimeout", c.BlocksTimeout, blocksTimeout, t)
 	test.CheckDuration("IdleBestPeerTimeout", c.IdleBestPeerTimeout, idleBestPeerTimeout, t)
 	test.CheckDuration("PeerSuspensionInterval", c.PeerSuspensionInterval, peerSuspensionInterval, t)
+	test.CheckDuration("StatusUpdateInterval", c.StatusUpdateInterval, statusUpdateInterval, t)
 }
 
 func TestBlockPoolOverrideConfig(t *testing.T) {
 	test.LogInit()
 	blockPool := &BlockPool{Config: &Config{}, chainEvents: &event.TypeMux{}}
-	c := &Config{128, 32, 1, 0, 300 * time.Millisecond, 100 * time.Millisecond, 90 * time.Second, 0, 30 * time.Second, 30 * time.Second}
+	c := &Config{128, 32, 1, 0, 300 * time.Millisecond, 100 * time.Millisecond, 90 * time.Second, 0, 30 * time.Second, 30 * time.Second, 4 * time.Second}
 
 	blockPool.Config = c
 	blockPool.Start()
@@ -42,4 +43,5 @@ func TestBlockPoolOverrideConfig(t *testing.T) {
 	test.CheckDuration("BlocksTimeout", c.BlocksTimeout, blocksTimeout, t)
 	test.CheckDuration("IdleBestPeerTimeout", c.IdleBestPeerTimeout, 30*time.Second, t)
 	test.CheckDuration("PeerSuspensionInterval", c.PeerSuspensionInterval, 30*time.Second, t)
+	test.CheckDuration("StatusUpdateInterval", c.StatusUpdateInterval, 4*time.Second, t)
 }

blockpool/peers.go

@@ -11,6 +11,7 @@ import (
 	"github.com/ethereum/go-ethereum/errs"
 )
 
+// the blockpool's model of a peer
 type peer struct {
 	lock sync.RWMutex
 
@@ -104,12 +105,14 @@ func (self *peers) peerError(id string, code int, format string, params ...inter
 	self.addToBlacklist(id)
 }
 
+// record time of offence in blacklist to implement suspension for PeerSuspensionInterval
 func (self *peers) addToBlacklist(id string) {
 	self.lock.Lock()
 	defer self.lock.Unlock()
 	self.blacklist[id] = time.Now()
 }
 
+// suspended checks if peer is still suspended
 func (self *peers) suspended(id string) (s bool) {
 	self.lock.Lock()
 	defer self.lock.Unlock()
@@ -160,8 +163,8 @@ func (self *peer) setChainInfoFromBlock(block *types.Block) {
 	}()
 }
 
+// distribute block request among known peers
 func (self *peers) requestBlocks(attempts int, hashes []common.Hash) {
-	// distribute block request among known peers
 	self.lock.RLock()
 	defer self.lock.RUnlock()
 	peerCount := len(self.peers)
@@ -196,7 +199,9 @@ func (self *peers) requestBlocks(attempts int, hashes []common.Hash) {
 }
 
 // addPeer implements the logic for blockpool.AddPeer
-// returns true iff peer is promoted as best peer in the pool
+// returns 2 bool values
+// 1. true iff peer is promoted as best peer in the pool
+// 2. true iff peer is still suspended
 func (self *peers) addPeer(
 	td *big.Int,
 	currentBlockHash common.Hash,
@@ -214,10 +219,13 @@ func (self *peers) addPeer(
 	self.lock.Lock()
 	p, found := self.peers[id]
 	if found {
+		// when called on an already connected peer, it means a newBlockMsg is received
+		// peer head info is updated
 		if p.currentBlockHash != currentBlockHash {
 			previousBlockHash = p.currentBlockHash
 			plog.Debugf("addPeer: Update peer <%s> with td %v and current block %s (was %v)", id, td, hex(currentBlockHash), hex(previousBlockHash))
 			p.setChainInfo(td, currentBlockHash)
+
 			self.status.lock.Lock()
 			self.status.values.NewBlocks++
 			self.status.lock.Unlock()
@@ -235,7 +243,7 @@ func (self *peers) addPeer(
 	}
 	self.lock.Unlock()
 
-	// check peer current head
+	// check if peer's current head block is known
 	if self.bp.hasBlock(currentBlockHash) {
 		// peer not ahead
 		plog.Debugf("addPeer: peer <%v> with td %v and current block %s is behind", id, td, hex(currentBlockHash))
@@ -255,6 +263,7 @@ func (self *peers) addPeer(
 		}
 		best = true
 	} else {
+		// baseline is our own TD
 		currentTD := self.bp.getTD()
 		if self.best != nil {
 			currentTD = self.best.td
@@ -314,6 +323,7 @@ func (self *peers) removePeer(id string) {
 func (self *BlockPool) switchPeer(oldp, newp *peer) {
 
 	// first quit AddBlockHashes, requestHeadSection and activateChain
+	// by closing the old peer's switchC channel
 	if oldp != nil {
 		plog.DebugDetailf("<%s> quit peer processes", oldp.id)
 		close(oldp.switchC)
@@ -366,11 +376,12 @@ func (self *BlockPool) switchPeer(oldp, newp *peer) {
 	// newp activating section process changes the quit channel for this reason
 	if oldp != nil {
 		plog.DebugDetailf("<%s> quit section processes", oldp.id)
-		//
 		close(oldp.idleC)
 	}
 }
 
+// getPeer looks up peer by id, returns peer and a bool value
+// that is true iff peer is current best peer
 func (self *peers) getPeer(id string) (p *peer, best bool) {
 	self.lock.RLock()
 	defer self.lock.RUnlock()
@@ -381,6 +392,8 @@ func (self *peers) getPeer(id string) (p *peer, best bool) {
 	return
 }
 
+// head section process
+
 func (self *peer) handleSection(sec *section) {
 	self.lock.Lock()
 	defer self.lock.Unlock()
@@ -516,7 +529,7 @@ func (self *peer) run() {
 LOOP:
 	for {
 		select {
-		// to minitor section process behaviou
+		// to minitor section process behaviour
 		case <-ping.C:
 			plog.Debugf("HeadSection: <%s> section with head %s, idle: %v", self.id, hex(self.currentBlockHash), self.idle)
 

blockpool/peers_test.go

@@ -142,3 +142,47 @@ func TestAddPeer(t *testing.T) {
 
 	blockPool.Stop()
 }
+
+func TestPeerPromotionByOptionalTdOnBlock(t *testing.T) {
+	test.LogInit()
+	_, blockPool, blockPoolTester := newTestBlockPool(t)
+	blockPoolTester.blockChain[0] = nil
+	blockPoolTester.initRefBlockChain(4)
+	peer0 := blockPoolTester.newPeer("peer0", 2, 2)
+	peer1 := blockPoolTester.newPeer("peer1", 1, 1)
+	peer2 := blockPoolTester.newPeer("peer2", 4, 4)
+
+	blockPool.Start()
+	blockPoolTester.tds = make(map[int]int)
+	blockPoolTester.tds[3] = 3
+
+	// pool
+	peer0.AddPeer()
+	peer0.serveBlocks(1, 2)
+	best := peer1.AddPeer()
+	// this tests that peer1 is not promoted over peer0 yet
+	if best {
+		t.Errorf("peer1 (TD=1) should not be set as best")
+	}
+	best = peer2.AddPeer()
+	peer2.serveBlocks(3, 4)
+	peer2.serveBlockHashes(4, 3, 2, 1)
+	hashes := blockPoolTester.hashPool.IndexesToHashes([]int{2, 3})
+	peer1.waitBlocksRequests(3)
+	blockPool.AddBlock(&types.Block{
+		HeaderHash:       common.Bytes(hashes[1]),
+		ParentHeaderHash: common.Bytes(hashes[0]),
+		Td:               common.Big3,
+	}, "peer1")
+
+	blockPool.RemovePeer("peer2")
+	if blockPool.peers.best.id != "peer1" {
+		t.Errorf("peer1 (TD=3) should be set as best")
+	}
+	peer1.serveBlocks(0, 1, 2)
+
+	blockPool.Wait(waitTimeout)
+	blockPool.Stop()
+	blockPoolTester.refBlockChain[4] = []int{}
+	blockPoolTester.checkBlockChain(blockPoolTester.refBlockChain)
+}

blockpool/test/hash_pool.go

@@ -7,8 +7,12 @@ import (
 	"github.com/ethereum/go-ethereum/crypto"
 )
 
-// test helpers
-// TODO: move into common test helper package (see p2p/crypto etc.)
+// hashPool is a test helper, that allows random hashes to be referred to by integers
+type TestHashPool struct {
+	intToHash
+	hashToInt
+	lock sync.Mutex
+}
 
 func NewHashPool() *TestHashPool {
 	return &TestHashPool{intToHash: make(intToHash), hashToInt: make(hashToInt)}
@@ -18,13 +22,6 @@ type intToHash map[int]common.Hash
 
 type hashToInt map[common.Hash]int
 
-// hashPool is a test helper, that allows random hashes to be referred to by integers
-type TestHashPool struct {
-	intToHash
-	hashToInt
-	lock sync.Mutex
-}
-
 func newHash(i int) common.Hash {
 	return common.BytesToHash(crypto.Sha3([]byte(string(i))))
 }

blockpool/test/logger.go

@@ -9,6 +9,8 @@ import (
 	"github.com/ethereum/go-ethereum/logger"
 )
 
+// logging in tests
+
 var once sync.Once
 
 /* usage:

blockpool/test/util.go

@@ -6,6 +6,8 @@ import (
 	"time"
 )
 
+// miscellaneous test helpers
+
 func CheckInt(name string, got int, expected int, t *testing.T) (err error) {
 	if got != expected {
 		t.Errorf("status for %v incorrect. expected %v, got %v", name, expected, got)