Skip to content

BOLT07: don't send historical updates for gossip_timestamp_filter#641

Closed
cfromknecht wants to merge 1 commit intolightning:masterfrom
cfromknecht:no-send-historical-updates
Closed

BOLT07: don't send historical updates for gossip_timestamp_filter#641
cfromknecht wants to merge 1 commit intolightning:masterfrom
cfromknecht:no-send-historical-updates

Conversation

@cfromknecht
Copy link
Collaborator

This commit removes the suggestion for nodes to respond with all known
updates that satisfy the timestamps of a gossip_timestamp_filter
message. This behavior is identical to using the initial_routing_sync,
which has been deprecated due to the inefficient nature of sending the
entire graph on every connection. Now, the requirements simply say that
only new messages should be sent to peers.

Given the upcoming addition of extended_gossip_queries,
implementations should use this feature to locate missing graph
information instead of relying the on the remote peer to dump
everything.

This commit removes the suggestion for nodes to respond with all known
updates that satisfy the timestamps of a gossip_timestamp_filter
message. This behavior is identical to using the `initial_routing_sync`,
which has been deprecated due to the inefficient nature of sending the
entire graph on every connection. Now, the requirements simply say that
only new messages should be sent to peers.

Given the upcoming addition of `extended_gossip_queries`,
implementations should use this feature to locate missing graph
information instead of relying the on the remote peer to dump
everything.
@cfromknecht
Copy link
Collaborator Author

Looking to get some feedback, this requirement caused a lot of issues as the graph grew last year. Given that we already tried to deprecate initial_routing_sync, I think we should move to not sending historical updates on every connection. Thoughts?

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

Choose a reason for hiding this comment

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

SHOULD send all gossip messages whose timestamp is greater or
equal to first_timestamp, and less than first_timestamp plus
timestamp_range.

This still seems like something we'd want to keep. Perhaps it's better to note that implementations should be aware of the consequences of setting first_timestamp to a value too far in the past?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This still seems like something we'd want to keep.

Why? The whole point of moving to gossip_queries was to avoid dumping the full channel graph on every connection, and you can't really make use of this without triggering it. Maybe it should be changed to MAY rather than SHOULD?

Perhaps it's better to note that implementations should be aware of the consequences of setting first_timestamp to a value too far in the past?

We want to be able to set timestamps in the past, that way we catch new updates arriving at tip that may be slow to propagate due to the staggered gossip. You could say that then we shouldn't set it too far back, but how do we arrive at that value.

It becomes harder to pick a value when some nodes send historical traffic and others don't. You have to choose a balance between DOSing the remote node and forcing yourself to filter out a mound of gossip data, or just accept the loss and miss out on slow-to-propagate updates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also with extended_gossip_queries I think it makes this functionality redundant, and we should encourage people to get missing data through that rather dumping the graph

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you really want a complete gossip blast, you can always reconnect. Generally asking for everything to be re-transmitted is a bit antisocial.

@rustyrussell
Copy link
Collaborator

rustyrussell commented Jul 16, 2019

We currently rely on this behavior for the case where we find that gossip is somehow missing (pick a random peer, ask for everything). We need to be more refined in our search, I think, but that was easy to implement.

@pm47
Copy link
Collaborator

pm47 commented Jul 16, 2019

We want to be able to set timestamps in the past, that way we catch new updates arriving at tip that may be slow to propagate due to the staggered gossip. You could say that then we shouldn't set it too far back, but how do we arrive at that value.

It's almost as if we needed a more fine grained mechanism, maybe like #557, which we have been working on for 6+ months. Then you could set first_timestamp to now and sync older messages easily.

The inefficient sync algorithm has caused us a lot of trouble with Eclair Mobile for the better part of the past 12 months. I think we should instead move forward with #557 which addresses the core issue.

@cfromknecht
Copy link
Collaborator Author

@pm47 this doesn't replace #557, it is complimentary. This is about reducing everyone's dependence on this insanely inefficient method of synchronization, now that we have the more efficient method outlined in #557.

It's honestly just not practical to respond to these queries, esp. on mobile if you have a channel partner that requests the whole graph every time the app is opened. I've been experimenting with ripping this out of lnd simply because it's heavy and easy to abuse.

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.

5 participants