eth: upgrade the block header query packet to new spec

eth/handler.go

@@ -336,29 +336,67 @@ func (pm *ProtocolManager) handleMsg(p *peer) error {
 			pm.downloader.DeliverBlocks(p.id, blocks)
 		}
 
+	// Block header query, collect the requested headers and reply
 	case p.version >= eth62 && msg.Code == GetBlockHeadersMsg:
-		// Decode the retrieval message
+		// Decode the complex header query
-		msgStream := rlp.NewStream(msg.Payload, uint64(msg.Size))
+		var query getBlockHeadersData
-		if _, err := msgStream.List(); err != nil {
+		if err := msg.Decode(&query); err != nil {
-			return err
+			return errResp(ErrDecode, "%v: %v", msg, err)
 		}
 		// Gather blocks until the fetch or network limits is reached
 		var (
-			hash    common.Hash
 			bytes   common.StorageSize
 			headers []*types.Header
+			unknown bool
 		)
-		for bytes < softResponseLimit && len(headers) < downloader.MaxHeaderFetch {
+		for !unknown && len(headers) < int(query.Amount) && bytes < softResponseLimit && len(headers) < downloader.MaxHeaderFetch {
-			//Retrieve the hash of the next block
+			// Retrieve the next block satisfying the query
-			if err := msgStream.Decode(&hash); err == rlp.EOL {
+			var origin *types.Block
-				break
+			if query.Origin.Hash != (common.Hash{}) {
-			} else if err != nil {
+				origin = pm.chainman.GetBlock(query.Origin.Hash)
-				return errResp(ErrDecode, "msg %v: %v", msg, err)
+			} else {
+				origin = pm.chainman.GetBlockByNumber(query.Origin.Number)
 			}
-			// Retrieve the requested block, stopping if enough was found
+			if origin == nil {
-			if block := pm.chainman.GetBlock(hash); block != nil {
+				break
-				headers = append(headers, block.Header())
+			}
-				bytes += block.Size()
+			headers = append(headers, origin.Header())
+			bytes += origin.Size()
+
+			// Advance to the next block of the query
+			switch {
+			case query.Origin.Hash != (common.Hash{}) && query.Reverse:
+				// Hash based traversal towards the genesis block
+				for i := 0; i < int(query.Skip)+1; i++ {
+					if block := pm.chainman.GetBlock(query.Origin.Hash); block != nil {
+						query.Origin.Hash = block.ParentHash()
+					} else {
+						unknown = true
+						break
+					}
+				}
+			case query.Origin.Hash != (common.Hash{}) && !query.Reverse:
+				// Hash based traversal towards the leaf block
+				if block := pm.chainman.GetBlockByNumber(origin.NumberU64() + query.Skip + 1); block != nil {
+					if pm.chainman.GetBlockHashesFromHash(block.Hash(), query.Skip+1)[query.Skip] == query.Origin.Hash {
+						query.Origin.Hash = block.Hash()
+					} else {
+						unknown = true
+					}
+				} else {
+					unknown = true
+				}
+			case query.Reverse:
+				// Number based traversal towards the genesis block
+				if query.Origin.Number >= query.Skip+1 {
+					query.Origin.Number -= (query.Skip + 1)
+				} else {
+					unknown = true
+				}
+
+			case !query.Reverse:
+				// Number based traversal towards the leaf block
+				query.Origin.Number += (query.Skip + 1)
 			}
 		}
 		return p.SendBlockHeaders(headers)

eth/handler_test.go

@@ -174,7 +174,7 @@ func testGetBlocks(t *testing.T, protocol int) {
 	}
 }
 
-// Tests that block headers can be retrieved from a remote chain based on their hashes.
+// Tests that block headers can be retrieved from a remote chain based on user queries.
 func TestGetBlockHeaders62(t *testing.T) { testGetBlockHeaders(t, 62) }
 func TestGetBlockHeaders63(t *testing.T) { testGetBlockHeaders(t, 63) }
 func TestGetBlockHeaders64(t *testing.T) { testGetBlockHeaders(t, 64) }
@@ -184,62 +184,116 @@ func testGetBlockHeaders(t *testing.T, protocol int) {
 	peer, _ := newTestPeer("peer", protocol, pm, true)
 	defer peer.close()
 
+	// Create a "random" unknown hash for testing
+	var unknown common.Hash
+	for i, _ := range unknown {
+		unknown[i] = byte(i)
+	}
 	// Create a batch of tests for various scenarios
-	limit := downloader.MaxHeaderFetch
+	limit := uint64(downloader.MaxHeaderFetch)
 	tests := []struct {
-		random    int           // Number of blocks to fetch randomly from the chain
+		query  *getBlockHeadersData // The query to execute for header retrieval
-		explicit  []common.Hash // Explicitly requested blocks
+		expect []common.Hash        // The hashes of the block whose headers are expected
-		available []bool        // Availability of explicitly requested blocks
-		expected  int           // Total number of existing blocks to expect
 	}{
-		{1, nil, nil, 1},                                                       // A single random block should be retrievable
+		// A single random block should be retrievable by hash and number too
-		{10, nil, nil, 10},                                                     // Multiple random blocks should be retrievable
+		{
-		{limit, nil, nil, limit},                                               // The maximum possible blocks should be retrievable
+			&getBlockHeadersData{Origin: hashOrNumber{Hash: pm.chainman.GetBlockByNumber(limit / 2).Hash()}, Amount: 1},
-		{limit + 1, nil, nil, limit},                                           // No more that the possible block count should be returned
+			[]common.Hash{pm.chainman.GetBlockByNumber(limit / 2).Hash()},
-		{0, []common.Hash{pm.chainman.Genesis().Hash()}, []bool{true}, 1},      // The genesis block should be retrievable
+		}, {
-		{0, []common.Hash{pm.chainman.CurrentBlock().Hash()}, []bool{true}, 1}, // The chains head block should be retrievable
+			&getBlockHeadersData{Origin: hashOrNumber{Number: limit / 2}, Amount: 1},
-		{0, []common.Hash{common.Hash{}}, []bool{false}, 0},                    // A non existent block should not be returned
+			[]common.Hash{pm.chainman.GetBlockByNumber(limit / 2).Hash()},
-
+		},
-		// Existing and non-existing blocks interleaved should not cause problems
+		// Multiple headers should be retrievable in both directions
-		{0, []common.Hash{
+		{
-			common.Hash{},
+			&getBlockHeadersData{Origin: hashOrNumber{Number: limit / 2}, Amount: 3},
+			[]common.Hash{
+				pm.chainman.GetBlockByNumber(limit / 2).Hash(),
+				pm.chainman.GetBlockByNumber(limit/2 + 1).Hash(),
+				pm.chainman.GetBlockByNumber(limit/2 + 2).Hash(),
+			},
+		}, {
+			&getBlockHeadersData{Origin: hashOrNumber{Number: limit / 2}, Amount: 3, Reverse: true},
+			[]common.Hash{
+				pm.chainman.GetBlockByNumber(limit / 2).Hash(),
+				pm.chainman.GetBlockByNumber(limit/2 - 1).Hash(),
+				pm.chainman.GetBlockByNumber(limit/2 - 2).Hash(),
+			},
+		},
+		// Multiple headers with skip lists should be retrievable
+		{
+			&getBlockHeadersData{Origin: hashOrNumber{Number: limit / 2}, Skip: 3, Amount: 3},
+			[]common.Hash{
+				pm.chainman.GetBlockByNumber(limit / 2).Hash(),
+				pm.chainman.GetBlockByNumber(limit/2 + 4).Hash(),
+				pm.chainman.GetBlockByNumber(limit/2 + 8).Hash(),
+			},
+		}, {
+			&getBlockHeadersData{Origin: hashOrNumber{Number: limit / 2}, Skip: 3, Amount: 3, Reverse: true},
+			[]common.Hash{
+				pm.chainman.GetBlockByNumber(limit / 2).Hash(),
+				pm.chainman.GetBlockByNumber(limit/2 - 4).Hash(),
+				pm.chainman.GetBlockByNumber(limit/2 - 8).Hash(),
+			},
+		},
+		// The chain endpoints should be retrievable
+		{
+			&getBlockHeadersData{Origin: hashOrNumber{Number: 0}, Amount: 1},
+			[]common.Hash{pm.chainman.GetBlockByNumber(0).Hash()},
+		}, {
+			&getBlockHeadersData{Origin: hashOrNumber{Number: pm.chainman.CurrentBlock().NumberU64()}, Amount: 1},
+			[]common.Hash{pm.chainman.CurrentBlock().Hash()},
+		},
+		// Ensure protocol limits are honored
+		{
+			&getBlockHeadersData{Origin: hashOrNumber{Number: pm.chainman.CurrentBlock().NumberU64() - 1}, Amount: limit + 10, Reverse: true},
+			pm.chainman.GetBlockHashesFromHash(pm.chainman.CurrentBlock().Hash(), limit),
+		},
+		// Check that requesting more than available is handled gracefully
+		{
+			&getBlockHeadersData{Origin: hashOrNumber{Number: pm.chainman.CurrentBlock().NumberU64() - 4}, Skip: 3, Amount: 3},
+			[]common.Hash{
+				pm.chainman.GetBlockByNumber(pm.chainman.CurrentBlock().NumberU64() - 4).Hash(),
+				pm.chainman.GetBlockByNumber(pm.chainman.CurrentBlock().NumberU64()).Hash(),
+			},
+		}, {
+			&getBlockHeadersData{Origin: hashOrNumber{Number: 4}, Skip: 3, Amount: 3, Reverse: true},
+			[]common.Hash{
+				pm.chainman.GetBlockByNumber(4).Hash(),
+				pm.chainman.GetBlockByNumber(0).Hash(),
+			},
+		},
+		// Check that requesting more than available is handled gracefully, even if mid skip
+		{
+			&getBlockHeadersData{Origin: hashOrNumber{Number: pm.chainman.CurrentBlock().NumberU64() - 4}, Skip: 2, Amount: 3},
+			[]common.Hash{
+				pm.chainman.GetBlockByNumber(pm.chainman.CurrentBlock().NumberU64() - 4).Hash(),
+				pm.chainman.GetBlockByNumber(pm.chainman.CurrentBlock().NumberU64() - 1).Hash(),
+			},
+		}, {
+			&getBlockHeadersData{Origin: hashOrNumber{Number: 4}, Skip: 2, Amount: 3, Reverse: true},
+			[]common.Hash{
+				pm.chainman.GetBlockByNumber(4).Hash(),
 				pm.chainman.GetBlockByNumber(1).Hash(),
-			common.Hash{},
+			},
-			pm.chainman.GetBlockByNumber(10).Hash(),
+		},
-			common.Hash{},
+		// Check that non existing headers aren't returned
-			pm.chainman.GetBlockByNumber(100).Hash(),
+		{
-			common.Hash{},
+			&getBlockHeadersData{Origin: hashOrNumber{Hash: unknown}, Amount: 1},
-		}, []bool{false, true, false, true, false, true, false}, 3},
+			[]common.Hash{},
+		}, {
+			&getBlockHeadersData{Origin: hashOrNumber{Number: pm.chainman.CurrentBlock().NumberU64() + 1}, Amount: 1},
+			[]common.Hash{},
+		},
 	}
 	// Run each of the tests and verify the results against the chain
 	for i, tt := range tests {
-		// Collect the hashes to request, and the response to expect
+		// Collect the headers to expect in the response
-		hashes, seen := []common.Hash{}, make(map[int64]bool)
 		headers := []*types.Header{}
-
+		for _, hash := range tt.expect {
-		for j := 0; j < tt.random; j++ {
-			for {
-				num := rand.Int63n(int64(pm.chainman.CurrentBlock().NumberU64()))
-				if !seen[num] {
-					seen[num] = true
-
-					block := pm.chainman.GetBlockByNumber(uint64(num))
-					hashes = append(hashes, block.Hash())
-					if len(headers) < tt.expected {
-						headers = append(headers, block.Header())
-					}
-					break
-				}
-			}
-		}
-		for j, hash := range tt.explicit {
-			hashes = append(hashes, hash)
-			if tt.available[j] && len(headers) < tt.expected {
 			headers = append(headers, pm.chainman.GetBlock(hash).Header())
 		}
-		}
 		// Send the hash request and verify the response
-		p2p.Send(peer.app, 0x03, hashes)
+		p2p.Send(peer.app, 0x03, tt.query)
 		if err := p2p.ExpectMsg(peer.app, 0x04, headers); err != nil {
 			t.Errorf("test %d: headers mismatch: %v", i, err)
 		}

eth/protocol.go

@@ -17,10 +17,13 @@
 package eth
 
 import (
+	"fmt"
+	"io"
 	"math/big"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/rlp"
 )
 
 // Constants to match up protocol versions and messages
@@ -153,6 +156,50 @@ type getBlockHashesFromNumberData struct {
 	Amount uint64
 }
 
+// getBlockHeadersData represents a block header query.
+type getBlockHeadersData struct {
+	Origin  hashOrNumber // Block from which to retrieve headers
+	Amount  uint64       // Maximum number of headers to retrieve
+	Skip    uint64       // Blocks to skip between consecutive headers
+	Reverse bool         // Query direction (false = rising towards latest, true = falling towards genesis)
+}
+
+// hashOrNumber is a combined field for specifying an origin block.
+type hashOrNumber struct {
+	Hash   common.Hash // Block hash from which to retrieve headers (excludes Number)
+	Number uint64      // Block hash from which to retrieve headers (excludes Hash)
+}
+
+// EncodeRLP is a specialized encoder for hashOrNumber to encode only one of the
+// two contained union fields.
+func (hn *hashOrNumber) EncodeRLP(w io.Writer) error {
+	if hn.Hash == (common.Hash{}) {
+		return rlp.Encode(w, hn.Number)
+	}
+	if hn.Number != 0 {
+		return fmt.Errorf("both origin hash (%x) and number (%d) provided", hn.Hash, hn.Number)
+	}
+	return rlp.Encode(w, hn.Hash)
+}
+
+// DecodeRLP is a specialized decoder for hashOrNumber to decode the contents
+// into either a block hash or a block number.
+func (hn *hashOrNumber) DecodeRLP(s *rlp.Stream) error {
+	_, size, _ := s.Kind()
+	origin, err := s.Raw()
+	if err == nil {
+		switch {
+		case size == 32:
+			err = rlp.DecodeBytes(origin, &hn.Hash)
+		case size <= 8:
+			err = rlp.DecodeBytes(origin, &hn.Number)
+		default:
+			err = fmt.Errorf("invalid input size %d for origin", size)
+		}
+	}
+	return err
+}
+
 // newBlockData is the network packet for the block propagation message.
 type newBlockData struct {
 	Block *types.Block

eth/protocol_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/crypto"
 	"github.com/ethereum/go-ethereum/p2p"
+	"github.com/ethereum/go-ethereum/rlp"
 )
 
 func init() {
@@ -181,3 +182,47 @@ func testSendTransactions(t *testing.T, protocol int) {
 	}
 	wg.Wait()
 }
+
+// Tests that the custom union field encoder and decoder works correctly.
+func TestGetBlockHeadersDataEncodeDecode(t *testing.T) {
+	// Create a "random" hash for testing
+	var hash common.Hash
+	for i, _ := range hash {
+		hash[i] = byte(i)
+	}
+	// Assemble some table driven tests
+	tests := []struct {
+		packet *getBlockHeadersData
+		fail   bool
+	}{
+		// Providing the origin as either a hash or a number should both work
+		{fail: false, packet: &getBlockHeadersData{Origin: hashOrNumber{Number: 314}}},
+		{fail: false, packet: &getBlockHeadersData{Origin: hashOrNumber{Hash: hash}}},
+
+		// Providing arbitrary query field should also work
+		{fail: false, packet: &getBlockHeadersData{Origin: hashOrNumber{Number: 314}, Amount: 314, Skip: 1, Reverse: true}},
+		{fail: false, packet: &getBlockHeadersData{Origin: hashOrNumber{Hash: hash}, Amount: 314, Skip: 1, Reverse: true}},
+
+		// Providing both the origin hash and origin number must fail
+		{fail: true, packet: &getBlockHeadersData{Origin: hashOrNumber{Hash: hash, Number: 314}}},
+	}
+	// Iterate over each of the tests and try to encode and then decode
+	for i, tt := range tests {
+		bytes, err := rlp.EncodeToBytes(tt.packet)
+		if err != nil && !tt.fail {
+			t.Fatalf("test %d: failed to encode packet: %v", i, err)
+		} else if err == nil && tt.fail {
+			t.Fatalf("test %d: encode should have failed", i)
+		}
+		if !tt.fail {
+			packet := new(getBlockHeadersData)
+			if err := rlp.DecodeBytes(bytes, packet); err != nil {
+				t.Fatalf("test %d: failed to decode packet: %v", i, err)
+			}
+			if packet.Origin.Hash != tt.packet.Origin.Hash || packet.Origin.Number != tt.packet.Origin.Number || packet.Amount != tt.packet.Amount ||
+				packet.Skip != tt.packet.Skip || packet.Reverse != tt.packet.Reverse {
+				t.Fatalf("test %d: encode decode mismatch: have %+v, want %+v", i, packet, tt.packet)
+			}
+		}
+	}
+}