From 582b827a4d3651aeb4002d4c357cdd8156e97c89 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 4 Nov 2022 12:28:36 -0400 Subject: [PATCH 1/4] Refactor HTLCForwardInfo::AddHTLC for intercept forwards In upcoming commit(s), we'll want to store intercepted HTLC forwards in ChannelManager before the user signals that they should be forwarded. It wouldn't make sense to store a HTLCForwardInfo as-is because the FailHTLC variant doesn't make sense, so we refactor out the ::AddHTLC contents into its own struct for storage. Co-authored-by: John Cantrell Co-authored-by: Valentine Wallace --- lightning/src/ln/channelmanager.rs | 85 +++++++++++++++------------ lightning/src/ln/onion_route_tests.rs | 18 +++--- 2 files changed, 58 insertions(+), 45 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 465256d21ea..b3eed7db4d2 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -129,20 +129,22 @@ pub(super) enum PendingHTLCStatus { Fail(HTLCFailureMsg), } -pub(super) enum HTLCForwardInfo { - AddHTLC { - forward_info: PendingHTLCInfo, +pub(super) struct PendingAddHTLCInfo { + pub(super) forward_info: PendingHTLCInfo, + + // These fields are produced in `forward_htlcs()` and consumed in + // `process_pending_htlc_forwards()` for constructing the + // `HTLCSource::PreviousHopData` for failed and forwarded + // HTLCs. + // + // Note that this may be an outbound SCID alias for the associated channel. + prev_short_channel_id: u64, + prev_htlc_id: u64, + prev_funding_outpoint: OutPoint, +} - // These fields are produced in `forward_htlcs()` and consumed in - // `process_pending_htlc_forwards()` for constructing the - // `HTLCSource::PreviousHopData` for failed and forwarded - // HTLCs. - // - // Note that this may be an outbound SCID alias for the associated channel. - prev_short_channel_id: u64, - prev_htlc_id: u64, - prev_funding_outpoint: OutPoint, - }, +pub(super) enum HTLCForwardInfo { + AddHTLC(PendingAddHTLCInfo), FailHTLC { htlc_id: u64, err_packet: msgs::OnionErrorPacket, @@ -3149,9 +3151,13 @@ impl ChannelManager { for forward_info in pending_forwards.drain(..) { match forward_info { - HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo { - routing, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value }, - prev_funding_outpoint } => { + HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { + prev_short_channel_id, prev_htlc_id, prev_funding_outpoint, + forward_info: PendingHTLCInfo { + routing, incoming_shared_secret, payment_hash, amt_to_forward, + outgoing_cltv_value + } + }) => { macro_rules! failure_handler { ($msg: expr, $err_code: expr, $err_data: expr, $phantom_ss: expr, $next_hop_unknown: expr) => { log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg); @@ -3252,11 +3258,13 @@ impl ChannelManager { + HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { + prev_short_channel_id, prev_htlc_id, prev_funding_outpoint , + forward_info: PendingHTLCInfo { + incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value, + routing: PendingHTLCRouting::Forward { onion_packet, .. }, + }, + }) => { log_trace!(self.logger, "Adding HTLC from short id {} with payment_hash {} to channel with short id {} after delay", prev_short_channel_id, log_bytes!(payment_hash.0), short_chan_id); let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id: prev_short_channel_id, @@ -3377,9 +3385,12 @@ impl ChannelManager { + HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { + prev_short_channel_id, prev_htlc_id, prev_funding_outpoint, + forward_info: PendingHTLCInfo { + routing, incoming_shared_secret, payment_hash, amt_to_forward, .. + } + }) => { let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret) = match routing { PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } => { let _legacy_hop_data = Some(payment_data.clone()); @@ -5089,12 +5100,12 @@ impl ChannelManager 0, }) { hash_map::Entry::Occupied(mut entry) => { - entry.get_mut().push(HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_funding_outpoint, - prev_htlc_id, forward_info }); + entry.get_mut().push(HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { + prev_short_channel_id, prev_funding_outpoint, prev_htlc_id, forward_info })); }, hash_map::Entry::Vacant(entry) => { - entry.insert(vec!(HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_funding_outpoint, - prev_htlc_id, forward_info })); + entry.insert(vec!(HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { + prev_short_channel_id, prev_funding_outpoint, prev_htlc_id, forward_info }))); } } } @@ -6681,18 +6692,20 @@ impl_writeable_tlv_based_enum!(HTLCFailReason, }, ;); +impl_writeable_tlv_based!(PendingAddHTLCInfo, { + (0, forward_info, required), + (2, prev_short_channel_id, required), + (4, prev_htlc_id, required), + (6, prev_funding_outpoint, required), +}); + impl_writeable_tlv_based_enum!(HTLCForwardInfo, - (0, AddHTLC) => { - (0, forward_info, required), - (2, prev_short_channel_id, required), - (4, prev_htlc_id, required), - (6, prev_funding_outpoint, required), - }, (1, FailHTLC) => { (0, htlc_id, required), (2, err_packet, required), - }, -;); + }; + (0, AddHTLC) +); impl_writeable_tlv_based!(PendingInboundPayment, { (0, payment_secret, required), diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 14def2f6b7a..fb630abb9a8 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -15,7 +15,7 @@ use crate::chain::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GR use crate::chain::keysinterface::{KeysInterface, Recipient}; use crate::ln::{PaymentHash, PaymentSecret}; use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS; -use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, HTLCForwardInfo, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingHTLCInfo, PendingHTLCRouting, PaymentId}; +use crate::ln::channelmanager::{self, ChannelManager, ChannelManagerReadArgs, HTLCForwardInfo, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingAddHTLCInfo, PendingHTLCInfo, PendingHTLCRouting, PaymentId}; use crate::ln::onion_utils; use crate::routing::gossip::{NetworkUpdate, RoutingFees, NodeId}; use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop}; @@ -554,7 +554,7 @@ fn test_onion_failure() { for (_, pending_forwards) in nodes[1].node.forward_htlcs.lock().unwrap().iter_mut() { for f in pending_forwards.iter_mut() { match f { - &mut HTLCForwardInfo::AddHTLC { ref mut forward_info, .. } => + &mut HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { ref mut forward_info, .. }) => forward_info.outgoing_cltv_value += 1, _ => {}, } @@ -567,7 +567,7 @@ fn test_onion_failure() { for (_, pending_forwards) in nodes[1].node.forward_htlcs.lock().unwrap().iter_mut() { for f in pending_forwards.iter_mut() { match f { - &mut HTLCForwardInfo::AddHTLC { ref mut forward_info, .. } => + &mut HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { ref mut forward_info, .. }) => forward_info.amt_to_forward -= 1, _ => {}, } @@ -1035,12 +1035,12 @@ fn test_phantom_onion_hmac_failure() { let mut forward_htlcs = nodes[1].node.forward_htlcs.lock().unwrap(); let mut pending_forward = forward_htlcs.get_mut(&phantom_scid).unwrap(); match pending_forward[0] { - HTLCForwardInfo::AddHTLC { + HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { forward_info: PendingHTLCInfo { routing: PendingHTLCRouting::Forward { ref mut onion_packet, .. }, .. }, .. - } => { + }) => { onion_packet.hmac[onion_packet.hmac.len() - 1] ^= 1; Sha256::hash(&onion_packet.hop_data).into_inner().to_vec() }, @@ -1095,12 +1095,12 @@ fn test_phantom_invalid_onion_payload() { for (_, pending_forwards) in nodes[1].node.forward_htlcs.lock().unwrap().iter_mut() { for f in pending_forwards.iter_mut() { match f { - &mut HTLCForwardInfo::AddHTLC { + &mut HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { forward_info: PendingHTLCInfo { routing: PendingHTLCRouting::Forward { ref mut onion_packet, .. }, .. }, .. - } => { + }) => { // Construct the onion payloads for the entire route and an invalid amount. let height = nodes[0].best_block_info().1; let session_priv = SecretKey::from_slice(&session_priv).unwrap(); @@ -1166,9 +1166,9 @@ fn test_phantom_final_incorrect_cltv_expiry() { for (_, pending_forwards) in nodes[1].node.forward_htlcs.lock().unwrap().iter_mut() { for f in pending_forwards.iter_mut() { match f { - &mut HTLCForwardInfo::AddHTLC { + &mut HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { forward_info: PendingHTLCInfo { ref mut outgoing_cltv_value, .. }, .. - } => { + }) => { *outgoing_cltv_value += 1; }, _ => panic!("Unexpected forward"), From b3d257d70f2c02d5e4c455507a30838672862658 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 4 Nov 2022 14:16:20 -0400 Subject: [PATCH 2/4] Delete unnecessary whitespace in process_pending_forwards Only whitespace diff --- lightning/src/ln/channelmanager.rs | 128 ++++++++++++++--------------- 1 file changed, 64 insertions(+), 64 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b3eed7db4d2..9e087785b53 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3158,79 +3158,79 @@ impl ChannelManager { - macro_rules! failure_handler { - ($msg: expr, $err_code: expr, $err_data: expr, $phantom_ss: expr, $next_hop_unknown: expr) => { - log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg); - - let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData { - short_channel_id: prev_short_channel_id, - outpoint: prev_funding_outpoint, - htlc_id: prev_htlc_id, - incoming_packet_shared_secret: incoming_shared_secret, - phantom_shared_secret: $phantom_ss, - }); - - let reason = if $next_hop_unknown { - HTLCDestination::UnknownNextHop { requested_forward_scid: short_chan_id } - } else { - HTLCDestination::FailedPayment{ payment_hash } - }; - - failed_forwards.push((htlc_source, payment_hash, - HTLCFailReason::Reason { failure_code: $err_code, data: $err_data }, - reason - )); - continue; - } + macro_rules! failure_handler { + ($msg: expr, $err_code: expr, $err_data: expr, $phantom_ss: expr, $next_hop_unknown: expr) => { + log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg); + + let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData { + short_channel_id: prev_short_channel_id, + outpoint: prev_funding_outpoint, + htlc_id: prev_htlc_id, + incoming_packet_shared_secret: incoming_shared_secret, + phantom_shared_secret: $phantom_ss, + }); + + let reason = if $next_hop_unknown { + HTLCDestination::UnknownNextHop { requested_forward_scid: short_chan_id } + } else { + HTLCDestination::FailedPayment{ payment_hash } + }; + + failed_forwards.push((htlc_source, payment_hash, + HTLCFailReason::Reason { failure_code: $err_code, data: $err_data }, + reason + )); + continue; } - macro_rules! fail_forward { - ($msg: expr, $err_code: expr, $err_data: expr, $phantom_ss: expr) => { - { - failure_handler!($msg, $err_code, $err_data, $phantom_ss, true); - } + } + macro_rules! fail_forward { + ($msg: expr, $err_code: expr, $err_data: expr, $phantom_ss: expr) => { + { + failure_handler!($msg, $err_code, $err_data, $phantom_ss, true); } } - macro_rules! failed_payment { - ($msg: expr, $err_code: expr, $err_data: expr, $phantom_ss: expr) => { - { - failure_handler!($msg, $err_code, $err_data, $phantom_ss, false); - } + } + macro_rules! failed_payment { + ($msg: expr, $err_code: expr, $err_data: expr, $phantom_ss: expr) => { + { + failure_handler!($msg, $err_code, $err_data, $phantom_ss, false); } } - if let PendingHTLCRouting::Forward { onion_packet, .. } = routing { - let phantom_secret_res = self.keys_manager.get_node_secret(Recipient::PhantomNode); - if phantom_secret_res.is_ok() && fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, short_chan_id) { - let phantom_shared_secret = SharedSecret::new(&onion_packet.public_key.unwrap(), &phantom_secret_res.unwrap()).secret_bytes(); - let next_hop = match onion_utils::decode_next_payment_hop(phantom_shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) { - Ok(res) => res, - Err(onion_utils::OnionDecodeErr::Malformed { err_msg, err_code }) => { - let sha256_of_onion = Sha256::hash(&onion_packet.hop_data).into_inner(); - // In this scenario, the phantom would have sent us an - // `update_fail_malformed_htlc`, meaning here we encrypt the error as - // if it came from us (the second-to-last hop) but contains the sha256 - // of the onion. - failed_payment!(err_msg, err_code, sha256_of_onion.to_vec(), None); - }, - Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code }) => { - failed_payment!(err_msg, err_code, Vec::new(), Some(phantom_shared_secret)); - }, - }; - match next_hop { - onion_utils::Hop::Receive(hop_data) => { - match self.construct_recv_pending_htlc_info(hop_data, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value, Some(phantom_shared_secret)) { - Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, vec![(info, prev_htlc_id)])), - Err(ReceiveError { err_code, err_data, msg }) => failed_payment!(msg, err_code, err_data, Some(phantom_shared_secret)) - } - }, - _ => panic!(), - } - } else { - fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new(), None); + } + if let PendingHTLCRouting::Forward { onion_packet, .. } = routing { + let phantom_secret_res = self.keys_manager.get_node_secret(Recipient::PhantomNode); + if phantom_secret_res.is_ok() && fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, short_chan_id) { + let phantom_shared_secret = SharedSecret::new(&onion_packet.public_key.unwrap(), &phantom_secret_res.unwrap()).secret_bytes(); + let next_hop = match onion_utils::decode_next_payment_hop(phantom_shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) { + Ok(res) => res, + Err(onion_utils::OnionDecodeErr::Malformed { err_msg, err_code }) => { + let sha256_of_onion = Sha256::hash(&onion_packet.hop_data).into_inner(); + // In this scenario, the phantom would have sent us an + // `update_fail_malformed_htlc`, meaning here we encrypt the error as + // if it came from us (the second-to-last hop) but contains the sha256 + // of the onion. + failed_payment!(err_msg, err_code, sha256_of_onion.to_vec(), None); + }, + Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code }) => { + failed_payment!(err_msg, err_code, Vec::new(), Some(phantom_shared_secret)); + }, + }; + match next_hop { + onion_utils::Hop::Receive(hop_data) => { + match self.construct_recv_pending_htlc_info(hop_data, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value, Some(phantom_shared_secret)) { + Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, vec![(info, prev_htlc_id)])), + Err(ReceiveError { err_code, err_data, msg }) => failed_payment!(msg, err_code, err_data, Some(phantom_shared_secret)) + } + }, + _ => panic!(), } } else { fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new(), None); } - }, + } else { + fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new(), None); + } + }, HTLCForwardInfo::FailHTLC { .. } => { // Channel went away before we could fail it. This implies // the channel is now on chain and our counterparty is From 203394ff94fb3fe406f075e7c5509e1eb38cc655 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 4 Nov 2022 12:42:48 -0400 Subject: [PATCH 3/4] Track incoming amount in PendingHTLCInfo Used in upcoming commit(s) when we generate the PaymentIntercepted event for intercepted payments. Co-authored-by: John Cantrell Co-authored-by: Valentine Wallace --- lightning/src/ln/channelmanager.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 9e087785b53..c28ca5cf39d 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -113,6 +113,7 @@ pub(super) struct PendingHTLCInfo { pub(super) incoming_shared_secret: [u8; 32], payment_hash: PaymentHash, pub(super) amt_to_forward: u64, + pub(super) amt_incoming: Option, // Added in 0.0.113 pub(super) outgoing_cltv_value: u32, } @@ -2196,6 +2197,7 @@ impl ChannelManager ChannelManager ChannelManager { macro_rules! failure_handler { @@ -3262,7 +3265,7 @@ impl ChannelManager { log_trace!(self.logger, "Adding HTLC from short id {} with payment_hash {} to channel with short id {} after delay", prev_short_channel_id, log_bytes!(payment_hash.0), short_chan_id); @@ -6470,7 +6473,8 @@ impl_writeable_tlv_based!(PendingHTLCInfo, { (2, incoming_shared_secret, required), (4, payment_hash, required), (6, amt_to_forward, required), - (8, outgoing_cltv_value, required) + (8, outgoing_cltv_value, required), + (9, amt_incoming, option), }); From c7935497fcab2bccd0ccca7e5a833d51a8e31c66 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 7 Nov 2022 17:29:23 -0500 Subject: [PATCH 4/4] Fix scid_utils::is_valid* false positive cargo bench was able to find an scid of 0 as a valid fake scid --- lightning/src/ln/channelmanager.rs | 4 ++-- lightning/src/util/scid_utils.rs | 16 +++++++++------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index c28ca5cf39d..ba372920707 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2313,7 +2313,7 @@ impl ChannelManager { // unknown_next_peer // Note that this is likely a timing oracle for detecting whether an scid is a // phantom. - if fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, *short_channel_id) { + if fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash) { None } else { break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None)); @@ -3202,7 +3202,7 @@ impl ChannelManager res, diff --git a/lightning/src/util/scid_utils.rs b/lightning/src/util/scid_utils.rs index b851cca81aa..c3529a6cbd2 100644 --- a/lightning/src/util/scid_utils.rs +++ b/lightning/src/util/scid_utils.rs @@ -73,7 +73,7 @@ pub(crate) mod fake_scid { use core::convert::TryInto; use core::ops::Deref; - const TEST_SEGWIT_ACTIVATION_HEIGHT: u32 = 0; + const TEST_SEGWIT_ACTIVATION_HEIGHT: u32 = 1; const MAINNET_SEGWIT_ACTIVATION_HEIGHT: u32 = 481_824; const MAX_TX_INDEX: u32 = 2_500; const MAX_NAMESPACES: u8 = 8; // We allocate 3 bits for the namespace identifier. @@ -151,12 +151,13 @@ pub(crate) mod fake_scid { } /// Returns whether the given fake scid falls into the given namespace. - pub fn is_valid_phantom(fake_scid_rand_bytes: &[u8; 32], scid: u64) -> bool { + pub fn is_valid_phantom(fake_scid_rand_bytes: &[u8; 32], scid: u64, genesis_hash: &BlockHash) -> bool { let block_height = scid_utils::block_from_scid(&scid); let tx_index = scid_utils::tx_index_from_scid(&scid); let namespace = Namespace::Phantom; let valid_vout = namespace.get_encrypted_vout(block_height, tx_index, fake_scid_rand_bytes); - valid_vout == scid_utils::vout_from_scid(&scid) as u8 + block_height >= segwit_activation_height(genesis_hash) + && valid_vout == scid_utils::vout_from_scid(&scid) as u8 } #[cfg(test)] @@ -194,11 +195,12 @@ pub(crate) mod fake_scid { fn test_is_valid_phantom() { let namespace = Namespace::Phantom; let fake_scid_rand_bytes = [0; 32]; + let testnet_genesis = genesis_block(Network::Testnet).header.block_hash(); let valid_encrypted_vout = namespace.get_encrypted_vout(0, 0, &fake_scid_rand_bytes); - let valid_fake_scid = scid_utils::scid_from_parts(0, 0, valid_encrypted_vout as u64).unwrap(); - assert!(is_valid_phantom(&fake_scid_rand_bytes, valid_fake_scid)); - let invalid_fake_scid = scid_utils::scid_from_parts(0, 0, 12).unwrap(); - assert!(!is_valid_phantom(&fake_scid_rand_bytes, invalid_fake_scid)); + let valid_fake_scid = scid_utils::scid_from_parts(1, 0, valid_encrypted_vout as u64).unwrap(); + assert!(is_valid_phantom(&fake_scid_rand_bytes, valid_fake_scid, &testnet_genesis)); + let invalid_fake_scid = scid_utils::scid_from_parts(1, 0, 12).unwrap(); + assert!(!is_valid_phantom(&fake_scid_rand_bytes, invalid_fake_scid, &testnet_genesis)); } #[test]