Allow to set manual node penalties #1592
Conversation
|
Feel free to squash the test fix commit. |
tnull
force-pushed
the
2022-07-manual-penalty
branch
from
7238559
to
2a09fe7
Compare
|
Squashed. |
A user might want to explicitly penalize or prioritize a particular node. We now allow them to do so by specifying a manual penalty override for a given node that is then returned by the scorer.
tnull
force-pushed
the
2022-07-manual-penalty
branch
from
2a09fe7
to
1ddc6f1
Compare
Codecov Report
@@ Coverage Diff @@
## main #1592 +/- ##
==========================================
- Coverage 91.07% 91.02% -0.06%
==========================================
Files 80 80
Lines 44128 44136 +8
Branches 44128 44136 +8
==========================================
- Hits 40190 40175 -15
- Misses 3938 3961 +23
Continue to review full report at Codecov.
|
| @@ -713,8 +725,8 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> Score for Probabilis | |||
| fn channel_penalty_msat( | |||
| &self, short_channel_id: u64, source: &NodeId, target: &NodeId, usage: ChannelUsage | |||
| ) -> u64 { | |||
| if self.params.banned_nodes.contains(source) || self.params.banned_nodes.contains(target) { | |||
| return u64::max_value(); | |||
| if let Some(penalty) = self.params.manual_node_penalties.get(target) { | |||
There was a problem hiding this comment.
Ugh, I need to stop reviewing pre-coffee, sorry, this should probably look up the source, not the target. I believe both are correct, but in general the routing happens destination -> sender so if we check the source when getting the channel penalty we'll penalize the path earlier in the routing process. Its a pretty minor difference, but still.
There was a problem hiding this comment.
I see your point that a penalty for the source would be considered a bit earlier. However, I chose the target here since it generally seems more in line with the behavior I would expect, and in particular it allows us to penalize/prefer specific single-hop paths. This could come handy when a user would like to use this mechanism to always choose a particular channel when routing to a neighbor node.
There was a problem hiding this comment.
it allows us to penalize/prefer specific single-hop paths. his could come handy when a user would like to use this mechanism to always choose a particular channel when routing to a neighbor node.
Wait, can it? If you're paying a given peer, this API doesn't let you pick a channel to that peer, you can punish that peer, but I'm not sure what that does?
There was a problem hiding this comment.
Wait, can it? If you're paying a given peer, this API doesn't let you pick a channel to that peer, you can punish that peer, but I'm not sure what that does?
Well I assume when you have multiple channels open to different peers you can set a low/high penalty to prefer/avoid a specific one over the others for example? I'd think this could even get used for simple balancing purposes, i.e., set higher penalties the more a channel is imbalanced. A nice side-effect in this case would be that you could always still fall back on a penalized peer, as long as you not set the penalty outright to u64::max_value().
There was a problem hiding this comment.
Right, but I don't see how this accomplishes what you're saying - the penalties here are on nodes, not channels, so you can't balance between different channels you have with the same peer. As for across peers, we'll basically always take the direct path.
Are you suggesting users could use this to penalize the direct path enough that we'd prefer to take an indirect one? I suppose you'd be right there.
There was a problem hiding this comment.
Yes, this gives us the possibility to essentially prioritize first hops. I agree that a more fine-grained prioritization on a per-channel basis could be an improvement, but I think this can be a good first step.
A user might want to explicitly penalize or prioritize a particular node. We now allow them to do so by specifying a manual penalty override for a given node that is then returned by the scorer.
This is a simple generalization of/follow-up to the approach taken in #1550.
Closes #1239