Prune channels if either not updated + track pruning time

[naumenkogs]

Closes #1671

[naumenkogs]

Marking this draft until #1649 is merged.
I might add some tests once I get some initial code approval.

[TheBlueMatt]

Alright, sorry for the delay...#1649 has landed so we should be good for a rebase and then good to go here!

It did end up with a nice solution for no-std and std using the explicit numbers like you had here, so that should reduce the diff size here.

[codecov-commenter]

Codecov Report

Base: 90.72% // Head: 90.73% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (ac1c21f) compared to base (2e343e7).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head ac1c21f differs from pull request most recent head 1769e34. Consider uploading reports for the commit 1769e34 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1735      +/-   ##
==========================================
+ Coverage   90.72%   90.73%   +0.01%     
==========================================
  Files          87       87              
  Lines       47364    47374      +10     
  Branches    47364    47374      +10     
==========================================
+ Hits        42970    42984      +14     
+ Misses       4394     4390       -4     

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	92.20% <100.00%> (+0.04%)	⬆️
lightning/src/ln/peer_channel_encryptor.rs	93.38% <0.00%> (-0.25%)	⬇️
lightning/src/chain/channelmonitor.rs	90.65% <0.00%> (-0.06%)	⬇️
lightning/src/ln/channelmanager.rs	85.18% <0.00%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.00% <0.00%> (+0.02%)	⬆️
lightning-net-tokio/src/lib.rs	77.64% <0.00%> (+0.90%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[dunxen]

ACK 1769e34

Does what it says on the tin.

[naumenkogs]

Please be attentive to the first commit btw, this is the thing I'm least confident in :)

[dunxen]

Please be attentive to the first commit btw, this is the thing I'm least confident in :)

Hmm. Right, I didn't really look into why that test would have had a problem with the following commit. Was more checking the removal logic. Maybe someone has an idea otherwise I'll try debug.

[TheBlueMatt]

So the issue for the first commit is that the test only fills in one direction of the channel, rather than two. You should consider swapping the first commit with this instead (but maybe with a comment in the function that explains that flags |= 1 means "other direction").

diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index 1706e0692..36fe9c135 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -2573,6 +2573,10 @@ mod tests {
                assert!(gossip_sync.handle_channel_update(&valid_channel_update).is_ok());
                assert!(network_graph.read_only().channels().get(&short_channel_id).unwrap().one_to_two.is_some());
 
+               let valid_channel_update_2 = get_signed_channel_update(|update| {update.flags |=1;}, node_2_privkey, &secp_ctx);
+               gossip_sync.handle_channel_update(&valid_channel_update_2).unwrap();
+               assert!(network_graph.read_only().channels().get(&short_channel_id).unwrap().two_to_one.is_some());
+
                network_graph.remove_stale_channels_and_tracking_with_time(100 + STALE_CHANNEL_UPDATE_AGE_LIMIT_SECS);
                assert_eq!(network_graph.read_only().channels().len(), 1);
                assert_eq!(network_graph.read_only().nodes().len(), 2);

[TheBlueMatt]

Further, once this and #1815 lands, we should adapt the check in #1815 to check for both directional updates being filled in rather than just one - what will end up happening here is, because we don't persist the removed set, on startup we'll re-sync the channels that only have one directional update filled in, then keep it around until the info.announcement_received_time < min_time_unix as u64 check passes. During that time we'll happily route over a channel that we consider useless and a candidate for removal, and we should not.

[naumenkogs]

I pushed a broken code, nvm, fixing.

---

Alright fixed the code but will fix the tests a bit later.

[TheBlueMatt]

I don't think we need to do this at the router level, but rather directly in as_directed_from/to. This is gonna need a bunch of test updates, though, so may be worth waiting for a followup pr.