From 8b947226570fa92eaff6861dd6f657002b7171fa Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Wed, 19 Aug 2020 10:53:29 -0400 Subject: [PATCH 1/3] [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. --- lightning/src/ln/channelmanager.rs | 15 ++++++++++---- lightning/src/ln/functional_tests.rs | 30 ++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a17eb1a610b..ee0098894f8 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1158,9 +1158,6 @@ impl if !chan.is_live() { // channel_disabled break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap()))); } - if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum - break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap()))); - } 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) }); if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient 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 { let mut res = Vec::with_capacity(8 + 128); if let Some(chan_update) = chan_update { - if code == 0x1000 | 11 || code == 0x1000 | 12 { + if code == 0x1000 | 12 { res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat)); } else if code == 0x1000 | 13 { @@ -1587,6 +1584,16 @@ impl htlc_id: prev_htlc_id, incoming_packet_shared_secret: incoming_shared_secret, }); + if amt_to_forward < chan.get().get_our_htlc_minimum_msat() { + let mut data = Vec::with_capacity(8 + 128); + data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward)); + let chan_update = self.get_channel_update(chan.get()).unwrap(); + data.extend_from_slice(&chan_update.encode_with_len()[..]); + failed_forwards.push((htlc_source, payment_hash, + HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } + )); + continue; + } match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) { Err(e) => { if let ChannelError::Ignore(msg) = e { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index aaf4fa956b8..fe712c7ad9d 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -5909,10 +5909,11 @@ fn run_onion_failure_test(_name: &str, test_case: u8, nodes: &Vec, } // test_case -// 0: node1 fails backward +// 0: final node fails backward // 1: final node fails backward // 2: payment completed but the user rejects the payment // 3: final node fails backward (but tamper onion payloads from node0) +// 4: intermediate node failure, fails backward // 100: trigger error in the intermediate node and tamper returning fail_htlc // 200: trigger error in the final node and tamper returning fail_htlc fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: u8, nodes: &Vec, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option, expected_channel_update: Option) @@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: assert!(update_1_0.update_fail_htlcs.len() == 1); update_1_0 }, + 4 => { // intermediate node failure; failing backward to start node + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + // forwarding on 1 + expect_htlc_forward!(&nodes[1]); + + // backward fail on 1 + expect_htlc_forward!(&nodes[1]); + check_added_monitors!(nodes[1], 1); + let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + assert!(update_1_0.update_fail_htlcs.len() == 1); + update_1_0 + }, _ => unreachable!(), }; // 1 => 0 commitment_signed_dance if update_1_0.update_fail_htlcs.len() > 0 { let mut fail_msg = update_1_0.update_fail_htlcs[0].clone(); - if test_case == 100 { + if test_case == 100 || test_case == 4 { callback_fail(&mut fail_msg); } nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg); @@ -6269,11 +6282,20 @@ fn test_onion_failure() { run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true})); - let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1; + let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1; let mut bogus_route = route.clone(); let route_len = bogus_route.paths[0].len(); bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward; - run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); + run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| { + let session_priv = SecretKey::from_slice(&[3; 32]).unwrap(); + let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); + let mut data = Vec::with_capacity(8 + 128); + data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward)); + let mut chan_update = ChannelUpdate::dummy(); + chan_update.contents.htlc_minimum_msat = amt_to_forward + 1; + data.extend_from_slice(&chan_update.encode_with_len()[..]); + msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data); + }, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); //TODO: with new config API, we will be able to generate both valid and //invalid channel_update cases. From 1cf928ebfc1fdb4c993c754c7b07df71c9e4d1dc Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Sat, 22 Aug 2020 09:40:57 -0400 Subject: [PATCH 2/3] [channel] Remove get_their_htlc_minimum_msat Compliance of forwarded packets with regards to upstream peer policy must be done inside Channel::send_htlc() and not at the relay layer by ChannelManager. --- lightning/src/ln/channel.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 2bea55a57bc..053419f92c2 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3158,11 +3158,6 @@ impl Channel { ); } - /// Allowed in any state (including after shutdown) - pub fn get_their_htlc_minimum_msat(&self) -> u64 { - self.our_htlc_minimum_msat - } - pub fn get_value_satoshis(&self) -> u64 { self.channel_value_satoshis } From 0e6d0b0102ea62f2a2e2345c42eabeefcdbe81a2 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Sat, 22 Aug 2020 09:47:48 -0400 Subject: [PATCH 3/3] [channelmanager] Partially document usage/encoding of onion errors --- lightning/src/ln/channelmanager.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ee0098894f8..fd13e0f6026 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1185,7 +1185,7 @@ impl { let mut res = Vec::with_capacity(8 + 128); if let Some(chan_update) = chan_update { - if code == 0x1000 | 12 { + if code == 0x1000 | 12 { // fee_insufficient res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat)); } else if code == 0x1000 | 13 { @@ -1585,12 +1585,12 @@ impl incoming_packet_shared_secret: incoming_shared_secret, }); if amt_to_forward < chan.get().get_our_htlc_minimum_msat() { - let mut data = Vec::with_capacity(8 + 128); + let mut data = Vec::with_capacity(8 + 128); // 8-bytes-htlc_msat + 2-byte-length + length-byte-channel_update data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward)); let chan_update = self.get_channel_update(chan.get()).unwrap(); data.extend_from_slice(&chan_update.encode_with_len()[..]); failed_forwards.push((htlc_source, payment_hash, - HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } + HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } // amount_below_minimum )); continue; }