Skip to content

Commit 1f06b6a

Browse files
TheBlueMattTheBlueMatt
committed
(XXX: Fully document) [router] Avoid re-processing peers when a score component decreases
1 parent df5730d commit 1f06b6a

File tree

1 file changed

+109
-74
lines changed

1 file changed

+109
-74
lines changed

‎lightning/src/routing/router.rs

Lines changed: 109 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,12 @@ struct PathBuildingHop {
200200
/// A mirror of the same field in RouteGraphNode. Note that this is only used during the graph
201201
/// walk and may be invalid thereafter.
202202
path_htlc_minimum_msat: u64,
203+
/// If we've already processed a node as the best node, we shouldn't process it again. Normally
204+
/// we'd just ignore it if we did as all channels would have a higher new fee, but because we
205+
/// may decrease the amounts in use as we walk the graph, the actual calculated fee may
206+
/// decrease as well. Thus, we have to explicitly track which nodes have been processed and
207+
/// avoid processing them again.
208+
was_processed: bool,
203209
}
204210

205211
// Instantiated with a list of hops with correct data in them collected during path finding,
@@ -614,85 +620,97 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
614620
total_fee_msat: u64::max_value(),
615621
htlc_minimum_msat: $directional_info.htlc_minimum_msat,
616622
path_htlc_minimum_msat,
623+
was_processed: false,
617624
}
618625
});
619626

620-
let mut hop_use_fee_msat = 0;
621-
let mut total_fee_msat = $next_hops_fee_msat;
622-
623-
// Ignore hop_use_fee_msat for channel-from-us as we assume all channels-from-us
624-
// will have the same effective-fee
625-
if $src_node_id != *our_node_id {
626-
match compute_fees(amount_to_transfer_over_msat, $directional_info.fees) {
627-
// max_value means we'll always fail
628-
// the old_entry.total_fee_msat > total_fee_msat check
629-
None => total_fee_msat = u64::max_value(),
630-
Some(fee_msat) => {
631-
hop_use_fee_msat = fee_msat;
632-
total_fee_msat += hop_use_fee_msat;
633-
// When calculating the lowest inbound fees to a node, we
634-
// calculate fees here not based on the actual value we think
635-
// will flow over this channel, but on the minimum value that
636-
// we'll accept flowing over it. Otherwise we may later find a
637-
// different path to the source node that is more expensive,
638-
// but which we consider to be cheaper because we are capacity
639-
// constrained and the relative fee becomes lower.
640-
match compute_fees(minimal_value_contribution_msat, old_entry.src_lowest_inbound_fees)
641-
.map(|a| a.checked_add(total_fee_msat)) {
642-
Some(Some(v)) => {
643-
total_fee_msat = v;
644-
},
645-
_ => {
646-
total_fee_msat = u64::max_value();
647-
}
648-
};
627+
if !old_entry.was_processed {
628+
let mut hop_use_fee_msat = 0;
629+
let mut total_fee_msat = $next_hops_fee_msat;
630+
631+
// Ignore hop_use_fee_msat for channel-from-us as we assume all channels-from-us
632+
// will have the same effective-fee
633+
if $src_node_id != *our_node_id {
634+
match compute_fees(amount_to_transfer_over_msat, $directional_info.fees) {
635+
// max_value means we'll always fail
636+
// the old_entry.total_fee_msat > total_fee_msat check
637+
None => total_fee_msat = u64::max_value(),
638+
Some(fee_msat) => {
639+
hop_use_fee_msat = fee_msat;
640+
total_fee_msat += hop_use_fee_msat;
641+
// When calculating the lowest inbound fees to a node, we
642+
// calculate fees here not based on the actual value we think
643+
// will flow over this channel, but on the minimum value that
644+
// we'll accept flowing over it. Otherwise we may later find a
645+
// different path to the source node that is more expensive,
646+
// but which we consider to be cheaper because we are capacity
647+
// constrained and the relative fee becomes lower.
648+
match compute_fees(minimal_value_contribution_msat, old_entry.src_lowest_inbound_fees)
649+
.map(|a| a.checked_add(total_fee_msat)) {
650+
Some(Some(v)) => {
651+
total_fee_msat = v;
652+
},
653+
_ => {
654+
total_fee_msat = u64::max_value();
655+
}
656+
};
657+
658+
match compute_fees(minimal_value_contribution_msat, old_entry.src_lowest_inbound_fees).map(|a| a.checked_add(total_fee_msat)) {
659+
Some(Some(v)) => {
660+
total_fee_msat = v;
661+
},
662+
_ => {
663+
total_fee_msat = u64::max_value();
664+
}
665+
};
666+
}
649667
}
650668
}
651-
}
652-
653-
let new_graph_node = RouteGraphNode {
654-
pubkey: $src_node_id,
655-
lowest_fee_to_peer_through_node: total_fee_msat,
656-
lowest_fee_to_node: $next_hops_fee_msat as u64 + hop_use_fee_msat,
657-
value_contribution_msat: value_contribution_msat,
658-
path_htlc_minimum_msat,
659-
};
660669

661-
// Update the way of reaching $src_node_id with the given $chan_id (from $dest_node_id),
662-
// if this way is cheaper than the already known
663-
// (considering the cost to "reach" this channel from the route destination,
664-
// the cost of using this channel,
665-
// and the cost of routing to the source node of this channel).
666-
// Also, consider that htlc_minimum_msat_difference, because we might end up
667-
// paying it. Consider the following exploit:
668-
// we use 2 paths to transfer 1.5 BTC. One of them is 0-fee normal 1 BTC path,
669-
// and for the other one we picked a 1sat-fee path with htlc_minimum_msat of
670-
// 1 BTC. Now, since the latter is more expensive, we gonna try to cut it
671-
// by 0.5 BTC, but then match htlc_minimum_msat by paying a fee of 0.5 BTC
672-
// to this channel.
673-
// Ideally the scoring could be smarter (e.g. 0.5*htlc_minimum_msat here),
674-
// but it may require additional tracking - we don't want to double-count
675-
// the fees included in $incl_fee_next_hops_htlc_minimum_msat, but also
676-
// can't use something that may decrease on future hops.
677-
let old_cost = cmp::max(old_entry.total_fee_msat, old_entry.path_htlc_minimum_msat);
678-
let new_cost = cmp::max(total_fee_msat, path_htlc_minimum_msat);
679-
680-
if new_cost < old_cost {
681-
targets.push(new_graph_node);
682-
old_entry.next_hops_fee_msat = $next_hops_fee_msat;
683-
old_entry.hop_use_fee_msat = hop_use_fee_msat;
684-
old_entry.total_fee_msat = total_fee_msat;
685-
old_entry.route_hop = RouteHop {
686-
pubkey: $dest_node_id.clone(),
687-
node_features: NodeFeatures::empty(),
688-
short_channel_id: $chan_id.clone(),
689-
channel_features: $chan_features.clone(),
690-
fee_msat: 0, // This value will be later filled with hop_use_fee_msat of the following channel
691-
cltv_expiry_delta: $directional_info.cltv_expiry_delta as u32,
670+
let new_graph_node = RouteGraphNode {
671+
pubkey: $src_node_id,
672+
lowest_fee_to_peer_through_node: total_fee_msat,
673+
lowest_fee_to_node: $next_hops_fee_msat as u64 + hop_use_fee_msat,
674+
value_contribution_msat: value_contribution_msat,
675+
path_htlc_minimum_msat,
692676
};
693-
old_entry.channel_fees = $directional_info.fees;
694-
old_entry.htlc_minimum_msat = $directional_info.htlc_minimum_msat;
695-
old_entry.path_htlc_minimum_msat = path_htlc_minimum_msat;
677+
678+
// Update the way of reaching $src_node_id with the given $chan_id (from $dest_node_id),
679+
// if this way is cheaper than the already known
680+
// (considering the cost to "reach" this channel from the route destination,
681+
// the cost of using this channel,
682+
// and the cost of routing to the source node of this channel).
683+
// Also, consider that htlc_minimum_msat_difference, because we might end up
684+
// paying it. Consider the following exploit:
685+
// we use 2 paths to transfer 1.5 BTC. One of them is 0-fee normal 1 BTC path,
686+
// and for the other one we picked a 1sat-fee path with htlc_minimum_msat of
687+
// 1 BTC. Now, since the latter is more expensive, we gonna try to cut it
688+
// by 0.5 BTC, but then match htlc_minimum_msat by paying a fee of 0.5 BTC
689+
// to this channel.
690+
// Ideally the scoring could be smarter (e.g. 0.5*htlc_minimum_msat here),
691+
// but it may require additional tracking - we don't want to double-count
692+
// the fees included in $incl_fee_next_hops_htlc_minimum_msat, but also
693+
// can't use something that may decrease on future hops.
694+
let old_cost = cmp::max(old_entry.total_fee_msat, old_entry.path_htlc_minimum_msat);
695+
let new_cost = cmp::max(total_fee_msat, path_htlc_minimum_msat);
696+
697+
if new_cost < old_cost {
698+
targets.push(new_graph_node);
699+
old_entry.next_hops_fee_msat = $next_hops_fee_msat;
700+
old_entry.hop_use_fee_msat = hop_use_fee_msat;
701+
old_entry.total_fee_msat = total_fee_msat;
702+
old_entry.route_hop = RouteHop {
703+
pubkey: $dest_node_id.clone(),
704+
node_features: NodeFeatures::empty(),
705+
short_channel_id: $chan_id.clone(),
706+
channel_features: $chan_features.clone(),
707+
fee_msat: 0, // This value will be later filled with hop_use_fee_msat of the following channel
708+
cltv_expiry_delta: $directional_info.cltv_expiry_delta as u32,
709+
};
710+
old_entry.channel_fees = $directional_info.fees;
711+
old_entry.htlc_minimum_msat = $directional_info.htlc_minimum_msat;
712+
old_entry.path_htlc_minimum_msat = path_htlc_minimum_msat;
713+
}
696714
}
697715
}
698716
}
@@ -707,7 +725,19 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
707725
// This data can later be helpful to optimize routing (pay lower fees).
708726
macro_rules! add_entries_to_cheapest_to_target_node {
709727
( $node: expr, $node_id: expr, $fee_to_target_msat: expr, $next_hops_value_contribution: expr, $incl_fee_next_hops_htlc_minimum_msat: expr ) => {
710-
if first_hops.is_some() {
728+
let skip_node = if let Some(elem) = dist.get_mut($node_id) {
729+
let v = elem.was_processed;
730+
elem.was_processed = true;
731+
v
732+
} else {
733+
// Entries are added to dist in add_entry!() when there is a channel from a node.
734+
// Because there are no channels from payee, it will not have a dist entry at this point.
735+
// If we're processing any other node, it is always be the result of a channel from it.
736+
assert_eq!($node_id, payee);
737+
false
738+
};
739+
740+
if !skip_node && first_hops.is_some() {
711741
if let Some(&(ref first_hop, ref features, ref outbound_capacity_msat)) = first_hop_targets.get(&$node_id) {
712742
add_entry!(first_hop, *our_node_id, $node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), $fee_to_target_msat, $next_hops_value_contribution, $incl_fee_next_hops_htlc_minimum_msat);
713743
}
@@ -720,7 +750,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
720750
features = NodeFeatures::empty();
721751
}
722752

723-
if !features.requires_unknown_bits() {
753+
if !skip_node && !features.requires_unknown_bits() {
724754
for chan_id in $node.channels.iter() {
725755
let chan = network.get_channels().get(chan_id).unwrap();
726756
if !chan.features.requires_unknown_bits() {
@@ -940,6 +970,11 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
940970
break 'path_construction;
941971
}
942972

973+
// If we found a path back to the payee, we shouldn't try to process it again. This is
974+
// the equivalent of the `elem.was_processed` check in
975+
// add_entries_to_cheapest_to_target_node!() (see comment there for more info).
976+
if pubkey == *payee { continue 'path_construction; }
977+
943978
// Otherwise, since the current target node is not us,
944979
// keep "unrolling" the payment graph from payee to payer by
945980
// finding a way to reach the current target from the payer side.

0 commit comments

Comments
 (0)