Skip to content

Commit 8c4e0d9

Browse files
author
Antoine Riard
committed
[channelmanager] Move htlc_minimum_msat policy check on outgoing channel
A forwarded packet must conform to our HTLC relay policy, this includes compliance with our outgoing channel policy, as announced previously to upstream peer. Previously, we checked HTLC acceptance with regards to htlc_minimum_msat on our incoming channel in decode_update_add_htlc_onion(). This is a misinterpretation of BOLT specs as we already proceed to the same check in update_add_htlc(). This check must be done on our outgoing channel, as thus it is moved in process_pending_htlc_forwards() before to call send_htlc(). This latter function still verify upstream peer's htlc_minimum_msat before to decide on forwarding HTLC. Modify run_onion_failure_test_with_fail_intercept to test onion packet failure by intermediate node at HTLC forwarding processing, previously uncovered by onion failure test framework API.
1 parent d0b4f52 commit 8c4e0d9

File tree

2 files changed

+39
-8
lines changed

2 files changed

+39
-8
lines changed

lightning/src/ln/channelmanager.rs

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1158,9 +1158,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
11581158
if !chan.is_live() { // channel_disabled
11591159
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
11601160
}
1161-
if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum
1162-
break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
1163-
}
11641161
let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_our_fee_base_msat(&self.fee_estimator) as u64) });
11651162
if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
11661163
break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
@@ -1188,7 +1185,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
11881185
{
11891186
let mut res = Vec::with_capacity(8 + 128);
11901187
if let Some(chan_update) = chan_update {
1191-
if code == 0x1000 | 11 || code == 0x1000 | 12 {
1188+
if code == 0x1000 | 12 {
11921189
res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat));
11931190
}
11941191
else if code == 0x1000 | 13 {
@@ -1587,6 +1584,16 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
15871584
htlc_id: prev_htlc_id,
15881585
incoming_packet_shared_secret: incoming_shared_secret,
15891586
});
1587+
if amt_to_forward < chan.get().get_our_htlc_minimum_msat() {
1588+
let mut data = Vec::with_capacity(8 + 128);
1589+
data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
1590+
let chan_update = self.get_channel_update(chan.get()).unwrap();
1591+
data.extend_from_slice(&chan_update.encode_with_len()[..]);
1592+
failed_forwards.push((htlc_source, payment_hash,
1593+
HTLCFailReason::Reason { failure_code: 0x1000 | 11, data }
1594+
));
1595+
continue;
1596+
}
15901597
match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
15911598
Err(e) => {
15921599
if let ChannelError::Ignore(msg) = e {
@@ -1679,6 +1686,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
16791686
continue;
16801687
}
16811688
channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
1689+
16821690
node_id: chan.get().get_their_node_id(),
16831691
updates: msgs::CommitmentUpdate {
16841692
update_add_htlcs: add_htlc_msgs,
@@ -1779,6 +1787,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
17791787
if new_events.is_empty() { return }
17801788
let mut events = self.pending_events.lock().unwrap();
17811789
events.append(&mut new_events);
1790+
17821791
}
17831792

17841793
/// If a peer is disconnected we mark any channels with that peer as 'disabled'.

lightning/src/ln/functional_tests.rs

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5909,10 +5909,11 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
59095909
}
59105910

59115911
// test_case
5912-
// 0: node1 fails backward
5912+
// 0: final node fails backward
59135913
// 1: final node fails backward
59145914
// 2: payment completed but the user rejects the payment
59155915
// 3: final node fails backward (but tamper onion payloads from node0)
5916+
// 4: intermediate node failure, fails backward
59165917
// 100: trigger error in the intermediate node and tamper returning fail_htlc
59175918
// 200: trigger error in the final node and tamper returning fail_htlc
59185919
fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
@@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
60136014
assert!(update_1_0.update_fail_htlcs.len() == 1);
60146015
update_1_0
60156016
},
6017+
4 => { // intermediate node failure; failing backward to start node
6018+
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
6019+
// forwarding on 1
6020+
expect_htlc_forward!(&nodes[1]);
6021+
6022+
// backward fail on 1
6023+
expect_htlc_forward!(&nodes[1]);
6024+
check_added_monitors!(nodes[1], 1);
6025+
let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
6026+
assert!(update_1_0.update_fail_htlcs.len() == 1);
6027+
update_1_0
6028+
},
60166029
_ => unreachable!(),
60176030
};
60186031

60196032
// 1 => 0 commitment_signed_dance
60206033
if update_1_0.update_fail_htlcs.len() > 0 {
60216034
let mut fail_msg = update_1_0.update_fail_htlcs[0].clone();
6022-
if test_case == 100 {
6035+
if test_case == 100 || test_case == 4 {
60236036
callback_fail(&mut fail_msg);
60246037
}
60256038
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
@@ -6269,11 +6282,20 @@ fn test_onion_failure() {
62696282
run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10),
62706283
Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true}));
62716284

6272-
let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1;
6285+
let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1;
62736286
let mut bogus_route = route.clone();
62746287
let route_len = bogus_route.paths[0].len();
62756288
bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
6276-
run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
6289+
run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| {
6290+
let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
6291+
let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
6292+
let mut data = Vec::with_capacity(8 + 128);
6293+
data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
6294+
let mut chan_update = ChannelUpdate::dummy();
6295+
chan_update.contents.htlc_minimum_msat = amt_to_forward + 1;
6296+
data.extend_from_slice(&chan_update.encode_with_len()[..]);
6297+
msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data);
6298+
}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
62776299

62786300
//TODO: with new config API, we will be able to generate both valid and
62796301
//invalid channel_update cases.

0 commit comments

Comments
 (0)