Implement permanent node failure handling #1649

dunxen · 2022-08-04T11:26:35Z

We never had the NetworkGraph::node_failed method implemented. The scorer now handles non-permanent failures by downgrading nodes, so we don't want that implemented.

The method is renamed to node_failed_permanent to explicitly indicate that this is the only case it handles.
There is a test case for non-permanent failure which asserts that the graph local storage does not change, and one for permanent failure which checks that the node and associated channels were removed.

Fixes #1644

dunxen · 2022-08-04T12:45:03Z

Oof, will fix up in a bit.

lightning/src/routing/gossip.rs

TheBlueMatt · 2022-08-06T00:00:35Z

One general issue - what will happen in practice here is we'll remove a node and then immediately re-add it when we resync gossip. Now, its probably not actually a big deal because we never actually get perm-node-failures, but we should implement something to track "I removed node id X" and just remember that for like a week or two. We should also do the same for channels, which we currently dont.

dunxen · 2022-08-07T17:32:23Z

we should implement something to track "I removed node id X" and just remember that for like a week or two

Thanks for the insight here. Do we by chance track anything else in a similar way? Not sure what would be the most appropriate way to implement this.

TheBlueMatt · 2022-08-07T18:19:15Z

Hmm, I don't think so. We should be able to track it just as a HashMap here, from the node_id/SCID to the time we decided to remove it. Then when we do expiration passes in the network graph we can do a similar pass on the map here and remove anything that was added more than a week ago.

dunxen · 2022-08-15T10:49:17Z

Hmm. So I'm quite stumped with the "lock order violation" in the test. I've narrowed it down to network_graph.channels.write() in node_failed_permanent(), but can't see anything obvious here. I don't see any specific scoping issues at least.

I'll get started on the expirations after the lock issue is resolved.

lightning/src/routing/gossip.rs

dunxen · 2022-08-16T13:38:01Z

Hmm, I don't think so. We should be able to track it just as a HashMap here, from the node_id/SCID to the time we decided to remove it. Then when we do expiration passes in the network graph we can do a similar pass on the map here and remove anything that was added more than a week ago.

So I'm currently thinking of storing the HashMap in NetworkGraph behind a mutex, and then since we will have time involved I'm basically going to create extra no-std versions of the methods that take in UNIX time like some of the other methods already do (unless that's unnecessary). NetworkGraph::channel_failed would also need one.

Then of course for a while ignoring those nodes/channels when resyncing gossip, so they're not re-added.

Is that what you had imagined somewhat?

TheBlueMatt · 2022-08-16T17:26:36Z

Yea, that all sounds basically good. I wonder if we should just disable this behavior entirely for no-std - we don't currently have a decent way to measure the passing of time in the network graph, and while we could receive block header updates there to do it like we do elsewhere, I'm a bit hesitant to bother.

dunxen · 2022-08-18T06:58:25Z

I wonder if we should just disable this behaviour entirely for no-std - we don't currently have a decent way to measure the passing of time in the network graph, and while we could receive block header updates there to do it like we do elsewhere, I'm a bit hesitant to bother.

I guess it's also not super likely that path finding would be something that happens on (embedded) devices that use no-std. At least to me, it seems that would be much harder to do nicely anyway.

dunxen · 2022-08-18T14:39:23Z

Almost ready to be promoted from draft. Just need to add the tracking/expiry test and clean up. Pushed these for transparency into current work.

I did go the way of not adding nodes to the HashMap or expiring them for no_std. Probably could be cleaned up a little too.

lightning/src/routing/gossip.rs

TheBlueMatt

If we're not going to remove from the new maps on no-std, we can't insert into them either.

lightning-background-processor/src/lib.rs

lightning/src/routing/gossip.rs

dunxen · 2022-08-18T19:30:40Z

If we're not going to remove from the new maps on no-std, we can't insert into them either.

Definitely missed this, thanks!

lightning/src/routing/gossip.rs

dunxen · 2022-08-26T10:34:48Z

Actually going to squash before opening up for review as a lot has changed and it's a bit cumbersome to review right now. Plus some failing checks still.

codecov-commenter · 2022-08-26T11:21:03Z

Codecov Report

Base: 90.73% // Head: 90.85% // Increases project coverage by +0.11% 🎉

Coverage data is based on head (468814f) compared to base (5211bfd).
Patch coverage: 96.74% of modified lines in pull request are covered.

❗ Current head 468814f differs from pull request most recent head 243ca91. Consider uploading reports for the commit 243ca91 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1649      +/-   ##
==========================================
+ Coverage   90.73%   90.85%   +0.11%     
==========================================
  Files          87       87              
  Lines       46688    47601     +913     
  Branches    46688    47601     +913     
==========================================
+ Hits        42361    43246     +885     
- Misses       4327     4355      +28

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	`96.23% <ø> (ø)`
lightning/src/routing/gossip.rs	`92.23% <96.74%> (+0.79%)`	⬆️
lightning/src/chain/onchaintx.rs	`94.71% <0.00%> (-0.92%)`	⬇️
lightning/src/util/events.rs	`38.13% <0.00%> (-0.27%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.08% <0.00%> (+0.01%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.03% <0.00%> (+0.30%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.92% <0.00%> (+1.80%)`	⬆️
lightning/src/util/wakers.rs	`89.26% <0.00%> (+2.55%)`	⬆️

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.

lightning/src/util/time.rs

lightning/src/routing/gossip.rs

lightning/src/util/time.rs

lightning/src/routing/gossip.rs

TheBlueMatt · 2022-09-18T12:58:53Z

lightning/src/routing/gossip.rs

+
+		#[cfg(feature = "std")]
+		{
+			self.removed_channels.lock().unwrap().retain(|_, time| time.elapsed().as_secs() < REMOVED_ENTRIES_TRACKING_AGE_LIMIT_SECS);


Not a huge deal, but it'd be nice to use current_time_unix here, its a bit strange to pass it in and then use the wall-clock time.

Hmm yeah it would be nice but I'm not sure I quite get how to make that work when we have Instants :(

Maybe we should swap the Instant for a u64? If not, we at least need to only fetch the time once in these retains, not for each element. I'm not sure if there's a platform where it actually is materially slow (ie a syscall), but not all platforms are as fast as rtdsc, so best to not, IMO.

Could we convert to Duration using Time::duration_since_epoch?

rust-lightning/lightning/src/util/time.rs

Lines 76 to 78 in 7bc52aa

fn duration_since_epoch() -> Duration {

use std::time::SystemTime;

SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap()

Then we can continue to use Time throughout and in tests. Or at least use Time still but convert to Duration before putting it into the HashMap.

No, since-epoch is the system clock, not the monotonic clock. We shouldn't use it for time keeping.

I think he's also tracking for no-std. Can't remember why. Maybe we just not do that in that patch too?

Anyway, let me do last fixups and get this ready for merge

Right i guess that's the question - do we want to do all this tracking with no-std as well or only std. I'm not sure any of this matters all that much - on a week timescale they should be equivalent, and no-std users are unlikely to want a full graph I assume.... either way let's keep it consistent, I don't really care, just let's not query every iteration.

I guess if I had to pick id pick supporting no-std and using system clock over not, but...it's a wash

Ok well, supporting no-std and using system time will clear up a lot of the conditional compilation and I suppose on the order of a week, system time is alright.

Then we will also be using the current_unix_time arg and other nice things. If we ever decide to serialise and persist the maps later then it's easy also.

Please leave a big fat comment explaining this :)

TheBlueMatt · 2022-09-21T13:14:17Z

Feel free to squash, I think.

TheBlueMatt · 2022-09-22T15:00:42Z

lightning/src/routing/gossip.rs

 					*entry.get_mut() = channel_info;
+					return Ok(());


Ah, hmm, I may take that back - I guess in principle the nodes on the channel can change in a reorg (ie cause of a short-channel-id collision where another channel takes the old channel's place on a reorg). In that case we need to remove the entries for this channel on the old nodes and re-add the entries for the new nodes, which I guess is why the code was the way it was.

I wouldn't have remembered why I did it that particular way but the reasoning sounds familiar.

You did it this way because I asked you to change it and suggested this would be a totally fine and acceptable change....sorry about that.

TheBlueMatt

One last nit, then LGTM, for real this time.

TheBlueMatt · 2022-09-26T10:59:39Z

lightning/src/routing/gossip.rs

+
+		#[cfg(feature = "std")]
+		{
+			self.removed_channels.lock().unwrap().retain(|_, time| time.elapsed().as_secs() < REMOVED_ENTRIES_TRACKING_AGE_LIMIT_SECS);


Maybe we should swap the Instant for a u64? If not, we at least need to only fetch the time once in these retains, not for each element. I'm not sure if there's a platform where it actually is materially slow (ie a syscall), but not all platforms are as fast as rtdsc, so best to not, IMO.

TheBlueMatt · 2022-09-26T11:04:04Z

@jkczyz wanna take another look?

jkczyz · 2022-09-26T17:01:24Z

lightning/src/routing/gossip.rs

+			for scid in node.channels.iter() {
+				if let Some(chan_info) = channels.remove(scid) {
+					let other_node_id = if node_id == chan_info.node_one { chan_info.node_two } else { chan_info.node_one };
+					if let BtreeEntry::Occupied(mut node_entry) = nodes.entry(other_node_id) {


s/node_entry/other_node_entry

jkczyz · 2022-09-26T17:04:27Z

lightning/src/routing/gossip.rs

+				}
+			}
+			#[cfg(feature = "std")]
+			removed_nodes.insert(node_id, time);


If the other node has no more channels, should it also be removed? Or will that be a problem if another channel is advertised for it?

Ah, actually, I guess this only for permanent failures for the passed in node.

Yeah exactly. We don't want to track it as removed as it wasn't a permanent failure.

jkczyz · 2022-09-26T17:16:23Z

lightning/src/routing/gossip.rs

+
+		#[cfg(feature = "std")]
+		{
+			self.removed_channels.lock().unwrap().retain(|_, time| time.elapsed().as_secs() < REMOVED_ENTRIES_TRACKING_AGE_LIMIT_SECS);


Could we convert to Duration using Time::duration_since_epoch?

rust-lightning/lightning/src/util/time.rs

Lines 76 to 78 in 7bc52aa

fn duration_since_epoch() -> Duration {

use std::time::SystemTime;

SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap()

Then we can continue to use Time throughout and in tests. Or at least use Time still but convert to Duration before putting it into the HashMap.

jkczyz · 2022-09-26T17:34:50Z

lightning/src/routing/gossip.rs

+			#[cfg(feature = "std")]
+			removed_nodes: Mutex::new(HashMap::new()),
+			#[cfg(feature = "std")]
+			removed_channels: Mutex::new(HashMap::new()),


Should we write these out as Duration like we do in the scorer? Related, see other comment about using Time but putting a Duration in the HashMap.

dunxen · 2022-09-28T11:38:52Z

Ok, so there was enough change to warrant an extra commit just for review. I'll squash when everyone is happy :)

jkczyz · 2022-09-28T17:25:47Z

lightning/src/routing/gossip.rs

+	#[cfg(feature = "std")]
 	/// Marks a channel in the graph as failed if a corresponding HTLC fail was sent.
 	/// If permanent, removes a channel from the local storage.
 	/// May cause the removal of nodes too, if this was their last channel.
 	/// If not permanent, makes channels unavailable for routing.
+	///
+	/// This method is only available with the `std` feature. See
+	/// [`NetworkGraph::channel_failed_with_time`] for `no-std` use.
 	pub fn channel_failed(&self, short_channel_id: u64, is_permanent: bool) {


This is called in the NetworkGraph's event handler, which doesn't have any conditioning on std.

Ahh right. I think we run into the issue of not being able to pass through current_time_unix from somewhere for no-std. I’ll think about it for a bit.

So there's no way for us to pass through current UNIX time in the even handler for no-std without changing the EventHandler and EventsProvider traits to maybe support passing it in form the public API :/

Where would someone using no-std get the current Unix time from? If they could implement util::time::Time to return it, then NetworkGraph could be parameterized by it if we desire this behavior in no-std.

Right, duh, uhmmmm....so let's do two things - let's track the last timestamp where the user ran the cleanup method (and if one is set, use that for removal time), and if not, we'll have to not track removals in no-std. Sorry about the mess here.

No worries. I kind of didn't see the whole picture until going back and forth on this one.

dunxen · 2022-10-01T10:36:21Z

Ok let's try that again and squashed to avoid a review mess :)

Edit: Oh stupid me. Will refactor not to use std::swap and instead use core::swap

naumenkogs · 2022-10-04T08:57:33Z

lightning/src/routing/gossip.rs

+	//     week).
+	//
+	/// Keeps track of short channel IDs for channels we have explicitly removed due to permanent
+	/// failure so that we don't resync them from gossip. Each SCID is mapped to the time in seconds


nit: I think this comment is too verbose and may be compressed x2.

Keeps track of short channel IDs for channels we have explicitly removed due to permanent failure so that we don't resync them from gossip. Each SCID is mapped to the time (in seconds) it was removed so that once some time passes, we can potentially resync it from gossip again.

Thanks, I've added these to the latest (squashed) changes :)

naumenkogs · 2022-10-04T08:58:29Z

ACK a80331d

We never had the `NetworkGraph::node_failed` method implemented. The scorer now handles non-permanent failures by downgrading nodes, so we don't want that implemented. The method is renamed to `node_failed_permanent` to explicitly indicate that this is the only case it handles. We also add tracking in the form of two maps as fields of `NetworkGraph`, namely, `removed_nodes` and `removed_channels`. We track these removed graph entries to ensure we don't just resync them right away from gossip soon after removing them. We stop tracking these removed nodes whenever `remove_stale_channels_and_tracking()` is called and the entries have been tracked for longer than `REMOVED_ENTRIES_TRACKING_AGE_LIMIT_SECS` which is currently set to one week.

jkczyz self-requested a review August 4, 2022 14:11

TheBlueMatt reviewed Aug 5, 2022

View reviewed changes

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

dunxen marked this pull request as draft August 9, 2022 20:01

TheBlueMatt reviewed Aug 15, 2022

View reviewed changes

lightning/src/routing/gossip.rs Show resolved Hide resolved

dunxen force-pushed the 2022-08-implementnodefailure branch from a01a791 to 3691cfa Compare August 16, 2022 08:50

dunxen force-pushed the 2022-08-implementnodefailure branch from 3691cfa to ea8572c Compare August 18, 2022 14:35

dunxen commented Aug 18, 2022

View reviewed changes

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Aug 18, 2022

View reviewed changes

dunxen commented Aug 22, 2022

View reviewed changes

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Aug 22, 2022

View reviewed changes

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2022-08-implementnodefailure branch from afa3249 to f3fd340 Compare August 26, 2022 10:16

dunxen force-pushed the 2022-08-implementnodefailure branch from f3fd340 to 8551d98 Compare August 26, 2022 11:20

dunxen force-pushed the 2022-08-implementnodefailure branch from 8551d98 to 0326774 Compare August 26, 2022 12:03

dunxen marked this pull request as ready for review August 26, 2022 12:06

TheBlueMatt reviewed Aug 26, 2022

View reviewed changes

lightning/src/util/time.rs Outdated Show resolved Hide resolved

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

TheBlueMatt added the Seeking Code Review label Aug 29, 2022

TheBlueMatt reviewed Aug 29, 2022

View reviewed changes

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

jkczyz reviewed Aug 29, 2022

View reviewed changes

TheBlueMatt assigned jkczyz and TheBlueMatt Sep 15, 2022

jkczyz reviewed Sep 15, 2022

View reviewed changes

TheBlueMatt reviewed Sep 18, 2022

View reviewed changes

dunxen force-pushed the 2022-08-implementnodefailure branch from 9ca073c to b97e4ec Compare September 21, 2022 16:02

TheBlueMatt reviewed Sep 22, 2022

View reviewed changes

dunxen force-pushed the 2022-08-implementnodefailure branch from b97e4ec to 7a03fe9 Compare September 22, 2022 16:39

naumenkogs mentioned this pull request Sep 23, 2022

Prune channels if either not updated + track pruning time #1735

Merged

dunxen force-pushed the 2022-08-implementnodefailure branch from 7a03fe9 to ffc6553 Compare September 23, 2022 12:01

TheBlueMatt previously approved these changes Sep 26, 2022

View reviewed changes

jkczyz reviewed Sep 26, 2022

View reviewed changes

dunxen dismissed TheBlueMatt’s stale review via 468814f September 28, 2022 11:37

jkczyz reviewed Sep 28, 2022

View reviewed changes

dunxen force-pushed the 2022-08-implementnodefailure branch from 468814f to 243ca91 Compare October 1, 2022 10:35

dunxen force-pushed the 2022-08-implementnodefailure branch 2 times, most recently from 818a5c3 to a80331d Compare October 1, 2022 17:00

naumenkogs reviewed Oct 4, 2022

View reviewed changes

TheBlueMatt previously approved these changes Oct 4, 2022

View reviewed changes

dunxen dismissed TheBlueMatt’s stale review via 63d7b2b October 7, 2022 10:02

dunxen force-pushed the 2022-08-implementnodefailure branch from a80331d to 63d7b2b Compare October 7, 2022 10:02

jkczyz approved these changes Oct 7, 2022

View reviewed changes

TheBlueMatt approved these changes Oct 7, 2022

View reviewed changes

TheBlueMatt merged commit a626828 into lightningdevkit:main Oct 7, 2022

dunxen deleted the 2022-08-implementnodefailure branch January 20, 2025 08:33

	fn duration_since_epoch() -> Duration {
	use std::time::SystemTime;
	SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap()

Implement permanent node failure handling #1649

Implement permanent node failure handling #1649

Uh oh!

Conversation

dunxen commented Aug 4, 2022

Uh oh!

dunxen commented Aug 4, 2022

Uh oh!

Uh oh!

TheBlueMatt commented Aug 6, 2022

Uh oh!

dunxen commented Aug 7, 2022

Uh oh!

TheBlueMatt commented Aug 7, 2022

Uh oh!

dunxen commented Aug 15, 2022

Uh oh!

Uh oh!

dunxen commented Aug 16, 2022

Uh oh!

TheBlueMatt commented Aug 16, 2022

Uh oh!

dunxen commented Aug 18, 2022

Uh oh!

dunxen commented Aug 18, 2022

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dunxen commented Aug 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dunxen commented Aug 26, 2022

Uh oh!

codecov-commenter commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 26, 2022 •

edited

Loading