Add a method to prune stale channels from NetworkGraph

[TheBlueMatt]

We define "stale" as "haven't heard an updated channel_update in
two weeks", as described in BOLT 7.

We also filter out stale channels at write-time for std users, as
we can look up the current time.

[codecov-commenter]

Codecov Report

Merging #1212 (1c7dd91) into main (61518f9) will increase coverage by 0.40%.
The diff coverage is 90.66%.

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

@@            Coverage Diff             @@
##             main    #1212      +/-   ##
==========================================
+ Coverage   90.57%   90.97%   +0.40%     
==========================================
  Files          69       69              
  Lines       37455    40428    +2973     
==========================================
+ Hits        33925    36780    +2855     
- Misses       3530     3648     +118     

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	83.89% <ø> (ø)
lightning/src/routing/network_graph.rs	93.82% <90.66%> (+2.46%)	⬆️
lightning/src/ln/functional_tests.rs	97.63% <0.00%> (+0.20%)	⬆️
lightning/src/ln/msgs.rs	87.43% <0.00%> (+0.90%)	⬆️
lightning-background-processor/src/lib.rs	95.91% <0.00%> (+0.91%)	⬆️
lightning/src/ln/channel.rs	91.97% <0.00%> (+2.90%)	⬆️
lightning/src/ln/peer_handler.rs	53.71% <0.00%> (+3.37%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61518f9...bb22a71. Read the comment docs.

[valentinewallace]

Pretty much LGTM

[jkczyz]

Why use this test? It's used to test handling of NetworkUpdate from PaymentPathFailed events, which is unrelated to this.

[TheBlueMatt]

Because I didn't want to sit down and write out an entirely redundant test which copies all the manual message creation stuff. I could, I suppose, but its a bunch of additional test code to do the same thing over and over, which tends to create pain later.

[jkczyz]

Wouldn't the handling_channel_update test be a better place? It's already testing timestamp-related code.

[TheBlueMatt]

Hmm, that handles rejecting some channel updates (which ideally we'd test the new "reject if in the past or future" checks in, but sadly for now we have to disable those in testing as ~every one of our tests currently fails with it :/), but network_update here tests removing things which felt like it lined up better? I dunno, if you feel strongly I can move it.

[jkczyz]

Hmm... could we just make a new test and copy the setup part, then? That part really isn't much code and could always be refactored later. I just have a preference for keeping tests limited as it makes reading them easier than if they have conditional logic about what is being tested.

[TheBlueMatt]

Ok, I refactored the tests in network_graph top-to-bottom so that adding new tests doesn't result in nearly as much code duplication then moved the new test code into a new tests.

[jkczyz]

Not sure I understand what is going on here with 100 vs 101. Aren't both well in the past?

[TheBlueMatt]

The updates created in the test have timestamps of 100.

[jkczyz]

This may be a bit of surprising side effect. Perhaps it should be documented on the struct?

[TheBlueMatt]

Yea, I waffled on it. I'm happy to remove it if you feel strongly, but it did seem nice to "do the obvious thing that users want" here. I suppose its awkward that there's no way to turn it off, but if users want a historical channel view they'll need to do their own thing anyway. In terms of things "just working", I think the only alternative would be to deref the NetGraphMsgHandler and call the timeout function regularly in the background-processor. For now I'll document it but I'm open to discussion either way.

[jkczyz]

NetGraphMsgHandler (possibly renamed in the future :) seems like something that should be responsible for this. I'd even argue the method could be moved there as well. And then, as you said, called in the background processor.

[TheBlueMatt]

Wait, why would a message handler -> storage backend adapter be responsible for pruning? I'm fine with doing it in BP, but the message handler?

[jkczyz]

Oh, I was just alluding to my opinion that message handling is simply one implementation detail (literally impl RoutingMessageHandler) of a broader gossip abstraction, of which the latter would also encompass pruning the network graph either by timestamp or monitoring the chain. Fine to keep this in NetworkGraph for now. Either way, it will be accessed through NetGraphMsgHandler in the background processor.

[TheBlueMatt]

Oh, I was just alluding to my opinion that message handling is simply one implementation detail (literally impl RoutingMessageHandler) of a broader gossip abstraction, of which the latter would also encompass pruning the network graph either by timestamp or monitoring the chain.

Huh, definitely feel like if we can keep these separate we should. But, sure, for now I'll move it to BP.

[valentinewallace]

Still going through a first pass. I'm getting the same weird CI failures on #1177

[valentinewallace]

Couldn't we make this a u32? Seems other unix timestamps are u32s

[TheBlueMatt]

Unix timestamps on the wire for lightning are u32s to save space, unix timestamps in most (reasonable) APIs are u64s as u32s have a 2038/2106 problem.

[valentinewallace]

Shaping up!

[valentinewallace]

Very nice test cleanup.

CI sad

[TheBlueMatt]

Squashed without diff:

$ git diff-tree -U1 affab46a4 bb22a716d
$

[TheBlueMatt]

Oops, looks like Instant on OSX + Windows is relative to process (or maybe host?) start, so we can't reliably subtract an hour from it. Swapped for an explicit bool check. Note that remaining CI failures should be #1214.

$ git diff-tree -U1 bb22a716d 801aae97a
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index 8bd115b0d..350570ea1 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -217,6 +217,4 @@ impl BackgroundProcessor {
 			let mut last_ping_call = Instant::now();
-			// Note that we want to run a graph prune once not long after startup before falling
-			// back to our usual hourly prunes. This avoids short-lived clients never pruning their
-			// network graph. We default here to once a minute after startup.
-			let mut last_prune_call = Instant::now() + Duration::from_secs(60) - Duration::from_secs(NETWORK_PRUNE_TIMER);
+			let mut last_prune_call = Instant::now();
+			let mut have_pruned = false;
 
@@ -260,3 +258,7 @@ impl BackgroundProcessor {
 
-				if last_prune_call.elapsed().as_secs() > NETWORK_PRUNE_TIMER {
+				// Note that we want to run a graph prune once not long after startup before
+				// falling back to our usual hourly prunes. This avoids short-lived clients never
+				// pruning their network graph. We run once 60 seconds after startup before
+				// continuing our normal cadence.
+				if last_prune_call.elapsed().as_secs() > if have_pruned { NETWORK_PRUNE_TIMER } else { 60 } {
 					if let Some(ref handler) = net_graph_msg_handler {
@@ -265,2 +267,3 @@ impl BackgroundProcessor {
 						last_prune_call = Instant::now();
+						have_pruned = true;
 					}

[jkczyz]

Thanks for cleaning up the tests! Feel free to ignore the comment if you don't want to affect the blame layer.

[jkczyz]

I think some of the scopes in these tests are hanging around from a former locking mechanism. Probably could remove them if you're touching most of the code within them. Would simplify the short_channel_id assignment here and elsewhere, for instance.

[TheBlueMatt]

Yea, would prefer to just do this later. Like Val noted above there's more to clean up here, but changing all the tests is pretty unrelated to this PR so I'm hesitant to Do It All right now.

[TheBlueMatt]

Rebased to pick up CI fix. range-diff:

$ git range-diff 61518f97212fd6a4c40cdd62a3ee5ab7a15d1971...801aae97a70beabeb169bc594f7089d3a48694a3 6e27ca0316ee161ae328394606d708b1d4fd8ab0..73e8dc41a6c277b001587d88426f0c0872f21a0f
1:  330cf750a ! 1:  11d644824 DRY up network_graph tests substantially with message creation fns
    @@ lightning/src/routing/network_graph.rs: mod tests {
     +          }, node_1_privkey, &secp_ctx);
                match net_graph_msg_handler.handle_channel_update(&valid_channel_update) {
                        Ok(_) => panic!(),
    -                   Err(e) => assert_eq!(e.err, "Update older than last processed update")
    +                   Err(e) => assert_eq!(e.err, "Update had same timestamp as last processed update")
                };
     -          unsigned_channel_update.timestamp += 500;
      
2:  c76dbc719 = 2:  c57542963 Drop `allow_wallclock_use` feature in favor of simply using `std`
3:  13505d85d = 3:  4677e14c0 Add a method to prune stale channels from `NetworkGraph`
4:  f597c8ee9 = 4:  cd43ff4a5 Reject channel_update messages with timestamps too old or new
5:  801aae97a = 5:  73e8dc41a Automatically prune NetworkGraph of stale channels hourly in BP
$