From 803366a41a8808a1221ec687cb8f56145537b4fe Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 8 Sep 2024 16:09:12 +0000 Subject: [PATCH 1/3] Do not check the ordering of HTLCs in `PaymentClaim[able,ed]` In the next commit we'll change the order of HTLCs in `PaymentClaim[able,ed]` events. This shouldn't break anything, but our current functional tests check that the HTLCs are provided in the order they expect (the order they were received). Instead, here we only validate that each claimed HTLC matches one expected path. --- lightning/src/ln/functional_test_utils.rs | 28 +++++++++++++++-------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 1c5af0d8f06..fe5cd18d50e 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1083,19 +1083,29 @@ macro_rules! check_added_monitors { } } -/// Checks whether the claimed HTLC for the specified path has the correct channel information. -/// -/// This will panic if the path is empty, if the HTLC's channel ID is not actually a channel that -/// connects the final two nodes in the path, or if the `user_channel_id` is incorrect. -pub fn check_claimed_htlc_channel<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, path: &[&Node<'a, 'b, 'c>], htlc: &ClaimedHTLC) { +fn claimed_htlc_matches_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, path: &[&Node<'a, 'b, 'c>], htlc: &ClaimedHTLC) -> bool { let mut nodes = path.iter().rev(); let dest = nodes.next().expect("path should have a destination").node; let prev = nodes.next().unwrap_or(&origin_node).node; let dest_channels = dest.list_channels(); let ch = dest_channels.iter().find(|ch| ch.channel_id == htlc.channel_id) .expect("HTLC's channel should be one of destination node's channels"); - assert_eq!(htlc.user_channel_id, ch.user_channel_id); - assert_eq!(ch.counterparty.node_id, prev.get_our_node_id()); + htlc.user_channel_id == ch.user_channel_id && + ch.counterparty.node_id == prev.get_our_node_id() +} + +fn check_claimed_htlcs_match_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: &[&[&Node<'a, 'b, 'c>]], htlcs: &[ClaimedHTLC]) { + assert_eq!(route.len(), htlcs.len()); + for path in route { + let mut found_matching_htlc = false; + for htlc in htlcs { + if claimed_htlc_matches_path(origin_node, path, htlc) { + found_matching_htlc = true; + break; + } + } + assert!(found_matching_htlc); + } } pub fn _reload_node<'a, 'b, 'c>(node: &'a Node<'a, 'b, 'c>, default_config: UserConfig, chanman_encoded: &[u8], monitors_encoded: &[&[u8]]) -> TestChannelManager<'b, 'c> { @@ -2832,7 +2842,7 @@ pub fn pass_claimed_payment_along_route(args: ClaimAlongRouteArgs) -> u64 { assert_eq!(htlcs.len(), expected_paths.len()); // One per path. assert_eq!(htlcs.iter().map(|h| h.value_msat).sum::(), amount_msat); assert_eq!(onion_fields.as_ref().unwrap().custom_tlvs, custom_tlvs); - expected_paths.iter().zip(htlcs).for_each(|(path, htlc)| check_claimed_htlc_channel(origin_node, path, htlc)); + check_claimed_htlcs_match_route(origin_node, expected_paths, htlcs); fwd_amt_msat = amount_msat; }, Event::PaymentClaimed { @@ -2849,7 +2859,7 @@ pub fn pass_claimed_payment_along_route(args: ClaimAlongRouteArgs) -> u64 { assert_eq!(htlcs.len(), expected_paths.len()); // One per path. assert_eq!(htlcs.iter().map(|h| h.value_msat).sum::(), amount_msat); assert_eq!(onion_fields.as_ref().unwrap().custom_tlvs, custom_tlvs); - expected_paths.iter().zip(htlcs).for_each(|(path, htlc)| check_claimed_htlc_channel(origin_node, path, htlc)); + check_claimed_htlcs_match_route(origin_node, expected_paths, htlcs); fwd_amt_msat = amount_msat; } _ => panic!(), From aa09c33a1719944769ba98624bfe18ea33083f44 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 8 Sep 2024 16:38:22 +0000 Subject: [PATCH 2/3] Add an `inbound_payment_id_secret` to `ChannelManager` In the next commit we'll start generating `PaymentId`s for inbound payments randomly by HMAC'ing the HTLC set of the payment. Here we start by defining the HMAC secret for these HMACs. This requires one small test adaptation and a full_stack_target fuzz change because it changes the RNG consumption. --- fuzz/src/full_stack.rs | 2 +- lightning/src/ln/channelmanager.rs | 11 +++++++++++ lightning/src/ln/functional_tests.rs | 4 ++-- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 90449248e32..e0ce11537ef 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -664,7 +664,7 @@ pub fn do_test(mut data: &[u8], logger: &Arc) { // Adding new calls to `EntropySource::get_secure_random_bytes` during startup can change all the // keys subsequently generated in this test. Rather than regenerating all the messages manually, // it's easier to just increment the counter here so the keys don't change. - keys_manager.counter.fetch_sub(3, Ordering::AcqRel); + keys_manager.counter.fetch_sub(4, Ordering::AcqRel); let network_graph = Arc::new(NetworkGraph::new(network, Arc::clone(&logger))); let gossip_sync = Arc::new(P2PGossipSync::new(Arc::clone(&network_graph), None, Arc::clone(&logger))); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 2cfa60ea761..0f9bfd88907 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2261,6 +2261,9 @@ where /// keeping additional state. probing_cookie_secret: [u8; 32], + /// When generating [`PaymentId`]s for inbound payments, we HMAC the HTLCs with this secret. + inbound_payment_id_secret: [u8; 32], + /// The highest block timestamp we've seen, which is usually a good guess at the current time. /// Assuming most miners are generating blocks with reasonable timestamps, this shouldn't be /// very far in the past, and can only ever be up to two hours in the future. @@ -3152,6 +3155,7 @@ where fake_scid_rand_bytes: entropy_source.get_secure_random_bytes(), probing_cookie_secret: entropy_source.get_secure_random_bytes(), + inbound_payment_id_secret: entropy_source.get_secure_random_bytes(), highest_seen_timestamp: AtomicUsize::new(current_timestamp as usize), @@ -12381,6 +12385,7 @@ where let mut events_override = None; let mut in_flight_monitor_updates: Option>> = None; let mut decode_update_add_htlcs: Option>> = None; + let mut inbound_payment_id_secret = None; read_tlv_fields!(reader, { (1, pending_outbound_payments_no_retry, option), (2, pending_intercepted_htlcs, option), @@ -12395,6 +12400,7 @@ where (11, probing_cookie_secret, option), (13, claimable_htlc_onion_fields, optional_vec), (14, decode_update_add_htlcs, option), + (15, inbound_payment_id_secret, option), }); let mut decode_update_add_htlcs = decode_update_add_htlcs.unwrap_or_else(|| new_hash_map()); if fake_scid_rand_bytes.is_none() { @@ -12405,6 +12411,10 @@ where probing_cookie_secret = Some(args.entropy_source.get_secure_random_bytes()); } + if inbound_payment_id_secret.is_none() { + inbound_payment_id_secret = Some(args.entropy_source.get_secure_random_bytes()); + } + if let Some(events) = events_override { pending_events_read = events; } @@ -12930,6 +12940,7 @@ where fake_scid_rand_bytes: fake_scid_rand_bytes.unwrap(), probing_cookie_secret: probing_cookie_secret.unwrap(), + inbound_payment_id_secret: inbound_payment_id_secret.unwrap(), our_network_pubkey, secp_ctx, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index efd2fc9e9d6..31346c6b78b 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -7670,8 +7670,8 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { assert_ne!(node_txn[0].input[0].previous_output, node_txn[2].input[0].previous_output); assert_ne!(node_txn[1].input[0].previous_output, node_txn[2].input[0].previous_output); - assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output); - assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output); + assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output); + assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output); // node_txn[3] spends the revoked outputs from the revoked_htlc_txn (which only have one // output, checked above). From aaf529d19366bde82c0bee632dddfd8e38692caa Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 12 Sep 2024 18:36:58 +0000 Subject: [PATCH 3/3] Add a `PaymentId` for inbound payments We expect our users to have fully idempotent `Event` handling as we may replay events on restart for one of a number of reasons. This isn't a big deal as long as all our events have some kind of identifier users can use to check if the `Event` has already been handled. For outbound payments, this is the `PaymentId` they provide in the send methods, however for inbound payments we don't have a great option. `PaymentHash` largely suffices - users can simply always claim in response to a `PaymentClaimable` of sufficient value and treat a `PaymentClaimed` event as duplicate any time they see a second one for the same `PaymentHash`. This mostly works, but may result in accepting duplicative payments if someone (incorrectly) pays twice for the same `PaymentHash`. Users could also fail for duplicative `PaymentClaimable` events of the same `PaymentHash`, but doing so may result in spuriously failing a payment if the `PaymentClaimable` event is a replay and they never saw a corresponding `PaymentClaimed` event. While none of this will result in spuriously thinking they've been paid when they have not, it does result in some pretty awkward semantics which we'd rather avoid our users having to deal with. Instead, here, we add a new `PaymentId` which is simply an HMAC of the HTLCs (as Channel ID, HTLC ID pairs) which were included in the payment. --- lightning/src/events/mod.rs | 31 ++++++++++++- lightning/src/ln/channelmanager.rs | 73 ++++++++++++++++++++++++++---- lightning/src/ln/msgs.rs | 3 +- 3 files changed, 95 insertions(+), 12 deletions(-) diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index fe52d08c9e1..379a0d79a9a 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -749,6 +749,14 @@ pub enum Event { /// /// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds claim_deadline: Option, + /// A unique ID describing this payment (derived from the list of HTLCs in the payment). + /// + /// Payers may pay for the same [`PaymentHash`] multiple times (though this is unsafe and + /// an intermediary node may steal the funds). Thus, in order to accurately track when + /// payments are received and claimed, you should use this identifier. + /// + /// Only filled in for payments received on LDK versions 0.1 and higher. + payment_id: Option, }, /// Indicates a payment has been claimed and we've received money! /// @@ -796,6 +804,14 @@ pub enum Event { /// /// Payments received on LDK versions prior to 0.0.124 will have this field unset. onion_fields: Option, + /// A unique ID describing this payment (derived from the list of HTLCs in the payment). + /// + /// Payers may pay for the same [`PaymentHash`] multiple times (though this is unsafe and + /// an intermediary node may steal the funds). Thus, in order to accurately track when + /// payments are received and claimed, you should use this identifier. + /// + /// Only filled in for payments received on LDK versions 0.1 and higher. + payment_id: Option, }, /// Indicates that a peer connection with a node is needed in order to send an [`OnionMessage`]. /// @@ -1432,7 +1448,7 @@ impl Writeable for Event { }, &Event::PaymentClaimable { ref payment_hash, ref amount_msat, counterparty_skimmed_fee_msat, ref purpose, ref receiver_node_id, ref via_channel_id, ref via_user_channel_id, - ref claim_deadline, ref onion_fields + ref claim_deadline, ref onion_fields, ref payment_id, } => { 1u8.write(writer)?; let mut payment_secret = None; @@ -1478,6 +1494,7 @@ impl Writeable for Event { (9, onion_fields, option), (10, skimmed_fee_opt, option), (11, payment_context, option), + (13, payment_id, option), }); }, &Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => { @@ -1634,7 +1651,10 @@ impl Writeable for Event { // We never write the OpenChannelRequest events as, upon disconnection, peers // drop any channels which have not yet exchanged funding_signed. }, - &Event::PaymentClaimed { ref payment_hash, ref amount_msat, ref purpose, ref receiver_node_id, ref htlcs, ref sender_intended_total_msat, ref onion_fields } => { + &Event::PaymentClaimed { ref payment_hash, ref amount_msat, ref purpose, + ref receiver_node_id, ref htlcs, ref sender_intended_total_msat, ref onion_fields, + ref payment_id, + } => { 19u8.write(writer)?; write_tlv_fields!(writer, { (0, payment_hash, required), @@ -1644,6 +1664,7 @@ impl Writeable for Event { (5, *htlcs, optional_vec), (7, sender_intended_total_msat, option), (9, onion_fields, option), + (11, payment_id, option), }); }, &Event::ProbeSuccessful { ref payment_id, ref payment_hash, ref path } => { @@ -1767,6 +1788,7 @@ impl MaybeReadable for Event { let mut via_user_channel_id = None; let mut onion_fields = None; let mut payment_context = None; + let mut payment_id = None; read_tlv_fields!(reader, { (0, payment_hash, required), (1, receiver_node_id, option), @@ -1780,6 +1802,7 @@ impl MaybeReadable for Event { (9, onion_fields, option), (10, counterparty_skimmed_fee_msat_opt, option), (11, payment_context, option), + (13, payment_id, option), }); let purpose = match payment_secret { Some(secret) => PaymentPurpose::from_parts(payment_preimage, secret, payment_context), @@ -1796,6 +1819,7 @@ impl MaybeReadable for Event { via_user_channel_id, claim_deadline, onion_fields, + payment_id, })) }; f() @@ -2037,6 +2061,7 @@ impl MaybeReadable for Event { let mut htlcs: Option> = Some(vec![]); let mut sender_intended_total_msat: Option = None; let mut onion_fields = None; + let mut payment_id = None; read_tlv_fields!(reader, { (0, payment_hash, required), (1, receiver_node_id, option), @@ -2045,6 +2070,7 @@ impl MaybeReadable for Event { (5, htlcs, optional_vec), (7, sender_intended_total_msat, option), (9, onion_fields, option), + (11, payment_id, option), }); Ok(Some(Event::PaymentClaimed { receiver_node_id, @@ -2054,6 +2080,7 @@ impl MaybeReadable for Event { htlcs: htlcs.unwrap_or_default(), sender_intended_total_msat, onion_fields, + payment_id, })) }; f() diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0f9bfd88907..241b02e0957 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -23,7 +23,7 @@ use bitcoin::constants::ChainHash; use bitcoin::key::constants::SECRET_KEY_SIZE; use bitcoin::network::Network; -use bitcoin::hashes::Hash; +use bitcoin::hashes::{Hash, HashEngine, HmacEngine}; use bitcoin::hashes::hmac::Hmac; use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::hash_types::{BlockHash, Txid}; @@ -368,6 +368,7 @@ pub(crate) struct HTLCPreviousHopData { counterparty_node_id: Option, } +#[derive(PartialEq, Eq)] enum OnionPayload { /// Indicates this incoming onion payload is for the purpose of paying an invoice. Invoice { @@ -380,6 +381,7 @@ enum OnionPayload { } /// HTLCs that are to us and can be failed/claimed by the user +#[derive(PartialEq, Eq)] struct ClaimableHTLC { prev_hop: HTLCPreviousHopData, cltv_expiry: u32, @@ -411,6 +413,23 @@ impl From<&ClaimableHTLC> for events::ClaimedHTLC { } } +impl PartialOrd for ClaimableHTLC { + fn partial_cmp(&self, other: &ClaimableHTLC) -> Option { + Some(self.cmp(other)) + } +} +impl Ord for ClaimableHTLC { + fn cmp(&self, other: &ClaimableHTLC) -> cmp::Ordering { + let res = (self.prev_hop.channel_id, self.prev_hop.htlc_id).cmp( + &(other.prev_hop.channel_id, other.prev_hop.htlc_id) + ); + if res.is_eq() { + debug_assert!(self == other, "ClaimableHTLCs from the same source should be identical"); + } + res + } +} + /// A trait defining behavior for creating and verifing the HMAC for authenticating a given data. pub trait Verification { /// Constructs an HMAC to include in [`OffersContext`] for the data along with the given @@ -491,6 +510,22 @@ impl Verification for PaymentId { } } +impl PaymentId { + fn for_inbound_from_htlcs>(key: &[u8; 32], htlcs: I) -> PaymentId { + let mut prev_pair = None; + let mut hasher = HmacEngine::new(key); + for (channel_id, htlc_id) in htlcs { + hasher.input(&channel_id.0); + hasher.input(&htlc_id.to_le_bytes()); + if let Some(prev) = prev_pair { + debug_assert!(prev < (channel_id, htlc_id), "HTLCs should be sorted"); + } + prev_pair = Some((channel_id, htlc_id)); + } + PaymentId(Hmac::::from_engine(hasher).to_byte_array()) + } +} + impl Writeable for PaymentId { fn write(&self, w: &mut W) -> Result<(), io::Error> { self.0.write(w) @@ -764,6 +799,7 @@ struct ClaimingPayment { htlcs: Vec, sender_intended_value: Option, onion_fields: Option, + payment_id: Option, } impl_writeable_tlv_based!(ClaimingPayment, { (0, amount_msat, required), @@ -772,6 +808,7 @@ impl_writeable_tlv_based!(ClaimingPayment, { (5, htlcs, optional_vec), (7, sender_intended_value, option), (9, onion_fields, option), + (11, payment_id, option), }); struct ClaimablePayment { @@ -780,6 +817,15 @@ struct ClaimablePayment { htlcs: Vec, } +impl ClaimablePayment { + fn inbound_payment_id(&self, secret: &[u8; 32]) -> PaymentId { + PaymentId::for_inbound_from_htlcs( + secret, + self.htlcs.iter().map(|htlc| (htlc.prev_hop.channel_id, htlc.prev_hop.htlc_id)) + ) + } +} + /// Represent the channel funding transaction type. enum FundingType { /// This variant is useful when we want LDK to validate the funding transaction and @@ -5749,10 +5795,9 @@ where } else { claimable_payment.onion_fields = Some(onion_fields); } - let ref mut htlcs = &mut claimable_payment.htlcs; let mut total_value = claimable_htlc.sender_intended_value; let mut earliest_expiry = claimable_htlc.cltv_expiry; - for htlc in htlcs.iter() { + for htlc in claimable_payment.htlcs.iter() { total_value += htlc.sender_intended_value; earliest_expiry = cmp::min(earliest_expiry, htlc.cltv_expiry); if htlc.total_msat != claimable_htlc.total_msat { @@ -5774,13 +5819,18 @@ where #[allow(unused_assignments)] { committed_to_claimable = true; } - htlcs.push(claimable_htlc); - let amount_msat = htlcs.iter().map(|htlc| htlc.value).sum(); - htlcs.iter_mut().for_each(|htlc| htlc.total_value_received = Some(amount_msat)); - let counterparty_skimmed_fee_msat = htlcs.iter() + claimable_payment.htlcs.push(claimable_htlc); + let amount_msat = + claimable_payment.htlcs.iter().map(|htlc| htlc.value).sum(); + claimable_payment.htlcs.iter_mut() + .for_each(|htlc| htlc.total_value_received = Some(amount_msat)); + let counterparty_skimmed_fee_msat = claimable_payment.htlcs.iter() .map(|htlc| htlc.counterparty_skimmed_fee_msat.unwrap_or(0)).sum(); debug_assert!(total_value.saturating_sub(amount_msat) <= counterparty_skimmed_fee_msat); + claimable_payment.htlcs.sort(); + let payment_id = + claimable_payment.inbound_payment_id(&self.inbound_payment_id_secret); new_events.push_back((events::Event::PaymentClaimable { receiver_node_id: Some(receiver_node_id), payment_hash, @@ -5791,13 +5841,14 @@ where via_user_channel_id: Some(prev_user_channel_id), claim_deadline: Some(earliest_expiry - HTLC_FAIL_BACK_BUFFER), onion_fields: claimable_payment.onion_fields.clone(), + payment_id: Some(payment_id), }, None)); payment_claimable_generated = true; } else { // Nothing to do - we haven't reached the total // payment value yet, wait until we receive more // MPP parts. - htlcs.push(claimable_htlc); + claimable_payment.htlcs.push(claimable_htlc); #[allow(unused_assignments)] { committed_to_claimable = true; } @@ -6594,6 +6645,7 @@ where } } + let payment_id = payment.inbound_payment_id(&self.inbound_payment_id_secret); let claiming_payment = claimable_payments.pending_claiming_payments .entry(payment_hash) .and_modify(|_| { @@ -6611,6 +6663,7 @@ where htlcs, sender_intended_value, onion_fields: payment.onion_fields, + payment_id: Some(payment_id), } }); @@ -7128,6 +7181,7 @@ where htlcs, sender_intended_value: sender_intended_total_msat, onion_fields, + payment_id, }) = payment { self.pending_events.lock().unwrap().push_back((events::Event::PaymentClaimed { payment_hash, @@ -7137,6 +7191,7 @@ where htlcs, sender_intended_total_msat, onion_fields, + payment_id, }, None)); } }, @@ -12863,6 +12918,7 @@ where previous_hop_monitor.provide_payment_preimage(&payment_hash, &payment_preimage, &args.tx_broadcaster, &bounded_fee_estimator, &args.logger); } } + let payment_id = payment.inbound_payment_id(&inbound_payment_id_secret.unwrap()); pending_events_read.push_back((events::Event::PaymentClaimed { receiver_node_id, payment_hash, @@ -12871,6 +12927,7 @@ where htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(), sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat), onion_fields: payment.onion_fields, + payment_id: Some(payment_id), }, None)); } } diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index c89274f47d8..be5ecb27ae0 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -1729,8 +1729,7 @@ pub trait OnionMessageHandler { fn provided_init_features(&self, their_node_id: PublicKey) -> InitFeatures; } -#[derive(Clone)] -#[cfg_attr(test, derive(Debug, PartialEq))] +#[derive(Clone, Debug, PartialEq, Eq)] /// Information communicated in the onion to the recipient for multi-part tracking and proof that /// the payment is associated with an invoice. pub struct FinalOnionHopData {