Avoid applying onion's channel updates in an observable way

[tnull]

Fixes #2598.

If we receive a channel update from an intermediary via a failure onion we shouldn't apply them in a persisted and network-observable way to our network graph, as this might introduce a privacy leak.

Here, we therefore avoid applying such updates to our network graph.

[tnull]

Currently in draft until the approach is clarified. I already started some commits including the ChannelUpdate in PaymentParameters::previously_failed_channels, but don't think this is necessary if we don't reuse any previously failed channels anyways.

[codecov-commenter]

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (1852715) 88.98% compared to head (e64a293) 88.94%.

❗ Current head e64a293 differs from pull request most recent head 1c35255. Consider uploading reports for the commit 1c35255 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2666      +/-   ##
==========================================
- Coverage   88.98%   88.94%   -0.04%     
==========================================
  Files         112      112              
  Lines       87632    87663      +31     
  Branches    87632    87663      +31     
==========================================
- Hits        77978    77975       -3     
- Misses       7421     7441      +20     
- Partials     2233     2247      +14     

Files	Coverage Δ
lightning/src/ln/onion_utils.rs	91.52% <100.00%> (+0.01%)	⬆️
lightning/src/ln/functional_test_utils.rs	90.46% <0.00%> (-0.59%)	⬇️
lightning/src/routing/gossip.rs	85.89% <61.29%> (-0.40%)	⬇️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TheBlueMatt]

Yea, this makes sense to me. Lets just remove the network_update field in PathFailure::OnPath entirely?

[tnull]

Yea, this makes sense to me. Lets just remove the network_update field in PathFailure::OnPath entirely?

Mh, not sure if we want to keep the ability to manually apply updates to a graph around for users knowing what they doing? But if we want to remove it, I think it may make sense to also drop NetworkGraph::handle_network_update and the entire NetworkUpdate type?

[TheBlueMatt]

I mean, I think (a) we think its a terrible idea to apply the update, and would be a lot of work to do so safely (you'd probably have to keep a second copy of the network graph), and (b) we expect to receive the update via the normal gossip network soon anyway, so its not like we're missing out for too long, and (c) we score the channel negatively cause the payment failed (I hope, need to double-check that?) so we shouldn't be retrying over the same channel soon even for a new payment, and (d) probably the network updates will go away in the spec cause its such a bad issue anyway....

I don't really think its worth keeping a bunch of code around for such a rarely-useful case, much better to have less code :)

I do think we should keep NetworkGraph::handle_network_update just cause who knows where people are getting their gossip data. I'm a bit torn on applying the "perm fail" case still, because while it exhibits a similar issue, the cost to attack is much higher (you lose out on a lot of fees going forward cause we never route through you now) and its nice to remove graph entries optimistically (we don't currently look at the chain, so the only other reason we remove entries is if they aren't getting any more updates).

[tnull]

I mean, I think (a) we think its a terrible idea to apply the update, and would be a lot of work to do so safely (you'd probably have to keep a second copy of the network graph), and (b) we expect to receive the update via the normal gossip network soon anyway, so its not like we're missing out for too long, and (c) we score the channel negatively cause the payment failed (I hope, need to double-check that?) so we shouldn't be retrying over the same channel soon even for a new payment, and (d) probably the network updates will go away in the spec cause its such a bad issue anyway....

I don't really think its worth keeping a bunch of code around for such a rarely-useful case, much better to have less code :)

Alright, makes sense.

I do think we should keep NetworkGraph::handle_network_update just cause who knows where people are getting their gossip data.

Hum, but they'd likely receive gossip data as NodeAnnouncements/ChannelAnnouncements/ChannelUpdates anyways, not NetworkUpdate, which really is only a, to quote its docs, "Update to the NetworkGraph based on payment failure information conveyed via the Onion return packet by a node along the route.", and handle_network_update "Handles any network updates originating from Events."?

Given that it's really just a wrapper type used for the one purpose we're about to drop, it's really tempting to drop all that associated code.

[TheBlueMatt]

Hum, but they'd likely receive gossip data as NodeAnnouncements/ChannelAnnouncements/ChannelUpdates anyways, not NetworkUpdate, which really is only a, to quote its docs, "Update to the NetworkGraph based on payment failure information conveyed via the Onion return packet by a node along the route.", and handle_network_update "Handles any network updates originating from Events."?

Oh, duh, yea.

[TheBlueMatt]

Given that it's really just a wrapper type used for the one purpose we're about to drop, it's really tempting to drop all that associated code.

Yea, I'm just a bit torn on removing from perm failures - it does seem like something worth doing given we don't currently look at the chain to remove after the funding outpoint is spent (and rely on timeouts of the channel_updates). The timeouts are after a week or two, though.

[tnull]

Yea, I'm just a bit torn on removing from perm failures - it does seem like something worth doing given we don't currently look at the chain to remove after the funding outpoint is spent (and rely on timeouts of the channel_updates). The timeouts are after a week or two, though.

Mh, will think about that once more, but currently have no strong opinion on it. I now pushed a commit removing the failure field and dependants. If we're positive we want to go this way, I may look into also removing NetworkUpdate etc.

[tnull]

Alright, after more and more backpedaling I now pushed an MVP that just skips application of the NetworkUpdate::ChannelUpdateMessages. I think we should see how to remove them entirely eventually, but this will be a much larger change that requires more coordination beforehand to not lose too much test coverage and not break compatibilty.