Skip to content

Commit f317215

Browse files
committed
[router] Do not fail if we are exactly (or 3x) limited by a maximum
The new MPP routing algorithm attempts to build paths with a higher value than our payment to find paths which may allow us to pay a fee to meet an htlc_minimum limit. This is great if we're min-bounded, however it results in us rejecting paths where we are bounded by a maximum near the end of the path, with fees, and then bounded by a minimum earlier in the path. Further, it means we will not find the cheapest path where paths have a lower relative fee but a higher absolute fee. Instead, we calculate routes using the actual amount we wish to send. To maintain the previous behavior of searching for cheaper paths where we can "pay the difference" to meet an htlc_minimum, we detect if we were minimum-bounded during graph walking and, if we are, we walk the graph again with a higher value.
1 parent 81ab453 commit f317215

File tree

1 file changed

+102
-14
lines changed

1 file changed

+102
-14
lines changed

lightning/src/routing/router.rs

Lines changed: 102 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -422,13 +422,18 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
422422
let mut targets = BinaryHeap::new(); //TODO: Do we care about switching to eg Fibbonaci heap?
423423
let mut dist = HashMap::with_capacity(network.get_nodes().len());
424424

425+
// During routing, if we ignore a path due to an htlc_minimum_msat limit, we set this,
426+
// indicating that we may wish to try again with a higher value, potentially paying to meet an
427+
// htlc_minimum with extra fees while still finding a cheaper path.
428+
let mut hit_minimum_limit;
429+
425430
// When arranging a route, we select multiple paths so that we can make a multi-path payment.
426-
// Don't stop searching for paths when we think they're
427-
// sufficient to transfer a given value aggregately.
428-
// Search for higher value, so that we collect many more paths,
429-
// and then select the best combination among them.
431+
// We start with a path_value of the exact amount we want, and if that generates a route we may
432+
// return it immediately. Otherwise, we don't stop searching for paths until we have 3x the
433+
// amount we want in total across paths, selecting the best subset at the end.
430434
const ROUTE_CAPACITY_PROVISION_FACTOR: u64 = 3;
431435
let recommended_value_msat = final_value_msat * ROUTE_CAPACITY_PROVISION_FACTOR as u64;
436+
let mut path_value_msat = final_value_msat;
432437

433438
// Step (1).
434439
// Prepare the data we'll use for payee-to-payer search by
@@ -536,8 +541,9 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
536541
// Since we're choosing amount_to_transfer_over_msat as maximum possible, it can
537542
// be only reduced later (not increased), so this channel should just be skipped
538543
// as not sufficient.
539-
// TODO: Explore simply adding fee to hit htlc_minimum_msat
540-
if contributes_sufficient_value && amount_to_transfer_over_msat >= $directional_info.htlc_minimum_msat {
544+
if amount_to_transfer_over_msat < $directional_info.htlc_minimum_msat {
545+
hit_minimum_limit = true;
546+
} else if contributes_sufficient_value {
541547
// Note that low contribution here (limited by available_liquidity_msat)
542548
// might violate htlc_minimum_msat on the hops which are next along the
543549
// payment path (upstream to the payee). To avoid that, we recompute path
@@ -724,12 +730,13 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
724730
// the further iterations of path finding. Also don't erase first_hop_targets.
725731
targets.clear();
726732
dist.clear();
733+
hit_minimum_limit = false;
727734

728735
// If first hop is a private channel and the only way to reach the payee, this is the only
729736
// place where it could be added.
730737
if first_hops.is_some() {
731738
if let Some(&(ref first_hop, ref features, ref outbound_capacity_msat)) = first_hop_targets.get(&payee) {
732-
add_entry!(first_hop, *our_node_id, payee, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), 0, recommended_value_msat);
739+
add_entry!(first_hop, *our_node_id, payee, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), 0, path_value_msat);
733740
}
734741
}
735742

@@ -742,7 +749,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
742749
// If not, targets.pop() will not even let us enter the loop in step 2.
743750
None => {},
744751
Some(node) => {
745-
add_entries_to_cheapest_to_target_node!(node, payee, 0, recommended_value_msat);
752+
add_entries_to_cheapest_to_target_node!(node, payee, 0, path_value_msat);
746753
},
747754
}
748755

@@ -761,7 +768,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
761768
// bit lazy here. In the future, we should pull them out via our
762769
// ChannelManager, but there's no reason to waste the space until we
763770
// need them.
764-
add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), 0, recommended_value_msat);
771+
add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), 0, path_value_msat);
765772
true
766773
} else {
767774
// In any other case, only add the hop if the source is in the regular network
@@ -781,7 +788,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
781788
htlc_maximum_msat: hop.htlc_maximum_msat,
782789
fees: hop.fees,
783790
};
784-
add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, None::<u64>, ChannelFeatures::empty(), 0, recommended_value_msat);
791+
add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, None::<u64>, ChannelFeatures::empty(), 0, path_value_msat);
785792
}
786793
}
787794

@@ -901,13 +908,23 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
901908
}
902909

903910
// Step (3).
904-
// Stop either when recommended value is reached,
905-
// or if during last iteration no new path was found.
906-
// In the latter case, making another path finding attempt could not help,
907-
// because we deterministically terminate the search due to low liquidity.
911+
// Stop either when the recommended value is reached or if no new path was found in this
912+
// iteration.
913+
// In the latter case, making another path finding attempt won't help,
914+
// because we deterministically terminated the search due to low liquidity.
908915
if already_collected_value_msat >= recommended_value_msat || !found_new_path {
909916
break 'paths_collection;
910917
}
918+
// Further, if this was our first walk of the graph, and we weren't limited by an
919+
// htlc_minim_msat, return immediately because this path should suffice. If we were limited
920+
// by an htlc_minimum_msat value, find another path with a higher value, potentially
921+
// allowing us to pay fees to meet the htlc_minimum while still keeping a lower total fee.
922+
if already_collected_value_msat == final_value_msat && payment_paths.len() == 1 {
923+
if !hit_minimum_limit {
924+
break 'paths_collection;
925+
}
926+
path_value_msat = recommended_value_msat;
927+
}
911928
}
912929

913930
// Step (4).
@@ -3371,6 +3388,77 @@ mod tests {
33713388
}
33723389
}
33733390

3391+
#[test]
3392+
fn exact_fee_liquidity_limit() {
3393+
// Test that if, while walking the graph, we find a hop that has exactly enough liquidity
3394+
// for us, including later hop fees, we take it. In the first version of our MPP algorithm
3395+
// we calculated fees on a higher value, resulting in us ignoring such paths.
3396+
let (secp_ctx, net_graph_msg_handler, _, logger) = build_graph();
3397+
let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx);
3398+
3399+
// We modify the graph to set the htlc_maximum of channel 2 to below the value we wish to
3400+
// send.
3401+
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
3402+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
3403+
short_channel_id: 2,
3404+
timestamp: 2,
3405+
flags: 0,
3406+
cltv_expiry_delta: 0,
3407+
htlc_minimum_msat: 0,
3408+
htlc_maximum_msat: OptionalField::Present(85_000),
3409+
fee_base_msat: 0,
3410+
fee_proportional_millionths: 0,
3411+
excess_data: Vec::new()
3412+
});
3413+
3414+
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
3415+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
3416+
short_channel_id: 12,
3417+
timestamp: 2,
3418+
flags: 0,
3419+
cltv_expiry_delta: (4 << 8) | 1,
3420+
htlc_minimum_msat: 0,
3421+
htlc_maximum_msat: OptionalField::Present(270_000),
3422+
fee_base_msat: 0,
3423+
fee_proportional_millionths: 1000000,
3424+
excess_data: Vec::new()
3425+
});
3426+
3427+
update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[2], UnsignedChannelUpdate {
3428+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
3429+
short_channel_id: 13,
3430+
timestamp: 2,
3431+
flags: 1 | 2,
3432+
cltv_expiry_delta: (13 << 8) | 2,
3433+
htlc_minimum_msat: 0,
3434+
htlc_maximum_msat: OptionalField::Absent,
3435+
fee_base_msat: 0,
3436+
fee_proportional_millionths: 0,
3437+
excess_data: Vec::new()
3438+
});
3439+
3440+
{
3441+
// Now, attempt to route 125 sats (just a bit below the capacity of 3 channels).
3442+
// Our algorithm should provide us with these 3 paths.
3443+
let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, &Vec::new(), 90_000, 42, Arc::clone(&logger)).unwrap();
3444+
assert_eq!(route.paths.len(), 1);
3445+
assert_eq!(route.paths[0].len(), 2);
3446+
3447+
assert_eq!(route.paths[0][0].pubkey, nodes[7]);
3448+
assert_eq!(route.paths[0][0].short_channel_id, 12);
3449+
assert_eq!(route.paths[0][0].fee_msat, 90_000*2);
3450+
assert_eq!(route.paths[0][0].cltv_expiry_delta, (13 << 8) | 1);
3451+
assert_eq!(route.paths[0][0].node_features.le_flags(), &id_to_feature_flags(8));
3452+
assert_eq!(route.paths[0][0].channel_features.le_flags(), &id_to_feature_flags(12));
3453+
3454+
assert_eq!(route.paths[0][1].pubkey, nodes[2]);
3455+
assert_eq!(route.paths[0][1].short_channel_id, 13);
3456+
assert_eq!(route.paths[0][1].fee_msat, 90_000);
3457+
assert_eq!(route.paths[0][1].cltv_expiry_delta, 42);
3458+
assert_eq!(route.paths[0][1].node_features.le_flags(), &id_to_feature_flags(3));
3459+
assert_eq!(route.paths[0][1].channel_features.le_flags(), &id_to_feature_flags(13));
3460+
}
3461+
}
33743462
}
33753463

33763464
#[cfg(all(test, feature = "unstable"))]

0 commit comments

Comments
 (0)