BOLT 7: add extended channel queries by sstone · Pull Request #519 · lightning/bolts

sstone · 2018-12-02T14:08:27Z

Extended channel queries include extra timestamps (one for each channel_update), which
allows nodes that are often online to easily sync their routing tables.

cfromknecht

great work @sstone, these are welcome improvements to the gossip querying! i wonder though, should also include a new (extended_gossip_queries) feature bit to go along with the proposal? if we go down that path, we would probably want to include language around extended_gossip_queries overriding gossip_queries in the event that both nodes advertise support for both flavors

cfromknecht · 2018-12-02T23:01:56Z

07-routing-gossip.md

 ### The `query_channel_range` and `reply_channel_range` Messages

-1. type: 263 (`query_channel_range`) (`gossip_queries`)
+1. type: 1011 (`query_channel_range`) (`gossip_queries`)


s/1011/1013

cfromknecht · 2018-12-02T23:08:30Z

07-routing-gossip.md

+
+### The `query_short_channel_ids_with_timestamps`/`reply_short_channel_ids_with_timestamps_end` Messages
+
+1. type: 261 (`query_short_channel_ids_with_timestamps`) (`gossip_queries`)


what's the rationale for breaking compatibility with older nodes, instead of allocating the 101x messages to the extended queries?

I think this merits it's own (extended_gossip_queries) feature bit, instead of overloading (gossip_queries)?

Ah yes I messed up the new message types, it's fixed now

And I've added a new feature bit, with the value we've used for our own tests

SomberNight · 2018-12-02T23:41:31Z

07-routing-gossip.md

 ### The `query_channel_range` and `reply_channel_range` Messages

-1. type: 263 (`query_channel_range`) (`gossip_queries`)
+1. type: 1011 (`query_channel_range`) (`gossip_queries`)


collision with query_short_channel_ids?

Extended channel queries include extra timestamps (one for each channel_update), which allows nodes that are often online to easily sync their routing tables.

cfromknecht · 2019-01-16T02:38:26Z

07-routing-gossip.md

  that the channel was opened within:
    - for the _Bitcoin blockchain_:
-      - MUST set `chain_hash` value (encoded in hex) equal to `6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000`.
+      - MUST set `chain_hash` value (encoded in hex) equal to `6Feb28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000`.


hash now says Feb28

Thanks! I'm really useless with spellcheck/aspell :(

This checksum is used to identify updates which carry new information and should not cover static fields.

cdecker

Quite a nice PR, however I do have conceptual objections to the idea of introducing yet another set of messages, and yet more information we need to keep track of. The whole gossip protocol is quickly devolving into a game of whack-a-mole and premature optimizations.

I'd very much prefer throwing everything overboard and going back to the drawing board, to create a sensible and permanent solution, rather than hacking more and more things on what was supposed to be the first simple iteration of the gossip protocol.

So from my side this is a NACK. I see the issues you are facing with resource limited devices, but adding these incremental hacks is not the way to go.

cdecker · 2019-01-21T12:45:19Z

07-routing-gossip.md

+- they want to avoid downloading channel updates that do not carry new information
+
+The second issue is typically caused by channels that have been disabled then enabled again while the local
+node what offline. 


what -> was

cdecker · 2019-01-21T12:46:52Z

07-routing-gossip.md

+
+A new feature bit is used to specify which type of queries a node will support. If 
+both nodes support extended queries then the first message that they will send once they're
+connected will be `query_channel_range_extended` and they will only use extended queries 


This should probably be stronger and say MUST only use extended queries

cdecker · 2019-01-21T12:50:58Z

07-routing-gossip.md

+#### Rationale
+
+Future nodes may not have complete information; they certainly won't have
+complete information on unknown `chain_hash` chains.  While this `complete`


This implies that we want to allow forwarding of channel information for chains that we ourselves don't support. I don't think this is in any way reasonable, since the channel_announcement mandates that we verify a channel's existence before applying it locally or forwarding it. I a node wants to perform multi-chain payment I think it is reasonable for it to connect to one node for each traversed chain (but not mandate opening a channel), and sync gossip independently.

cdecker · 2019-01-21T12:52:52Z

09-features.md

 | 3  | `initial_routing_sync` | Indicates that the sending node needs a complete routing information dump | [BOLT #7](07-routing-gossip.md#initial-sync) |
 | 4/5  | `option_upfront_shutdown_script` | Commits to a shutdown scriptpubkey when opening channel | [BOLT #2](02-peer-protocol.md#the-open_channel-message) |
 | 6/7  | `gossip_queries`           | More sophisticated gossip control | [BOLT #7](07-routing-gossip.md#query-messages) |
+| 16/17  | `extended_gossip_queries`           | Gossip queries with extra timestamps | [BOLT #7](07-routing-gossip.md#extended-query-messages) |


Why skip bits 8-15? We've always allocated the numbers in increasing order without gaps.

cdecker · 2019-01-21T12:53:58Z

07-routing-gossip.md

 In the case where the `channel_announcement` is nonetheless missed,
 `query_short_channel_ids` can be used to retrieve it.

+## Extended Query Messages


Needs an entry in the ToC

cdecker · 2019-01-21T12:57:13Z

07-routing-gossip.md

+2. data:
+    * [`32`:`chain_hash`]
+    * [`2`:`len`]
+    * [`len`:`encoded_short_ids_and_flag`]


len is the length in bytes, not the number of tuples right? The same question applies to the original query_short_channel_ids probably.

cdecker · 2019-01-21T12:57:44Z

07-routing-gossip.md

+    * [`2`:`len`]
+    * [`len`:`encoded_short_ids_and_flag`]
+
+This message encodes an ordered list of [short channel id (8 bytes) | flag (1 byte)] where flag


Suggested change

This message encodes an ordered list of [short channel id (8 bytes) | flag (1 byte)] where flag

This message encodes an ordered list of `[short channel id (8 bytes) | flag (1 byte)]` where flag

cdecker · 2019-01-21T12:58:38Z

07-routing-gossip.md

+1. type: 1012 (`reply_short_channel_ids_extended_end`) (`gossip_queries`)
+2. data:
+    * [`32`:`chain_hash`]
+    * [`1`:`complete`]


This is identical to reply_short_channel_ids_end and doesn't need to be redefined with a new type and name.

cdecker · 2019-01-21T13:01:58Z

07-routing-gossip.md

+- checksum for the `channel_update` for node 1
+- checksum for the `channel_update` for node 2
+
+The checksum for a `channel_update` is a CRC32 checksum that covers the following fields: `short_channel_id`, `channel_flags`, `cltv_expiry_delta`, `fee_base_msat`, `fee_proportional_millionths`. It can be used to avoid querying new updates that do not include new information, and does not cover static fields such as `htlc_minimum_msat` or `htlc_maximum_msat`.


The serialization for these needs to be defined, i.e., we need to have the same message format definition as for any other wire message.

cdecker · 2019-01-21T13:02:55Z

07-routing-gossip.md

+
+* `1`: include `channel_update` for node 1
+* `2`: include `channel_update` for node 2
+* `4`: include `channel_announcement`


We've used bitfields everywhere else, so we should probably define bits here.

sstone · 2019-02-06T10:30:43Z

Closed in favour of #557

sstone added this to the v1.1 milestone Dec 2, 2018

sstone force-pushed the bolt7-channel-queries-with-timestamps branch 2 times, most recently from 65c4e48 to 01223e3 Compare December 2, 2018 18:43

cfromknecht reviewed Dec 2, 2018

View reviewed changes

SomberNight reviewed Dec 2, 2018

View reviewed changes

sstone force-pushed the bolt7-channel-queries-with-timestamps branch 3 times, most recently from 527b5ed to db454f5 Compare December 3, 2018 16:10

BOLT 7: add extended channel queries

70a5dad

Extended channel queries include extra timestamps (one for each channel_update), which allows nodes that are often online to easily sync their routing tables.

sstone force-pushed the bolt7-channel-queries-with-timestamps branch 3 times, most recently from cb67f70 to 7f57a41 Compare January 7, 2019 16:05

Rename extended channel queries

20113ad

sstone force-pushed the bolt7-channel-queries-with-timestamps branch from 7f57a41 to 20113ad Compare January 7, 2019 16:19

cfromknecht reviewed Jan 16, 2019

View reviewed changes

sstone added 2 commits January 16, 2019 11:29

Fix typo in chain_hash

32c3074

Extended queries: remove static fields from channel_update checksums

2bf8b67

This checksum is used to identify updates which carry new information and should not cover static fields.

cdecker requested changes Jan 21, 2019

View reviewed changes

sstone mentioned this pull request Jan 23, 2019

BOLT7: extend channel range queries with optional fields #557

Merged

sstone closed this Feb 6, 2019

cfromknecht mentioned this pull request May 9, 2019

BOLT01: TLV proposal #607

Merged


		### The `query_short_channel_ids_with_timestamps`/`reply_short_channel_ids_with_timestamps_end` Messages

		1. type: 261 (`query_short_channel_ids_with_timestamps`) (`gossip_queries`)

	This message encodes an ordered list of [short channel id (8 bytes) \| flag (1 byte)] where flag
	This message encodes an ordered list of `[short channel id (8 bytes) \| flag (1 byte)]` where flag

Conversation

sstone commented Dec 2, 2018

Uh oh!

cfromknecht left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdecker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstone commented Feb 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants