Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow to set manual node penalties #1592

Merged
merged 1 commit into from Jul 8, 2022
+37 −15
Merged

Allow to set manual node penalties #1592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 13 additions & 3 deletions lightning/src/routing/router.rs
View file
Open in desktop
Expand Up @@ -1876,7 +1876,7 @@ fn build_route_from_hops_internal<L: Deref>(

#[cfg(test)]
mod tests {
use routing::gossip::{NetworkGraph, P2PGossipSync, NodeId};
use routing::gossip::{NetworkGraph, P2PGossipSync, NodeId, EffectiveCapacity};
use routing::router::{get_route, build_route_from_hops_internal, add_random_cltv_offset, default_node_features,
PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RoutingFees,
DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA, MAX_PATH_LENGTH_ESTIMATE};
Expand Down Expand Up @@ -5717,7 +5717,7 @@ mod tests {
}

#[test]
fn avoids_banned_nodes() {
fn honors_manual_penalties() {
let (secp_ctx, network_graph, _, _, logger) = build_line_graph();
let (_, our_id, _, nodes) = get_nodes(&secp_ctx);

Expand All @@ -5727,7 +5727,17 @@ mod tests {
let scorer_params = ProbabilisticScoringParameters::default();
let mut scorer = ProbabilisticScorer::new(scorer_params, Arc::clone(&network_graph), Arc::clone(&logger));

// First check we can get a route.
// First check set manual penalties are returned by the scorer.
let usage = ChannelUsage {
amount_msat: 0,
inflight_htlc_msat: 0,
effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024_000, htlc_maximum_msat: Some(1_000) },
};
scorer.set_manual_penalty(&NodeId::from_pubkey(&nodes[3]), 123);
scorer.set_manual_penalty(&NodeId::from_pubkey(&nodes[4]), 456);
assert_eq!(scorer.channel_penalty_msat(42, &NodeId::from_pubkey(&nodes[3]), &NodeId::from_pubkey(&nodes[4]), usage), 456);

// Then check we can get a normal route
let payment_params = PaymentParameters::from_node_id(nodes[10]);
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 100, 42, Arc::clone(&logger), &scorer, &random_seed_bytes);
assert!(route.is_ok());
Expand Down
36 changes: 24 additions & 12 deletions lightning/src/routing/scoring.rs
View file
Open in desktop
Expand Up @@ -362,10 +362,12 @@ pub struct ProbabilisticScoringParameters {
/// Default value: 256 msat
pub amount_penalty_multiplier_msat: u64,

/// A list of nodes that won't be considered during path finding.
/// Manual penalties used for the given nodes. Allows to set a particular penalty for a given
/// node. Note that a manual penalty of `u64::max_value()` means the node would not ever be
/// considered during path finding.
///
/// (C-not exported)
pub banned_nodes: HashSet<NodeId>,
pub manual_node_penalties: HashMap<NodeId, u64>,

/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the
/// channel's capacity, which makes us prefer nodes with a smaller `htlc_maximum_msat`. We
Expand Down Expand Up @@ -468,17 +470,27 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> ProbabilisticScorerU
/// Marks the node with the given `node_id` as banned, i.e.,
/// it will be avoided during path finding.
pub fn add_banned(&mut self, node_id: &NodeId) {
self.params.banned_nodes.insert(*node_id);
self.params.manual_node_penalties.insert(*node_id, u64::max_value());
}

/// Removes the node with the given `node_id` from the list of nodes to avoid.
pub fn remove_banned(&mut self, node_id: &NodeId) {
self.params.banned_nodes.remove(node_id);
self.params.manual_node_penalties.remove(node_id);
}

/// Clears the list of nodes that are avoided during path finding.
pub fn clear_banned(&mut self) {
self.params.banned_nodes = HashSet::new();
/// Sets a manual penalty for the given node.
pub fn set_manual_penalty(&mut self, node_id: &NodeId, penalty: u64) {
self.params.manual_node_penalties.insert(*node_id, penalty);
}

/// Removes the node with the given `node_id` from the list of manual penalties.
pub fn remove_manual_penalty(&mut self, node_id: &NodeId) {
self.params.manual_node_penalties.remove(node_id);
}

/// Clears the list of manual penalties that are applied during path finding.
pub fn clear_manual_penalties(&mut self) {
self.params.manual_node_penalties = HashMap::new();
}
}

Expand All @@ -490,7 +502,7 @@ impl ProbabilisticScoringParameters {
liquidity_penalty_multiplier_msat: 0,
liquidity_offset_half_life: Duration::from_secs(3600),
amount_penalty_multiplier_msat: 0,
banned_nodes: HashSet::new(),
manual_node_penalties: HashMap::new(),
anti_probing_penalty_msat: 0,
}
}
Expand All @@ -499,7 +511,7 @@ impl ProbabilisticScoringParameters {
/// they will be avoided during path finding.
pub fn add_banned_from_list(&mut self, node_ids: Vec<NodeId>) {
for id in node_ids {
self.banned_nodes.insert(id);
self.manual_node_penalties.insert(id, u64::max_value());
}
}
}
Expand All @@ -511,7 +523,7 @@ impl Default for ProbabilisticScoringParameters {
liquidity_penalty_multiplier_msat: 40_000,
liquidity_offset_half_life: Duration::from_secs(3600),
amount_penalty_multiplier_msat: 256,
banned_nodes: HashSet::new(),
manual_node_penalties: HashMap::new(),
anti_probing_penalty_msat: 250,
}
}
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@tnull tnull Jul 5, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return *penalty;
}

let mut anti_probing_penalty_msat = 0;
Expand Down