Handle sign_counterparty_commitment failing during outb funding
authorMatt Corallo <git@bluematt.me>
Tue, 5 Sep 2023 21:06:22 +0000 (21:06 +0000)
committerChris Waterson <waterson@gmail.com>
Wed, 1 Nov 2023 21:41:02 +0000 (14:41 -0700)
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.

lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_tests.rs

index 9ab9e7e07b060a0395a825ba3407075812bf9e4c..c6375e1a7c733327885780bac4c36b63e6d759ac 100644 (file)
@@ -756,6 +756,10 @@ pub(super) struct ChannelContext<SP: Deref> 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<SP: Deref> Channel<SP> 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<SP: Deref> OutboundV1Channel<SP> 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<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
                })
        }
 
-       /// If an Err is returned, it is a ChannelError::Close (for get_funding_created)
-       fn get_funding_created_signature<L: Deref>(&mut self, logger: &L) -> Result<Signature, ChannelError> where L::Target: Logger {
+       fn get_funding_created_signature<L: Deref>(&mut self, logger: &L) -> Result<Signature, ()> 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<SP: Deref> OutboundV1Channel<SP> 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<L: Deref>(mut self, funding_transaction: Transaction, funding_txo: OutPoint, is_batch_funding: bool, logger: &L)
-       -> Result<(Channel<SP>, msgs::FundingCreated), (Self, ChannelError)> where L::Target: Logger {
+       -> Result<(Channel<SP>, Option<msgs::FundingCreated>), (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<SP: Deref> OutboundV1Channel<SP> 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<SP: Deref> OutboundV1Channel<SP> 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<SP: Deref> InboundV1Channel<SP> 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,
index fe050de913616f7508a39a6466a669c876949f4c..b91669131211b998b1c476f069b62f13f5ae31a6 100644 (file)
@@ -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?");
index 1d67b736ae386e6e09d696973ec103806b13e896..8ceb9728931972e4d172540b801db379370404eb 100644 (file)
@@ -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);