From 6a9b96122d908b80fc8a6fdd720eb1cdd270cd2b Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 2 Dec 2019 19:21:10 -0800 Subject: [PATCH] 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 | 34 ++++++++++++++++--- discovery/syncer_test.go | 73 +++++++++++++++++++++++++++++++++------- 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/discovery/syncer.go b/discovery/syncer.go index 65a1b8f3..79dc1456 100644 --- a/discovery/syncer.go +++ b/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 diff --git a/discovery/syncer_test.go b/discovery/syncer_test.go index ea99e4ec..60ba3d5f 100644 --- a/discovery/syncer_test.go +++ b/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.