lightning / bolts

BOLT: Basis of Lightning Technology (Lightning Network Specifications)
2.1k stars 491 forks source link

Inconsistent behavior around graph dump on `gossip_timestamp_filter`. #980

Open TheBlueMatt opened 2 years ago

TheBlueMatt commented 2 years ago

In conversation with @t-bast at https://github.com/lightningdevkit/rust-lightning/pull/1382#discussion_r835038791, we recently discovered that (at least) CLN and eclair treat gossip_timestamp_filter messages differently.

Ultimately it comes down to the spec having this to say:

The receiver: SHOULD send all gossip messages whose timestamp is greater or equal to first_timestamp, and less than first_timestamp plus timestamp_range. MAY wait for the next outgoing gossip flush to send these.

which seems to say that when you get a gossip_timestamp_filter you've gotta go do a graph dump at the peer, ala initial-graph-sync. But this is somewhat confusing in that...what's the point of all the gossip queries if we just end up receiving a dump of gossip from the peer anyway (assuming you set the filter to two-weeks-ago to ensure you don't miss updates)?

It seems at least CLN does give you a graph dump, t-bast indicated eclair does not, and I'm not sure about lnd.

Of course at this point I figure we probably all want to just deprecate the gossip sync stuff in favor of something minisketch-based, but it'd be nice to get consistency here in the short-term. At least personally, I'd suggest we keep it as-is and do a graph dump when receiving a timestamp filter message, allowing us to "implement" gossip queries without bothering with the actual queries at all :).

vincenzopalazzo commented 2 years ago

At least personally, I'd suggest we keep it as-is and do a graph dump when receiving a timestamp filter message, allowing us to "implement" gossip queries without bothering with the actual queries at all :).

Concept ACK, If we are going to propose a solution with minisketch I think the solution is to keep as-is with graph dump to avoid some implementation to do the double work!

TheBlueMatt commented 2 years ago

Concept ACK, If we are going to propose a solution with minisketch I think the solution is to keep as-is with graph dump to avoid some implementation to do the double work!

For context, in LDK we had the whole query stuff implemented, but when we noticed nodes doing a dump when we sent a gossip_timestamp_filter we yanked the query logic from the codebase entirely and replaced it with a three paragraph comment and a single message....much cleaner :)

Roasbeef commented 2 years ago

SHOULD -> MUST?

rustyrussell commented 2 years ago

So, there are two cases:

  1. The very first gossip_timestamp_filter, which activates gossip (assuming gossip_queries feature).
  2. Successive gossip_timestamp_filter.

On the former, you really want to send all gossip that matches. On the latter, it could well apply only to future gossip, which would save load on the server; we currently seek back to the start to find any gossip which might match your updated timestamp.

In practice, we only send this in a binary fashion: start = 10 minutes ago, when we want to get gossip, and start = 0xFFFFFFFF, when we want to suppress it. We choose 3 peers to gossip with, generally.

rustyrussell commented 2 years ago

Suggest we turn the timestamp filter into a trinary:

This matches current use in practice and is trivial to implement.