From: Matt Corallo Date: Tue, 5 Sep 2023 21:06:22 +0000 (+0000) Subject: Handle sign_counterparty_commitment failing during outb funding X-Git-Tag: v0.0.119~65^2~5 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=d86f73b8d50b19c65264c3ed53c98edbc68613f3;p=rust-lightning Handle sign_counterparty_commitment failing during outb funding If sign_counterparty_commitment fails (i.e. because the signer is temporarily disconnected), this really indicates that we should retry the message sending which required the signature later, rather than force-closing the channel (which probably won't even work if the signer is missing). Here we add initial handling of sign_counterparty_commitment failing during outbound channel funding, setting a new flag in `ChannelContext` which indicates we should retry sending the `funding_created` later. We don't yet add any ability to do that retry. --- diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 9ab9e7e0..c6375e1a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -756,6 +756,10 @@ pub(super) struct ChannelContext where SP::Target: SignerProvider { /// This flag is set in such a case. Note that we don't need to persist this as we'll end up /// setting it again as a side-effect of [`Channel::channel_reestablish`]. signer_pending_commitment_update: bool, + /// Similar to [`Self::signer_pending_commitment_update`] but we're waiting to send either a + /// [`msgs::FundingCreated`] or [`msgs::FundingSigned`] depending on if this channel is + /// outbound or inbound. + signer_pending_funding: bool, // pending_update_fee is filled when sending and receiving update_fee. // @@ -4817,6 +4821,12 @@ impl Channel where return None; } + // If we're still pending the signature on a funding transaction, then we're not ready to send a + // channel_ready yet. + if self.context.signer_pending_funding { + return None; + } + // Note that we don't include ChannelState::WaitingForBatch as we don't want to send // channel_ready until the entire batch is ready. let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS); @@ -5874,6 +5884,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { monitor_pending_finalized_fulfills: Vec::new(), signer_pending_commitment_update: false, + signer_pending_funding: false, #[cfg(debug_assertions)] holder_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)), @@ -5955,15 +5966,14 @@ impl OutboundV1Channel where SP::Target: SignerProvider { }) } - /// If an Err is returned, it is a ChannelError::Close (for get_funding_created) - fn get_funding_created_signature(&mut self, logger: &L) -> Result where L::Target: Logger { + fn get_funding_created_signature(&mut self, logger: &L) -> Result where L::Target: Logger { let counterparty_keys = self.context.build_remote_transaction_keys(); let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; match &self.context.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ecdsa) => { - Ok(ecdsa.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), &self.context.secp_ctx) - .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0) + ecdsa.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), &self.context.secp_ctx) + .map(|(sig, _)| sig) } } } @@ -5976,7 +5986,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { /// Do NOT broadcast the funding transaction until after a successful funding_signed call! /// If an Err is returned, it is a ChannelError::Close. pub fn get_funding_created(mut self, funding_transaction: Transaction, funding_txo: OutPoint, is_batch_funding: bool, logger: &L) - -> Result<(Channel, msgs::FundingCreated), (Self, ChannelError)> where L::Target: Logger { + -> Result<(Channel, Option), (Self, ChannelError)> where L::Target: Logger { if !self.context.is_outbound() { panic!("Tried to create outbound funding_created message on an inbound channel!"); } @@ -5992,15 +6002,6 @@ impl OutboundV1Channel where SP::Target: SignerProvider { self.context.channel_transaction_parameters.funding_outpoint = Some(funding_txo); self.context.holder_signer.as_mut().provide_channel_parameters(&self.context.channel_transaction_parameters); - let signature = match self.get_funding_created_signature(logger) { - Ok(res) => res, - Err(e) => { - log_error!(logger, "Got bad signatures: {:?}!", e); - self.context.channel_transaction_parameters.funding_outpoint = None; - return Err((self, e)); - } - }; - let temporary_channel_id = self.context.channel_id; // Now that we're past error-generating stuff, update our local state: @@ -6019,20 +6020,27 @@ impl OutboundV1Channel where SP::Target: SignerProvider { self.context.funding_transaction = Some(funding_transaction); self.context.is_batch_funding = Some(()).filter(|_| is_batch_funding); + let funding_created = if let Ok(signature) = self.get_funding_created_signature(logger) { + Some(msgs::FundingCreated { + temporary_channel_id, + funding_txid: funding_txo.txid, + funding_output_index: funding_txo.index, + signature, + #[cfg(taproot)] + partial_signature_with_nonce: None, + #[cfg(taproot)] + next_local_nonce: None, + }) + } else { + self.context.signer_pending_funding = true; + None + }; + let channel = Channel { context: self.context, }; - Ok((channel, msgs::FundingCreated { - temporary_channel_id, - funding_txid: funding_txo.txid, - funding_output_index: funding_txo.index, - signature, - #[cfg(taproot)] - partial_signature_with_nonce: None, - #[cfg(taproot)] - next_local_nonce: None, - })) + Ok((channel, funding_created)) } fn get_initial_channel_type(config: &UserConfig, their_features: &InitFeatures) -> ChannelTypeFeatures { @@ -6530,6 +6538,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { monitor_pending_finalized_fulfills: Vec::new(), signer_pending_commitment_update: false, + signer_pending_funding: false, #[cfg(debug_assertions)] holder_max_commitment_tx_output: Mutex::new((msg.push_msat, msg.funding_satoshis * 1000 - msg.push_msat)), @@ -7623,6 +7632,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch monitor_pending_finalized_fulfills: monitor_pending_finalized_fulfills.unwrap(), signer_pending_commitment_update: false, + signer_pending_funding: false, pending_update_fee, holding_cell_update_fee, @@ -7895,7 +7905,7 @@ mod tests { }]}; let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 }; let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(tx.clone(), funding_outpoint, false, &&logger).map_err(|_| ()).unwrap(); - let (_, funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&keys_provider, &&logger).map_err(|_| ()).unwrap(); + let (_, funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg.unwrap(), best_block, &&keys_provider, &&logger).map_err(|_| ()).unwrap(); // Node B --> Node A: funding signed let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, &&logger).unwrap(); @@ -8022,7 +8032,7 @@ mod tests { }]}; let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 }; let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(tx.clone(), funding_outpoint, false, &&logger).map_err(|_| ()).unwrap(); - let (mut node_b_chan, funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&keys_provider, &&logger).map_err(|_| ()).unwrap(); + let (mut node_b_chan, funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg.unwrap(), best_block, &&keys_provider, &&logger).map_err(|_| ()).unwrap(); // Node B --> Node A: funding signed let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, &&logger).unwrap(); @@ -8210,7 +8220,7 @@ mod tests { }]}; let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 }; let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(tx.clone(), funding_outpoint, false, &&logger).map_err(|_| ()).unwrap(); - let (_, funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&keys_provider, &&logger).map_err(|_| ()).unwrap(); + let (_, funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg.unwrap(), best_block, &&keys_provider, &&logger).map_err(|_| ()).unwrap(); // Node B --> Node A: funding signed let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, &&logger).unwrap(); @@ -9282,7 +9292,7 @@ mod tests { &&logger, ).map_err(|_| ()).unwrap(); let (mut node_b_chan, funding_signed_msg, _) = node_b_chan.funding_created( - &funding_created_msg, + &funding_created_msg.unwrap(), best_block, &&keys_provider, &&logger, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index fe050de9..b9166913 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3802,7 +3802,7 @@ where let mut peer_state_lock = peer_state_mutex.lock().unwrap(); let peer_state = &mut *peer_state_lock; - let (chan, msg) = match peer_state.channel_by_id.remove(temporary_channel_id) { + let (chan, msg_opt) = match peer_state.channel_by_id.remove(temporary_channel_id) { Some(ChannelPhase::UnfundedOutboundV1(chan)) => { let funding_txo = find_funding_output(&chan, &funding_transaction)?; @@ -3841,10 +3841,12 @@ where }), }; - peer_state.pending_msg_events.push(events::MessageSendEvent::SendFundingCreated { - node_id: chan.context.get_counterparty_node_id(), - msg, - }); + if let Some(msg) = msg_opt { + peer_state.pending_msg_events.push(events::MessageSendEvent::SendFundingCreated { + node_id: chan.context.get_counterparty_node_id(), + msg, + }); + } match peer_state.channel_by_id.entry(chan.context.channel_id()) { hash_map::Entry::Occupied(_) => { panic!("Generated duplicate funding txid?"); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 1d67b736..8ceb9728 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -9045,7 +9045,7 @@ fn test_duplicate_chan_id() { } }; check_added_monitors!(nodes[0], 0); - nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created); + nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created.unwrap()); // At this point we'll look up if the channel_id is present and immediately fail the channel // without trying to persist the `ChannelMonitor`. check_added_monitors!(nodes[1], 0);