From 7fc2cf4c3c8808e33b99a5a47f6a6c411098d104 Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Thu, 21 Nov 2024 12:09:15 +0200 Subject: [PATCH 1/7] Use debug_assert & error on unexpected initial commitment_signed This replaces the hard panic from this case. --- lightning/src/ln/channel.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 3e448119820..dd73374ceaf 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4501,8 +4501,13 @@ impl ChannelContext where SP::Target: SignerProvider { { if !matches!( self.channel_state, ChannelState::NegotiatingFunding(flags) - if flags == (NegotiatingFundingFlags::OUR_INIT_SENT | NegotiatingFundingFlags::THEIR_INIT_SENT)) { - panic!("Tried to get an initial commitment_signed messsage at a time other than immediately after initial handshake completion (or tried to get funding_created twice)"); + if flags == (NegotiatingFundingFlags::OUR_INIT_SENT | NegotiatingFundingFlags::THEIR_INIT_SENT) + ) { + debug_assert!(false); + return Err(ChannelError::Close(("Tried to get an initial commitment_signed messsage at a time other than \ + immediately after initial handshake completion (or tried to get funding_created twice)".to_string(), + ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) } + ))); } let signature = match self.get_initial_counterparty_commitment_signature(funding, logger) { From ac060f93f28058ed8c3b31b8600c56d75377ae1a Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Thu, 9 Jan 2025 15:33:12 +0200 Subject: [PATCH 2/7] Remove dual_funding cfg-flags Since we now have release branches, and dual-funded channels are expected in v0.2.0, we can remove the cfg flags. This should also speed up CI a bit as a bonus. --- Cargo.toml | 1 - lightning/src/ln/channel.rs | 14 +------------- lightning/src/ln/channelmanager.rs | 16 ---------------- lightning/src/ln/dual_funding_tests.rs | 4 ---- lightning/src/ln/peer_handler.rs | 1 - 5 files changed, 1 insertion(+), 35 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 698f5112b9d..1c42def7939 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,5 +66,4 @@ check-cfg = [ "cfg(require_route_graph_test)", "cfg(splicing)", "cfg(async_payments)", - "cfg(dual_funding)", ] diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index dd73374ceaf..efabc4ca15e 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1182,7 +1182,6 @@ enum ChannelPhase where SP::Target: SignerProvider { Undefined, UnfundedOutboundV1(OutboundV1Channel), UnfundedInboundV1(InboundV1Channel), - #[allow(dead_code)] // TODO(dual_funding): Remove once creating V2 channels is enabled. UnfundedV2(PendingV2Channel), Funded(FundedChannel), } @@ -1399,7 +1398,6 @@ impl Channel where debug_assert!(false); ReconnectionMsg::None }, - #[cfg(dual_funding)] ChannelPhase::UnfundedV2(chan) => { if chan.context.is_outbound() { ReconnectionMsg::Open(OpenChannelMessage::V2( @@ -1413,8 +1411,6 @@ impl Channel where ReconnectionMsg::None } }, - #[cfg(not(dual_funding))] - ChannelPhase::UnfundedV2(_) => ReconnectionMsg::None, } } @@ -1434,7 +1430,6 @@ impl Channel where .map(|msg| Some(OpenChannelMessage::V1(msg))) }, ChannelPhase::UnfundedInboundV1(_) => Ok(None), - #[cfg(dual_funding)] ChannelPhase::UnfundedV2(chan) => { if chan.context.is_outbound() { chan.maybe_handle_error_without_close(chain_hash, fee_estimator) @@ -1443,11 +1438,6 @@ impl Channel where Ok(None) } }, - #[cfg(not(dual_funding))] - ChannelPhase::UnfundedV2(_) => { - debug_assert!(false); - Ok(None) - }, } } @@ -4530,7 +4520,7 @@ impl ChannelContext where SP::Target: SignerProvider { }) } - #[cfg(all(test, dual_funding))] + #[cfg(all(test))] pub fn get_initial_counterparty_commitment_signature_for_test( &mut self, funding: &FundingScope, logger: &L, channel_transaction_parameters: ChannelTransactionParameters, counterparty_cur_commitment_point_override: PublicKey, @@ -9571,7 +9561,6 @@ impl PendingV2Channel where SP::Target: SignerProvider { /// If we receive an error message, it may only be a rejection of the channel type we tried, /// not of our ability to open any channel at all. Thus, on error, we should first call this /// and see if we get a new `OpenChannelV2` message, otherwise the channel is failed. - #[cfg(dual_funding)] pub(crate) fn maybe_handle_error_without_close( &mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator ) -> Result @@ -9582,7 +9571,6 @@ impl PendingV2Channel where SP::Target: SignerProvider { Ok(self.get_open_channel_v2(chain_hash)) } - #[cfg(dual_funding)] pub fn get_open_channel_v2(&self, chain_hash: ChainHash) -> msgs::OpenChannelV2 { if !self.context.is_outbound() { debug_assert!(false, "Tried to send open_channel2 for an inbound channel?"); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 666258d383c..ee886e8d7de 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -49,7 +49,6 @@ use crate::ln::inbound_payment; use crate::ln::types::ChannelId; use crate::types::payment::{PaymentHash, PaymentPreimage, PaymentSecret}; use crate::ln::channel::{self, Channel, ChannelError, ChannelUpdateStatus, FundedChannel, ShutdownResult, UpdateFulfillCommitFetch, OutboundV1Channel, ReconnectionMsg, InboundV1Channel, WithChannelContext}; -#[cfg(any(dual_funding, splicing))] use crate::ln::channel::PendingV2Channel; use crate::ln::channel_state::ChannelDetails; use crate::types::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures}; @@ -1450,13 +1449,11 @@ impl PeerState where SP::Target: SignerProvider { #[derive(Clone)] pub(super) enum OpenChannelMessage { V1(msgs::OpenChannel), - #[cfg(dual_funding)] V2(msgs::OpenChannelV2), } pub(super) enum OpenChannelMessageRef<'a> { V1(&'a msgs::OpenChannel), - #[cfg(dual_funding)] V2(&'a msgs::OpenChannelV2), } @@ -7846,7 +7843,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ (*temporary_channel_id, Channel::from(channel), message_send_event) }) }, - #[cfg(dual_funding)] OpenChannelMessage::V2(open_channel_msg) => { PendingV2Channel::new_inbound( &self.fee_estimator, &self.entropy_source, &self.signer_provider, @@ -8009,7 +8005,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ fn internal_open_channel(&self, counterparty_node_id: &PublicKey, msg: OpenChannelMessageRef<'_>) -> Result<(), MsgHandleErrInternal> { let common_fields = match msg { OpenChannelMessageRef::V1(msg) => &msg.common_fields, - #[cfg(dual_funding)] OpenChannelMessageRef::V2(msg) => &msg.common_fields, }; @@ -8087,7 +8082,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ funding_satoshis: common_fields.funding_satoshis, channel_negotiation_type: match msg { OpenChannelMessageRef::V1(msg) => InboundChannelFunds::PushMsat(msg.push_msat), - #[cfg(dual_funding)] OpenChannelMessageRef::V2(_) => InboundChannelFunds::DualFunded, }, channel_type, @@ -8097,7 +8091,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ peer_state.inbound_channel_request_by_id.insert(channel_id, InboundChannelRequest { open_channel_msg: match msg { OpenChannelMessageRef::V1(msg) => OpenChannelMessage::V1(msg.clone()), - #[cfg(dual_funding)] OpenChannelMessageRef::V2(msg) => OpenChannelMessage::V2(msg.clone()), }, ticks_remaining: UNACCEPTED_INBOUND_CHANNEL_AGE_LIMIT_TICKS, @@ -8133,7 +8126,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ }); (Channel::from(channel), message_send_event) }, - #[cfg(dual_funding)] OpenChannelMessageRef::V2(msg) => { let channel = PendingV2Channel::new_inbound( &self.fee_estimator, &self.entropy_source, &self.signer_provider, @@ -11659,7 +11651,6 @@ where // Note that we never need to persist the updated ChannelManager for an inbound // open_channel message - pre-funded channels are never written so there should be no // change to the contents. - #[cfg(dual_funding)] let _persistence_guard = PersistenceNotifierGuard::optionally_notify(self, || { let res = self.internal_open_channel(&counterparty_node_id, OpenChannelMessageRef::V2(msg)); let persist = match &res { @@ -11672,10 +11663,6 @@ where let _ = handle_error!(self, res, counterparty_node_id); persist }); - #[cfg(not(dual_funding))] - let _: Result<(), _> = handle_error!(self, Err(MsgHandleErrInternal::send_err_msg_no_close( - "Dual-funded channels not supported".to_owned(), - msg.common_fields.temporary_channel_id.clone())), counterparty_node_id); } fn handle_accept_channel(&self, counterparty_node_id: PublicKey, msg: &msgs::AcceptChannel) { @@ -12074,7 +12061,6 @@ where node_id: chan.context().get_counterparty_node_id(), msg, }), - #[cfg(dual_funding)] ReconnectionMsg::Open(OpenChannelMessage::V2(msg)) => pending_msg_events.push(events::MessageSendEvent::SendOpenChannelV2 { node_id: chan.context().get_counterparty_node_id(), @@ -12181,7 +12167,6 @@ where }); return; }, - #[cfg(dual_funding)] Ok(Some(OpenChannelMessage::V2(msg))) => { peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannelV2 { node_id: counterparty_node_id, @@ -12751,7 +12736,6 @@ pub fn provided_init_features(config: &UserConfig) -> InitFeatures { if config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx { features.set_anchors_zero_fee_htlc_tx_optional(); } - #[cfg(dual_funding)] features.set_dual_fund_optional(); // Only signal quiescence support in tests for now, as we don't yet support any // quiescent-dependent protocols (e.g., splicing). diff --git a/lightning/src/ln/dual_funding_tests.rs b/lightning/src/ln/dual_funding_tests.rs index 7f0f9c2023e..9e52540c7f4 100644 --- a/lightning/src/ln/dual_funding_tests.rs +++ b/lightning/src/ln/dual_funding_tests.rs @@ -9,7 +9,6 @@ //! Tests that test the creation of dual-funded channels in ChannelManager. -#[cfg(dual_funding)] use { crate::chain::chaininterface::{ConfirmationTarget, LowerBoundedFeeEstimator}, crate::events::{Event, MessageSendEvent, MessageSendEventsProvider}, @@ -29,14 +28,12 @@ use { crate::util::test_utils, }; -#[cfg(dual_funding)] // Dual-funding: V2 Channel Establishment Tests struct V2ChannelEstablishmentTestSession { funding_input_sats: u64, initiator_input_value_satoshis: u64, } -#[cfg(dual_funding)] // TODO(dual_funding): Use real node and API for creating V2 channels as initiator when available, // instead of manually constructing messages. fn do_test_v2_channel_establishment( @@ -248,7 +245,6 @@ fn do_test_v2_channel_establishment( } #[test] -#[cfg(dual_funding)] fn test_v2_channel_establishment() { // Only initiator contributes, no persist pending do_test_v2_channel_establishment( diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index aca1afbff39..89d54a8cdcd 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -346,7 +346,6 @@ impl ChannelMessageHandler for ErroringMessageHandler { features.set_basic_mpp_optional(); features.set_wumbo_optional(); features.set_shutdown_any_segwit_optional(); - #[cfg(dual_funding)] features.set_dual_fund_optional(); features.set_channel_type_optional(); features.set_scid_privacy_optional(); From eb189134d39a4cb2d162446710fd05d2c082e8f9 Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Thu, 21 Nov 2024 12:13:02 +0200 Subject: [PATCH 3/7] Move txid check to start of `tx_signatures` handling By moving the txid check to the start of `tx_signatures` handling, we can avoid spurious witness count mismatch errors. Also, it just makes more sense to check the txid earlier. --- lightning/src/ln/channel.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index efabc4ca15e..41ad15f95bb 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -6133,6 +6133,14 @@ impl FundedChannel where } if let Some(ref mut signing_session) = self.interactive_tx_signing_session { + if msg.tx_hash != signing_session.unsigned_tx.compute_txid() { + return Err(ChannelError::Close( + ( + "The txid for the transaction does not match".to_string(), + ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, + ))); + } + if msg.witnesses.len() != signing_session.remote_inputs_count() { return Err(ChannelError::Warn( "Witness count did not match contributed input count".to_string() @@ -6154,14 +6162,6 @@ impl FundedChannel where // for spending. Doesn't seem to be anything in rust-bitcoin. } - if msg.tx_hash != signing_session.unsigned_tx.compute_txid() { - return Err(ChannelError::Close( - ( - "The txid for the transaction does not match".to_string(), - ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, - ))); - } - let (tx_signatures_opt, funding_tx_opt) = signing_session.received_tx_signatures(msg.clone()) .map_err(|_| ChannelError::Warn("Witness count did not match contributed input count".to_string()))?; if funding_tx_opt.is_some() { From f0f6a35846c4dfe9c3492f96fb58bea4b56a1a04 Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Wed, 27 Nov 2024 10:43:10 +0200 Subject: [PATCH 4/7] Remove unnecessary funding tx clone & remote tx_sigs received check We actually don't need to check if the counterparty had already sent their `tx_signatures` in `InteractiveTxSigningSession::received_tx_signatures`. Further, we can get rid of the clone of `funding_tx_opt` in `FundedChannel::tx_signatures` when setting the `ChannelContext::funding_transaction` as we don't actually need to propagate it through to `ChannelManager::internal_tx_complete` as we can use `ChannelContext::unbroadcasted_funding()` which clones the `ChannelContext::funding_transaction` anyway. --- lightning/src/ln/channel.rs | 51 +++++++++++------ lightning/src/ln/channelmanager.rs | 4 +- lightning/src/ln/dual_funding_tests.rs | 77 ++++++++++++-------------- lightning/src/ln/interactivetxs.rs | 17 ++++-- lightning/src/ln/types.rs | 21 +++++++ 5 files changed, 102 insertions(+), 68 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 41ad15f95bb..caab9299107 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2280,6 +2280,7 @@ impl PendingV2Channel where SP::Target: SignerProvider { context: self.context, interactive_tx_signing_session: Some(signing_session), holder_commitment_point, + is_v2_established: true, }; Ok((funded_chan, commitment_signed, funding_ready_for_sig_event)) }, @@ -4645,6 +4646,9 @@ pub(super) struct FundedChannel where SP::Target: SignerProvider { pub context: ChannelContext, pub interactive_tx_signing_session: Option, holder_commitment_point: HolderCommitmentPoint, + /// Indicates whether this funded channel had been established with V2 channel + /// establishment. + is_v2_established: bool, } #[cfg(any(test, fuzzing))] @@ -6125,10 +6129,10 @@ impl FundedChannel where } } - pub fn tx_signatures(&mut self, msg: &msgs::TxSignatures, logger: &L) -> Result<(Option, Option), ChannelError> + pub fn tx_signatures(&mut self, msg: &msgs::TxSignatures, logger: &L) -> Result, ChannelError> where L::Target: Logger { - if !matches!(self.context.channel_state, ChannelState::FundingNegotiated) { + if !matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(_)) { return Err(ChannelError::close("Received tx_signatures in strange state!".to_owned())); } @@ -6162,25 +6166,25 @@ impl FundedChannel where // for spending. Doesn't seem to be anything in rust-bitcoin. } - let (tx_signatures_opt, funding_tx_opt) = signing_session.received_tx_signatures(msg.clone()) + let (holder_tx_signatures_opt, funding_tx_opt) = signing_session.received_tx_signatures(msg.clone()) .map_err(|_| ChannelError::Warn("Witness count did not match contributed input count".to_string()))?; - if funding_tx_opt.is_some() { - self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); - } - self.context.funding_transaction = funding_tx_opt.clone(); - self.context.next_funding_txid = None; - // Clear out the signing session - self.interactive_tx_signing_session = None; + if funding_tx_opt.is_some() { + // We have a finalized funding transaction, so we can set the funding transaction and reset the + // signing session fields. + self.context.funding_transaction = funding_tx_opt; + self.context.next_funding_txid = None; + self.interactive_tx_signing_session = None; + } - if tx_signatures_opt.is_some() && self.context.channel_state.is_monitor_update_in_progress() { + if holder_tx_signatures_opt.is_some() && self.is_awaiting_initial_mon_persist() { log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures."); - self.context.monitor_pending_tx_signatures = tx_signatures_opt; - return Ok((None, None)); + self.context.monitor_pending_tx_signatures = holder_tx_signatures_opt; + return Ok(None); } - Ok((tx_signatures_opt, funding_tx_opt)) + Ok(holder_tx_signatures_opt) } else { Err(ChannelError::Close(( "Unexpected tx_signatures. No funding transaction awaiting signatures".to_string(), @@ -6397,12 +6401,12 @@ impl FundedChannel where assert!(self.context.channel_state.is_monitor_update_in_progress()); self.context.channel_state.clear_monitor_update_in_progress(); - // If we're past (or at) the AwaitingChannelReady stage on an outbound channel, try to - // (re-)broadcast the funding transaction as we may have declined to broadcast it when we + // If we're past (or at) the AwaitingChannelReady stage on an outbound (or V2-established) channel, + // try to (re-)broadcast the funding transaction as we may have declined to broadcast it when we // first received the funding_signed. let mut funding_broadcastable = None; if let Some(funding_transaction) = &self.context.funding_transaction { - if self.context.is_outbound() && + if (self.context.is_outbound() || self.is_v2_established()) && (matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(flags) if !flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH)) || matches!(self.context.channel_state, ChannelState::ChannelReady(_))) { @@ -8918,6 +8922,10 @@ impl FundedChannel where false } } + + pub fn is_v2_established(&self) -> bool { + self.is_v2_established + } } /// A not-yet-funded outbound (from holder) channel using V1 channel establishment. @@ -9185,6 +9193,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { funding: self.funding, context: self.context, interactive_tx_signing_session: None, + is_v2_established: false, holder_commitment_point, }; @@ -9452,6 +9461,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { funding: self.funding, context: self.context, interactive_tx_signing_session: None, + is_v2_established: false, holder_commitment_point, }; let need_channel_ready = channel.check_get_channel_ready(0, logger).is_some() @@ -10263,7 +10273,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch let mut _val: u64 = Readable::read(reader)?; } - let channel_id = Readable::read(reader)?; + let channel_id: ChannelId = Readable::read(reader)?; let channel_state = ChannelState::from_u32(Readable::read(reader)?).map_err(|_| DecodeError::InvalidValue)?; let channel_value_satoshis = Readable::read(reader)?; @@ -10699,6 +10709,10 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch } }, }; + let is_v2_established = channel_id.is_v2_channel_id( + &channel_parameters.holder_pubkeys.revocation_basepoint, + &channel_parameters.counterparty_parameters.as_ref() + .expect("Persisted channel must have counterparty parameters").pubkeys.revocation_basepoint); Ok(FundedChannel { funding: FundingScope { @@ -10841,6 +10855,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch is_holder_quiescence_initiator: None, }, interactive_tx_signing_session: None, + is_v2_established, holder_commitment_point, }) } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ee886e8d7de..82b00011dbb 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -8524,14 +8524,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ match chan_entry.get_mut().as_funded_mut() { Some(chan) => { let logger = WithChannelContext::from(&self.logger, &chan.context, None); - let (tx_signatures_opt, funding_tx_opt) = try_channel_entry!(self, peer_state, chan.tx_signatures(msg, &&logger), chan_entry); + let tx_signatures_opt = try_channel_entry!(self, peer_state, chan.tx_signatures(msg, &&logger), chan_entry); if let Some(tx_signatures) = tx_signatures_opt { peer_state.pending_msg_events.push(events::MessageSendEvent::SendTxSignatures { node_id: *counterparty_node_id, msg: tx_signatures, }); } - if let Some(ref funding_tx) = funding_tx_opt { + if let Some(ref funding_tx) = chan.context.unbroadcasted_funding() { self.tx_broadcaster.broadcast_transactions(&[funding_tx]); { let mut pending_events = self.pending_events.lock().unwrap(); diff --git a/lightning/src/ln/dual_funding_tests.rs b/lightning/src/ln/dual_funding_tests.rs index 9e52540c7f4..e07f503e7f9 100644 --- a/lightning/src/ln/dual_funding_tests.rs +++ b/lightning/src/ln/dual_funding_tests.rs @@ -20,12 +20,13 @@ use { crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint, RevocationBasepoint}, crate::ln::functional_test_utils::*, crate::ln::msgs::ChannelMessageHandler, - crate::ln::msgs::{CommitmentSigned, TxAddInput, TxAddOutput, TxComplete}, + crate::ln::msgs::{CommitmentSigned, TxAddInput, TxAddOutput, TxComplete, TxSignatures}, crate::ln::types::ChannelId, crate::prelude::*, crate::sign::ChannelSigner as _, crate::util::ser::TransactionU16LenLimited, crate::util::test_utils, + bitcoin::Witness, }; // Dual-funding: V2 Channel Establishment Tests @@ -36,9 +37,7 @@ struct V2ChannelEstablishmentTestSession { // TODO(dual_funding): Use real node and API for creating V2 channels as initiator when available, // instead of manually constructing messages. -fn do_test_v2_channel_establishment( - session: V2ChannelEstablishmentTestSession, test_async_persist: bool, -) { +fn do_test_v2_channel_establishment(session: V2ChannelEstablishmentTestSession) { 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]); @@ -196,11 +195,7 @@ fn do_test_v2_channel_establishment( partial_signature_with_nonce: None, }; - if test_async_persist { - chanmon_cfgs[1] - .persister - .set_update_ret(crate::chain::ChannelMonitorUpdateStatus::InProgress); - } + chanmon_cfgs[1].persister.set_update_ret(crate::chain::ChannelMonitorUpdateStatus::InProgress); // Handle the initial commitment_signed exchange. Order is not important here. nodes[1] @@ -208,25 +203,15 @@ fn do_test_v2_channel_establishment( .handle_commitment_signed(nodes[0].node.get_our_node_id(), &msg_commitment_signed_from_0); check_added_monitors(&nodes[1], 1); - if test_async_persist { - let events = nodes[1].node.get_and_clear_pending_events(); - assert!(events.is_empty()); + // The funding transaction should not have been broadcast before persisting initial monitor has + // been completed. + assert_eq!(nodes[1].tx_broadcaster.txn_broadcast().len(), 0); + assert_eq!(nodes[1].node.get_and_clear_pending_events().len(), 0); - chanmon_cfgs[1] - .persister - .set_update_ret(crate::chain::ChannelMonitorUpdateStatus::Completed); - let (latest_update, _) = *nodes[1] - .chain_monitor - .latest_monitor_update_id - .lock() - .unwrap() - .get(&channel_id) - .unwrap(); - nodes[1] - .chain_monitor - .chain_monitor - .force_channel_monitor_updated(channel_id, latest_update); - } + // Complete the persistence of the monitor. + let events = nodes[1].node.get_and_clear_pending_events(); + assert!(events.is_empty()); + nodes[1].chain_monitor.complete_sole_pending_chan_update(&channel_id); let events = nodes[1].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); @@ -242,24 +227,30 @@ fn do_test_v2_channel_establishment( ); assert_eq!(tx_signatures_msg.channel_id, channel_id); + + let mut witness = Witness::new(); + witness.push([0x0]); + // Receive tx_signatures from channel initiator. + nodes[1].node.handle_tx_signatures( + nodes[0].node.get_our_node_id(), + &TxSignatures { + channel_id, + tx_hash: funding_outpoint.unwrap().txid, + witnesses: vec![witness], + shared_input_signature: None, + }, + ); + + // For an inbound channel V2 channel the transaction should be broadcast once receiving a + // tx_signature and applying local tx_signatures: + let broadcasted_txs = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(broadcasted_txs.len(), 1); } #[test] fn test_v2_channel_establishment() { - // Only initiator contributes, no persist pending - do_test_v2_channel_establishment( - V2ChannelEstablishmentTestSession { - funding_input_sats: 100_000, - initiator_input_value_satoshis: 150_000, - }, - false, - ); - // Only initiator contributes, persist pending - do_test_v2_channel_establishment( - V2ChannelEstablishmentTestSession { - funding_input_sats: 100_000, - initiator_input_value_satoshis: 150_000, - }, - true, - ); + do_test_v2_channel_establishment(V2ChannelEstablishmentTestSession { + funding_input_sats: 100_00, + initiator_input_value_satoshis: 150_000, + }); } diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 2b72133ec09..bc015d1f6fd 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -316,14 +316,18 @@ impl InteractiveTxSigningSession { /// Handles a `tx_signatures` message received from the counterparty. /// + /// If the holder is required to send their `tx_signatures` message and these signatures have + /// already been provided to the signing session, then this return value will be `Some`, otherwise + /// None. + /// + /// If the holder has already provided their `tx_signatures` to the signing session, a funding + /// transaction will be finalized and returned as Some, otherwise None. + /// /// Returns an error if the witness count does not equal the counterparty's input count in the /// unsigned transaction. pub fn received_tx_signatures( &mut self, tx_signatures: TxSignatures, ) -> Result<(Option, Option), ()> { - if self.counterparty_sent_tx_signatures { - return Ok((None, None)); - }; if self.remote_inputs_count() != tx_signatures.witnesses.len() { return Err(()); } @@ -336,13 +340,16 @@ impl InteractiveTxSigningSession { None }; - let funding_tx = if self.holder_tx_signatures.is_some() { + // Check if the holder has provided its signatures and if so, + // return the finalized funding transaction. + let funding_tx_opt = if self.holder_tx_signatures.is_some() { Some(self.finalize_funding_tx()) } else { + // This means we're still waiting for the holder to provide their signatures. None }; - Ok((holder_tx_signatures, funding_tx)) + Ok((holder_tx_signatures, funding_tx_opt)) } /// Provides the holder witnesses for the unsigned transaction. diff --git a/lightning/src/ln/types.rs b/lightning/src/ln/types.rs index e1ce9d0e078..5d72ba685cb 100644 --- a/lightning/src/ln/types.rs +++ b/lightning/src/ln/types.rs @@ -99,6 +99,13 @@ impl ChannelId { let our_revocation_point_bytes = our_revocation_basepoint.0.serialize(); Self(Sha256::hash(&[[0u8; 33], our_revocation_point_bytes].concat()).to_byte_array()) } + + /// Indicates whether this is a V2 channel ID for the given local and remote revocation basepoints. + pub fn is_v2_channel_id( + &self, ours: &RevocationBasepoint, theirs: &RevocationBasepoint, + ) -> bool { + *self == Self::v2_from_revocation_basepoints(ours, theirs) + } } impl Writeable for ChannelId { @@ -213,4 +220,18 @@ mod tests { assert_eq!(ChannelId::v2_from_revocation_basepoints(&ours, &theirs), expected_id); } + + #[test] + fn test_is_v2_channel_id() { + let our_pk = "0324653eac434488002cc06bbfb7f10fe18991e35f9fe4302dbea6d2353dc0ab1c"; + let ours = RevocationBasepoint(PublicKey::from_str(&our_pk).unwrap()); + let their_pk = "02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619"; + let theirs = RevocationBasepoint(PublicKey::from_str(&their_pk).unwrap()); + + let channel_id = ChannelId::v2_from_revocation_basepoints(&ours, &theirs); + assert!(channel_id.is_v2_channel_id(&ours, &theirs)); + + let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1); + assert!(!channel_id.is_v2_channel_id(&ours, &theirs)) + } } From a86a745a62d223b660a16816d25fc05046c9ccb2 Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Wed, 27 Nov 2024 11:39:54 +0200 Subject: [PATCH 5/7] s/`Option`/`()` for return from `provide_holder_witnesses` In a following commit we change the state at which a V2 channel is promoted to a `Channel` (after monitor persistence) to avoid a crash upon reading a `Channel` with no corresponding monitor persisted. This means that we will have no need for an optional (based on signing order) `TxSignatures` being returned when `provide_holder_witnesses` is called. The above is also the reason for removing the `received_commitment_signed` and signing order checks in the body of `provide_holder_witnesses`. These checks will only be important when we actually contribute inputs. Currently, we don't so we always send `tx_signatures` first in accordance with the Interactive Transaction Construction specification: https://github.com/lightning/bolts/blob/aa5207aea/02-peer-protocol.md?plain=1#L404 --- lightning/src/ln/interactivetxs.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index bc015d1f6fd..2ed66e83d43 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -358,7 +358,7 @@ impl InteractiveTxSigningSession { /// unsigned transaction. pub fn provide_holder_witnesses( &mut self, channel_id: ChannelId, witnesses: Vec, - ) -> Result, ()> { + ) -> Result<(), ()> { if self.local_inputs_count() != witnesses.len() { return Err(()); } @@ -370,13 +370,8 @@ impl InteractiveTxSigningSession { witnesses: witnesses.into_iter().collect(), shared_input_signature: None, }); - if self.received_commitment_signed - && (self.holder_sends_tx_signatures_first || self.counterparty_sent_tx_signatures) - { - Ok(self.holder_tx_signatures.clone()) - } else { - Ok(None) - } + + Ok(()) } pub fn remote_inputs_count(&self) -> usize { From d71c31d8ae8516a370edd5e2e2f724ae6b0c0cec Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Wed, 27 Nov 2024 13:35:04 +0200 Subject: [PATCH 6/7] Clean up conditional assignment of `funding_ready_for_sig_event` We don't yet support contibuting inputs to V2 channels, so we need to debug_assert and return an error if our signing session somehow has local inputs. We also return an error if for some mystical reason, in the no input contributions case, the input count does not equal the witness count of zero. --- lightning/src/ln/channel.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index caab9299107..eb1db3317a6 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2242,15 +2242,19 @@ impl PendingV2Channel where SP::Target: SignerProvider { }, }; - let funding_ready_for_sig_event = None; - if signing_session.local_inputs_count() == 0 { + let funding_ready_for_sig_event = if signing_session.local_inputs_count() == 0 { debug_assert_eq!(our_funding_satoshis, 0); if signing_session.provide_holder_witnesses(self.context.channel_id, Vec::new()).is_err() { debug_assert!( false, "Zero inputs were provided & zero witnesses were provided, but a count mismatch was somehow found", ); + return Err((self, ChannelError::Close(( + "V2 channel rejected due to sender error".into(), + ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) } + )))); } + None } else { // TODO(dual_funding): Send event for signing if we've contributed funds. // Inform the user that SIGHASH_ALL must be used for all signatures when contributing @@ -2266,7 +2270,15 @@ impl PendingV2Channel where SP::Target: SignerProvider { // will prevent the funding transaction from being relayed on the bitcoin network and hence being // confirmed. // - } + debug_assert!( + false, + "We don't support users providing inputs but somehow we had more than zero inputs", + ); + return Err((self, ChannelError::Close(( + "V2 channel rejected due to sender error".into(), + ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) } + )))); + }; self.context.channel_state = ChannelState::FundingNegotiated; From 9f9a7a92e1c15cad13111f7b0b072640edf2b28b Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Wed, 27 Nov 2024 13:56:16 +0200 Subject: [PATCH 7/7] Promote V2 channels to `FundedChannel` on initial `commitment_signed` receipt Before this commit, unfunded V2 channels were promoted to `FundedChannel`s in `PendingV2Channel::funding_tx_constructed`. Since a monitor is only created upon receipt of an initial `commitment_signed`, this would cause a crash if the channel was read from persisted data between those two events. Consequently, we also need to hold an `interactive_tx_signing_session` for both of our unfunded V2 channel structs. --- lightning/src/ln/channel.rs | 114 +++++++++++++++++------------ lightning/src/ln/channelmanager.rs | 32 ++++---- 2 files changed, 80 insertions(+), 66 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index eb1db3317a6..01feaf017df 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1478,32 +1478,67 @@ impl Channel where where L::Target: Logger { - let phase = core::mem::replace(&mut self.phase, ChannelPhase::Undefined); - let result = if let ChannelPhase::UnfundedV2(chan) = phase { + if let ChannelPhase::UnfundedV2(chan) = &mut self.phase { let logger = WithChannelContext::from(logger, &chan.context, None); - match chan.funding_tx_constructed(signing_session, &&logger) { - Ok((chan, commitment_signed, event)) => { - self.phase = ChannelPhase::Funded(chan); - Ok((commitment_signed, event)) - }, - Err((chan, e)) => { - self.phase = ChannelPhase::UnfundedV2(chan); - Err(e) - }, - } + chan.funding_tx_constructed(signing_session, &&logger) } else { - self.phase = phase; Err(ChannelError::Warn("Got a tx_complete message with no interactive transaction construction expected or in-progress".to_owned())) - }; - - debug_assert!(!matches!(self.phase, ChannelPhase::Undefined)); - result + } } pub fn force_shutdown(&mut self, should_broadcast: bool, closure_reason: ClosureReason) -> ShutdownResult { let (funding, context) = self.funding_and_context_mut(); context.force_shutdown(funding, should_broadcast, closure_reason) } + + pub fn commitment_signed( + &mut self, msg: &msgs::CommitmentSigned, best_block: BestBlock, signer_provider: &SP, logger: &L + ) -> Result<(Option::EcdsaSigner>>, Option), ChannelError> + where + L::Target: Logger + { + let phase = core::mem::replace(&mut self.phase, ChannelPhase::Undefined); + match phase { + ChannelPhase::UnfundedV2(chan) => { + let holder_commitment_point = match chan.unfunded_context.holder_commitment_point { + Some(point) => point, + None => { + let channel_id = chan.context.channel_id(); + // TODO(dual_funding): Add async signing support. + return Err( ChannelError::close( + format!("Expected to have holder commitment points available upon finishing interactive tx construction for channel {}", + channel_id))); + } + }; + let mut funded_channel = FundedChannel { + funding: chan.funding, + context: chan.context, + interactive_tx_signing_session: chan.interactive_tx_signing_session, + holder_commitment_point, + is_v2_established: true, + }; + let res = funded_channel.commitment_signed_initial_v2(msg, best_block, signer_provider, logger) + .map(|monitor| (Some(monitor), None)) + // TODO: Change to `inspect_err` when MSRV is high enough. + .map_err(|err| { + // We always expect a `ChannelError` close. + debug_assert!(matches!(err, ChannelError::Close(_))); + err + }); + self.phase = ChannelPhase::Funded(funded_channel); + res + }, + ChannelPhase::Funded(mut funded_channel) => { + let res = funded_channel.commitment_signed(msg, logger).map(|monitor_update_opt| (None, monitor_update_opt)); + self.phase = ChannelPhase::Funded(funded_channel); + res + }, + _ => { + debug_assert!(!matches!(self.phase, ChannelPhase::Undefined)); + Err(ChannelError::close("Got a commitment_signed message for an unfunded V1 channel!".into())) + } + } + } } impl From> for Channel @@ -2194,8 +2229,8 @@ impl PendingV2Channel where SP::Target: SignerProvider { } pub fn funding_tx_constructed( - mut self, mut signing_session: InteractiveTxSigningSession, logger: &L - ) -> Result<(FundedChannel, msgs::CommitmentSigned, Option), (PendingV2Channel, ChannelError)> + &mut self, mut signing_session: InteractiveTxSigningSession, logger: &L + ) -> Result<(msgs::CommitmentSigned, Option), ChannelError> where L::Target: Logger { @@ -2211,7 +2246,7 @@ impl PendingV2Channel where SP::Target: SignerProvider { ( "Multiple outputs matched the expected script and value".to_owned(), ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, - ))).map_err(|e| (self, e)); + ))); } output_index = Some(idx as u16); } @@ -2223,7 +2258,7 @@ impl PendingV2Channel where SP::Target: SignerProvider { ( "No output matched the funding script_pubkey".to_owned(), ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, - ))).map_err(|e| (self, e)); + ))); }; self.context.channel_transaction_parameters.funding_outpoint = Some(outpoint); self.context.holder_signer.as_mut().provide_channel_parameters(&self.context.channel_transaction_parameters); @@ -2237,8 +2272,7 @@ impl PendingV2Channel where SP::Target: SignerProvider { }, Err(err) => { self.context.channel_transaction_parameters.funding_outpoint = None; - return Err(ChannelError::Close((err.to_string(), ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }))) - .map_err(|e| (self, e)); + return Err(ChannelError::Close((err.to_string(), ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }))); }, }; @@ -2249,10 +2283,10 @@ impl PendingV2Channel where SP::Target: SignerProvider { false, "Zero inputs were provided & zero witnesses were provided, but a count mismatch was somehow found", ); - return Err((self, ChannelError::Close(( + return Err(ChannelError::Close(( "V2 channel rejected due to sender error".into(), ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) } - )))); + ))); } None } else { @@ -2274,37 +2308,19 @@ impl PendingV2Channel where SP::Target: SignerProvider { false, "We don't support users providing inputs but somehow we had more than zero inputs", ); - return Err((self, ChannelError::Close(( + return Err(ChannelError::Close(( "V2 channel rejected due to sender error".into(), ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) } - )))); + ))); }; self.context.channel_state = ChannelState::FundingNegotiated; // Clear the interactive transaction constructor self.interactive_tx_constructor.take(); + self.interactive_tx_signing_session = Some(signing_session); - match self.unfunded_context.holder_commitment_point { - Some(holder_commitment_point) => { - let funded_chan = FundedChannel { - funding: self.funding, - context: self.context, - interactive_tx_signing_session: Some(signing_session), - holder_commitment_point, - is_v2_established: true, - }; - Ok((funded_chan, commitment_signed, funding_ready_for_sig_event)) - }, - None => { - Err(ChannelError::close( - format!( - "Expected to have holder commitment points available upon finishing interactive tx construction for channel {}", - self.context.channel_id(), - ))) - .map_err(|e| (self, e)) - }, - } + Ok((commitment_signed, funding_ready_for_sig_event)) } } @@ -9511,6 +9527,8 @@ pub(super) struct PendingV2Channel where SP::Target: SignerProvider { pub dual_funding_context: DualFundingChannelContext, /// The current interactive transaction construction session under negotiation. pub interactive_tx_constructor: Option, + /// The signing session created after `tx_complete` handling + pub interactive_tx_signing_session: Option, } impl PendingV2Channel where SP::Target: SignerProvider { @@ -9576,6 +9594,7 @@ impl PendingV2Channel where SP::Target: SignerProvider { our_funding_inputs: funding_inputs, }, interactive_tx_constructor: None, + interactive_tx_signing_session: None, }; Ok(chan) } @@ -9747,6 +9766,7 @@ impl PendingV2Channel where SP::Target: SignerProvider { context, dual_funding_context, interactive_tx_constructor, + interactive_tx_signing_session: None, unfunded_context, }) } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 82b00011dbb..1f12e51bf14 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -8950,14 +8950,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let peer_state = &mut *peer_state_lock; match peer_state.channel_by_id.entry(msg.channel_id) { hash_map::Entry::Occupied(mut chan_entry) => { - if let Some(chan) = chan_entry.get_mut().as_funded_mut() { - let logger = WithChannelContext::from(&self.logger, &chan.context, None); - let funding_txo = chan.context.get_funding_txo(); - - if chan.interactive_tx_signing_session.is_some() { - let monitor = try_channel_entry!( - self, peer_state, chan.commitment_signed_initial_v2(msg, best_block, &self.signer_provider, &&logger), - chan_entry); + let chan = chan_entry.get_mut(); + let logger = WithChannelContext::from(&self.logger, &chan.context(), None); + let funding_txo = chan.context().get_funding_txo(); + let (monitor_opt, monitor_update_opt) = try_channel_entry!( + self, peer_state, chan.commitment_signed(msg, best_block, &self.signer_provider, &&logger), + chan_entry); + + if let Some(chan) = chan.as_funded_mut() { + if let Some(monitor) = monitor_opt { let monitor_res = self.chain_monitor.watch_channel(monitor.channel_id(), monitor); if let Ok(persist_state) = monitor_res { handle_new_monitor_update!(self, persist_state, peer_state_lock, peer_state, @@ -8972,19 +8973,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ ) )), chan_entry) } - } else { - let monitor_update_opt = try_channel_entry!( - self, peer_state, chan.commitment_signed(msg, &&logger), chan_entry); - if let Some(monitor_update) = monitor_update_opt { - handle_new_monitor_update!(self, funding_txo.unwrap(), monitor_update, peer_state_lock, - peer_state, per_peer_state, chan); - } + } else if let Some(monitor_update) = monitor_update_opt { + handle_new_monitor_update!(self, funding_txo.unwrap(), monitor_update, peer_state_lock, + peer_state, per_peer_state, chan); } - Ok(()) - } else { - return try_channel_entry!(self, peer_state, Err(ChannelError::close( - "Got a commitment_signed message for an unfunded channel!".into())), chan_entry); } + Ok(()) }, hash_map::Entry::Vacant(_) => Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id)) }