From 1e285cb417fe43503c96533508f1127fba26d99b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 27 Aug 2024 19:17:06 +0000 Subject: [PATCH 1/5] Only generate an `Event::DiscardFunding` when we need to 5e874c3dc9cf1606a3cbbccab3a0d25089a97280 changed `Event::DiscardFunding` to not include a dummy transaction when we were funded without a full funding tx, but in doing so started generating `DiscardFunding` events on every channel closure rather than only when there's actually still a pending funding broadcast. This restores the previous behavior to only generate the event when we should actually discard the funding tx. --- lightning/src/ln/channelmanager.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b14369c432a..258bc066125 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3547,12 +3547,15 @@ where channel_funding_txo: shutdown_res.channel_funding_txo, }, None)); - let funding_info = if is_manual_broadcast { - shutdown_res.channel_funding_txo.map(|outpoint| FundingInfo::OutPoint{ outpoint }) - } else { - shutdown_res.unbroadcasted_funding_tx.map(|transaction| FundingInfo::Tx{ transaction }) - }; - if let Some(funding_info) = funding_info { + if let Some(transaction) = shutdown_res.unbroadcasted_funding_tx { + let funding_info = if is_manual_broadcast { + FundingInfo::OutPoint { + outpoint: shutdown_res.channel_funding_txo + .expect("We had an unbroadcasted funding tx, so should also have had a funding outpoint"), + } + } else { + FundingInfo::Tx{ transaction } + }; pending_events.push_back((events::Event::DiscardFunding { channel_id: shutdown_res.channel_id, funding_info }, None)); From 3b70bd294c7776dd0dc242e4014265fe77a30d8b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 27 Aug 2024 19:21:09 +0000 Subject: [PATCH 2/5] Add missing pending changelog for downgrade on unsafe funding --- pending_changelog/3259-no-downgrade.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 pending_changelog/3259-no-downgrade.txt diff --git a/pending_changelog/3259-no-downgrade.txt b/pending_changelog/3259-no-downgrade.txt new file mode 100644 index 00000000000..ed4193da480 --- /dev/null +++ b/pending_changelog/3259-no-downgrade.txt @@ -0,0 +1,4 @@ +# Backwards Compatibility + * Downgrading after using `ChannelManager`'s + `unsafe_manual_funding_transaction_generated` may cause deserialization of + `ChannelManager` to fail with an `Err` (#3259). From 6a81d5d4bb894e130f753dda9a04f8016c71b8a9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 28 Aug 2024 13:50:17 +0000 Subject: [PATCH 3/5] Add additional documentation on `Channel::unbroadcasted_funding` --- lightning/src/ln/channel.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 576ae665ade..fe01a26290c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3457,6 +3457,9 @@ impl ChannelContext where SP::Target: SignerProvider { /// Returns the transaction if there is a pending funding transaction that is yet to be /// broadcast. + /// + /// Note that if [`Self::is_manual_broadcast`] is true the transaction will be a dummy + /// transaction. pub fn unbroadcasted_funding(&self) -> Option { self.if_unbroadcasted_funding(|| self.funding_transaction.clone()) } From 683aa8350ed545add81bbcbca6a9281ddeba3609 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 28 Aug 2024 14:35:54 +0000 Subject: [PATCH 4/5] Correct manual shutdown detection on channel closure In 5e874c3dc9cf1606a3cbbccab3a0d25089a97280 we'd intended to not reveal the dummy funding transaction in `Event::DiscardFunding`. However, instead of looking at the channel that was just closed, the logic only looks at any other channels which were funded as a part of the same batch. Because manually-funded transactions cannot currently be done for batch funding, this was actually dead code, preventing the new changes from taking effect. --- lightning/src/ln/channel.rs | 4 ++++ lightning/src/ln/channelmanager.rs | 7 +------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index fe01a26290c..26e385082f9 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -936,6 +936,7 @@ pub(crate) struct ShutdownResult { pub(crate) user_channel_id: u128, pub(crate) channel_capacity_satoshis: u64, pub(crate) counterparty_node_id: PublicKey, + pub(crate) is_manual_broadcast: bool, pub(crate) unbroadcasted_funding_tx: Option, pub(crate) channel_funding_txo: Option, } @@ -3540,6 +3541,7 @@ impl ChannelContext where SP::Target: SignerProvider { channel_capacity_satoshis: self.channel_value_satoshis, counterparty_node_id: self.counterparty_node_id, unbroadcasted_funding_tx, + is_manual_broadcast: self.is_manual_broadcast, channel_funding_txo: self.get_funding_txo(), } } @@ -6243,6 +6245,7 @@ impl Channel where channel_capacity_satoshis: self.context.channel_value_satoshis, counterparty_node_id: self.context.counterparty_node_id, unbroadcasted_funding_tx: self.context.unbroadcasted_funding(), + is_manual_broadcast: self.context.is_manual_broadcast, channel_funding_txo: self.context.get_funding_txo(), }; let tx = self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig); @@ -6275,6 +6278,7 @@ impl Channel where channel_capacity_satoshis: self.context.channel_value_satoshis, counterparty_node_id: self.context.counterparty_node_id, unbroadcasted_funding_tx: self.context.unbroadcasted_funding(), + is_manual_broadcast: self.context.is_manual_broadcast, channel_funding_txo: self.context.get_funding_txo(), }; if closing_signed.is_some() { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 258bc066125..0e377b348db 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3510,7 +3510,6 @@ where let _ = self.chain_monitor.update_channel(funding_txo, &monitor_update); } let mut shutdown_results = Vec::new(); - let mut is_manual_broadcast = false; if let Some(txid) = shutdown_res.unbroadcasted_batch_funding_txid { let mut funding_batch_states = self.funding_batch_states.lock().unwrap(); let affected_channels = funding_batch_states.remove(&txid).into_iter().flatten(); @@ -3520,10 +3519,6 @@ where if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) { let mut peer_state = peer_state_mutex.lock().unwrap(); if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) { - // We override the previous value, so we could change from true -> false, - // but this is fine because if a channel has manual_broadcast set to false - // we should choose the safier condition. - is_manual_broadcast = chan.context().is_manual_broadcast(); update_maps_on_chan_removal!(self, &chan.context()); shutdown_results.push(chan.context_mut().force_shutdown(false, ClosureReason::FundingBatchClosure)); } @@ -3548,7 +3543,7 @@ where }, None)); if let Some(transaction) = shutdown_res.unbroadcasted_funding_tx { - let funding_info = if is_manual_broadcast { + let funding_info = if shutdown_res.is_manual_broadcast { FundingInfo::OutPoint { outpoint: shutdown_res.channel_funding_txo .expect("We had an unbroadcasted funding tx, so should also have had a funding outpoint"), From ea646ae8881172952b2ba6777f2d74db4f1b5370 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 28 Aug 2024 14:50:52 +0000 Subject: [PATCH 5/5] Test manual funding transaction `Event::DiscardFunding` generation --- lightning/src/ln/functional_tests.rs | 44 +++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 12f1466e025..a85bb1ae4a2 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -18,7 +18,7 @@ use crate::chain::channelmonitor; use crate::chain::channelmonitor::{CLOSED_CHANNEL_UPDATE_ID, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY}; use crate::chain::transaction::OutPoint; use crate::sign::{ecdsa::EcdsaChannelSigner, EntropySource, OutputSpender, SignerProvider}; -use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination, PaymentFailureReason}; +use crate::events::{Event, FundingInfo, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination, PaymentFailureReason}; use crate::ln::types::{ChannelId, PaymentPreimage, PaymentSecret, PaymentHash}; use crate::ln::channel::{CONCURRENT_INBOUND_HTLC_FEE_BUFFER, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, MIN_AFFORDABLE_HTLC_COUNT, get_holder_selected_channel_reserve_satoshis, OutboundV1Channel, InboundV1Channel, COINBASE_MATURITY, ChannelPhase}; use crate::ln::channelmanager::{self, PaymentId, RAACommitmentOrder, PaymentSendFailure, RecipientOnionFields, BREAKDOWN_TIMEOUT, ENABLE_GOSSIP_TICKS, DISABLE_GOSSIP_TICKS, MIN_CLTV_EXPIRY_DELTA}; @@ -11209,6 +11209,48 @@ fn test_accept_inbound_channel_errors_queued() { open_channel_msg.common_fields.temporary_channel_id); } +#[test] +fn test_manual_funding_abandon() { + let mut cfg = UserConfig::default(); + cfg.channel_handshake_config.minimum_depth = 1; + 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, &[Some(cfg), Some(cfg)]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + assert!(nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 0, 42, None, None).is_ok()); + let open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id()); + + nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel); + let accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()); + + nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_channel); + let (temporary_channel_id, _tx, funding_outpoint) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100_000, 42); + nodes[0].node.unsafe_manual_funding_transaction_generated(temporary_channel_id, nodes[1].node.get_our_node_id(), funding_outpoint).unwrap(); + check_added_monitors!(nodes[0], 0); + + let funding_created = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()); + nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created); + check_added_monitors!(nodes[1], 1); + expect_channel_pending_event(&nodes[1], &nodes[0].node.get_our_node_id()); + + let funding_signed = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()); + let err = msgs::ErrorMessage { channel_id: funding_signed.channel_id, data: "".to_string() }; + nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &err); + + let close_events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(close_events.len(), 2); + assert!(close_events.iter().any(|ev| matches!(ev, Event::ChannelClosed { .. }))); + assert!(close_events.iter().any(|ev| match ev { + Event::DiscardFunding { channel_id, funding_info: FundingInfo::OutPoint { outpoint } } => { + assert_eq!(*channel_id, err.channel_id); + assert_eq!(*outpoint, funding_outpoint); + true + } + _ => false, + })); +} + #[test] fn test_funding_signed_event() { let mut cfg = UserConfig::default();