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

BOLT 7: add extended channel queries #519

Closed

Conversation

sstone
Copy link
Collaborator

@sstone 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 bolt7-channel-queries-with-timestamps branch 2 times, most recently from 65c4e48 to 01223e3 Compare December 2, 2018 18:43
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

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`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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`)
Copy link
Contributor

Choose a reason for hiding this comment

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

collision with query_short_channel_ids?

@sstone sstone force-pushed the bolt7-channel-queries-with-timestamps branch 3 times, most recently from 527b5ed to db454f5 Compare December 3, 2018 16:10
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 bolt7-channel-queries-with-timestamps branch 3 times, most recently from cb67f70 to 7f57a41 Compare January 7, 2019 16:05
@sstone sstone force-pushed the bolt7-channel-queries-with-timestamps branch from 7f57a41 to 20113ad Compare January 7, 2019 16:19
@@ -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`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

hash now says Feb28

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.
Copy link
Collaborator

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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`
Copy link
Collaborator

Choose a reason for hiding this comment

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

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) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs an entry in the ToC

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

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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`
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@sstone
Copy link
Collaborator Author

sstone commented Feb 6, 2019

Closed in favour of #557

@sstone sstone closed this Feb 6, 2019
@cfromknecht cfromknecht mentioned this pull request May 9, 2019
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.

4 participants