Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

discovery: properly set FirstBlockHeight and NumBlocks in responses #3785

Merged
merged 1 commit into from Dec 11, 2019

Conversation

@Roasbeef
Copy link
Member

Roasbeef commented Dec 3, 2019

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.

@Roasbeef Roasbeef requested review from halseth and wpaulino as code owners Dec 3, 2019
@halseth halseth requested review from bhandras and removed request for halseth Dec 3, 2019
Copy link
Collaborator

wpaulino left a comment

Changes LGTM, haven't tested in the wild yet.

Linter is failing with:

discovery/syncer_test.go:722: File is not `gofmt`-ed with `-s` (gofmt)
		lnwire.ShortChannelID{
discovery/syncer.go Show resolved Hide resolved
Copy link
Collaborator

cfromknecht left a comment

LGTM 🍣

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Dec 10, 2019

@Roasbeef needs squash!

@cfromknecht cfromknecht added the v0.9.0 label Dec 10, 2019
@cfromknecht cfromknecht added this to WIP in v0.9.0-beta via automation Dec 10, 2019
@cfromknecht cfromknecht added this to the 0.9.0 milestone Dec 10, 2019
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.
@Roasbeef Roasbeef force-pushed the Roasbeef:gossip-queries-fix branch from fae17f7 to 6a9b961 Dec 11, 2019
@Roasbeef Roasbeef merged commit a7c2b12 into lightningnetwork:master Dec 11, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 62.196%
Details
v0.9.0-beta automation moved this from WIP to Done Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
4 participants
You can’t perform that action at this time.