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

BOLT07: prune if oldest channel_update is > 2 weeks old #767

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

cfromknecht
Copy link
Collaborator

@cfromknecht cfromknecht commented Apr 13, 2020

This PR modifies the recommended pruning requirements to use the oldest
channel_update's timestamp rather than the latest. The rationale is that
using the latest timestamp inadvertently retrains channels for which only
one side of the channel continues to send fresh channel_updates.

Consider a new node that makes a channel to y'alls, but then disappears.
The current pruning strategy will keep the channel in its routing table
because y'alls continues to send fresh channel_updates, but the channel
is actually unusable because the other party is never online. The new strategy
will remove these low-success nodes/channels from the routing table and
only keep channels where both endpoints have indicated recent activity.

This reduces the number of channels in the public graph by 25% at the time
of writing.

EDIT: updated stats, see relevant network stats

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK, this is clearly something we need to do! EDIT: see my next comment, unfortunately the real world doesn't seem to play so nicely with this :/

I ran the same kind of computation last week and just re-ran it, but in my case it's a 25% reduction in the number of channels. Is there another pruning heuristic you added on top to get to 40% (like rejecting some incoming channel_updates)? Otherwise you may be missing 15% of valid channels in the network somehow...

@t-bast
Copy link
Collaborator

t-bast commented Apr 14, 2020

Consider a new node that makes a channel to y'alls, but then disappears.

By disappear do you mean goes offline or stays online but inactive? If he goes offline, y'alls should not be sending any channel_update so the pruning should happen without any change to the spec.

If you meant stays online but inactive, unfortunately it looks like in practice it's not working correctly.
I'm seeing many channels that are clearly active and shouldn't be pruned for which I get channel_updates for one side less regularly than once every two weeks. I ran this pruning and sorted the pruned channels by capacity, and looked at the top ones. When doing that I see many channels from BlueWallet that don't send a channel_update regularly enough (for example for 611448x1083x1 I didn't receive any update between march 11 and april 9 from the BlueWallet side). I don't know if it's because BlueWallet doesn't send those updates, or if they're not propagated correctly all the way to my node. Could you do the same experiment with your data?

But in any case, I definitely don't want to prune those channels. Maybe a safer spec addition would be to require that in a channel A <-> B, B shouldn't send a channel_update if A hasn't sent one in 14 days? Because if B is the most well-placed to know whether A sent a channel_update or not (whereas remote nodes may simply be unlucky and didn't correctly get the gossip propagated to them for some reason that still eludes me). That means it would take a month to get rid of those inactive channels (instead of 14 days).

Maybe the real issue is a bug that prevents nodes from correctly sending their regular channel_updates, or from having them propagated throughout the network? And once that's fixed we'd see that we would get close to no gain from that extra pruning?

I think many of us need to be testing what channels that proposal would prune on their node, and verify that these are channels that really should be pruned.

@cdecker
Copy link
Collaborator

cdecker commented Apr 24, 2020

By disappear do you mean goes offline or stays online but inactive? If he goes offline, y'alls should not be sending any channel_update so the pruning should happen without any change to the spec.

This is what I'd expect to happen: channel is unusable, don't update after the last disable message, and cause the channel to slowly be forgotten. Why would yalls continue to send updates?

If the channel becomes active again at a later time, it should send an announcement followed by an update so everybody learns about its existence again. Eagerly pruning around nodes mistakenly sending channel updates seems weird. For example a node may never activate its side of the channel with an update because it doesn't have outgoing capacity, that doesn't mean the channel as a whole is unusable.

@niftynei
Copy link
Collaborator

imo 'oldest channel update' is unclear -- do you mean 'either node's most-recent channel_update' is older than 2 weeks?

@TheBlueMatt
Copy link
Collaborator

I dont have super strong feelings about the actual logic, but the text in BOLT 7 needs significant tweaks for this.

"SHOULD base timestamp on a UNIX timestamp." needs to get an additional "MUST set timestamp to a number greater than the time in the median of the last 11 Bitcoin block headers" added. Further, to reduce the risk of time-based issues, I think the thing being changed here should be tweaked to reference MTP as well, instead of wall-clock time, given P2P apps tend to get the Wrong Time all the time.

@cfromknecht
Copy link
Collaborator Author

Further, to reduce the risk of time-based issues, I think the thing being changed here should be tweaked to reference MTP as well, instead of wall-clock time, given P2P apps tend to get the Wrong Time all the time.

I wouldn't be surprised if some of the existing propagation issues are related to poorly synced clocks

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🗿

@niftynei
Copy link
Collaborator

ACK dc43078

@rustyrussell
Copy link
Collaborator

@rustyrussell rustyrussell merged commit 7e8c478 into lightning:master Aug 20, 2020
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 20, 2020
See lightning/bolts#767

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: channels now pruned after two weeks unless both peers refresh it (see lightning-rfc#767)
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 20, 2020
See lightning/bolts#767

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: channels now pruned after two weeks unless both peers refresh it (see lightning-rfc#767)
rustyrussell added a commit to ElementsProject/lightning that referenced this pull request Aug 24, 2020
See lightning/bolts#767

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: channels now pruned after two weeks unless both peers refresh it (see lightning-rfc#767)
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 25, 2020
See lightning/bolts#767

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: channels now pruned after two weeks unless both peers refresh it (see lightning-rfc#767)
@cfromknecht cfromknecht deleted the stricter-pruning branch March 9, 2021 18:43
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.

7 participants