discovery: properly set FirstBlockHeight and NumBlocks in responses

In this commit we fix in a bug in `lnd` that could cause other
implementations which implement a strict version of the spec to
disconnect when trying to sync their channel graph using the gossip
query feature. Before this commit, we would embed the request to a
`QueryChannelRange` in the response, causing some clients to reject the
response as the `FirstBlockHeight` and `NumBlocks` field would be
identical for each chunk of the response.

In order to remedy this, we now properly set these two fields with each
returned chunk. Note that even after this commit, we keep our existing
behavior surrounding the `Complete` field as is. Otherwise, current
`lnd` clients which rely on this field (rather than the two
aforementioned fields) wouldn't be able to properly detect when a set of
responses to their query was "complete".

Partially fixes #3728.

discovery/syncer.go

@@ -824,6 +824,14 @@ func (g *GossipSyncer) replyChanRangeQuery(query *lnwire.QueryChannelRange) erro
 	// TODO(roasbeef): means can't send max uint above?
 	//  * or make internal 64
 
+	// In the base case (no actual response) the first block and the last
+	// block in the query will be the same. In the loop below, we'll update
+	// these two variables incrementally with each chunk to properly
+	// compute the starting block for each response and the number of
+	// blocks in a response.
+	firstBlockHeight := query.FirstBlockHeight
+	lastBlockHeight := query.FirstBlockHeight
+
 	numChannels := int32(len(channelRange))
 	numChansSent := int32(0)
 	for {
@@ -854,13 +862,31 @@ func (g *GossipSyncer) replyChanRangeQuery(query *lnwire.QueryChannelRange) erro
 				"size=%v", g.cfg.peerPub[:], len(channelChunk))
 		}
 
+		// If we have any channels at all to return, then we need to
+		// update our pointers to the first and last blocks for each
+		// response.
+		if len(channelChunk) > 0 {
+			firstBlockHeight = channelChunk[0].BlockHeight
+			lastBlockHeight = channelChunk[len(channelChunk)-1].BlockHeight
+		}
+
+		// The number of blocks contained in this response (the total
+		// span) is the difference between the last channel ID and the
+		// first in the range. We add one as even if all channels
+		// returned are in the same block, we need to count that.
+		numBlocksInResp := lastBlockHeight - firstBlockHeight + 1
+
 		// With our chunk assembled, we'll now send to the remote peer
 		// the current chunk.
 		replyChunk := lnwire.ReplyChannelRange{
-			QueryChannelRange: *query,
-			Complete:          0,
-			EncodingType:      g.cfg.encodingType,
-			ShortChanIDs:      channelChunk,
+			QueryChannelRange: lnwire.QueryChannelRange{
+				ChainHash:        query.ChainHash,
+				NumBlocks:        numBlocksInResp,
+				FirstBlockHeight: firstBlockHeight,
+			},
+			Complete:     0,
+			EncodingType: g.cfg.encodingType,
+			ShortChanIDs: channelChunk,
 		}
 		if isFinalChunk {
 			replyChunk.Complete = 1

discovery/syncer_test.go

@@ -709,20 +709,31 @@ func TestGossipSyncerReplyChanRangeQuery(t *testing.T) {
 
 	// Next, we'll craft a query to ask for all the new chan ID's after
 	// block 100.
+	const startingBlockHeight int = 100
 	query := &lnwire.QueryChannelRange{
-		FirstBlockHeight: 100,
+		FirstBlockHeight: uint32(startingBlockHeight),
 		NumBlocks:        50,
 	}
 
 	// We'll then launch a goroutine to reply to the query with a set of 5
 	// responses. This will ensure we get two full chunks, and one partial
 	// chunk.
-	resp := []lnwire.ShortChannelID{
-		lnwire.NewShortChanIDFromInt(1),
-		lnwire.NewShortChanIDFromInt(2),
-		lnwire.NewShortChanIDFromInt(3),
-		lnwire.NewShortChanIDFromInt(4),
-		lnwire.NewShortChanIDFromInt(5),
+	queryResp := []lnwire.ShortChannelID{
+		{
+			BlockHeight: uint32(startingBlockHeight),
+		},
+		{
+			BlockHeight: 102,
+		},
+		{
+			BlockHeight: 104,
+		},
+		{
+			BlockHeight: 106,
+		},
+		{
+			BlockHeight: 108,
+		},
 	}
 
 	errCh := make(chan error, 1)
@@ -740,7 +751,7 @@ func TestGossipSyncerReplyChanRangeQuery(t *testing.T) {
 
 			// If the proper request was sent, then we'll respond
 			// with our set of short channel ID's.
-			chanSeries.filterRangeResp <- resp
+			chanSeries.filterRangeResp <- queryResp
 			errCh <- nil
 		}
 	}()
@@ -767,16 +778,52 @@ func TestGossipSyncerReplyChanRangeQuery(t *testing.T) {
 				t.Fatalf("expected ReplyChannelRange instead got %T", msg)
 			}
 
+			// Only for the first iteration do we set the offset to
+			// zero as no chunks have been processed yet. For every
+			// other iteration, we want to move forward by the
+			// chunkSize (from the staring block height).
+			offset := 0
+			if i != 0 {
+				offset = 1
+			}
+			expectedFirstBlockHeight := (i+offset)*2 + startingBlockHeight
+
+			switch {
 			// If this is not the last chunk, then Complete should
 			// be set to zero. Otherwise, it should be one.
-			switch {
 			case i < 2 && rangeResp.Complete != 0:
 				t.Fatalf("non-final chunk should have "+
 					"Complete=0: %v", spew.Sdump(rangeResp))
 
+			case i < 2 && rangeResp.NumBlocks != chunkSize+1:
+				t.Fatalf("NumBlocks fields in resp "+
+					"incorrect: expected %v got %v",
+					chunkSize+1, rangeResp.NumBlocks)
+
+			case i < 2 && rangeResp.FirstBlockHeight !=
+				uint32(expectedFirstBlockHeight):
+
+				t.Fatalf("FirstBlockHeight incorrect: "+
+					"expected %v got %v",
+					rangeResp.FirstBlockHeight,
+					expectedFirstBlockHeight)
 			case i == 2 && rangeResp.Complete != 1:
 				t.Fatalf("final chunk should have "+
 					"Complete=1: %v", spew.Sdump(rangeResp))
+
+			case i == 2 && rangeResp.NumBlocks != 1:
+				t.Fatalf("NumBlocks fields in resp "+
+					"incorrect: expected %v got %v", 1,
+					rangeResp.NumBlocks)
+
+			case i == 2 && rangeResp.FirstBlockHeight !=
+				queryResp[len(queryResp)-1].BlockHeight:
+
+				t.Fatalf("FirstBlockHeight incorrect: "+
+					"expected %v got %v",
+					rangeResp.FirstBlockHeight,
+					queryResp[len(queryResp)-1].BlockHeight)
+
 			}
 
 			respMsgs = append(respMsgs, rangeResp.ShortChanIDs...)
@@ -785,13 +832,13 @@ func TestGossipSyncerReplyChanRangeQuery(t *testing.T) {
 
 	// We should get back exactly 5 short chan ID's, and they should match
 	// exactly the ID's we sent as a reply.
-	if len(respMsgs) != len(resp) {
+	if len(respMsgs) != len(queryResp) {
 		t.Fatalf("expected %v chan ID's, instead got %v",
-			len(resp), spew.Sdump(respMsgs))
+			len(queryResp), spew.Sdump(respMsgs))
 	}
-	if !reflect.DeepEqual(resp, respMsgs) {
+	if !reflect.DeepEqual(queryResp, respMsgs) {
 		t.Fatalf("mismatched response: expected %v, got %v",
-			spew.Sdump(resp), spew.Sdump(respMsgs))
+			spew.Sdump(queryResp), spew.Sdump(respMsgs))
 	}
 
 	// Wait for error from goroutine.