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

Final: Channel range queries #392

Merged
merged 5 commits into from Jun 28, 2018

Conversation

Projects
None yet
7 participants
@rustyrussell
Collaborator

rustyrussell commented Mar 5, 2018

Here's an early draft, please bikeshed @Roasbeef @cdecker @pm47 @sstone

Encoding types:
* `0`: uncompressed array of `short_channel_id` types, in ascending order.

This comment has been minimized.

@sstone

sstone Mar 14, 2018

Collaborator

We could also use 1 for gzip compression, I guess it's easy enough to support ? and this way all ids up to a few dozen thousand channels should fit in a single message

* [`32`:`chain_hash`]
* [`4`:`first_timestamp`]
* [`4`:`timestamp_range`]

This comment has been minimized.

@sstone

sstone Mar 14, 2018

Collaborator

range here has not the same meaning as in query_channel_range/reply_channel_range, gossip_timestamp_filter would be more appropriate ?

This comment has been minimized.

@rustyrussell

rustyrussell Mar 20, 2018

Collaborator

Agreed.

This comment has been minimized.

@Roasbeef

Roasbeef Mar 23, 2018

Member

Just to clarify, this the timestamp_range is in seconds, right? May be good to note that here to be precise.

case where no `channel_update` has been received yet, any reasonable
value will do: it could be the block time of the first possible
announcement block (ie. six blocks following the funding transaction),
or simply the this node received it.

This comment has been minimized.

@sstone

sstone Mar 14, 2018

Collaborator

typo: or simply the timestamp this node received it at ?

recent messages: in that case `channel_announcement` will be sent
redundantly. The new node will already need to ask the old node for
old gossip, so it will also receive the `channel_announcement`.

This comment has been minimized.

@sstone

sstone Mar 14, 2018

Collaborator

There is still a problem with old inactive channels: if you prune and forget a channel, and later receive an announcement for this same channel, but no matching channel updates, you will broadcast again it (unless you set its timestamp to the timestamp of the block it appeared in ?). I think that broadcasting channel announcements only when we have a matching channel update should work. We could also group channel announcements and channel updates, it is less efficient and we'll send more data but it mitigated by the timestamp filter.

This comment has been minimized.

@Roasbeef

Roasbeef Mar 19, 2018

Member

Is there still an issue with old inactive channels? You'd start the gossip timestamp at the point beyond your "zombie horizon", so the stale channel update would never be sent to you right?

This comment has been minimized.

@rustyrussell

rustyrussell Mar 20, 2018

Collaborator

Ok, so as agreed in call, timestamp for announce filtering is that of any update. This means you don't send a useless announce which has no channel updates ever. It means you don't have to store the "timestamp" since you can just use the latest update, but you can, and you don't have to deal with the corner case "new update arrived, now announce timestamp is within the window so I have to send it too" since you can argue that you're using the older timestamp. You only need to consider that for the very first update.

This comment has been minimized.

@sstone

sstone Mar 20, 2018

Collaborator

Yes I think it works (we'll probably go with the "use latest timestamp" approach though). All that is missing now is the gzip option :)
We have updated endurance on testnet with a version of eclair that implements this PR

@sstone

I'm ok with this PR if we restrict sending out channel announcements only when we have a matching channel update, which should fix the "zombie churn" issue. Also, it would be nice to have gzip compression for short channel ids.

- MUST NOT relay any gossip messages unless explicitly requested.
- otherwise:
- upon establishing a connection:
- SHOULD set the `init` message's `initial_routing_sync` flag to 1, to

This comment has been minimized.

@jimpo

jimpo Mar 20, 2018

Collaborator

I'm confused by this. On this line it says to always set the flag upon establishing a connection, and on the next line it says to only set the flag if you need a full copy of the routing state.

This comment has been minimized.

@rustyrussell

rustyrussell Mar 22, 2018

Collaborator

Yes, that was actually in the prior text. Will fix.

Future nodes may not have complete information; they certainly won't have
complete information on unknown `chain_hash` chains. While this `complete`
field cannot be trusted, if 0 ir does indicate that the sender should search

This comment has been minimized.

@jimpo

jimpo Mar 20, 2018

Collaborator

typo: s/ir/it/. I would actually just remove the "if" and "it": "While this complete field cannot be trusted, 0 does indicate...".

This comment has been minimized.

@rustyrussell

rustyrussell Mar 22, 2018

Collaborator

Yep, will fix.

@sstone

sstone approved these changes Apr 13, 2018

@sstone

This comment has been minimized.

Collaborator

sstone commented Apr 26, 2018

@Roasbeef spotted a potential issue: there is no easy way to add a new compression format, or to prevent you from sending data encoded in a format that your peer does not support.
I think we have 2 options:

  • limit ourselves to uncompressed and zlib, and have everyone support both. zlib already does a fairly good job, and the tx index part of short channel ids is basically random numbers so it may be difficult to do much better.
  • specify that the first outgoing query message must be a query_channel_range message, which seems natural, and add an expected_format field to it. If the receiver does not support it they would fall back to uncompress.
@rustyrussell

This comment has been minimized.

Collaborator

rustyrussell commented Apr 28, 2018

We can add a new feature bit for new compression. But frankly we should just do zlib, since everyone has to support it anyway. While we can get factor-of-2 improvement with other options, it's not necessary AFAICT.

- SHOULD set the `initial_routing_sync` flag to 1.
- upon receiving an `init` message with the `initial_routing_sync` flag set to
1:
- SHOULD send `channel_announcement`s, `channel_update`s and

This comment has been minimized.

@cdecker

cdecker Apr 29, 2018

Collaborator

Should we use "gossip messages" alias here?

This comment has been minimized.

@rustyrussell

rustyrussell Jun 18, 2018

Collaborator

Ack, done.

#### Requirements
The sender:
- MUST NOT send this if it has sent a previous `query_short_channel_ids` to this peer and not received `reply_short_channel_ids_end`.

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj May 1, 2018

Collaborator

This section is inside a section that describes two messages, so "this" is ambiguous. From the sentence it probably means "MUST NOT send query_short_channel_ids" but it would be better to clarify.

This comment has been minimized.

@rustyrussell

rustyrussell Jun 18, 2018

Collaborator

Ack, done.

- MUST set the first byte of `encoded_short_ids` to the encoding type.
- MUST encode a whole number of `short_channel_id`s to `encoded_short_ids`
- MAY send this if it receives a `channel_update` for a
`short_channel_id` for which has no `channel_announcement`.

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj May 1, 2018

Collaborator

"for which it has no"

This comment has been minimized.

@rustyrussell

rustyrussell Jun 18, 2018

Collaborator

Ack.

to be stored. Instead, we allow any update to be used, which is simple to
implement.
In the case where it the `channel_announcement` is nonetheless missed,

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj May 1, 2018

Collaborator

"In the case where it the channel_announcement"

This comment has been minimized.

@rustyrussell

rustyrussell Jun 18, 2018

Collaborator

Ack

The receiver of `query_channel_range`:
- if it has not sent `reply_channel_range` to a previously received `query_channel_range` from this sender:
- MAY fail the connection.
- MUST respond with one or more `reply_channel_range` whose combined range

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj May 1, 2018

Collaborator

Multiple reply_channel_range may make it ambiguous whether we are violating "MUST NOT send this (query_channel_range) if it has sent a previous query_channel_range to this peer and not received reply_channel_range" requirement.

In particular if there are any data or message buffers between the operating code of the peers (e.g. c-lightning, where sending of messages is buffered in a message queue):

  1. Peer A sends a query_channel_range.
  2. Peer B sends two reply_channel_range in response.
  3. Peer A receives one reply_channel_range and thinks it has fulfilled the above requirement for the first query_channel_range.
  4. Peer A sends another query_channel_range.
  5. Peer B receives the above message, then gets stalled in processing due to OS scheduling or etc.
  6. Peer A receives the second reply_channel_range and thinks it has fulfilled the above requirement for the second query_channel_range.
  7. Peer A sends a third query_channel_range.
  8. Peer B is awakened by the OS again, and sees another query_channel_range even though it has not finished processing the second query_channel_range. It then takes the above clause "MAY fail the connection" and disconnects.

This comment has been minimized.

@sstone

sstone May 2, 2018

Collaborator

We could add an explicit reply_channel_range_end message here but I don't think that it is necessary: you can check the first_blocknum and number_of_blocks fields and tell when you've received all the replies to your query.

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj May 2, 2018

Collaborator

It would be good to clarify what rules exactly apply to determine if the last N reply_channel_range has fulfilled the most recent query_channel_range or not. Later the text mentions using canned 1000-block reply_channel_ranges, for instance; presumably a query is covered if all reply_channel_ranges have fully covered the range, but for instance is it allowed to return the replies in any order, including random?

This comment has been minimized.

@sstone

sstone May 3, 2018

Collaborator

Short channel ids are ordered and replies must be sent in order (but it is not explicit in the current wording when replies span several messages).

This comment has been minimized.

@rustyrussell

rustyrussell Jun 18, 2018

Collaborator

I've changed the language appropriately:

 The sender of `query_channel_range`:
-  - MUST NOT send this if it has sent a previous `query_channel_range` to this peer and not received `reply_channel_range`.
+  - MUST NOT send this if it has sent a previous `query_channel_range` to this peer and not received all `reply_channel_range` replies.

and

 The receiver of `query_channel_range`:
-  - if it has not sent `reply_channel_range` to a previously received `query_channel_range` from this sender:
+  - if it has not sent all `reply_channel_range` to a previously received `query_channel_range` from this sender:
     - MAY fail the connection.

Since it's made clear that the receiver has to cover the entire range, the definition of all seems simple enough to leave implied?

Encoding types:
* `0`: uncompressed array of `short_channel_id` types, in ascending order.
* `1`: array of `short_channel_id` types, in ascending order, compressed with zlib<sup>[1](#reference-1)</sup>

This comment has been minimized.

@Roasbeef

Roasbeef Jun 1, 2018

Member

Each new encoding type needs a distinct feature bit. Otherwise they're all mandatory. Some clients may not want to package zlib, nor have to implement defensive decoding measures to ensure an enormous amount of memory isn't allocated due to a maliciously crafted payload.

This comment has been minimized.

@sstone

sstone Jun 4, 2018

Collaborator

Yes, zlib and uncompressed are mandatory and we'll probably never need to add another format (zlib already does a fairly good job).

This comment has been minimized.

@Roasbeef

Roasbeef Jun 4, 2018

Member

I mean why should zlib itself be mandatory? IMO it was a bit of a premature optimization (a few additional round trips really aren't that bad). It forces implementations to implement defensive parsing strategies, or it may become a DoS vector if the spec doesn't explicitly pay out the risks and mitigation strategies.

This comment has been minimized.

@cfromknecht

cfromknecht Jun 5, 2018

Contributor

Is there a place where the current proposal negotiates the supported encoding types?

Seems that would be desirable to upgrade with future encoding types, but also to retroactively deprecate less-than-optimal encodings. IMO pushing this complexity off until the future seems like it would cause more headaches than just including provisions now.

This comment has been minimized.

@rustyrussell

rustyrussell Jun 18, 2018

Collaborator

zlib is pretty established and robust, and it's now deployed and thus too late :(

Any other compression methods would require a new feature bit. For now, don't advertise gossip_queries if you don't support receiving zlib.

@Roasbeef pointed out that feature bits also allow us to do fine-grained disable if problems are found with an implementation. Let this be a lesson-learned: every feature needs a feature bit, even if everyone intends to implement it!

This comment has been minimized.

@rustyrussell

rustyrussell Jun 18, 2018

Collaborator

Meanwhile, I've added a paragraph on worst-case behavior:

Note that a 65535-byte zlib message can decompress into 67632120
bytes<sup>[2](#reference-2)</sup>, but since the only valid contents 
are unique 8-byte values, no more than 14 bytes can be duplicated
across the stream: as each duplicate takes at least 2 bits, no valid
contents could decompress to more then 3669960 bytes.

This number is very conservative: you still need some literals, and you still need the two-bytes between the 14 duplicated bytes, etc.

@rustyrussell rustyrussell force-pushed the rustyrussell:channel_range_queries branch from 5c1bdce to deae33d Jun 18, 2018

@rustyrussell

This comment has been minimized.

Collaborator

rustyrussell commented Jun 18, 2018

Appended two commits and rebased since it had a wordlist conflict...

@rustyrussell

This comment has been minimized.

Collaborator

rustyrussell commented Jun 20, 2018

Would appreciate final approval on this at next meeting!

@cdecker

This comment has been minimized.

Collaborator

cdecker commented Jun 25, 2018

ACK

1 similar comment
@sstone

This comment has been minimized.

Collaborator

sstone commented Jun 26, 2018

ACK

@rustyrussell rustyrussell changed the title from WIP: Channel range queries to Final: Channel range queries Jun 26, 2018

rustyrussell added some commits Jun 26, 2018

BOLT 1: options must be included if negotiated, not if understood.
Obviously you can't include fields you don't understand, but importantly
if the other side doesn't agree, you don't need to include them.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT #7: introduce term "gossip messages" to refer to channel_announc…
…ement/channel_update/node_announcement.

This makes discussing them simpler for the next patch.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
tools/extract-formats.py: handle option labels on types.
We're about to introduce new messages which are gated by options (not
just single fields).  Ignore them for the purposes of parsing.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT 7: query_messages option.
[ This was a joint effort by many people, with iterations not
  indicated in this final commit: thanks to all who reviewed and
  polished!  Particularly: @jimpo @cdecker @sstone @ZmnSCPxj ]

This enables three new functions:

1. query_short_channel_ids: they will send channel_announcement /
   channel_update / node_announcement followed by reply_short_channel_ids_done.
2. query_channel_range: they will send one or more reply_channel_range
   with the short_channel_ids in these blocks.
3. gossip_timestamp_filter: filters what gossip they send.

It also changes behavior: we no longer send a `channel_announcement`
until we have at least one `channel_update`.  The announcement is
fairly useless without an update already, but this in particular
enables reasonable timestamp filtering (channel_announcement does not
have an explicit timestamp).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT 7: Add compressed (zlib) encoding.
[ Note: in retrospect, adding this in the initial draft without its
  own feature bit was a mistake.  It was a premature optimization,
  adds complexity and removes the ability to disable it if a problem
  is found without disabling gossip_queries entirely.  However, it
  is already deployed as-is. --RR ]
  
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:channel_range_queries branch from deae33d to ddad168 Jun 26, 2018

@rustyrussell

This comment has been minimized.

Collaborator

rustyrussell commented Jun 26, 2018

Rebased and squashed commits, with no textual changes.

@cdecker

This comment has been minimized.

Collaborator

cdecker commented Jun 27, 2018

Paging Dr. @Roasbeef for a final approval 😉

@Roasbeef

LGTM 🥁

@rustyrussell rustyrussell merged commit fd9da9b into lightningnetwork:master Jun 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment