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

Gossip queries: sync complete is back #826

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented Dec 14, 2020

We previously insisted that reply_channel_range messages were not overlapping: blocks content could not be split across multiple messages.

This made it possible to implicitly figure out when sync was complete, so we re-purposed the previous complete field to a full_information field.

We now revert that change to allow blocks to be split across multiple messages. An explicit flag is thus needed to signal that sync is complete. AFAIK, no-one actually reads the full_information field and use its info: so it's ok to re-purpose it.

Implementations should probably deploy this in multiple phases:

  1. Write the sync_complete field: set it to true only in your last reply_channel_range
  2. Read and interpret sync_complete
  3. Split blocks across multiple reply_channel_range

A safer approach would be to avoid re-using the full_information field and instead introduce a new TLV field.
Let me know if you think that's better.

Fixes #804

We previously insisted that `reply_channel_range` messages were not
overlapping: blocks content could not be split across multiple messages.

This made it possible to implicitly figure out when sync was complete, so we
re-purposed the previous `complete` field to a `full_information` field.

We now revert that change to allow blocks to be split across multiple
messages. An explicit flag is thus needed to signal that sync is complete.

Fixes #804
@bmancini55
Copy link

LGTM. Should we consider adding an error TLV to cover the intent of full_information as an error signal? I'm fine without it, but it might be beneficial to have a mechanism to signal failure.

@sstone
Copy link
Collaborator

sstone commented Jan 4, 2021

LGTM! In retrospect not having an explicit end of reply stream marker was probably a mistake and re-purposing full_information is a clean way of fixing it. I would prefer this over adding an optional TLV field (which would imho be necessary only if the general feeling was that this change will take too long to be acked and implemented).

t-bast added a commit to ACINQ/eclair that referenced this pull request Jan 7, 2021
We are restoring the previous behavior of using the `sync_complete` field
to signal the end of a `channel_range_query` sync.

The first step is to correctly set that field, before we can read it and
interpret it to mark the end of sync.

See lightning/bolts#826
@rustyrussell
Copy link
Collaborator

Ack: this won't break anything worse than it is now. We might get upset and send an error if the final block is split, so I'll fix that now.

t-bast added a commit to ACINQ/eclair that referenced this pull request Feb 4, 2021
We are restoring the previous behavior of using the `sync_complete` field
to signal the end of a `channel_range_query` sync.

The first step is to correctly set that field, before we can read it and
interpret it to mark the end of sync.

See lightning/bolts#826
t-bast added a commit to ACINQ/eclair that referenced this pull request Feb 15, 2021
We are restoring the previous behavior of using the `sync_complete` field
to signal the end of a `channel_range_query` sync.

The first step is to correctly set that field, before we can read it and
interpret it to mark the end of sync.

See lightning/bolts#826
@t-bast
Copy link
Collaborator Author

t-bast commented Feb 15, 2021

Merging as agreed during the spec meeting (#840)

@t-bast t-bast merged commit edd45ec into master Feb 15, 2021
@t-bast t-bast deleted the gossip-queries-complete branch February 15, 2021 20:37
- the final `reply_channel_range` message:
- MUST have `first_blocknum` plus `number_of_blocks` equal or greater than the `query_channel_range` `first_blocknum` plus `number_of_blocks`.
- MUST set `sync_complete` to `true`.
Copy link
Contributor

@ysangkok ysangkok Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is true/false in a byte typed value? 0x01 and 0x00?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth opening a PR to fix it, but, yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, 0x01. Feel free to clarify if you feel it's needed.

rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Feb 18, 2021
We assume if they set this to 0 (which nobody did previously), they're
using it as a modern flag and use it to indicate when they're
finished.  Otherwise, we count how many blocks they've sent and use
that to determine whether they've finished.

See: lightning/bolts#826

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Changed: Protocol: we use `sync_complete` for gossip range query replies, with detection for older spec nodes.
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Feb 22, 2021
We assume if they set this to 0 (which nobody did previously), they're
using it as a modern flag and use it to indicate when they're
finished.  Otherwise, we count how many blocks they've sent and use
that to determine whether they've finished.

See: lightning/bolts#826

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Changed: Protocol: we use `sync_complete` for gossip range query replies, with detection for older spec nodes.
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Feb 23, 2021
We assume if they set this to 0 (which nobody did previously), they're
using it as a modern flag and use it to indicate when they're
finished.  Otherwise, we count how many blocks they've sent and use
that to determine whether they've finished.

See: lightning/bolts#826

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Changed: Protocol: we use `sync_complete` for gossip range query replies, with detection for older spec nodes.
rustyrussell added a commit to ElementsProject/lightning that referenced this pull request Feb 25, 2021
We assume if they set this to 0 (which nobody did previously), they're
using it as a modern flag and use it to indicate when they're
finished.  Otherwise, we count how many blocks they've sent and use
that to determine whether they've finished.

See: lightning/bolts#826

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Changed: Protocol: we use `sync_complete` for gossip range query replies, with detection for older spec nodes.
vibhaa pushed a commit to spider-pcn/lightning that referenced this pull request Mar 24, 2021
We assume if they set this to 0 (which nobody did previously), they're
using it as a modern flag and use it to indicate when they're
finished.  Otherwise, we count how many blocks they've sent and use
that to determine whether they've finished.

See: lightning/bolts#826

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Changed: Protocol: we use `sync_complete` for gossip range query replies, with detection for older spec nodes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reply_channel_range is now too strict.
6 participants