Skip to content

Commit ff8d3f7

Browse files
committed
Reduce default max_channel_saturation_power_of_half to 2 (max 1/4)
Saturating a channel beyond 1/4 of its capacity seems like a more reasonable threshold for avoiding a path than 1/2, especially given we should still be willing to send a payment with a lower saturation limit if it comes to that. This requires an (obvious) change to some router tests, but also requires a change to the `fake_network_test`, opting to simply remove some over-limit test code there - `fake_network_test` was our first ever functional test, and while it worked great to ensure LDK worked at all on day one, we now have a rather large breadth of functional tests, and a broad "does it work at all" test is no longer all that useful.
1 parent 985153e commit ff8d3f7

File tree

2 files changed

+8
-31
lines changed

2 files changed

+8
-31
lines changed

lightning/src/ln/functional_tests.rs

-23
Original file line numberDiff line numberDiff line change
@@ -1059,26 +1059,6 @@ fn fake_network_test() {
10591059
fail_payment(&nodes[1], &vec!(&nodes[3], &nodes[2], &nodes[1])[..], payment_hash_2);
10601060
claim_payment(&nodes[1], &vec!(&nodes[2], &nodes[3], &nodes[1])[..], payment_preimage_1);
10611061

1062-
// Add a duplicate new channel from 2 to 4
1063-
let chan_5 = create_announced_chan_between_nodes(&nodes, 1, 3, InitFeatures::known(), InitFeatures::known());
1064-
1065-
// Send some payments across both channels
1066-
let payment_preimage_3 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
1067-
let payment_preimage_4 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
1068-
let payment_preimage_5 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
1069-
1070-
1071-
route_over_limit(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000);
1072-
let events = nodes[0].node.get_and_clear_pending_msg_events();
1073-
assert_eq!(events.len(), 0);
1074-
nodes[0].logger.assert_log_regex("lightning::ln::channelmanager".to_string(), regex::Regex::new(r"Cannot send value that would put us over the max HTLC value in flight our peer will accept \(\d+\)").unwrap(), 1);
1075-
1076-
//TODO: Test that routes work again here as we've been notified that the channel is full
1077-
1078-
claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_3);
1079-
claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_4);
1080-
claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_5);
1081-
10821062
// Close down the channels...
10831063
close_channel(&nodes[0], &nodes[1], &chan_1.2, chan_1.3, true);
10841064
check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure);
@@ -1092,9 +1072,6 @@ fn fake_network_test() {
10921072
close_channel(&nodes[1], &nodes[3], &chan_4.2, chan_4.3, false);
10931073
check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure);
10941074
check_closed_event!(nodes[3], 1, ClosureReason::CooperativeClosure);
1095-
close_channel(&nodes[1], &nodes[3], &chan_5.2, chan_5.3, false);
1096-
check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure);
1097-
check_closed_event!(nodes[3], 1, ClosureReason::CooperativeClosure);
10981075
}
10991076

11001077
#[test]

lightning/src/routing/router.rs

+8-8
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ pub struct PaymentParameters {
238238
/// A value of 0 will allow payments up to and including a channel's total announced usable
239239
/// capacity, a value of one will only use up to half its capacity, two 1/4, etc.
240240
///
241-
/// Default value: 1
241+
/// Default value: 2
242242
pub max_channel_saturation_power_of_half: u8,
243243

244244
/// A list of SCIDs which this payment was previously attempted over and which caused the
@@ -253,7 +253,7 @@ impl_writeable_tlv_based!(PaymentParameters, {
253253
(2, features, option),
254254
(3, max_path_count, (default_value, DEFAULT_MAX_PATH_COUNT)),
255255
(4, route_hints, vec_type),
256-
(5, max_channel_saturation_power_of_half, (default_value, 1)),
256+
(5, max_channel_saturation_power_of_half, (default_value, 2)),
257257
(6, expiry_time, option),
258258
(7, previously_failed_channels, vec_type),
259259
});
@@ -268,7 +268,7 @@ impl PaymentParameters {
268268
expiry_time: None,
269269
max_total_cltv_expiry_delta: DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA,
270270
max_path_count: DEFAULT_MAX_PATH_COUNT,
271-
max_channel_saturation_power_of_half: 1,
271+
max_channel_saturation_power_of_half: 2,
272272
previously_failed_channels: Vec::new(),
273273
}
274274
}
@@ -4757,7 +4757,7 @@ mod tests {
47574757
cltv_expiry_delta: 42,
47584758
htlc_minimum_msat: None,
47594759
htlc_maximum_msat: None,
4760-
}])]);
4760+
}])]).with_max_channel_saturation_power_of_half(0);
47614761

47624762
// Keep only two paths from us to nodes[2], both with a 99sat HTLC maximum, with one with
47634763
// no fee and one with a 1msat fee. Previously, trying to route 100 sats to nodes[2] here
@@ -5741,7 +5741,7 @@ mod tests {
57415741
flags: 0,
57425742
cltv_expiry_delta: (4 << 4) | 1,
57435743
htlc_minimum_msat: 0,
5744-
htlc_maximum_msat: OptionalField::Present(200_000_000),
5744+
htlc_maximum_msat: OptionalField::Present(250_000_000),
57455745
fee_base_msat: 0,
57465746
fee_proportional_millionths: 0,
57475747
excess_data: Vec::new()
@@ -5753,7 +5753,7 @@ mod tests {
57535753
flags: 0,
57545754
cltv_expiry_delta: (13 << 4) | 1,
57555755
htlc_minimum_msat: 0,
5756-
htlc_maximum_msat: OptionalField::Present(200_000_000),
5756+
htlc_maximum_msat: OptionalField::Present(250_000_000),
57575757
fee_base_msat: 0,
57585758
fee_proportional_millionths: 0,
57595759
excess_data: Vec::new()
@@ -5762,8 +5762,8 @@ mod tests {
57625762
let payment_params = PaymentParameters::from_node_id(nodes[2]).with_features(InvoiceFeatures::known());
57635763
let keys_manager = test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
57645764
let random_seed_bytes = keys_manager.get_secure_random_bytes();
5765-
// 150,000 sat is less than the available liquidity on each channel, set above.
5766-
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 150_000_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
5765+
// 100,000 sats is less than the available liquidity on each channel, set above.
5766+
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 100_000_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
57675767
assert_eq!(route.paths.len(), 2);
57685768
assert!((route.paths[0][1].short_channel_id == 4 && route.paths[1][1].short_channel_id == 13) ||
57695769
(route.paths[1][1].short_channel_id == 4 && route.paths[0][1].short_channel_id == 13));

0 commit comments

Comments
 (0)