Ignore partially-pruned channels during routing #3038
Conversation
| @@ -888,12 +889,14 @@ impl ChannelInfo { | |||
| return None; | |||
| } | |||
| }; | |||
| direction.map(|dir| (DirectedChannelInfo::new(self, dir, outbound), source)) | |||
| let dir = direction.expect("We checked that both directions are available at the start"); | |||
There was a problem hiding this comment.
Also, one quick question.
Usage of expect here would create an explicit panic in case direction.is_none().
But would explicitly panicking in a pub fn be a good idea?
Should we consider returning a Result<Ok, Err>, instead of panicking within the function?
There was a problem hiding this comment.
See the text in the expect, we expect this to be unreachable :)
There was a problem hiding this comment.
Ah! Right!
Then it makes sense! :)
If we prune one side of a channel's `ChannelUpdateInfo` that means the node hasn't been online for two weeks (as they haven't generated a new `channel_update` in that time). In such cases, even if we haven't yet pruned the channel entirely, we should definitely not be treating these channels as candidates for routing. Note that this requires some additional `channel_update`s in the router tests, but all of the new ones are added as disabled channels. Fixes lightningdevkit#1824
TheBlueMatt
force-pushed
the
2024-05-no-dir-with-on-side-offline
branch
from
29f8a54
to
4fd8cb8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3038 +/- ##
==========================================
+ Coverage 89.85% 90.54% +0.69%
==========================================
Files 116 116
Lines 96237 102306 +6069
Branches 96237 102306 +6069
==========================================
+ Hits 86471 92637 +6166
- Misses 7206 7214 +8
+ Partials 2560 2455 -105 ☔ View full report in Codecov by Sentry. |
| short_channel_id: 6, | ||
| timestamp: 1, | ||
| flags, | ||
| cltv_expiry_delta: (6 << 4) | 0, |
There was a problem hiding this comment.
Also a tangential question, but why are we writing cltv_expiry_delta: (x<<4) | 0 in this test when it is the same as writing cltv_expiry_delta: (x<<4)
There was a problem hiding this comment.
We also |1 in some places and it just keeps the same formatting.
There was a problem hiding this comment.
Alright! That makes sense!
Thanks for the clarification! :)
If we prune one side of a channel's
ChannelUpdateInfothat means the node hasn't been online for two weeks (as they haven't generated a newchannel_updatein that time). In such cases, even if we haven't yet pruned the channel entirely, we should definitely not be treating these channels as candidates for routing.Fixes #1824