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

BOLT 7: add extended channel queries #519

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@sstone
Copy link
Collaborator

sstone commented Dec 2, 2018

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 sstone added this to the v1.1 milestone Dec 2, 2018

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

@cfromknecht
Copy link
Contributor

cfromknecht left a comment

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

@@ -625,13 +625,13 @@ timeouts. It also causes a natural ratelimiting of queries.
### 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`)

This comment has been minimized.

@cfromknecht

cfromknecht Dec 2, 2018

Contributor

s/1011/1013

### 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 comment has been minimized.

@cfromknecht

cfromknecht Dec 2, 2018

Contributor

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)?

This comment has been minimized.

@sstone

sstone Dec 3, 2018

Author Collaborator

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

This comment has been minimized.

@sstone

sstone Dec 3, 2018

Author Collaborator

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

@@ -625,13 +625,13 @@ timeouts. It also causes a natural ratelimiting of queries.
### 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`)

This comment has been minimized.

@SomberNight

SomberNight Dec 2, 2018

collision with query_short_channel_ids?

@sstone sstone force-pushed the sstone:bolt7-channel-queries-with-timestamps branch 3 times, most recently from 527b5ed to db454f5 Dec 3, 2018

BOLT 7: add extended channel queries
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 sstone force-pushed the sstone:bolt7-channel-queries-with-timestamps branch 3 times, most recently from cb67f70 to 7f57a41 Jan 7, 2019

@sstone sstone force-pushed the sstone:bolt7-channel-queries-with-timestamps branch from 7f57a41 to 20113ad Jan 7, 2019

@@ -157,7 +157,7 @@ The origin node:
- MUST set `chain_hash` to the 32-byte hash that uniquely identifies the chain
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`.

This comment has been minimized.

@cfromknecht

cfromknecht Jan 16, 2019

Contributor

hash now says Feb28

This comment has been minimized.

@sstone

sstone Jan 16, 2019

Author Collaborator

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

sstone added some commits Jan 16, 2019

Extended queries: remove static fields from channel_update checksums
This checksum is used to identify updates which carry new information and should not
cover static fields.
@cdecker
Copy link
Collaborator

cdecker left a comment

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.

- 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.

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

what -> was

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 comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

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

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

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

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.

@@ -26,6 +26,7 @@ These flags may only be used in the `init` message:
| 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) |

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

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

@@ -727,6 +727,155 @@ is simple to implement.
In the case where the `channel_announcement` is nonetheless missed,
`query_short_channel_ids` can be used to retrieve it.
## Extended Query Messages

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

Needs an entry in the ToC

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

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

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

* [`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

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator
Suggested change Beta
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
1. type: 1012 (`reply_short_channel_ids_extended_end`) (`gossip_queries`)
2. data:
* [`32`:`chain_hash`]
* [`1`:`complete`]

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

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

- 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`.

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

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.

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

This comment has been minimized.

@cdecker

cdecker Jan 21, 2019

Collaborator

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

@sstone

This comment has been minimized.

Copy link
Collaborator Author

sstone commented Feb 6, 2019

Closed in favour of #557

@sstone sstone closed this Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment