-
Notifications
You must be signed in to change notification settings - Fork 493
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
[WIP] BOLT 7: Inventory-based gossip #584
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to push a small formatting fixup (we generate from the spec, so this makes it much easier to implement!)
07-routing-gossip.md
Outdated
### The `query_short_channel_ids`/`reply_short_channel_ids_end` Messages | ||
|
||
1. type: 261 (`query_short_channel_ids`) (`gossip_queries`) | ||
2. data: | ||
* [`32`:`chain_hash`] | ||
* [`2`:`len`] | ||
* [`len`:`encoded_short_ids`] | ||
* [`2`:`option_len`] | ||
* [`len`:`option_encoded_query_flags`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len
needs to be option_len
here.
07-routing-gossip.md
Outdated
@@ -636,6 +656,9 @@ timeouts. It also causes a natural ratelimiting of queries. | |||
* [`1`:`complete`] | |||
* [`2`:`len`] | |||
* [`len`:`encoded_short_ids`] | |||
* [`1`:`option_extended_query_flag`] | |||
* [`2`:`option_extended_info_len`] | |||
* [`len`:`option_extended_info`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, len
-> option_extended_info_len
.
Other thought was to get nodes to opt-out of updates for most channels from most peers. Since node has a full map of the network it could tell most peers not to send updates for a list of channels that it knows it will get quicker from multiple other peers. |
Fwiw we can already opt out entirely of updates on a peer by peer basis by
not sending `gossip_timestamp_filter`.
Le mer. 20 mars 2019 à 21:17, n1bor <notifications@github.com> a écrit :
… Other thought was to get nodes to opt-out of updates for most channels
from most peers. Since node has a full map of the network it could tell
most peers not to send updates for a list of channels that it knows it will
get quicker from multiple other peers.
Some info here: ACINQ/eclair#864
<ACINQ/eclair#864>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#584 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB7yvk3Hfrnfn4lYSv9XNsNlAhmDEBgGks5vYpdbgaJpZM4bXBIw>
.
|
07-routing-gossip.md
Outdated
|
||
A more efficient scheme based on inventory messages, similar to how transactions and blocks are propagated on the bitcoin p2p network, can optionally be used: if the `option_inv_gossip` feature bit is set, instead of broadcasting `channel_annoucement` and `channel_update` messages, nodes will broadcast a `gossip_inventory` message which is similar to a `reply_channel_range` message and can be used to filter announcements by timestamp or by content (checksum): | ||
|
||
1. type: 266 (`gossip_inventory`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a single message for all inventory types I think we should instead have distinct messages as this allows us to open encode certain fields that are relevant if a node wants to filter out a class of advertisements. For example a channel_inv
message would have things like the size of the channel, or the nodes that it's connecting included. This would let nodes performing filtering such as ignoring all channels that are below a certain size. With the checksum approach the node has no idea what it's about to fetch, and will waste bandwidth by fetching something that it will instantly abandon.
While we're at it, we may also need to extend the prior queries in order to allow fetching a node announcement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rebased to get the latest changes from #557, and I'm using TLV encoding now. I've adding a node inventory messages (just a list of node ids).
I don't think that we need to include channel capacity in the inv message: you would have to trust the sender anyway, and tiny channels will progressively go away.
I don't think we should include the node ids it's connecting: how would you use it ? You don't know what a new channel enables from a routing pov until you have the rest of the graph
Just some stats on scale of issues: |
What implementation and version is that node running? I see about an order
of magnitude less traffic on my node personally.
…On Wed, May 1, 2019, 12:30 AM n1bor ***@***.***> wrote:
Just some stats on scale of issues:
https://lightningconductor.net/grafana/d/CZQF3HzWz/lightning-stats?orgId=1&from=now-7d&to=now&fullscreen&panelId=22
Is for node:
***@***.***
:9735
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#584 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHTWLWGAJMV26MS5GOBA23PTFBILANCNFSM4G24CIYA>
.
|
A 2 week old version of Eclair. |
Those are orthogonal things: this PR reduces the bandwidth used by gossip, all things remaining equal (even if the number of @n1bor seems to have a pathological case of flapping channels, which caused eclair to generate a constant stream of |
Updated to HEAD de5a782 - lets see if it settles down in the next few hours. UPDATE--- So this change would reduce size of each update from about 140bytes to about 13 - and these are both sent and received. But probably as importantly would reduce the huge number of small network packets we currently send/receive. |
OK, I'm working on TLV parsing, so some cleanups (and introduce some conventions). Nothing significant changed in the format, except removal of some redundant lengths inside TLV records. |
1. Uses 'tlvs' for the "length" of tlv streams. 2. Allow - inside length of fields, since it's useful given an implied tlv_len. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
07-routing-gossip.md
Outdated
|
||
`channel_inv` contains information about channel announcements and updates. It uses the exact same encoding as `reply_channel_range` and includes: | ||
- a list of `short_channel_id`s | ||
- an optional `timestamps`, which includes `channel_update` timestamps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to avoid the redundancy of sending timestamps for both edges every time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not atm. You can choose to not send timestamps at all, but if you do you must include timestamps for all updates. I'm not sure how sending some but not all of them could be used ?
07-routing-gossip.md
Outdated
* [`tlv_len`:`encoded_checksums`] | ||
1. type: 7 (`node_inv`) | ||
2. data: | ||
* [`tlv_len`:`node_ids`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from my understanding, #557 doesn't have a way of querying for node_announcement
without the corresponding short_channel_id
. Can you explain how one would retrieve the short_channel_id
from this message to use in the query? If node_ids
is a different size from encoded_short_ids
, which one should the receiver use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point and now I see why @Roasbeef wanted a different INV message for node announcements. A simple answer would be to include node_id
, timestamp
and short_channel_id
in the INV message, receiver can then use the provided short_channel_id
to query the node announcement.
This needs a rebase. If we're gonna add more stuff to the route-sync state machine, can we improve it? The current design is super complicated, and its just kinda grown organically without a lot of thought to the end state. It has grown based on whats available without a lot of care to whats allowed (eg it relies heavily on sequence id fields being timestamps, which the spec doesn't even require, not to mention stale updates my propagate late and uses zlib as a cheap way to communicate data, instead of a purpose-designed thing like minisketch). At various points, I've suggested we move towards where Bitcoin Core is going and:
This would remove a ton of existing complexity. |
Inventory messages is an optional feature that is used to reduce duplicate gossip traffic. Instead of sending `channel_annoucement` or `channel_update` messages, nodes will send an inventory message to peers that support this option. Inventory messages are similar to channel queries and contain a list of short channel ids, checksums and timestamps. Receiving nodes will check inventory data and use channel queries to retrieve missing or outdated channel announcements and updates.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We add a specific node inventory message that includes node id, timestamp, and short channel id. The short channel id will be used as an alias to query the corresponding announcement, if needed, using extended channel range queries.
2f41dd4
to
4e68461
Compare
This is a proposal for adding inventory messages to the gossip protocol. With the current design, nodes which receive gossip messages (node announcements, channel announcements and updates) will relay them to their peers, which will result in duplicates for nodes that are connected to many different peers.
Nodes which support the
option_inv_gossip
feature will instead broadcast inventory messages which contain identifiers for gossip messages they've received. Receiving node will compare these identifiers to their local view of the routing table, and ask for missing or outdated messages using channel queries. This implies thatoption_inv_gossip
cannot be used withoutgossip_queries
.It builds upon PR #571 (unification of feature bits, which includes a definition for
option_inv_gossip
) and #557 (extended channel queries).More specifically, it is a "companion" PR to #557:
In fact, the inventory message that I propose to use here is almost identical to an extended
reply_channel_range
message and includes short channel ids, checksums and timestamps, to allow receiving to efficiently query messages based on content (checksum) and timestamps.Node announcements have been left out on purpose because they have a much more limited impact on bandwidth than channel updates, and because gains would be smaller (you would still need to advertise node ids which are 33 bytes long).