From 749ae12e2230ff2a9d34ec767bdc4623a2649ca7 Mon Sep 17 00:00:00 2001 From: Alec Chen Date: Thu, 20 Apr 2023 16:00:47 -0500 Subject: [PATCH 1/5] Allow user to opt-in to accepting MPP keysend To support receiving MPP keysends, we will add a new non-backward compatible field to `PendingHTLCRouting::ReceiveKeysend`, which will break deserialization of `ChannelManager` on downgrades, so we allow the user choose whether they want to do this. Note that this commit only adds the config flag, while full MPP support is added in later commits. --- lightning/src/util/config.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lightning/src/util/config.rs b/lightning/src/util/config.rs index 1e678152ccc..fef5338ce6f 100644 --- a/lightning/src/util/config.rs +++ b/lightning/src/util/config.rs @@ -552,6 +552,17 @@ pub struct UserConfig { /// [`ChannelManager::get_intercept_scid`]: crate::ln::channelmanager::ChannelManager::get_intercept_scid /// [`Event::HTLCIntercepted`]: crate::events::Event::HTLCIntercepted pub accept_intercept_htlcs: bool, + /// If this is set to false, when receiving a keysend payment we'll fail it if it has multiple + /// parts. If this is set to true, we'll accept the payment. + /// + /// Setting this to true will break backwards compatibility upon downgrading to an LDK + /// version < 0.0.116 while receiving an MPP keysend. If we have already received an MPP + /// keysend, downgrading will cause us to fail to deserialize [`ChannelManager`]. + /// + /// Default value: false. + /// + /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager + pub accept_mpp_keysend: bool, } impl Default for UserConfig { @@ -564,6 +575,7 @@ impl Default for UserConfig { accept_inbound_channels: true, manually_accept_inbound_channels: false, accept_intercept_htlcs: false, + accept_mpp_keysend: false, } } } From 07def9292a49bb240daba6dec8b289d03bbb3f50 Mon Sep 17 00:00:00 2001 From: Alec Chen Date: Mon, 8 May 2023 17:51:19 -0500 Subject: [PATCH 2/5] Help users support sending MPP keysend When routing a keysend payment, the user may want to signal to the router whether to find multi-path routes in the `PaymentParameters::for_keysend` helper, without going through manual construction. Since some implementations do not support MPP keysend, we have the user make the choice here rather than making it the default. Some implementations will reject keysend payments with payment secrets, so this commit also adds docs to `RecipientOnionFields` to communicate this to the user. --- lightning/src/ln/channelmanager.rs | 8 +++----- lightning/src/ln/features.rs | 8 +++++++- lightning/src/ln/functional_tests.rs | 4 ++-- lightning/src/ln/outbound_payment.rs | 13 ++++++++----- lightning/src/routing/router.rs | 13 +++++++++++-- 5 files changed, 31 insertions(+), 15 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f6cb81376e2..9007188fdaf 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2924,8 +2924,6 @@ where /// Similar to regular payments, you MUST NOT reuse a `payment_preimage` value. See /// [`send_payment`] for more information about the risks of duplicate preimage usage. /// - /// Note that `route` must have exactly one path. - /// /// [`send_payment`]: Self::send_payment pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option, recipient_onion: RecipientOnionFields, payment_id: PaymentId) -> Result { let best_block_height = self.best_block.read().unwrap().height(); @@ -8580,7 +8578,7 @@ mod tests { // Next, attempt a keysend payment and make sure it fails. let route_params = RouteParameters { - payment_params: PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV), + payment_params: PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV, false), final_value_msat: 100_000, }; let route = find_route( @@ -8673,7 +8671,7 @@ mod tests { let _chan = create_chan_between_nodes(&nodes[0], &nodes[1]); let route_params = RouteParameters { - payment_params: PaymentParameters::for_keysend(payee_pubkey, 40), + payment_params: PaymentParameters::for_keysend(payee_pubkey, 40, false), final_value_msat: 10_000, }; let network_graph = nodes[0].network_graph.clone(); @@ -8717,7 +8715,7 @@ mod tests { let _chan = create_chan_between_nodes(&nodes[0], &nodes[1]); let route_params = RouteParameters { - payment_params: PaymentParameters::for_keysend(payee_pubkey, 40), + payment_params: PaymentParameters::for_keysend(payee_pubkey, 40, false), final_value_msat: 10_000, }; let network_graph = nodes[0].network_graph.clone(); diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index 7d4b86f5aca..d24d32ba0fa 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -533,11 +533,17 @@ impl InvoiceFeatures { /// [`PaymentParameters::for_keysend`], thus omitting the need for payers to manually construct an /// `InvoiceFeatures` for [`find_route`]. /// + /// MPP keysend is not widely supported yet, so we parameterize support to allow the user to + /// choose whether their router should find multi-part routes. + /// /// [`PaymentParameters::for_keysend`]: crate::routing::router::PaymentParameters::for_keysend /// [`find_route`]: crate::routing::router::find_route - pub(crate) fn for_keysend() -> InvoiceFeatures { + pub(crate) fn for_keysend(allow_mpp: bool) -> InvoiceFeatures { let mut res = InvoiceFeatures::empty(); res.set_variable_length_onion_optional(); + if allow_mpp { + res.set_basic_mpp_optional(); + } res } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 98fc4bd95b6..1efca73d478 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -9478,7 +9478,7 @@ fn test_keysend_payments_to_public_node() { let payer_pubkey = nodes[0].node.get_our_node_id(); let payee_pubkey = nodes[1].node.get_our_node_id(); let route_params = RouteParameters { - payment_params: PaymentParameters::for_keysend(payee_pubkey, 40), + payment_params: PaymentParameters::for_keysend(payee_pubkey, 40, false), final_value_msat: 10000, }; let scorer = test_utils::TestScorer::new(); @@ -9509,7 +9509,7 @@ fn test_keysend_payments_to_private_node() { let _chan = create_chan_between_nodes(&nodes[0], &nodes[1]); let route_params = RouteParameters { - payment_params: PaymentParameters::for_keysend(payee_pubkey, 40), + payment_params: PaymentParameters::for_keysend(payee_pubkey, 40, false), final_value_msat: 10000, }; let network_graph = nodes[0].network_graph.clone(); diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index f107f3b5583..39d95e9b841 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -414,9 +414,9 @@ pub struct RecipientOnionFields { /// If you do not have one, the [`Route`] you pay over must not contain multiple paths as /// multi-path payments require a recipient-provided secret. /// - /// Note that for spontaneous payments most lightning nodes do not currently support MPP - /// receives, thus you should generally never be providing a secret here for spontaneous - /// payments. + /// Some implementations may reject spontaneous payments with payment secrets, so you may only + /// want to provide a secret for a spontaneous payment if MPP is needed and you know your + /// recipient will not reject it. pub payment_secret: Option, /// The payment metadata serves a similar purpose as [`Self::payment_secret`] but is of /// arbitrary length. This gives recipients substantially more flexibility to receive @@ -447,10 +447,13 @@ impl RecipientOnionFields { } /// Creates a new [`RecipientOnionFields`] with no fields. This generally does not create - /// payable HTLCs except for spontaneous payments, i.e. this should generally only be used for - /// calls to [`ChannelManager::send_spontaneous_payment`]. + /// payable HTLCs except for single-path spontaneous payments, i.e. this should generally + /// only be used for calls to [`ChannelManager::send_spontaneous_payment`]. If you are sending + /// a spontaneous MPP this will not work as all MPP require payment secrets; you may + /// instead want to use [`RecipientOnionFields::secret_only`]. /// /// [`ChannelManager::send_spontaneous_payment`]: super::channelmanager::ChannelManager::send_spontaneous_payment + /// [`RecipientOnionFields::secret_only`]: RecipientOnionFields::secret_only pub fn spontaneous_empty() -> Self { Self { payment_secret: None, payment_metadata: None } } diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index b33e021ab4f..da3b3d4eb1b 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -621,8 +621,17 @@ impl PaymentParameters { /// /// The `final_cltv_expiry_delta` should match the expected final CLTV delta the recipient has /// provided. - pub fn for_keysend(payee_pubkey: PublicKey, final_cltv_expiry_delta: u32) -> Self { - Self::from_node_id(payee_pubkey, final_cltv_expiry_delta).with_bolt11_features(InvoiceFeatures::for_keysend()).expect("PaymentParameters::from_node_id should always initialize the payee as unblinded") + /// + /// Note that MPP keysend is not widely supported yet. The `allow_mpp` lets you choose + /// whether your router will be allowed to find a multi-part route for this payment. If you + /// set `allow_mpp` to true, you should ensure a payment secret is set on send, likely via + /// [`RecipientOnionFields::secret_only`]. + /// + /// [`RecipientOnionFields::secret_only`]: crate::ln::channelmanager::RecipientOnionFields::secret_only + pub fn for_keysend(payee_pubkey: PublicKey, final_cltv_expiry_delta: u32, allow_mpp: bool) -> Self { + Self::from_node_id(payee_pubkey, final_cltv_expiry_delta) + .with_bolt11_features(InvoiceFeatures::for_keysend(allow_mpp)) + .expect("PaymentParameters::from_node_id should always initialize the payee as unblinded") } /// Includes the payee's features. Errors if the parameters were initialized with blinded payment From 4be3adb51ff7e3f9d3ae6b153e47f7e46497e551 Mon Sep 17 00:00:00 2001 From: Alec Chen Date: Mon, 1 May 2023 23:05:43 -0500 Subject: [PATCH 3/5] Track MPP data while receiving keysends This commit adds the field `payment_data: FinalOnionHopData` to `ReceiveKeysend` which will allow us to check for payment secrets and total amounts which is needed to support receiving MPP keysends. This field is non-backwards compatible since we wouldn't be able to handle an MPP keysend properly if we were to downgrade to a prior version. We also no longer reject keysends with payment secrets if we support MPP keysend. --- lightning/src/ln/channelmanager.rs | 50 ++++++++++++++++++------------ 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 9007188fdaf..f3ab0ac5ab3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -112,6 +112,8 @@ pub(super) enum PendingHTLCRouting { phantom_shared_secret: Option<[u8; 32]>, }, ReceiveKeysend { + /// This was added in 0.0.116 and will break deserialization on downgrades. + payment_data: Option, payment_preimage: PaymentPreimage, payment_metadata: Option>, incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed @@ -2342,20 +2344,7 @@ where }); }, msgs::OnionHopDataFormat::FinalNode { payment_data, keysend_preimage, payment_metadata } => { - if payment_data.is_some() && keysend_preimage.is_some() { - return Err(ReceiveError { - err_code: 0x4000|22, - err_data: Vec::new(), - msg: "We don't support MPP keysend payments", - }); - } else if let Some(data) = payment_data { - PendingHTLCRouting::Receive { - payment_data: data, - payment_metadata, - incoming_cltv_expiry: hop_data.outgoing_cltv_value, - phantom_shared_secret, - } - } else if let Some(payment_preimage) = keysend_preimage { + if let Some(payment_preimage) = keysend_preimage { // We need to check that the sender knows the keysend preimage before processing this // payment further. Otherwise, an intermediary routing hop forwarding non-keysend-HTLC X // could discover the final destination of X, by probing the adjacent nodes on the route @@ -2369,12 +2358,26 @@ where msg: "Payment preimage didn't match payment hash", }); } - + if !self.default_configuration.accept_mpp_keysend && payment_data.is_some() { + return Err(ReceiveError { + err_code: 0x4000|22, + err_data: Vec::new(), + msg: "We don't support MPP keysend payments", + }); + } PendingHTLCRouting::ReceiveKeysend { + payment_data, payment_preimage, payment_metadata, incoming_cltv_expiry: hop_data.outgoing_cltv_value, } + } else if let Some(data) = payment_data { + PendingHTLCRouting::Receive { + payment_data: data, + payment_metadata, + incoming_cltv_expiry: hop_data.outgoing_cltv_value, + phantom_shared_secret, + } } else { return Err(ReceiveError { err_code: 0x4000|0x2000|3, @@ -3490,10 +3493,13 @@ where (incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data }, Some(payment_data), phantom_shared_secret, onion_fields) }, - PendingHTLCRouting::ReceiveKeysend { payment_preimage, payment_metadata, incoming_cltv_expiry } => { - let onion_fields = RecipientOnionFields { payment_secret: None, payment_metadata }; + PendingHTLCRouting::ReceiveKeysend { payment_data, payment_preimage, payment_metadata, incoming_cltv_expiry } => { + let onion_fields = RecipientOnionFields { + payment_secret: payment_data.as_ref().map(|data| data.payment_secret), + payment_metadata + }; (incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), - None, None, onion_fields) + payment_data, None, onion_fields) }, _ => { panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive"); @@ -7058,6 +7064,7 @@ impl_writeable_tlv_based_enum!(PendingHTLCRouting, (0, payment_preimage, required), (2, incoming_cltv_expiry, required), (3, payment_metadata, option), + (4, payment_data, option), // Added in 0.0.116 }, ;); @@ -8704,10 +8711,13 @@ mod tests { #[test] fn test_keysend_msg_with_secret_err() { - // Test that we error as expected if we receive a keysend payment that includes a payment secret. + // Test that we error as expected if we receive a keysend payment that includes a payment + // secret when we don't support MPP keysend. + let mut reject_mpp_keysend_cfg = test_default_channel_config(); + reject_mpp_keysend_cfg.accept_mpp_keysend = false; let chanmon_cfgs = create_chanmon_cfgs(2); let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); - let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(reject_mpp_keysend_cfg)]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); let payer_pubkey = nodes[0].node.get_our_node_id(); From 8dde17773ace41d2d0bac2493abf2b01db67bd84 Mon Sep 17 00:00:00 2001 From: Alec Chen Date: Wed, 5 Apr 2023 17:45:41 -0500 Subject: [PATCH 4/5] Support receiving MPP keysend This commit refactors a significant portion of the receive validation in `ChannelManager::process_pending_htlc_forwards` now that we repurpose previous MPP validation logic to accomodate keysends. This also removes a previous restriction on claiming, as well as tests sending and receiving MPP keysends. --- lightning/src/ln/channelmanager.rs | 112 +++++--------- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/ln/payment_tests.rs | 177 +++++++++++++++++++++- 3 files changed, 212 insertions(+), 79 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f3ab0ac5ab3..702ae86fbb3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3505,7 +3505,7 @@ where panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive"); } }; - let mut claimable_htlc = ClaimableHTLC { + let claimable_htlc = ClaimableHTLC { prev_hop: HTLCPreviousHopData { short_channel_id: prev_short_channel_id, outpoint: prev_funding_outpoint, @@ -3555,13 +3555,11 @@ where } macro_rules! check_total_value { - ($payment_data: expr, $payment_preimage: expr) => {{ + ($purpose: expr) => {{ let mut payment_claimable_generated = false; - let purpose = || { - events::PaymentPurpose::InvoicePayment { - payment_preimage: $payment_preimage, - payment_secret: $payment_data.payment_secret, - } + let is_keysend = match $purpose { + events::PaymentPurpose::SpontaneousPayment(_) => true, + events::PaymentPurpose::InvoicePayment { .. } => false, }; let mut claimable_payments = self.claimable_payments.lock().unwrap(); if claimable_payments.pending_claiming_payments.contains_key(&payment_hash) { @@ -3573,9 +3571,18 @@ where .or_insert_with(|| { committed_to_claimable = true; ClaimablePayment { - purpose: purpose(), htlcs: Vec::new(), onion_fields: None, + purpose: $purpose.clone(), htlcs: Vec::new(), onion_fields: None, } }); + if $purpose != claimable_payment.purpose { + let log_keysend = |keysend| if keysend { "keysend" } else { "non-keysend" }; + log_trace!(self.logger, "Failing new {} HTLC with payment_hash {} as we already had an existing {} HTLC with the same payment hash", log_keysend(is_keysend), log_bytes!(payment_hash.0), log_keysend(!is_keysend)); + fail_htlc!(claimable_htlc, payment_hash); + } + if !self.default_configuration.accept_mpp_keysend && is_keysend && !claimable_payment.htlcs.is_empty() { + log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} as we already had an existing keysend HTLC with the same payment hash and our config states we don't accept MPP keysend", log_bytes!(payment_hash.0)); + fail_htlc!(claimable_htlc, payment_hash); + } if let Some(earlier_fields) = &mut claimable_payment.onion_fields { if earlier_fields.check_merge(&mut onion_fields).is_err() { fail_htlc!(claimable_htlc, payment_hash); @@ -3584,38 +3591,27 @@ where claimable_payment.onion_fields = Some(onion_fields); } let ref mut htlcs = &mut claimable_payment.htlcs; - if htlcs.len() == 1 { - if let OnionPayload::Spontaneous(_) = htlcs[0].onion_payload { - log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we already had an existing keysend HTLC with the same payment hash", log_bytes!(payment_hash.0)); - fail_htlc!(claimable_htlc, payment_hash); - } - } let mut total_value = claimable_htlc.sender_intended_value; let mut earliest_expiry = claimable_htlc.cltv_expiry; for htlc in htlcs.iter() { total_value += htlc.sender_intended_value; earliest_expiry = cmp::min(earliest_expiry, htlc.cltv_expiry); - match &htlc.onion_payload { - OnionPayload::Invoice { .. } => { - if htlc.total_msat != $payment_data.total_msat { - log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})", - log_bytes!(payment_hash.0), $payment_data.total_msat, htlc.total_msat); - total_value = msgs::MAX_VALUE_MSAT; - } - if total_value >= msgs::MAX_VALUE_MSAT { break; } - }, - _ => unreachable!(), + if htlc.total_msat != claimable_htlc.total_msat { + log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})", + log_bytes!(payment_hash.0), claimable_htlc.total_msat, htlc.total_msat); + total_value = msgs::MAX_VALUE_MSAT; } + if total_value >= msgs::MAX_VALUE_MSAT { break; } } // The condition determining whether an MPP is complete must // match exactly the condition used in `timer_tick_occurred` if total_value >= msgs::MAX_VALUE_MSAT { fail_htlc!(claimable_htlc, payment_hash); - } else if total_value - claimable_htlc.sender_intended_value >= $payment_data.total_msat { + } else if total_value - claimable_htlc.sender_intended_value >= claimable_htlc.total_msat { log_trace!(self.logger, "Failing HTLC with payment_hash {} as payment is already claimable", log_bytes!(payment_hash.0)); fail_htlc!(claimable_htlc, payment_hash); - } else if total_value >= $payment_data.total_msat { + } else if total_value >= claimable_htlc.total_msat { #[allow(unused_assignments)] { committed_to_claimable = true; } @@ -3626,7 +3622,7 @@ where new_events.push_back((events::Event::PaymentClaimable { receiver_node_id: Some(receiver_node_id), payment_hash, - purpose: purpose(), + purpose: $purpose, amount_msat, via_channel_id: Some(prev_channel_id), via_user_channel_id: Some(prev_user_channel_id), @@ -3674,49 +3670,23 @@ where fail_htlc!(claimable_htlc, payment_hash); } } - check_total_value!(payment_data, payment_preimage); + let purpose = events::PaymentPurpose::InvoicePayment { + payment_preimage: payment_preimage.clone(), + payment_secret: payment_data.payment_secret, + }; + check_total_value!(purpose); }, OnionPayload::Spontaneous(preimage) => { - let mut claimable_payments = self.claimable_payments.lock().unwrap(); - if claimable_payments.pending_claiming_payments.contains_key(&payment_hash) { - fail_htlc!(claimable_htlc, payment_hash); - } - match claimable_payments.claimable_payments.entry(payment_hash) { - hash_map::Entry::Vacant(e) => { - let amount_msat = claimable_htlc.value; - claimable_htlc.total_value_received = Some(amount_msat); - let claim_deadline = Some(claimable_htlc.cltv_expiry - HTLC_FAIL_BACK_BUFFER); - let purpose = events::PaymentPurpose::SpontaneousPayment(preimage); - e.insert(ClaimablePayment { - purpose: purpose.clone(), - onion_fields: Some(onion_fields.clone()), - htlcs: vec![claimable_htlc], - }); - let prev_channel_id = prev_funding_outpoint.to_channel_id(); - new_events.push_back((events::Event::PaymentClaimable { - receiver_node_id: Some(receiver_node_id), - payment_hash, - amount_msat, - purpose, - via_channel_id: Some(prev_channel_id), - via_user_channel_id: Some(prev_user_channel_id), - claim_deadline, - onion_fields: Some(onion_fields), - }, None)); - }, - hash_map::Entry::Occupied(_) => { - log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} for a duplicative payment hash", log_bytes!(payment_hash.0)); - fail_htlc!(claimable_htlc, payment_hash); - } - } + let purpose = events::PaymentPurpose::SpontaneousPayment(preimage); + check_total_value!(purpose); } } }, hash_map::Entry::Occupied(inbound_payment) => { - if payment_data.is_none() { + if let OnionPayload::Spontaneous(_) = claimable_htlc.onion_payload { log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} because we already have an inbound payment with the same payment hash", log_bytes!(payment_hash.0)); fail_htlc!(claimable_htlc, payment_hash); - }; + } let payment_data = payment_data.unwrap(); if inbound_payment.get().payment_secret != payment_data.payment_secret { log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", log_bytes!(payment_hash.0)); @@ -3726,7 +3696,11 @@ where log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap()); fail_htlc!(claimable_htlc, payment_hash); } else { - let payment_claimable_generated = check_total_value!(payment_data, inbound_payment.get().payment_preimage); + let purpose = events::PaymentPurpose::InvoicePayment { + payment_preimage: inbound_payment.get().payment_preimage, + payment_secret: payment_data.payment_secret, + }; + let payment_claimable_generated = check_total_value!(purpose); if payment_claimable_generated { inbound_payment.remove_entry(); } @@ -4265,18 +4239,6 @@ where break; } expected_amt_msat = htlc.total_value_received; - - if let OnionPayload::Spontaneous(_) = &htlc.onion_payload { - // We don't currently support MPP for spontaneous payments, so just check - // that there's one payment here and move on. - if sources.len() != 1 { - log_error!(self.logger, "Somehow ended up with an MPP spontaneous payment - this should not be reachable!"); - debug_assert!(false); - valid_mpp = false; - break; - } - } - claimable_amt_msat += htlc.value; } mem::drop(per_peer_state); diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index fa8bdcc58be..c325adcbef3 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -2115,7 +2115,7 @@ pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_p }, PaymentPurpose::SpontaneousPayment(payment_preimage) => { assert_eq!(expected_preimage.unwrap(), *payment_preimage); - assert!(our_payment_secret.is_none()); + assert_eq!(our_payment_secret, onion_fields.as_ref().unwrap().payment_secret); }, } assert_eq!(*amount_msat, recv_value); diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 9e044d1c92d..a051ce05473 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -17,13 +17,13 @@ use crate::sign::EntropySource; use crate::chain::transaction::OutPoint; use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason}; use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS; -use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS, RecentPaymentDetails, RecipientOnionFields}; +use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS, RecentPaymentDetails, RecipientOnionFields, HTLCForwardInfo, PendingHTLCRouting, PendingAddHTLCInfo}; use crate::ln::features::InvoiceFeatures; -use crate::ln::msgs; +use crate::ln::{msgs, PaymentSecret, PaymentPreimage}; use crate::ln::msgs::ChannelMessageHandler; use crate::ln::outbound_payment::Retry; use crate::routing::gossip::{EffectiveCapacity, RoutingFees}; -use crate::routing::router::{get_route, Path, PaymentParameters, Route, Router, RouteHint, RouteHintHop, RouteHop, RouteParameters}; +use crate::routing::router::{get_route, Path, PaymentParameters, Route, Router, RouteHint, RouteHintHop, RouteHop, RouteParameters, find_route}; use crate::routing::scoring::ChannelUsage; use crate::util::test_utils; use crate::util::errors::APIError; @@ -236,6 +236,177 @@ fn mpp_receive_timeout() { do_mpp_receive_timeout(false); } +#[test] +fn test_mpp_keysend() { + let mut mpp_keysend_config = test_default_channel_config(); + mpp_keysend_config.accept_mpp_keysend = true; + let chanmon_cfgs = create_chanmon_cfgs(4); + let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, Some(mpp_keysend_config)]); + let nodes = create_network(4, &node_cfgs, &node_chanmgrs); + + create_announced_chan_between_nodes(&nodes, 0, 1); + create_announced_chan_between_nodes(&nodes, 0, 2); + create_announced_chan_between_nodes(&nodes, 1, 3); + create_announced_chan_between_nodes(&nodes, 2, 3); + let network_graph = nodes[0].network_graph.clone(); + + let payer_pubkey = nodes[0].node.get_our_node_id(); + let payee_pubkey = nodes[3].node.get_our_node_id(); + let recv_value = 15_000_000; + let route_params = RouteParameters { + payment_params: PaymentParameters::for_keysend(payee_pubkey, 40, true), + final_value_msat: recv_value, + }; + let scorer = test_utils::TestScorer::new(); + let random_seed_bytes = chanmon_cfgs[0].keys_manager.get_secure_random_bytes(); + let route = find_route(&payer_pubkey, &route_params, &network_graph, None, nodes[0].logger, + &scorer, &(), &random_seed_bytes).unwrap(); + + let payment_preimage = PaymentPreimage([42; 32]); + let payment_secret = PaymentSecret(payment_preimage.0); + let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), + RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_preimage.0)).unwrap(); + check_added_monitors!(nodes[0], 2); + + let expected_route: &[&[&Node]] = &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]]; + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 2); + + let ev = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events); + pass_along_path(&nodes[0], expected_route[0], recv_value, payment_hash.clone(), + Some(payment_secret), ev.clone(), false, Some(payment_preimage)); + + let ev = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events); + pass_along_path(&nodes[0], expected_route[1], recv_value, payment_hash.clone(), + Some(payment_secret), ev.clone(), true, Some(payment_preimage)); + claim_payment_along_route(&nodes[0], expected_route, false, payment_preimage); +} + +#[test] +fn test_reject_mpp_keysend_htlc() { + // This test enforces that we reject MPP keysend HTLCs if our config states we don't support + // MPP keysend. When receiving a payment, if we don't support MPP keysend we'll reject the + // payment if it's keysend and has a payment secret, never reaching our payment validation + // logic. To check that we enforce rejecting MPP keysends in our payment logic, here we send + // keysend payments without payment secrets, then modify them by adding payment secrets in the + // final node in between receiving the HTLCs and actually processing them. + let mut reject_mpp_keysend_cfg = test_default_channel_config(); + reject_mpp_keysend_cfg.accept_mpp_keysend = false; + + let chanmon_cfgs = create_chanmon_cfgs(4); + let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, Some(reject_mpp_keysend_cfg)]); + let nodes = create_network(4, &node_cfgs, &node_chanmgrs); + let chan_1_id = create_announced_chan_between_nodes(&nodes, 0, 1).0.contents.short_channel_id; + let chan_2_id = create_announced_chan_between_nodes(&nodes, 0, 2).0.contents.short_channel_id; + let chan_3_id = create_announced_chan_between_nodes(&nodes, 1, 3).0.contents.short_channel_id; + let (update_a, _, chan_4_channel_id, _) = create_announced_chan_between_nodes(&nodes, 2, 3); + let chan_4_id = update_a.contents.short_channel_id; + let amount = 40_000; + let (mut route, payment_hash, payment_preimage, _) = get_route_and_payment_hash!(nodes[0], nodes[3], amount); + + // Pay along nodes[1] + route.paths[0].hops[0].pubkey = nodes[1].node.get_our_node_id(); + route.paths[0].hops[0].short_channel_id = chan_1_id; + route.paths[0].hops[1].short_channel_id = chan_3_id; + + let payment_id_0 = PaymentId(nodes[0].keys_manager.backing.get_secure_random_bytes()); + nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), payment_id_0).unwrap(); + check_added_monitors!(nodes[0], 1); + + let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + let update_add_0 = update_0.update_add_htlcs[0].clone(); + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add_0); + commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true); + expect_pending_htlcs_forwardable!(nodes[1]); + + check_added_monitors!(&nodes[1], 1); + let update_1 = get_htlc_update_msgs!(nodes[1], nodes[3].node.get_our_node_id()); + let update_add_1 = update_1.update_add_htlcs[0].clone(); + nodes[3].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &update_add_1); + commitment_signed_dance!(nodes[3], nodes[1], update_1.commitment_signed, false, true); + + assert!(nodes[3].node.get_and_clear_pending_msg_events().is_empty()); + for (_, pending_forwards) in nodes[3].node.forward_htlcs.lock().unwrap().iter_mut() { + for f in pending_forwards.iter_mut() { + match f { + &mut HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { ref mut forward_info, .. }) => { + match forward_info.routing { + PendingHTLCRouting::ReceiveKeysend { ref mut payment_data, .. } => { + *payment_data = Some(msgs::FinalOnionHopData { + payment_secret: PaymentSecret([42; 32]), + total_msat: amount * 2, + }); + }, + _ => panic!("Expected PendingHTLCRouting::ReceiveKeysend"), + } + }, + _ => {}, + } + } + } + expect_pending_htlcs_forwardable!(nodes[3]); + + // Pay along nodes[2] + route.paths[0].hops[0].pubkey = nodes[2].node.get_our_node_id(); + route.paths[0].hops[0].short_channel_id = chan_2_id; + route.paths[0].hops[1].short_channel_id = chan_4_id; + + let payment_id_1 = PaymentId(nodes[0].keys_manager.backing.get_secure_random_bytes()); + nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), payment_id_1).unwrap(); + check_added_monitors!(nodes[0], 1); + + let update_2 = get_htlc_update_msgs!(nodes[0], nodes[2].node.get_our_node_id()); + let update_add_2 = update_2.update_add_htlcs[0].clone(); + nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add_2); + commitment_signed_dance!(nodes[2], nodes[0], &update_2.commitment_signed, false, true); + expect_pending_htlcs_forwardable!(nodes[2]); + + check_added_monitors!(&nodes[2], 1); + let update_3 = get_htlc_update_msgs!(nodes[2], nodes[3].node.get_our_node_id()); + let update_add_3 = update_3.update_add_htlcs[0].clone(); + nodes[3].node.handle_update_add_htlc(&nodes[2].node.get_our_node_id(), &update_add_3); + commitment_signed_dance!(nodes[3], nodes[2], update_3.commitment_signed, false, true); + + assert!(nodes[3].node.get_and_clear_pending_msg_events().is_empty()); + for (_, pending_forwards) in nodes[3].node.forward_htlcs.lock().unwrap().iter_mut() { + for f in pending_forwards.iter_mut() { + match f { + &mut HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { ref mut forward_info, .. }) => { + match forward_info.routing { + PendingHTLCRouting::ReceiveKeysend { ref mut payment_data, .. } => { + *payment_data = Some(msgs::FinalOnionHopData { + payment_secret: PaymentSecret([42; 32]), + total_msat: amount * 2, + }); + }, + _ => panic!("Expected PendingHTLCRouting::ReceiveKeysend"), + } + }, + _ => {}, + } + } + } + expect_pending_htlcs_forwardable!(nodes[3]); + check_added_monitors!(nodes[3], 1); + + // Fail back along nodes[2] + let update_fail_0 = get_htlc_update_msgs!(&nodes[3], &nodes[2].node.get_our_node_id()); + nodes[2].node.handle_update_fail_htlc(&nodes[3].node.get_our_node_id(), &update_fail_0.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[2], nodes[3], update_fail_0.commitment_signed, false); + expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], vec![HTLCDestination::NextHopChannel { node_id: Some(nodes[3].node.get_our_node_id()), channel_id: chan_4_channel_id }]); + check_added_monitors!(nodes[2], 1); + + let update_fail_1 = get_htlc_update_msgs!(nodes[2], nodes[0].node.get_our_node_id()); + nodes[0].node.handle_update_fail_htlc(&nodes[2].node.get_our_node_id(), &update_fail_1.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[2], update_fail_1.commitment_signed, false); + + expect_payment_failed_conditions(&nodes[0], payment_hash, true, PaymentFailedConditions::new()); + expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[3], vec![HTLCDestination::FailedPayment { payment_hash }]); +} + + #[test] fn no_pending_leak_on_initial_send_failure() { // In an earlier version of our payment tracking, we'd have a retry entry even when the initial From 9db962c7192643d10722b7b675d45068e14407c8 Mon Sep 17 00:00:00 2001 From: Alec Chen Date: Mon, 22 May 2023 15:20:02 -0500 Subject: [PATCH 5/5] Add test for duplicate keysend payment The logic has been changed around duplicate keysend payments such that it's no longer explicitly clear that we reject duplicate keysend payments now that we handle receiving multi-part keysends. This test catches that. Note that this also tests that we reject MPP keysends when our config states we should, and that we reject MPP keysends without payemnt secrets when our config states we support MPP keysends. --- lightning/src/ln/channelmanager.rs | 62 +++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 702ae86fbb3..456c9da528f 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -8529,13 +8529,26 @@ mod tests { #[test] fn test_keysend_dup_payment_hash() { + do_test_keysend_dup_payment_hash(false); + do_test_keysend_dup_payment_hash(true); + } + + fn do_test_keysend_dup_payment_hash(accept_mpp_keysend: bool) { // (1): Test that a keysend payment with a duplicate payment hash to an existing pending // outbound regular payment fails as expected. // (2): Test that a regular payment with a duplicate payment hash to an existing keysend payment // fails as expected. + // (3): Test that a keysend payment with a duplicate payment hash to an existing keysend + // payment fails as expected. When `accept_mpp_keysend` is false, this tests that we + // reject MPP keysend payments, since in this case where the payment has no payment + // secret, a keysend payment with a duplicate hash is basically an MPP keysend. If + // `accept_mpp_keysend` is true, this tests that we only accept MPP keysends with + // payment secrets and reject otherwise. let chanmon_cfgs = create_chanmon_cfgs(2); let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); - let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let mut mpp_keysend_cfg = test_default_channel_config(); + mpp_keysend_cfg.accept_mpp_keysend = accept_mpp_keysend; + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(mpp_keysend_cfg)]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); create_announced_chan_between_nodes(&nodes, 0, 1); let scorer = test_utils::TestScorer::new(); @@ -8624,6 +8637,53 @@ mod tests { // Finally, succeed the keysend payment. claim_payment(&nodes[0], &expected_route, payment_preimage); + + // To start (3), send a keysend payment but don't claim it. + let payment_id_1 = PaymentId([44; 32]); + let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), + RecipientOnionFields::spontaneous_empty(), payment_id_1).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + let event = events.pop().unwrap(); + let path = vec![&nodes[1]]; + pass_along_path(&nodes[0], &path, 100_000, payment_hash, None, event, true, Some(payment_preimage)); + + // Next, attempt a keysend payment and make sure it fails. + let route_params = RouteParameters { + payment_params: PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV, false), + final_value_msat: 100_000, + }; + let route = find_route( + &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph, + None, nodes[0].logger, &scorer, &(), &random_seed_bytes + ).unwrap(); + let payment_id_2 = PaymentId([45; 32]); + nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), + RecipientOnionFields::spontaneous_empty(), payment_id_2).unwrap(); + check_added_monitors!(nodes[0], 1); + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + let ev = events.drain(..).next().unwrap(); + let payment_event = SendEvent::from_event(ev); + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); + check_added_monitors!(nodes[1], 0); + commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); + expect_pending_htlcs_forwardable!(nodes[1]); + expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![HTLCDestination::FailedPayment { payment_hash }]); + check_added_monitors!(nodes[1], 1); + let updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + assert!(updates.update_add_htlcs.is_empty()); + assert!(updates.update_fulfill_htlcs.is_empty()); + assert_eq!(updates.update_fail_htlcs.len(), 1); + assert!(updates.update_fail_malformed_htlcs.is_empty()); + assert!(updates.update_fee.is_none()); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, true, true); + expect_payment_failed!(nodes[0], payment_hash, true); + + // Finally, claim the original payment. + claim_payment(&nodes[0], &expected_route, payment_preimage); } #[test]