From b886679fc54b92491f580d7397b7ee8ce7ae812d Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Thu, 10 Aug 2023 12:53:59 -0700 Subject: [PATCH 01/10] Allow get_per_commitment_point to fail. This changes `ChannelSigner::get_per_commitment_point` to return a `Result ChannelMonitorImpl { }, commitment_txid: htlc.commitment_txid, per_commitment_number: htlc.per_commitment_number, - per_commitment_point: self.onchain_tx_handler.signer.get_per_commitment_point( - htlc.per_commitment_number, &self.onchain_tx_handler.secp_ctx, - ), + per_commitment_point: htlc.per_commitment_point, htlc: htlc.htlc, preimage: htlc.preimage, counterparty_sig: htlc.counterparty_sig, diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 35b7c86e700..6a55fca7a63 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -179,6 +179,7 @@ pub(crate) struct ExternalHTLCClaim { pub(crate) htlc: HTLCOutputInCommitment, pub(crate) preimage: Option, pub(crate) counterparty_sig: Signature, + pub(crate) per_commitment_point: bitcoin::secp256k1::PublicKey, } // Represents the different types of claims for which events are yielded externally to satisfy said @@ -1188,9 +1189,16 @@ impl OnchainTxHandler }) .map(|(htlc_idx, htlc)| { let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[htlc_idx]; + + // TODO(waterson) fallible: move this somewhere! + let per_commitment_point = self.signer.get_per_commitment_point( + trusted_tx.commitment_number(), &self.secp_ctx, + ).unwrap(); + ExternalHTLCClaim { commitment_txid: trusted_tx.txid(), per_commitment_number: trusted_tx.commitment_number(), + per_commitment_point: per_commitment_point, htlc: htlc.clone(), preimage: *preimage, counterparty_sig: counterparty_htlc_sig, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index da1364021ce..2670b2e8b58 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -35,7 +35,7 @@ use crate::chain::BestBlock; use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBoundedFeeEstimator}; use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS, CLOSED_CHANNEL_UPDATE_ID}; use crate::chain::transaction::{OutPoint, TransactionData}; -use crate::sign::{EcdsaChannelSigner, WriteableEcdsaChannelSigner, EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient}; +use crate::sign::{EcdsaChannelSigner, WriteableEcdsaChannelSigner, EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient, SignerError}; use crate::events::ClosureReason; use crate::routing::gossip::NodeId; use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter}; @@ -670,6 +670,13 @@ pub(super) struct ChannelContext where SP::Target: SignerProvider { // cost of others, but should really just be changed. cur_holder_commitment_transaction_number: u64, + + // The commitment point corresponding to `cur_holder_commitment_transaction_number`, which is the + // *next* state. We recompute it each time the state changes because the state changes in places + // that might be fallible: in particular, if the commitment point must be fetched from a remote + // source, we want to ensure it happens at a point where we can actually fail somewhat gracefully; + // i.e., force-closing a channel is better than a panic! + next_per_commitment_point: PublicKey, cur_counterparty_commitment_transaction_number: u64, value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees pending_inbound_htlcs: Vec, @@ -1152,6 +1159,39 @@ impl ChannelContext where SP::Target: SignerProvider { self.channel_ready_event_emitted = true; } + /// Returns the holder signer. + pub fn get_signer(&self) -> &ChannelSignerType<::Signer>{ + return &self.holder_signer + } + + /// Retrieves the next commitment point and its index from the signer. + /// + /// This maps a `SignerError` into an appropriate `ChannelError`. If the point cannot be + /// retrieved because the signer is unavailable, then `ChannelError::Ignore` is + /// returned. Otherwise, `ChannelError::Close` is returned. + /// + /// Note that this does _not_ advance the context's state. + pub fn get_next_holder_per_commitment_point(&self, logger: &L) -> Result<(u64, PublicKey), ChannelError> + where L::Target: Logger + { + let transaction_number = self.cur_holder_commitment_transaction_number - 1; + log_trace!(logger, "Retrieving commitment point for {} transaction number {}", self.channel_id(), transaction_number); + let per_commitment_point = + self.holder_signer.as_ref().get_per_commitment_point( + transaction_number, &self.secp_ctx + ).map_err(|e| match e { + SignerError::NotAvailable => { + log_warn!(logger, "Channel signer for {} is unavailable; try again later", self.channel_id()); + ChannelError::Ignore("Channel signer is unavailble; try again later".to_owned()) + }, + _ => { + log_error!(logger, "Channel signer for {} failed permanently; force-closing", self.channel_id()); + ChannelError::Close("Unable to generate commitment point".to_owned()) + } + })?; + Ok((transaction_number, per_commitment_point)) + } + /// Tracks the number of ticks elapsed since the previous [`ChannelConfig`] was updated. Once /// [`EXPIRE_PREV_CONFIG_TICKS`] is reached, the previous config is considered expired and will /// no longer be considered when forwarding HTLCs. @@ -1442,17 +1482,19 @@ impl ChannelContext where SP::Target: SignerProvider { #[inline] /// Creates a set of keys for build_commitment_transaction to generate a transaction which our - /// counterparty will sign (ie DO NOT send signatures over a transaction created by this to - /// our counterparty!) + /// counterparty will sign (ie DO NOT send signatures over a transaction created by this to our + /// counterparty!) The keys are specifically generated for the _next_ state to which the channel + /// is about to advance. /// The result is a transaction which we can revoke broadcastership of (ie a "local" transaction) /// TODO Some magic rust shit to compile-time check this? - fn build_holder_transaction_keys(&self, commitment_number: u64) -> TxCreationKeys { - let per_commitment_point = self.holder_signer.as_ref().get_per_commitment_point(commitment_number, &self.secp_ctx); + fn build_next_holder_transaction_keys(&self) -> TxCreationKeys { let delayed_payment_base = &self.get_holder_pubkeys().delayed_payment_basepoint; let htlc_basepoint = &self.get_holder_pubkeys().htlc_basepoint; let counterparty_pubkeys = self.get_counterparty_pubkeys(); - TxCreationKeys::derive_new(&self.secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint) + TxCreationKeys::derive_new( + &self.secp_ctx, &self.next_per_commitment_point, delayed_payment_base, htlc_basepoint, + &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint) } #[inline] @@ -2499,7 +2541,10 @@ impl Channel where log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}", &self.context.channel_id(), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction)); - let holder_signer = self.context.build_holder_transaction_keys(self.context.cur_holder_commitment_transaction_number); + // N.B. we'll have acquired the first per-commitment point from the signer during channel + // creation. Verify that the signature from the counterparty is correct so that we've got our + // signed refund transaction if we need to immediately close. + let holder_signer = self.context.build_next_holder_transaction_keys(); let initial_commitment_tx = self.context.build_commitment_transaction(self.context.cur_holder_commitment_transaction_number, &holder_signer, true, false, logger).tx; { let trusted_tx = initial_commitment_tx.trust(); @@ -2522,6 +2567,9 @@ impl Channel where self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, Vec::new()) .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?; + // Retrieve the next commitment point: if this results in a transient failure we'll unwind here + // and rely on retry to complete the funding_signed operation. + let (next_holder_commitment_transaction_number, next_per_commitment_point) = self.context.get_next_holder_per_commitment_point(logger)?; let funding_redeemscript = self.context.get_funding_redeemscript(); let funding_txo = self.context.get_funding_txo().unwrap(); @@ -2548,7 +2596,8 @@ impl Channel where assert_eq!(self.context.channel_state & (ChannelState::MonitorUpdateInProgress as u32), 0); // We have no had any monitor(s) yet to fail update! self.context.channel_state = ChannelState::FundingSent as u32; - self.context.cur_holder_commitment_transaction_number -= 1; + self.context.cur_holder_commitment_transaction_number = next_holder_commitment_transaction_number; + self.context.next_per_commitment_point = next_per_commitment_point; self.context.cur_counterparty_commitment_transaction_number -= 1; log_info!(logger, "Received funding_signed from peer for channel {}", &self.context.channel_id()); @@ -2870,7 +2919,7 @@ impl Channel where let funding_script = self.context.get_funding_redeemscript(); - let keys = self.context.build_holder_transaction_keys(self.context.cur_holder_commitment_transaction_number); + let keys = self.context.build_next_holder_transaction_keys(); let commitment_stats = self.context.build_commitment_transaction(self.context.cur_holder_commitment_transaction_number, &keys, true, false, logger); let commitment_txid = { @@ -2980,6 +3029,10 @@ impl Channel where self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.preimages) .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?; + // Retrieve the next commitment point: if this results in a transient failure we'll unwind here + // and rely on retry to complete the commitment_operation. + let (next_holder_commitment_transaction_number, next_per_commitment_point) = self.context.get_next_holder_per_commitment_point(logger)?; + // Update state now that we've passed all the can-fail calls... let mut need_commitment = false; if let &mut Some((_, ref mut update_state)) = &mut self.context.pending_update_fee { @@ -3033,7 +3086,9 @@ impl Channel where }] }; - self.context.cur_holder_commitment_transaction_number -= 1; + self.context.cur_holder_commitment_transaction_number = next_holder_commitment_transaction_number; + self.context.next_per_commitment_point = next_per_commitment_point; + // Note that if we need_commitment & !AwaitingRemoteRevoke we'll call // build_commitment_no_status_check() next which will reset this to RAAFirst. self.context.resend_order = RAACommitmentOrder::CommitmentFirst; @@ -3512,7 +3567,7 @@ impl Channel where // Before proposing a feerate update, check that we can actually afford the new fee. let inbound_stats = self.context.get_inbound_pending_htlc_stats(Some(feerate_per_kw)); let outbound_stats = self.context.get_outbound_pending_htlc_stats(Some(feerate_per_kw)); - let keys = self.context.build_holder_transaction_keys(self.context.cur_holder_commitment_transaction_number); + let keys = self.context.build_next_holder_transaction_keys(); let commitment_stats = self.context.build_commitment_transaction(self.context.cur_holder_commitment_transaction_number, &keys, true, true, logger); let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + outbound_stats.on_holder_tx_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; let holder_balance_msat = commitment_stats.local_balance_msat - outbound_stats.holding_cell_msat; @@ -3693,10 +3748,9 @@ impl Channel where assert!(!self.context.is_outbound() || self.context.minimum_depth == Some(0), "Funding transaction broadcast by the local client before it should have - LDK didn't do it!"); self.context.monitor_pending_channel_ready = false; - let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); Some(msgs::ChannelReady { channel_id: self.context.channel_id(), - next_per_commitment_point, + next_per_commitment_point: self.context.next_per_commitment_point, short_channel_id_alias: Some(self.context.outbound_scid_alias), }) } else { None }; @@ -3775,12 +3829,13 @@ impl Channel where } fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK { - let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); - let per_commitment_secret = self.context.holder_signer.as_ref().release_commitment_secret(self.context.cur_holder_commitment_transaction_number + 2); + // TODO(waterson): fallible! + let per_commitment_secret = self.context.holder_signer.as_ref().release_commitment_secret(self.context.cur_holder_commitment_transaction_number + 2) + .expect("release_per_commitment failed"); msgs::RevokeAndACK { channel_id: self.context.channel_id, per_commitment_secret, - next_per_commitment_point, + next_per_commitment_point: self.context.next_per_commitment_point, #[cfg(taproot)] next_local_nonce: None, } @@ -3890,7 +3945,9 @@ impl Channel where } if msg.next_remote_commitment_number > 0 { - let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.context.secp_ctx); + let state_index = INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1; + let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(state_index, &self.context.secp_ctx) + .map_err(|_| ChannelError::Close(format!("Unable to retrieve per-commitment point for state {state_index}")))?; let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret) .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { @@ -3949,11 +4006,10 @@ impl Channel where } // We have OurChannelReady set! - let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); return Ok(ReestablishResponses { channel_ready: Some(msgs::ChannelReady { channel_id: self.context.channel_id(), - next_per_commitment_point, + next_per_commitment_point: self.context.next_per_commitment_point, short_channel_id_alias: Some(self.context.outbound_scid_alias), }), raa: None, commitment_update: None, @@ -3989,10 +4045,9 @@ impl Channel where let channel_ready = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number == 1 { // We should never have to worry about MonitorUpdateInProgress resending ChannelReady - let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); Some(msgs::ChannelReady { channel_id: self.context.channel_id(), - next_per_commitment_point, + next_per_commitment_point: self.context.next_per_commitment_point, short_channel_id_alias: Some(self.context.outbound_scid_alias), }) } else { None }; @@ -4685,13 +4740,13 @@ impl Channel where if need_commitment_update { if self.context.channel_state & (ChannelState::MonitorUpdateInProgress as u32) == 0 { if self.context.channel_state & (ChannelState::PeerDisconnected as u32) == 0 { - let next_per_commitment_point = - self.context.holder_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &self.context.secp_ctx); - return Some(msgs::ChannelReady { - channel_id: self.context.channel_id, - next_per_commitment_point, - short_channel_id_alias: Some(self.context.outbound_scid_alias), - }); + if let Ok(next_per_commitment_point) = self.context.holder_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &self.context.secp_ctx) { + return Some(msgs::ChannelReady { + channel_id: self.context.channel_id, + next_per_commitment_point, + short_channel_id_alias: Some(self.context.outbound_scid_alias), + }); + } } } else { self.context.monitor_pending_channel_ready = true; @@ -5641,6 +5696,9 @@ impl OutboundV1Channel where SP::Target: SignerProvider { let temporary_channel_id = ChannelId::temporary_from_entropy_source(entropy_source); + let next_per_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) + .map_err(|_| APIError::ChannelUnavailable { err: "Unable to generate initial commitment point".to_owned()})?; + Ok(Self { context: ChannelContext { user_id, @@ -5669,6 +5727,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { destination_script, cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER, + next_per_commitment_point, cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER, value_to_self_msat, @@ -5910,7 +5969,6 @@ impl OutboundV1Channel where SP::Target: SignerProvider { panic!("Tried to send an open_channel for a channel that has already advanced"); } - let first_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); let keys = self.context.get_holder_pubkeys(); msgs::OpenChannel { @@ -5930,7 +5988,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { payment_point: keys.payment_point, delayed_payment_basepoint: keys.delayed_payment_basepoint, htlc_basepoint: keys.htlc_basepoint, - first_per_commitment_point, + first_per_commitment_point: self.context.next_per_commitment_point, channel_flags: if self.context.config.announced_channel {1} else {0}, shutdown_scriptpubkey: Some(match &self.context.shutdown_scriptpubkey { Some(script) => script.clone().into_inner(), @@ -6279,6 +6337,8 @@ impl InboundV1Channel where SP::Target: SignerProvider { } else { Some(cmp::max(config.channel_handshake_config.minimum_depth, 1)) }; + let next_per_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) + .map_err(|_| ChannelError::Close("Unable to generate initial commitment point".to_owned()))?; let chan = Self { context: ChannelContext { @@ -6307,6 +6367,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { destination_script, cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER, + next_per_commitment_point, cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER, value_to_self_msat: msg.push_msat, @@ -6437,7 +6498,6 @@ impl InboundV1Channel where SP::Target: SignerProvider { /// /// [`msgs::AcceptChannel`]: crate::ln::msgs::AcceptChannel fn generate_accept_channel_message(&self) -> msgs::AcceptChannel { - let first_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); let keys = self.context.get_holder_pubkeys(); msgs::AcceptChannel { @@ -6454,7 +6514,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { payment_point: keys.payment_point, delayed_payment_basepoint: keys.delayed_payment_basepoint, htlc_basepoint: keys.htlc_basepoint, - first_per_commitment_point, + first_per_commitment_point: self.context.next_per_commitment_point, shutdown_scriptpubkey: Some(match &self.context.shutdown_scriptpubkey { Some(script) => script.clone().into_inner(), None => Builder::new().into_script(), @@ -6477,7 +6537,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { fn funding_created_signature(&mut self, sig: &Signature, logger: &L) -> Result<(CommitmentTransaction, CommitmentTransaction, Signature), ChannelError> where L::Target: Logger { let funding_script = self.context.get_funding_redeemscript(); - let keys = self.context.build_holder_transaction_keys(self.context.cur_holder_commitment_transaction_number); + let keys = self.context.build_next_holder_transaction_keys(); let initial_commitment_tx = self.context.build_commitment_transaction(self.context.cur_holder_commitment_transaction_number, &keys, true, false, logger).tx; { let trusted_tx = initial_commitment_tx.trust(); @@ -6563,6 +6623,15 @@ impl InboundV1Channel where SP::Target: SignerProvider { return Err((self, ChannelError::Close("Failed to validate our commitment".to_owned()))); } + // Retrieve the next commitment point: if this results in a transient failure we'll unwind here + // and rely on retry to complete the funding_created operation. + let res = self.context.get_next_holder_per_commitment_point(logger); + if let Err(e) = res { + log_error!(logger, "Unable to handle funding_created: {}", e); + return Err((self, e)); + } + let (next_holder_commitment_transaction_number, next_per_commitment_point) = res.unwrap(); + // Now that we're past error-generating stuff, update our local state: let funding_redeemscript = self.context.get_funding_redeemscript(); @@ -6589,9 +6658,8 @@ impl InboundV1Channel where SP::Target: SignerProvider { self.context.channel_state = ChannelState::FundingSent as u32; self.context.channel_id = funding_txo.to_channel_id(); self.context.cur_counterparty_commitment_transaction_number -= 1; - self.context.cur_holder_commitment_transaction_number -= 1; - - log_info!(logger, "Generated funding_signed for peer for channel {}", &self.context.channel_id()); + self.context.cur_holder_commitment_transaction_number = next_holder_commitment_transaction_number; + self.context.next_per_commitment_point = next_per_commitment_point; // Promote the channel to a full-fledged one now that we have updated the state and have a // `ChannelMonitor`. @@ -7345,6 +7413,11 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch let mut secp_ctx = Secp256k1::new(); secp_ctx.seeded_randomize(&entropy_source.get_secure_random_bytes()); + // If we weren't able to load the next_per_commitment_point, ask the signer for it now. + let next_per_commitment_point = holder_signer.get_per_commitment_point( + cur_holder_commitment_transaction_number, &secp_ctx + ).map_err(|_| DecodeError::Io(io::ErrorKind::Other))?; + // `user_id` used to be a single u64 value. In order to remain backwards // compatible with versions prior to 0.0.113, the u128 is serialized as two // separate u64 values. @@ -7397,6 +7470,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch destination_script, cur_holder_commitment_transaction_number, + next_per_commitment_point, cur_counterparty_commitment_transaction_number, value_to_self_msat, @@ -8108,7 +8182,7 @@ mod tests { assert_eq!(counterparty_pubkeys.htlc_basepoint.serialize()[..], hex::decode("032c0b7cf95324a07d05398b240174dc0c2be444d96b159aa6c7f7b1e668680991").unwrap()[..]); - // We can't just use build_holder_transaction_keys here as the per_commitment_secret is not + // We can't just use build_next_holder_transaction_keys here as the per_commitment_secret is not // derived from a commitment_seed, so instead we copy it here and call // build_commitment_transaction. let delayed_payment_base = &chan.context.holder_signer.as_ref().pubkeys().delayed_payment_basepoint; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 64d5521dcb9..190c0a53380 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -5622,6 +5622,12 @@ where Some(inbound_chan) => { match inbound_chan.funding_created(msg, best_block, &self.signer_provider, &self.logger) { Ok(res) => res, + Err((inbound_chan, ChannelError::Ignore(_))) => { + // If we get an `Ignore` error then something transient went wrong. Put the channel + // back into the table and bail. + peer_state.inbound_v1_channel_by_id.insert(msg.temporary_channel_id, inbound_chan); + return Ok(()); + }, Err((mut inbound_chan, err)) => { // We've already removed this inbound channel from the map in `PeerState` // above so at this point we just need to clean up any lingering entries diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index b8c2e085999..9a09dcf1cb0 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -29,6 +29,7 @@ use crate::util::test_utils; use crate::util::test_utils::{panicking, TestChainMonitor, TestScorer, TestKeysInterface}; use crate::util::errors::APIError; use crate::util::config::{UserConfig, MaxDustHTLCExposure}; +use crate::util::logger::Logger; use crate::util::ser::{ReadableArgs, Writeable}; use bitcoin::blockdata::block::{Block, BlockHeader}; @@ -414,6 +415,31 @@ impl<'a, 'b, 'c> Node<'a, 'b, 'c> { pub fn get_block_header(&self, height: u32) -> BlockHeader { self.blocks.lock().unwrap()[height as usize].0.header } + /// Changes the channel signer's availability for the specified peer and channel. + /// + /// When `available` is set to `true`, the channel signer will behave normally. When set to + /// `false`, the channel signer will act like an off-line remote signer and will return + /// `SignerError::NotAvailable` for several of the signing methods. Currently, only + /// `get_per_commitment_point` and `release_commitment_secret` are affected by this setting. + #[cfg(test)] + pub fn set_channel_signer_available(&self, peer_id: &PublicKey, chan_id: &ChannelId, available: bool) { + let per_peer_state = self.node.per_peer_state.read().unwrap(); + let chan_lock = per_peer_state.get(peer_id).unwrap().lock().unwrap(); + let signer = (|| { + if let Some(local_chan) = chan_lock.channel_by_id.get(chan_id) { + return local_chan.get_signer(); + } + if let Some(local_chan) = chan_lock.inbound_v1_channel_by_id.get(chan_id) { + return local_chan.context.get_signer(); + } + if let Some(local_chan) = chan_lock.outbound_v1_channel_by_id.get(chan_id) { + return local_chan.context.get_signer(); + } + panic!("Couldn't find a channel with id {}", chan_id); + })(); + log_debug!(self.logger, "Setting channel {} as available={}", chan_id, available); + signer.as_ecdsa().unwrap().set_available(available); + } } /// If we need an unsafe pointer to a `Node` (ie to reference it in a thread @@ -2031,12 +2057,13 @@ macro_rules! expect_channel_shutdown_state { } #[cfg(any(test, ldk_bench, feature = "_test_utils"))] -pub fn expect_channel_pending_event<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, expected_counterparty_node_id: &PublicKey) { +pub fn expect_channel_pending_event<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, expected_counterparty_node_id: &PublicKey) -> ChannelId { let events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); - match events[0] { - crate::events::Event::ChannelPending { ref counterparty_node_id, .. } => { + match &events[0] { + crate::events::Event::ChannelPending { channel_id, counterparty_node_id, .. } => { assert_eq!(*expected_counterparty_node_id, *counterparty_node_id); + *channel_id }, _ => panic!("Unexpected event"), } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index d3401e5f068..9305b462b4c 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -714,7 +714,7 @@ fn test_update_fee_that_funder_cannot_afford() { let chan_signer = remote_chan.get_signer(); let pubkeys = chan_signer.as_ref().pubkeys(); (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx), + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(), pubkeys.funding_pubkey) }; @@ -1421,8 +1421,8 @@ fn test_fee_spike_violation_fails_htlc() { let pubkeys = chan_signer.as_ref().pubkeys(); (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER), - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx), + chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(), + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap(), chan_signer.as_ref().pubkeys().funding_pubkey) }; let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = { @@ -1432,7 +1432,7 @@ fn test_fee_spike_violation_fails_htlc() { let chan_signer = remote_chan.get_signer(); let pubkeys = chan_signer.as_ref().pubkeys(); (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx), + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(), chan_signer.as_ref().pubkeys().funding_pubkey) }; @@ -7660,15 +7660,15 @@ fn test_counterparty_raa_skip_no_crash() { // Make signer believe we got a counterparty signature, so that it allows the revocation keys.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1; - per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER); + per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(); // Must revoke without gaps keys.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1; - keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1); + keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1).expect("unable to release commitment secret"); keys.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1; next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(), - &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap()); + &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2).unwrap()).unwrap()); } nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), @@ -8979,6 +8979,193 @@ fn test_duplicate_chan_id() { send_payment(&nodes[0], &[&nodes[1]], 8000000); } +#[test] +fn test_signer_gpcp_unavailable_for_funding_signed() { + // Test that a transient failure of the Signer's get_per_commitment_point can be tolerated during + // channel open by specifically having it fail for an inbound channel during the handling of the + // funding_signed message. + 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 nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + // Create an initial channel + nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap(); + let mut open_chan_msg = 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_chan_msg); + nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id())); + + // Move the channel through the funding flow... + let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100000, 42); + + nodes[0].node.funding_transaction_generated(&temporary_channel_id, &nodes[1].node.get_our_node_id(), tx.clone()).unwrap(); + check_added_monitors(&nodes[0], 0); + + let mut funding_created_msg = 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_msg); + check_added_monitors(&nodes[1], 1); + + let chan_id = expect_channel_pending_event(&nodes[1], &nodes[0].node.get_our_node_id()); + + // Tweak the node[0] channel signer to produce an "unavailable" message before we ask it to handle + // the funding_signed message. + nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, false); + let funding_signed_msg = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()); + nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed_msg); + + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 0); + check_added_monitors(&nodes[0], 0); + + // Now make it available and verify that we can process the message. + nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, true); + nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed_msg); + check_added_monitors(&nodes[0], 1); + + expect_channel_pending_event(&nodes[0], &nodes[1].node.get_our_node_id()); + + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx); + + let (channel_ready, _) = create_chan_between_nodes_with_value_confirm(&nodes[0], &nodes[1], &tx); + let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &channel_ready); + update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update); + + send_payment(&nodes[0], &[&nodes[1]], 8000000); +} + +#[test] +fn test_signer_gpcp_unavailable_for_funding_created() { + // Test that a transient failure of the Signer's get_per_commitment_point can be tolerated during + // channel open by specifically having it fail for an outbound channel during generation of the + // funding_created message. + 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 nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + // Create an initial channel + nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap(); + let mut open_chan_msg = 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_chan_msg); + nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id())); + + // Move the channel through the funding flow... + let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100000, 42); + + nodes[0].node.funding_transaction_generated(&temporary_channel_id, &nodes[1].node.get_our_node_id(), tx.clone()).unwrap(); + check_added_monitors(&nodes[0], 0); + + // Tweak the node[1] channel signer to produce an "unavailable" result before we ask it to handle + // the funding_created message. + nodes[1].set_channel_signer_available(&nodes[0].node.get_our_node_id(), &temporary_channel_id, false); + let mut funding_created_msg = 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_msg); + check_added_monitors(&nodes[1], 0); + + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 0); + + // Now make it available and verify that we can process the message. + nodes[1].set_channel_signer_available(&nodes[0].node.get_our_node_id(), &temporary_channel_id, true); + nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg); + check_added_monitors(&nodes[1], 1); + + let funding_signed_msg = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()); + nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed_msg); + check_added_monitors(&nodes[0], 1); + + expect_channel_pending_event(&nodes[0], &nodes[1].node.get_our_node_id()); + expect_channel_pending_event(&nodes[1], &nodes[0].node.get_our_node_id()); + + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx); + + let (channel_ready, _) = create_chan_between_nodes_with_value_confirm(&nodes[0], &nodes[1], &tx); + let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &channel_ready); + update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update); + send_payment(&nodes[0], &[&nodes[1]], 8000000); +} + +#[test] +fn test_dest_signer_gpcp_unavailable_for_commitment_signed() { + // Test that a transient failure of the Signer's get_per_commitment_point can be tolerated by the + // destination node for a payment. + 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 nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + // Create an initial channel + nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap(); + let mut open_chan_msg = 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_chan_msg); + nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id())); + + // Move the channel through the funding flow... + let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100000, 42); + + nodes[0].node.funding_transaction_generated(&temporary_channel_id, &nodes[1].node.get_our_node_id(), tx.clone()).unwrap(); + check_added_monitors(&nodes[0], 0); + + let mut funding_created_msg = 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_msg); + check_added_monitors(&nodes[1], 1); + + let funding_signed_msg = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()); + nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed_msg); + check_added_monitors(&nodes[0], 1); + + let chan_id = expect_channel_pending_event(&nodes[0], &nodes[1].node.get_our_node_id()); + expect_channel_pending_event(&nodes[1], &nodes[0].node.get_our_node_id()); + + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx); + + let (channel_ready, _) = create_chan_between_nodes_with_value_confirm(&nodes[0], &nodes[1], &tx); + let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &channel_ready); + update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update); + + // Send a payment. + let src = &nodes[0]; + let dst = &nodes[1]; + let (route, our_payment_hash, _our_payment_preimage, our_payment_secret) = get_route_and_payment_hash!(src, dst, 8000000); + src.node.send_payment_with_route(&route, our_payment_hash, + RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0)).unwrap(); + check_added_monitors!(src, 1); + + // Pass the payment along the route. + let payment_event = { + let mut events = src.node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + SendEvent::from_event(events.remove(0)) + }; + assert_eq!(payment_event.node_id, dst.node.get_our_node_id()); + assert_eq!(payment_event.msgs.len(), 1); + + dst.node.handle_update_add_htlc(&src.node.get_our_node_id(), &payment_event.msgs[0]); + + // Mark dst's signer as unavailable. + dst.set_channel_signer_available(&src.node.get_our_node_id(), &chan_id, false); + dst.node.handle_commitment_signed(&src.node.get_our_node_id(), &payment_event.commitment_msg); + check_added_monitors(src, 0); + check_added_monitors(dst, 0); + + { + let src_events = src.node.get_and_clear_pending_msg_events(); + assert_eq!(src_events.len(), 0); + let dst_events = dst.node.get_and_clear_pending_msg_events(); + assert_eq!(dst_events.len(), 0); + } + + // Mark dst's signer as available and re-handle commitment_signed. We expect to see both the RAA + // and the CS. + dst.set_channel_signer_available(&src.node.get_our_node_id(), &chan_id, true); + dst.node.handle_commitment_signed(&src.node.get_our_node_id(), &payment_event.commitment_msg); + get_revoke_commit_msgs(dst, &src.node.get_our_node_id()); + check_added_monitors!(dst, 1); +} + #[test] fn test_error_chans_closed() { // Test that we properly handle error messages, closing appropriate channels. diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index f25c9476448..5554399283f 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -343,13 +343,23 @@ impl SpendableOutputDescriptor { } } +/// An error that occurred during signing. +#[derive(Copy, Clone, Debug, PartialEq)] +pub enum SignerError { + /// The signer is not currently available, and the operation may be tried again later. + NotAvailable, + /// The signer has failed in an unrecoverable way. If possible, anything relying on the signer + /// should be gracefully shut down. + PermanentFailure, +} + /// A trait to handle Lightning channel key material without concretizing the channel type or /// the signature mechanism. pub trait ChannelSigner { /// Gets the per-commitment point for a specific commitment number /// /// Note that the commitment number starts at `(1 << 48) - 1` and counts backwards. - fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> PublicKey; + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result; /// Gets the commitment secret for a specific commitment number as part of the revocation process /// @@ -360,7 +370,7 @@ pub trait ChannelSigner { /// /// Note that the commitment number starts at `(1 << 48) - 1` and counts backwards. // TODO: return a Result so we can signal a validation error - fn release_commitment_secret(&self, idx: u64) -> [u8; 32]; + fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], SignerError>; /// Validate the counterparty's signatures on the holder commitment transaction and HTLCs. /// @@ -954,13 +964,13 @@ impl EntropySource for InMemorySigner { } impl ChannelSigner for InMemorySigner { - fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> PublicKey { + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result { let commitment_secret = SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx)).unwrap(); - PublicKey::from_secret_key(secp_ctx, &commitment_secret) + Ok(PublicKey::from_secret_key(secp_ctx, &commitment_secret)) } - fn release_commitment_secret(&self, idx: u64) -> [u8; 32] { - chan_utils::build_commitment_secret(&self.commitment_seed, idx) + fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], SignerError> { + Ok(chan_utils::build_commitment_secret(&self.commitment_seed, idx)) } fn validate_holder_commitment(&self, _holder_tx: &HolderCommitmentTransaction, _preimages: Vec) -> Result<(), ()> { diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index 2fb1c494f93..288be615d9e 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -10,7 +10,7 @@ use crate::ln::channel::{ANCHOR_OUTPUT_VALUE_SATOSHI, MIN_CHAN_DUST_LIMIT_SATOSHIS}; use crate::ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, HolderCommitmentTransaction, CommitmentTransaction, ChannelTransactionParameters, TrustedCommitmentTransaction, ClosingTransaction}; use crate::ln::{chan_utils, msgs, PaymentPreimage}; -use crate::sign::{WriteableEcdsaChannelSigner, InMemorySigner, ChannelSigner, EcdsaChannelSigner}; +use crate::sign::{WriteableEcdsaChannelSigner, InMemorySigner, ChannelSigner, EcdsaChannelSigner, SignerError}; use crate::prelude::*; use core::cmp; @@ -56,6 +56,7 @@ pub struct TestChannelSigner { /// Channel state used for policy enforcement pub state: Arc>, pub disable_revocation_policy_check: bool, + available: Arc>, } impl PartialEq for TestChannelSigner { @@ -71,7 +72,8 @@ impl TestChannelSigner { Self { inner, state, - disable_revocation_policy_check: false + disable_revocation_policy_check: false, + available: Arc::new(Mutex::new(true)), } } @@ -84,7 +86,8 @@ impl TestChannelSigner { Self { inner, state, - disable_revocation_policy_check + disable_revocation_policy_check, + available: Arc::new(Mutex::new(true)), } } @@ -94,14 +97,32 @@ impl TestChannelSigner { pub fn get_enforcement_state(&self) -> MutexGuard { self.state.lock().unwrap() } + + /// Marks the signer's availability. + /// + /// When `true`, methods are forwarded to the underlying signer as normal. When `false`, some + /// methods will return `SignerError::NotAvailable`. In particular: + /// + /// - `get_per_commitment_point` + /// - `release_commitment_secret` + #[cfg(test)] + pub fn set_available(&self, available: bool) { + *self.available.lock().unwrap() = available; + } } impl ChannelSigner for TestChannelSigner { - fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> PublicKey { + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result { + if !*self.available.lock().unwrap() { + return Err(SignerError::NotAvailable); + } self.inner.get_per_commitment_point(idx, secp_ctx) } - fn release_commitment_secret(&self, idx: u64) -> [u8; 32] { + fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], SignerError> { + if !*self.available.lock().unwrap() { + return Err(SignerError::NotAvailable); + } { let mut state = self.state.lock().unwrap(); assert!(idx == state.last_holder_revoked_commitment || idx == state.last_holder_revoked_commitment - 1, "can only revoke the current or next unrevoked commitment - trying {}, last revoked {}", idx, state.last_holder_revoked_commitment); From 4fc90798140838b987f1b60e29960da4c9b8194f Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Tue, 29 Aug 2023 15:47:12 -0700 Subject: [PATCH 02/10] Clean up warnings. --- lightning/src/ln/channel.rs | 1 + lightning/src/ln/functional_test_utils.rs | 1 + lightning/src/ln/functional_tests.rs | 2 +- lightning/src/util/test_utils.rs | 1 - 4 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 2670b2e8b58..c8735a4c8ac 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1160,6 +1160,7 @@ impl ChannelContext where SP::Target: SignerProvider { } /// Returns the holder signer. + #[cfg(test)] pub fn get_signer(&self) -> &ChannelSignerType<::Signer>{ return &self.holder_signer } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 9a09dcf1cb0..ee2f642de40 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -29,6 +29,7 @@ use crate::util::test_utils; use crate::util::test_utils::{panicking, TestChainMonitor, TestScorer, TestKeysInterface}; use crate::util::errors::APIError; use crate::util::config::{UserConfig, MaxDustHTLCExposure}; +#[cfg(test)] use crate::util::logger::Logger; use crate::util::ser::{ReadableArgs, Writeable}; diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 9305b462b4c..1d8eec6f72d 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -17,7 +17,7 @@ use crate::chain::chaininterface::LowerBoundedFeeEstimator; use crate::chain::channelmonitor; use crate::chain::channelmonitor::{CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY}; use crate::chain::transaction::OutPoint; -use crate::sign::{ChannelSigner, EcdsaChannelSigner, EntropySource, SignerProvider}; +use crate::sign::{EcdsaChannelSigner, EntropySource, SignerProvider}; use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination, PaymentFailureReason}; use crate::ln::{ChannelId, PaymentPreimage, PaymentSecret, PaymentHash}; use crate::ln::channel::{commitment_tx_base_weight, COMMITMENT_TX_WEIGHT_PER_HTLC, CONCURRENT_INBOUND_HTLC_FEE_BUFFER, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, MIN_AFFORDABLE_HTLC_COUNT, get_holder_selected_channel_reserve_satoshis, OutboundV1Channel, InboundV1Channel}; diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 7a9ce06910b..e44a04eba8e 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -61,7 +61,6 @@ use regex; use crate::io; use crate::prelude::*; use core::cell::RefCell; -use core::ops::Deref; use core::time::Duration; use crate::sync::{Mutex, Arc}; use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; From ae055bccbd26058a17a019829236a47df1276d54 Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Tue, 29 Aug 2023 15:52:55 -0700 Subject: [PATCH 03/10] Get rid of SignerError. We'll just always retry when we get an Err. --- lightning/src/ln/channel.rs | 21 ++++++--------------- lightning/src/ln/functional_test_utils.rs | 6 +++--- lightning/src/sign/mod.rs | 21 +++++++-------------- lightning/src/util/test_channel_signer.rs | 12 ++++++------ 4 files changed, 22 insertions(+), 38 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index c8735a4c8ac..b1ec2eaa77d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -35,7 +35,7 @@ use crate::chain::BestBlock; use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBoundedFeeEstimator}; use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS, CLOSED_CHANNEL_UPDATE_ID}; use crate::chain::transaction::{OutPoint, TransactionData}; -use crate::sign::{EcdsaChannelSigner, WriteableEcdsaChannelSigner, EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient, SignerError}; +use crate::sign::{EcdsaChannelSigner, WriteableEcdsaChannelSigner, EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient}; use crate::events::ClosureReason; use crate::routing::gossip::NodeId; use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter}; @@ -1167,11 +1167,8 @@ impl ChannelContext where SP::Target: SignerProvider { /// Retrieves the next commitment point and its index from the signer. /// - /// This maps a `SignerError` into an appropriate `ChannelError`. If the point cannot be - /// retrieved because the signer is unavailable, then `ChannelError::Ignore` is - /// returned. Otherwise, `ChannelError::Close` is returned. - /// - /// Note that this does _not_ advance the context's state. + /// This maps an `Err` into `ChannelError::Ignore`. Note that this does _not_ advance the + /// context's state. pub fn get_next_holder_per_commitment_point(&self, logger: &L) -> Result<(u64, PublicKey), ChannelError> where L::Target: Logger { @@ -1180,15 +1177,9 @@ impl ChannelContext where SP::Target: SignerProvider { let per_commitment_point = self.holder_signer.as_ref().get_per_commitment_point( transaction_number, &self.secp_ctx - ).map_err(|e| match e { - SignerError::NotAvailable => { - log_warn!(logger, "Channel signer for {} is unavailable; try again later", self.channel_id()); - ChannelError::Ignore("Channel signer is unavailble; try again later".to_owned()) - }, - _ => { - log_error!(logger, "Channel signer for {} failed permanently; force-closing", self.channel_id()); - ChannelError::Close("Unable to generate commitment point".to_owned()) - } + ).map_err(|_| { + log_warn!(logger, "Channel signer for {} is unavailable; try again later", self.channel_id()); + ChannelError::Ignore("Channel signer is unavailble; try again later".to_owned()) })?; Ok((transaction_number, per_commitment_point)) } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index ee2f642de40..bfb59b6f007 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -419,9 +419,9 @@ impl<'a, 'b, 'c> Node<'a, 'b, 'c> { /// Changes the channel signer's availability for the specified peer and channel. /// /// When `available` is set to `true`, the channel signer will behave normally. When set to - /// `false`, the channel signer will act like an off-line remote signer and will return - /// `SignerError::NotAvailable` for several of the signing methods. Currently, only - /// `get_per_commitment_point` and `release_commitment_secret` are affected by this setting. + /// `false`, the channel signer will act like an off-line remote signer and will return `Err` for + /// several of the signing methods. Currently, only `get_per_commitment_point` and + /// `release_commitment_secret` are affected by this setting. #[cfg(test)] pub fn set_channel_signer_available(&self, peer_id: &PublicKey, chan_id: &ChannelId, available: bool) { let per_peer_state = self.node.per_peer_state.read().unwrap(); diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index 5554399283f..0294aaeda00 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -343,23 +343,16 @@ impl SpendableOutputDescriptor { } } -/// An error that occurred during signing. -#[derive(Copy, Clone, Debug, PartialEq)] -pub enum SignerError { - /// The signer is not currently available, and the operation may be tried again later. - NotAvailable, - /// The signer has failed in an unrecoverable way. If possible, anything relying on the signer - /// should be gracefully shut down. - PermanentFailure, -} - /// A trait to handle Lightning channel key material without concretizing the channel type or /// the signature mechanism. pub trait ChannelSigner { /// Gets the per-commitment point for a specific commitment number /// /// Note that the commitment number starts at `(1 << 48) - 1` and counts backwards. - fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result; + /// + /// If the signer returns `Err`, then the user is responsible for either force-closing the channel + /// or retrying once the signature is ready. + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result; /// Gets the commitment secret for a specific commitment number as part of the revocation process /// @@ -370,7 +363,7 @@ pub trait ChannelSigner { /// /// Note that the commitment number starts at `(1 << 48) - 1` and counts backwards. // TODO: return a Result so we can signal a validation error - fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], SignerError>; + fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], ()>; /// Validate the counterparty's signatures on the holder commitment transaction and HTLCs. /// @@ -964,12 +957,12 @@ impl EntropySource for InMemorySigner { } impl ChannelSigner for InMemorySigner { - fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result { + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result { let commitment_secret = SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx)).unwrap(); Ok(PublicKey::from_secret_key(secp_ctx, &commitment_secret)) } - fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], SignerError> { + fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], ()> { Ok(chan_utils::build_commitment_secret(&self.commitment_seed, idx)) } diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index 288be615d9e..b4f045412e7 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -10,7 +10,7 @@ use crate::ln::channel::{ANCHOR_OUTPUT_VALUE_SATOSHI, MIN_CHAN_DUST_LIMIT_SATOSHIS}; use crate::ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, HolderCommitmentTransaction, CommitmentTransaction, ChannelTransactionParameters, TrustedCommitmentTransaction, ClosingTransaction}; use crate::ln::{chan_utils, msgs, PaymentPreimage}; -use crate::sign::{WriteableEcdsaChannelSigner, InMemorySigner, ChannelSigner, EcdsaChannelSigner, SignerError}; +use crate::sign::{WriteableEcdsaChannelSigner, InMemorySigner, ChannelSigner, EcdsaChannelSigner}; use crate::prelude::*; use core::cmp; @@ -101,7 +101,7 @@ impl TestChannelSigner { /// Marks the signer's availability. /// /// When `true`, methods are forwarded to the underlying signer as normal. When `false`, some - /// methods will return `SignerError::NotAvailable`. In particular: + /// methods will return `Err`. In particular: /// /// - `get_per_commitment_point` /// - `release_commitment_secret` @@ -112,16 +112,16 @@ impl TestChannelSigner { } impl ChannelSigner for TestChannelSigner { - fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result { + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> Result { if !*self.available.lock().unwrap() { - return Err(SignerError::NotAvailable); + return Err(()); } self.inner.get_per_commitment_point(idx, secp_ctx) } - fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], SignerError> { + fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], ()> { if !*self.available.lock().unwrap() { - return Err(SignerError::NotAvailable); + return Err(()); } { let mut state = self.state.lock().unwrap(); From 07f026989c09b1b160016acdc63b856c6effa72d Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Tue, 29 Aug 2023 16:27:48 -0700 Subject: [PATCH 04/10] Introduce ChannelError::Retry --- lightning/src/ln/channel.rs | 5 ++++- lightning/src/ln/channelmanager.rs | 12 ++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b1ec2eaa77d..c56384f63cb 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -362,6 +362,7 @@ pub const MIN_THEIR_CHAN_RESERVE_SATOSHIS: u64 = 1000; /// channel_id in ChannelManager. pub(super) enum ChannelError { Ignore(String), + Retry(String), Warn(String), Close(String), } @@ -370,6 +371,7 @@ impl fmt::Debug for ChannelError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { &ChannelError::Ignore(ref e) => write!(f, "Ignore : {}", e), + &ChannelError::Retry(ref e) => write!(f, "Retry : {}", e), &ChannelError::Warn(ref e) => write!(f, "Warn : {}", e), &ChannelError::Close(ref e) => write!(f, "Close : {}", e), } @@ -380,6 +382,7 @@ impl fmt::Display for ChannelError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { &ChannelError::Ignore(ref e) => write!(f, "{}", e), + &ChannelError::Retry(ref e) => write!(f, "{}", e), &ChannelError::Warn(ref e) => write!(f, "{}", e), &ChannelError::Close(ref e) => write!(f, "{}", e), } @@ -1179,7 +1182,7 @@ impl ChannelContext where SP::Target: SignerProvider { transaction_number, &self.secp_ctx ).map_err(|_| { log_warn!(logger, "Channel signer for {} is unavailable; try again later", self.channel_id()); - ChannelError::Ignore("Channel signer is unavailble; try again later".to_owned()) + ChannelError::Retry("Channel signer is unavailble; try again later".to_owned()) })?; Ok((transaction_number, per_commitment_point)) } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 190c0a53380..8dae9700b77 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -470,7 +470,7 @@ impl MsgHandleErrInternal { log_level: Level::Warn, }, }, - ChannelError::Ignore(msg) => LightningError { + ChannelError::Ignore(msg) | ChannelError::Retry(msg) => LightningError { err: msg, action: msgs::ErrorAction::IgnoreError, }, @@ -1812,7 +1812,7 @@ macro_rules! convert_chan_err { ChannelError::Warn(msg) => { (false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), $channel_id.clone())) }, - ChannelError::Ignore(msg) => { + ChannelError::Ignore(msg) | ChannelError::Retry(msg) => { (false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), $channel_id.clone())) }, ChannelError::Close(msg) => { @@ -1828,7 +1828,7 @@ macro_rules! convert_chan_err { match $err { // We should only ever have `ChannelError::Close` when unfunded channels error. // In any case, just close the channel. - ChannelError::Warn(msg) | ChannelError::Ignore(msg) | ChannelError::Close(msg) => { + ChannelError::Warn(msg) | ChannelError::Ignore(msg) | ChannelError::Retry(msg) | ChannelError::Close(msg) => { log_error!($self.logger, "Closing unfunded channel {} due to an error: {}", &$channel_id, msg); update_maps_on_chan_removal!($self, &$channel_context); let shutdown_res = $channel_context.force_shutdown(false); @@ -5622,11 +5622,11 @@ where Some(inbound_chan) => { match inbound_chan.funding_created(msg, best_block, &self.signer_provider, &self.logger) { Ok(res) => res, - Err((inbound_chan, ChannelError::Ignore(_))) => { - // If we get an `Ignore` error then something transient went wrong. Put the channel + Err((inbound_chan, err @ ChannelError::Retry(_))) => { + // If we get an `Retry` error then something transient went wrong. Put the channel // back into the table and bail. peer_state.inbound_v1_channel_by_id.insert(msg.temporary_channel_id, inbound_chan); - return Ok(()); + return Err(MsgHandleErrInternal::from_chan_no_close(err, msg.temporary_channel_id)); }, Err((mut inbound_chan, err)) => { // We've already removed this inbound channel from the map in `PeerState` From 71e906564dcfda046d8468a8e312090bc50c7d90 Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Tue, 29 Aug 2023 18:09:21 -0700 Subject: [PATCH 05/10] Create a retry mechanism This allows us to resume a channel from where we left it suspended when the signer returned an error. --- lightning/src/ln/channelmanager.rs | 60 ++++++++++++++++++++++++++-- lightning/src/ln/functional_tests.rs | 6 +-- 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 8dae9700b77..6746c3bcc43 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -663,6 +663,12 @@ impl_writeable_tlv_based_enum!(RAAMonitorUpdateBlockingAction, (0, ForwardedPaymentInboundClaim) => { (0, channel_id, required), (2, htlc_id, required) } ;); +pub(super) enum ChannelRetryState { + FundingCreated(msgs::FundingCreated), + FundingSigned(msgs::FundingSigned), + CommitmentSigned(msgs::CommitmentSigned), +} + /// State we hold per-peer. pub(super) struct PeerState where SP::Target: SignerProvider { @@ -689,6 +695,8 @@ pub(super) struct PeerState where SP::Target: SignerProvider { /// removed, and an InboundV1Channel is created and placed in the `inbound_v1_channel_by_id` table. If /// the channel is rejected, then the entry is simply removed. pub(super) inbound_channel_request_by_id: HashMap, + /// Messages that we attempted to process but returned `ChannelError::Retry`. + pub(super) retry_state_by_id: HashMap, /// The latest `InitFeatures` we heard from the peer. latest_features: InitFeatures, /// Messages to send to the peer - pushed to in the same lock that they are generated in (except @@ -5626,6 +5634,7 @@ where // If we get an `Retry` error then something transient went wrong. Put the channel // back into the table and bail. peer_state.inbound_v1_channel_by_id.insert(msg.temporary_channel_id, inbound_chan); + peer_state.retry_state_by_id.insert(msg.temporary_channel_id, ChannelRetryState::FundingCreated(msg.clone())); return Err(MsgHandleErrInternal::from_chan_no_close(err, msg.temporary_channel_id)); }, Err((mut inbound_chan, err)) => { @@ -5704,8 +5713,13 @@ where let peer_state = &mut *peer_state_lock; match peer_state.channel_by_id.entry(msg.channel_id) { hash_map::Entry::Occupied(mut chan) => { - let monitor = try_chan_entry!(self, - chan.get_mut().funding_signed(&msg, best_block, &self.signer_provider, &self.logger), chan); + let res = chan.get_mut().funding_signed(&msg, best_block, &self.signer_provider, &self.logger); + if let Err(ref err) = &res { + if let ChannelError::Retry(_) = err { + peer_state.retry_state_by_id.insert(msg.channel_id, ChannelRetryState::FundingSigned(msg.clone())); + } + } + let monitor = try_chan_entry!(self, res, chan); let update_res = self.chain_monitor.watch_channel(chan.get().context.get_funding_txo().unwrap(), monitor); let mut res = handle_new_monitor_update!(self, update_res, peer_state_lock, peer_state, per_peer_state, chan, INITIAL_MONITOR); if let Err(MsgHandleErrInternal { ref mut shutdown_finish, .. }) = res { @@ -6014,7 +6028,13 @@ where match peer_state.channel_by_id.entry(msg.channel_id) { hash_map::Entry::Occupied(mut chan) => { let funding_txo = chan.get().context.get_funding_txo(); - let monitor_update_opt = try_chan_entry!(self, chan.get_mut().commitment_signed(&msg, &self.logger), chan); + let res = chan.get_mut().commitment_signed(&msg, &self.logger); + if let Err(ref err) = &res { + if let ChannelError::Retry(_) = err { + peer_state.retry_state_by_id.insert(msg.channel_id, ChannelRetryState::CommitmentSigned(msg.clone())); + } + } + let monitor_update_opt = try_chan_entry!(self, res, chan); 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).map(|_| ()) @@ -6836,6 +6856,38 @@ where let mut ev; process_events_body!(self, ev, { handler(ev).await }); } + + /// Resumes processing for a channel awaiting a signature. + /// + /// If the `ChannelSigner` for a channel has previously returned an `Err`, then activity on the + /// channel will have been suspended pending the required signature becoming available. This + /// resumes the channel's operation. + pub fn retry_channel(&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey) -> Result<(), APIError> { + let retry_state = { + let per_peer_state = self.per_peer_state.read().unwrap(); + let peer_state_mutex = per_peer_state.get(counterparty_node_id) + .ok_or(APIError::APIMisuseError { + err: format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id) + })?; + + let mut peer_state_lock = peer_state_mutex.lock().unwrap(); + let peer_state = &mut *peer_state_lock; + match peer_state.retry_state_by_id.remove(channel_id) { + Some(retry_state) => Ok(retry_state), + None => Err(APIError::APIMisuseError { + err: format!("Can't find a channel matching the passed channel_id {}", channel_id) + }), + } + }?; + + match retry_state { + ChannelRetryState::FundingCreated(ref msg) => self.handle_funding_created(counterparty_node_id, msg), + ChannelRetryState::FundingSigned(ref msg) => self.handle_funding_signed(counterparty_node_id, msg), + ChannelRetryState::CommitmentSigned(ref msg) => self.handle_commitment_signed(counterparty_node_id, msg), + }; + + Ok(()) + } } impl MessageSendEventsProvider for ChannelManager @@ -7510,6 +7562,7 @@ where outbound_v1_channel_by_id: HashMap::new(), inbound_v1_channel_by_id: HashMap::new(), inbound_channel_request_by_id: HashMap::new(), + retry_state_by_id: HashMap::new(), latest_features: init_msg.features.clone(), pending_msg_events: Vec::new(), in_flight_monitor_updates: BTreeMap::new(), @@ -8758,6 +8811,7 @@ where outbound_v1_channel_by_id: HashMap::new(), inbound_v1_channel_by_id: HashMap::new(), inbound_channel_request_by_id: HashMap::new(), + retry_state_by_id: HashMap::new(), latest_features: InitFeatures::empty(), pending_msg_events: Vec::new(), in_flight_monitor_updates: BTreeMap::new(), diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 1d8eec6f72d..03179f9d5a6 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -9019,7 +9019,7 @@ fn test_signer_gpcp_unavailable_for_funding_signed() { // Now make it available and verify that we can process the message. nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, true); - nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed_msg); + nodes[0].node.retry_channel(&chan_id, &nodes[1].node.get_our_node_id()).expect("retry_channel failed"); check_added_monitors(&nodes[0], 1); expect_channel_pending_event(&nodes[0], &nodes[1].node.get_our_node_id()); @@ -9068,7 +9068,7 @@ fn test_signer_gpcp_unavailable_for_funding_created() { // Now make it available and verify that we can process the message. nodes[1].set_channel_signer_available(&nodes[0].node.get_our_node_id(), &temporary_channel_id, true); - nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg); + nodes[1].node.retry_channel(&temporary_channel_id, &nodes[0].node.get_our_node_id()).expect("retry_channel failed"); check_added_monitors(&nodes[1], 1); let funding_signed_msg = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()); @@ -9161,7 +9161,7 @@ fn test_dest_signer_gpcp_unavailable_for_commitment_signed() { // Mark dst's signer as available and re-handle commitment_signed. We expect to see both the RAA // and the CS. dst.set_channel_signer_available(&src.node.get_our_node_id(), &chan_id, true); - dst.node.handle_commitment_signed(&src.node.get_our_node_id(), &payment_event.commitment_msg); + dst.node.retry_channel(&chan_id, &src.node.get_our_node_id()).expect("retry_channel failed"); get_revoke_commit_msgs(dst, &src.node.get_our_node_id()); check_added_monitors!(dst, 1); } From a3064fdd3e892ae5f1d2a38a80a59e31726d1066 Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Thu, 31 Aug 2023 07:27:39 -0700 Subject: [PATCH 06/10] Handle signer unavailable during inbound channel creation When accepting a new inbound channel, we need to acquire the first per-commitment point. If the signer is not immediately available to do so, then unwind and allow for retry later. This changes the next_per_commitment_point to be an Option which will only be None while waiting for the first per-commitment point. --- lightning/src/ln/channel.rs | 47 ++++++++++++++-------- lightning/src/ln/channelmanager.rs | 63 +++++++++++++++++++++++++++--- 2 files changed, 89 insertions(+), 21 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index c56384f63cb..8bbdea5c246 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -679,7 +679,10 @@ pub(super) struct ChannelContext where SP::Target: SignerProvider { // that might be fallible: in particular, if the commitment point must be fetched from a remote // source, we want to ensure it happens at a point where we can actually fail somewhat gracefully; // i.e., force-closing a channel is better than a panic! - next_per_commitment_point: PublicKey, + // + // On initial channel construction, this value may be None, in which case that means that the + // first commitment point wasn't ready at the time that the channel needed to be created. + next_per_commitment_point: Option, cur_counterparty_commitment_transaction_number: u64, value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees pending_inbound_htlcs: Vec, @@ -1187,6 +1190,10 @@ impl ChannelContext where SP::Target: SignerProvider { Ok((transaction_number, per_commitment_point)) } + pub fn has_first_holder_per_commitment_point(&self) -> bool { + self.next_per_commitment_point.is_some() + } + /// Tracks the number of ticks elapsed since the previous [`ChannelConfig`] was updated. Once /// [`EXPIRE_PREV_CONFIG_TICKS`] is reached, the previous config is considered expired and will /// no longer be considered when forwarding HTLCs. @@ -1488,7 +1495,7 @@ impl ChannelContext where SP::Target: SignerProvider { let counterparty_pubkeys = self.get_counterparty_pubkeys(); TxCreationKeys::derive_new( - &self.secp_ctx, &self.next_per_commitment_point, delayed_payment_base, htlc_basepoint, + &self.secp_ctx, &self.next_per_commitment_point.expect("channel isn't ready"), delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint) } @@ -2592,7 +2599,7 @@ impl Channel where assert_eq!(self.context.channel_state & (ChannelState::MonitorUpdateInProgress as u32), 0); // We have no had any monitor(s) yet to fail update! self.context.channel_state = ChannelState::FundingSent as u32; self.context.cur_holder_commitment_transaction_number = next_holder_commitment_transaction_number; - self.context.next_per_commitment_point = next_per_commitment_point; + self.context.next_per_commitment_point = Some(next_per_commitment_point); self.context.cur_counterparty_commitment_transaction_number -= 1; log_info!(logger, "Received funding_signed from peer for channel {}", &self.context.channel_id()); @@ -3082,7 +3089,7 @@ impl Channel where }; self.context.cur_holder_commitment_transaction_number = next_holder_commitment_transaction_number; - self.context.next_per_commitment_point = next_per_commitment_point; + self.context.next_per_commitment_point = Some(next_per_commitment_point); // Note that if we need_commitment & !AwaitingRemoteRevoke we'll call // build_commitment_no_status_check() next which will reset this to RAAFirst. @@ -3745,7 +3752,7 @@ impl Channel where self.context.monitor_pending_channel_ready = false; Some(msgs::ChannelReady { channel_id: self.context.channel_id(), - next_per_commitment_point: self.context.next_per_commitment_point, + next_per_commitment_point: self.context.next_per_commitment_point.expect("channel isn't ready"), short_channel_id_alias: Some(self.context.outbound_scid_alias), }) } else { None }; @@ -3830,7 +3837,7 @@ impl Channel where msgs::RevokeAndACK { channel_id: self.context.channel_id, per_commitment_secret, - next_per_commitment_point: self.context.next_per_commitment_point, + next_per_commitment_point: self.context.next_per_commitment_point.expect("channel isn't ready"), #[cfg(taproot)] next_local_nonce: None, } @@ -4004,7 +4011,7 @@ impl Channel where return Ok(ReestablishResponses { channel_ready: Some(msgs::ChannelReady { channel_id: self.context.channel_id(), - next_per_commitment_point: self.context.next_per_commitment_point, + next_per_commitment_point: self.context.next_per_commitment_point.expect("channel isn't ready"), short_channel_id_alias: Some(self.context.outbound_scid_alias), }), raa: None, commitment_update: None, @@ -4042,7 +4049,7 @@ impl Channel where // We should never have to worry about MonitorUpdateInProgress resending ChannelReady Some(msgs::ChannelReady { channel_id: self.context.channel_id(), - next_per_commitment_point: self.context.next_per_commitment_point, + next_per_commitment_point: self.context.next_per_commitment_point.expect("channel isn't ready"), short_channel_id_alias: Some(self.context.outbound_scid_alias), }) } else { None }; @@ -5691,8 +5698,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { let temporary_channel_id = ChannelId::temporary_from_entropy_source(entropy_source); - let next_per_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) - .map_err(|_| APIError::ChannelUnavailable { err: "Unable to generate initial commitment point".to_owned()})?; + let next_per_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx).ok(); Ok(Self { context: ChannelContext { @@ -5983,7 +5989,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { payment_point: keys.payment_point, delayed_payment_basepoint: keys.delayed_payment_basepoint, htlc_basepoint: keys.htlc_basepoint, - first_per_commitment_point: self.context.next_per_commitment_point, + first_per_commitment_point: self.context.next_per_commitment_point.expect("channel isn't ready"), channel_flags: if self.context.config.announced_channel {1} else {0}, shutdown_scriptpubkey: Some(match &self.context.shutdown_scriptpubkey { Some(script) => script.clone().into_inner(), @@ -6332,8 +6338,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { } else { Some(cmp::max(config.channel_handshake_config.minimum_depth, 1)) }; - let next_per_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) - .map_err(|_| ChannelError::Close("Unable to generate initial commitment point".to_owned()))?; + let next_per_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx).ok(); let chan = Self { context: ChannelContext { @@ -6469,6 +6474,16 @@ impl InboundV1Channel where SP::Target: SignerProvider { Ok(chan) } + pub fn set_first_holder_per_commitment_point(&mut self) -> Result<(), ChannelError> { + assert!(self.context.next_per_commitment_point.is_none()); + let transaction_number = self.context.cur_holder_commitment_transaction_number; + assert_eq!(transaction_number, INITIAL_COMMITMENT_NUMBER); + let point = self.context.holder_signer.as_ref().get_per_commitment_point(transaction_number, &self.context.secp_ctx) + .map_err(|_| ChannelError::Retry("Channel signer is unavailable; try again later".to_owned()))?; + self.context.next_per_commitment_point = Some(point); + Ok(()) + } + /// Marks an inbound channel as accepted and generates a [`msgs::AcceptChannel`] message which /// should be sent back to the counterparty node. /// @@ -6509,7 +6524,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { payment_point: keys.payment_point, delayed_payment_basepoint: keys.delayed_payment_basepoint, htlc_basepoint: keys.htlc_basepoint, - first_per_commitment_point: self.context.next_per_commitment_point, + first_per_commitment_point: self.context.next_per_commitment_point.expect("channel isn't ready"), shutdown_scriptpubkey: Some(match &self.context.shutdown_scriptpubkey { Some(script) => script.clone().into_inner(), None => Builder::new().into_script(), @@ -6654,7 +6669,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { self.context.channel_id = funding_txo.to_channel_id(); self.context.cur_counterparty_commitment_transaction_number -= 1; self.context.cur_holder_commitment_transaction_number = next_holder_commitment_transaction_number; - self.context.next_per_commitment_point = next_per_commitment_point; + self.context.next_per_commitment_point = Some(next_per_commitment_point); // Promote the channel to a full-fledged one now that we have updated the state and have a // `ChannelMonitor`. @@ -7411,7 +7426,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch // If we weren't able to load the next_per_commitment_point, ask the signer for it now. let next_per_commitment_point = holder_signer.get_per_commitment_point( cur_holder_commitment_transaction_number, &secp_ctx - ).map_err(|_| DecodeError::Io(io::ErrorKind::Other))?; + ).map(|point| Some(point)).map_err(|_| DecodeError::Io(io::ErrorKind::Other))?; // `user_id` used to be a single u64 value. In order to remain backwards // compatible with versions prior to 0.0.113, the u128 is serialized as two diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 6746c3bcc43..4f3e674d47b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -667,6 +667,7 @@ pub(super) enum ChannelRetryState { FundingCreated(msgs::FundingCreated), FundingSigned(msgs::FundingSigned), CommitmentSigned(msgs::CommitmentSigned), + CompleteAcceptingInboundV1Channel(), } @@ -5431,16 +5432,66 @@ where let outbound_scid_alias = self.create_and_insert_outbound_scid_alias(); channel.context.set_outbound_scid_alias(outbound_scid_alias); - peer_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel { - node_id: channel.context.get_counterparty_node_id(), - msg: channel.accept_inbound_channel(), - }); + // If the channel context has the first commitment point, then we can immediately complete the + // channel acceptance by sending the accept_channel message. Otherwise, we'll need to wait for + // the signer to assign us the first commitment point. + if channel.context.has_first_holder_per_commitment_point() { + peer_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel { + node_id: channel.context.get_counterparty_node_id(), + msg: channel.accept_inbound_channel(), + }); + } else { + log_info!(self.logger, "First per-commitment point is not available for {temporary_channel_id}, scheduling retry."); + peer_state.retry_state_by_id.insert(*temporary_channel_id, ChannelRetryState::CompleteAcceptingInboundV1Channel()); + } peer_state.inbound_v1_channel_by_id.insert(temporary_channel_id.clone(), channel); - Ok(()) } + /// Performs `f` with the mutable peer state for the specified counterparty node. + /// + /// Returns `None` if no such counterparty node exists; otherwise, returns the result of `f` + /// wrapped in `Some`. + fn with_mut_peer_state(&self, counterparty_node_id: &PublicKey, f: Fn) -> Option + where Fn: FnOnce(&mut PeerState) -> Out + { + let per_peer_state = self.per_peer_state.read().unwrap(); + let peer_state_mutex = per_peer_state.get(counterparty_node_id)?; + let mut peer_state_lock = peer_state_mutex.lock().unwrap(); + Some(f(&mut *peer_state_lock)) + } + + /// Retries an `InboundV1Channel` accept. + /// + /// Attempts to resolve the first per-commitment point, and if successful broadcasts the + /// `accept_channel` message to the peer. + fn retry_accepting_inbound_v1_channel(&self, counterparty_node_id: &PublicKey, temporary_channel_id: &ChannelId) { + log_info!(self.logger, "Retrying accept for channel {temporary_channel_id}"); + self.with_mut_peer_state(counterparty_node_id, |peer_state: &mut PeerState| { + let channel_opt = peer_state.inbound_v1_channel_by_id.get_mut(temporary_channel_id); + if channel_opt.is_none() { + log_error!(self.logger, "Cannot find channel with temporary ID {temporary_channel_id} for which to complete accepting"); + return; + } + + log_info!(self.logger, "Found channel with temporary ID {temporary_channel_id}"); + + let channel = channel_opt.unwrap(); + if channel.set_first_holder_per_commitment_point().is_err() { + log_error!(self.logger, "Commitment point for channel with temporary ID {temporary_channel_id} was not available on retry"); + return; + } + + log_info!(self.logger, "Set first per-commitment point for channel {temporary_channel_id}"); + + peer_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel { + node_id: channel.context.get_counterparty_node_id(), + msg: channel.accept_inbound_channel(), + }); + }); + } + /// Gets the number of peers which match the given filter and do not have any funded, outbound, /// or 0-conf channels. /// @@ -6863,6 +6914,7 @@ where /// channel will have been suspended pending the required signature becoming available. This /// resumes the channel's operation. pub fn retry_channel(&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey) -> Result<(), APIError> { + log_info!(self.logger, "Running retry_channel for {channel_id} on {counterparty_node_id}"); let retry_state = { let per_peer_state = self.per_peer_state.read().unwrap(); let peer_state_mutex = per_peer_state.get(counterparty_node_id) @@ -6884,6 +6936,7 @@ where ChannelRetryState::FundingCreated(ref msg) => self.handle_funding_created(counterparty_node_id, msg), ChannelRetryState::FundingSigned(ref msg) => self.handle_funding_signed(counterparty_node_id, msg), ChannelRetryState::CommitmentSigned(ref msg) => self.handle_commitment_signed(counterparty_node_id, msg), + ChannelRetryState::CompleteAcceptingInboundV1Channel() => self.retry_accepting_inbound_v1_channel(counterparty_node_id, channel_id), }; Ok(()) From 2dd3a3e37315f0bce38bfef250b04da3aa37e3a0 Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Thu, 31 Aug 2023 14:42:00 -0700 Subject: [PATCH 07/10] Handle signer unavailable during channel reestablish As elsewhere, when reestablishing a channel we need to get a commitment point. Here, we need an _arbitrary_ point, so refactored code in channel context appropriately. --- lightning/src/ln/channel.rs | 25 +++++++++++++++++-------- lightning/src/ln/channelmanager.rs | 12 ++++++++++-- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8bbdea5c246..6a9e1580754 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1171,14 +1171,12 @@ impl ChannelContext where SP::Target: SignerProvider { return &self.holder_signer } - /// Retrieves the next commitment point and its index from the signer. + /// Retrieves a specific commitment point for an index. /// - /// This maps an `Err` into `ChannelError::Ignore`. Note that this does _not_ advance the - /// context's state. - pub fn get_next_holder_per_commitment_point(&self, logger: &L) -> Result<(u64, PublicKey), ChannelError> + /// This maps an `Err` into `ChannelError::Retry`. + pub fn get_holder_per_commitment_point_for(&self, transaction_number: u64, logger: &L) -> Result where L::Target: Logger { - let transaction_number = self.cur_holder_commitment_transaction_number - 1; log_trace!(logger, "Retrieving commitment point for {} transaction number {}", self.channel_id(), transaction_number); let per_commitment_point = self.holder_signer.as_ref().get_per_commitment_point( @@ -1187,7 +1185,19 @@ impl ChannelContext where SP::Target: SignerProvider { log_warn!(logger, "Channel signer for {} is unavailable; try again later", self.channel_id()); ChannelError::Retry("Channel signer is unavailble; try again later".to_owned()) })?; - Ok((transaction_number, per_commitment_point)) + Ok(per_commitment_point) + } + + /// Retrieves the next commitment point and its index from the signer. + /// + /// This maps an `Err` into `ChannelError::Retry`. Note that this does _not_ advance the context's + /// state. + pub fn get_next_holder_per_commitment_point(&self, logger: &L) -> Result<(u64, PublicKey), ChannelError> + where L::Target: Logger + { + let transaction_number = self.cur_holder_commitment_transaction_number - 1; + log_trace!(logger, "Retrieving commitment point for {} transaction number {}", self.channel_id(), transaction_number); + self.get_holder_per_commitment_point_for(transaction_number, logger).map(|point| (transaction_number, point)) } pub fn has_first_holder_per_commitment_point(&self) -> bool { @@ -3948,8 +3958,7 @@ impl Channel where if msg.next_remote_commitment_number > 0 { let state_index = INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1; - let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(state_index, &self.context.secp_ctx) - .map_err(|_| ChannelError::Close(format!("Unable to retrieve per-commitment point for state {state_index}")))?; + let expected_point = self.context.get_holder_per_commitment_point_for(state_index, logger)?; let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret) .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 4f3e674d47b..c41c97b63ae 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -666,6 +666,7 @@ impl_writeable_tlv_based_enum!(RAAMonitorUpdateBlockingAction, pub(super) enum ChannelRetryState { FundingCreated(msgs::FundingCreated), FundingSigned(msgs::FundingSigned), + ChannelReestablish(msgs::ChannelReestablish), CommitmentSigned(msgs::CommitmentSigned), CompleteAcceptingInboundV1Channel(), } @@ -6357,9 +6358,15 @@ where // disconnect, so Channel's reestablish will never hand us any holding cell // freed HTLCs to fail backwards. If in the future we no longer drop pending // add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here. - let responses = try_chan_entry!(self, chan.get_mut().channel_reestablish( + let res = chan.get_mut().channel_reestablish( msg, &self.logger, &self.node_signer, self.genesis_hash, - &self.default_configuration, &*self.best_block.read().unwrap()), chan); + &self.default_configuration, &*self.best_block.read().unwrap()); + if let Err(ref err) = &res { + if let ChannelError::Retry(_) = err { + peer_state.retry_state_by_id.insert(msg.channel_id, ChannelRetryState::ChannelReestablish(msg.clone())); + } + } + let responses = try_chan_entry!(self, res, chan); let mut channel_update = None; if let Some(msg) = responses.shutdown_msg { peer_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown { @@ -6935,6 +6942,7 @@ where match retry_state { ChannelRetryState::FundingCreated(ref msg) => self.handle_funding_created(counterparty_node_id, msg), ChannelRetryState::FundingSigned(ref msg) => self.handle_funding_signed(counterparty_node_id, msg), + ChannelRetryState::ChannelReestablish(ref msg) => self.handle_channel_reestablish(counterparty_node_id, msg), ChannelRetryState::CommitmentSigned(ref msg) => self.handle_commitment_signed(counterparty_node_id, msg), ChannelRetryState::CompleteAcceptingInboundV1Channel() => self.retry_accepting_inbound_v1_channel(counterparty_node_id, channel_id), }; From e2f054d64a003a691750c490e7a20c762ada6617 Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Fri, 1 Sep 2023 10:05:25 -0700 Subject: [PATCH 08/10] Fix misspelling --- lightning/src/ln/channel.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 6a9e1580754..8b69e637ee1 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1183,7 +1183,7 @@ impl ChannelContext where SP::Target: SignerProvider { transaction_number, &self.secp_ctx ).map_err(|_| { log_warn!(logger, "Channel signer for {} is unavailable; try again later", self.channel_id()); - ChannelError::Retry("Channel signer is unavailble; try again later".to_owned()) + ChannelError::Retry("Channel signer is unavailable; try again later".to_owned()) })?; Ok(per_commitment_point) } From b02b39f5689a220679edd66f68a26da8b9e3bc63 Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Fri, 1 Sep 2023 10:11:28 -0700 Subject: [PATCH 09/10] Allow release_commitment_secret to be fallible Rather than assume that we can release the commitment secret arbitrarily, this releases the secret eagerly: as soon as the counterparty has commited to the new state. The secret is then stored as part of the channel context and can be accessed without a subsequent API request as needed. Also adds support for serializing and deserializing the previous commitment secret and next per-commitment point with the channel state. --- lightning/src/ln/channel.rs | 66 +++++++++++++++++++---- lightning/src/util/test_channel_signer.rs | 2 +- 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8b69e637ee1..a761c94e652 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -683,6 +683,9 @@ pub(super) struct ChannelContext where SP::Target: SignerProvider { // On initial channel construction, this value may be None, in which case that means that the // first commitment point wasn't ready at the time that the channel needed to be created. next_per_commitment_point: Option, + // The secret corresponding to `cur_holder_commitment_transaction_number + 2`, which is the + // *previous* state. + prev_commitment_secret: Option<[u8; 32]>, cur_counterparty_commitment_transaction_number: u64, value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees pending_inbound_htlcs: Vec, @@ -1200,6 +1203,20 @@ impl ChannelContext where SP::Target: SignerProvider { self.get_holder_per_commitment_point_for(transaction_number, logger).map(|point| (transaction_number, point)) } + /// Retrieves the previous commitment secret from the signer. + pub fn get_prev_commitment_secret(&self, logger: &L) -> Result<[u8; 32], ChannelError> + where L::Target: Logger + { + let transaction_number = self.cur_holder_commitment_transaction_number + 1; + assert!(transaction_number <= INITIAL_COMMITMENT_NUMBER); + log_trace!(logger, "Requesting commitment secret for {} transaction number {}", self.channel_id(), transaction_number); + let secret = self.holder_signer.as_ref().release_commitment_secret(transaction_number).map_err(|_| { + log_warn!(logger, "Channel signer for {} is unavailable; try again later", self.channel_id()); + ChannelError::Retry("Channel signer is unavailable; try again later".to_owned()) + })?; + Ok(secret) + } + pub fn has_first_holder_per_commitment_point(&self) -> bool { self.next_per_commitment_point.is_some() } @@ -3041,9 +3058,10 @@ impl Channel where self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.preimages) .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?; - // Retrieve the next commitment point: if this results in a transient failure we'll unwind here - // and rely on retry to complete the commitment_operation. + // Retrieve the next commitment point and previous commitment secret: if this results in a + // transient failure we'll unwind here and rely on retry to complete the commitment_operation. let (next_holder_commitment_transaction_number, next_per_commitment_point) = self.context.get_next_holder_per_commitment_point(logger)?; + let prev_commitment_secret = self.context.get_prev_commitment_secret(logger)?; // Update state now that we've passed all the can-fail calls... let mut need_commitment = false; @@ -3100,6 +3118,7 @@ impl Channel where self.context.cur_holder_commitment_transaction_number = next_holder_commitment_transaction_number; self.context.next_per_commitment_point = Some(next_per_commitment_point); + self.context.prev_commitment_secret = Some(prev_commitment_secret); // Note that if we need_commitment & !AwaitingRemoteRevoke we'll call // build_commitment_no_status_check() next which will reset this to RAAFirst. @@ -3841,13 +3860,12 @@ impl Channel where } fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK { - // TODO(waterson): fallible! - let per_commitment_secret = self.context.holder_signer.as_ref().release_commitment_secret(self.context.cur_holder_commitment_transaction_number + 2) - .expect("release_per_commitment failed"); + let per_commitment_secret = self.context.prev_commitment_secret.expect("missing prev_commitment_secret"); + let next_per_commitment_point = self.context.next_per_commitment_point.expect("channel isn't ready"); msgs::RevokeAndACK { channel_id: self.context.channel_id, per_commitment_secret, - next_per_commitment_point: self.context.next_per_commitment_point.expect("channel isn't ready"), + next_per_commitment_point, #[cfg(taproot)] next_local_nonce: None, } @@ -5738,6 +5756,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER, next_per_commitment_point, + prev_commitment_secret: None, cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER, value_to_self_msat, @@ -6377,6 +6396,7 @@ impl InboundV1Channel where SP::Target: SignerProvider { cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER, next_per_commitment_point, + prev_commitment_secret: None, cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER, value_to_self_msat: msg.push_msat, @@ -7073,6 +7093,8 @@ impl Writeable for Channel where SP::Target: SignerProvider { (31, channel_pending_event_emitted, option), (35, pending_outbound_skimmed_fees, optional_vec), (37, holding_cell_skimmed_fees, optional_vec), + (39, self.context.next_per_commitment_point, option), + (41, self.context.prev_commitment_secret, option), }); Ok(()) @@ -7355,6 +7377,9 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch let mut pending_outbound_skimmed_fees_opt: Option>> = None; let mut holding_cell_skimmed_fees_opt: Option>> = None; + + let mut next_per_commitment_point: Option = None; + let mut prev_commitment_secret: Option<[u8; 32]> = None; read_tlv_fields!(reader, { (0, announcement_sigs, option), @@ -7381,6 +7406,8 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch (31, channel_pending_event_emitted, option), (35, pending_outbound_skimmed_fees_opt, optional_vec), (37, holding_cell_skimmed_fees_opt, optional_vec), + (39, next_per_commitment_point, option), + (41, prev_commitment_secret, option), }); let (channel_keys_id, holder_signer) = if let Some(channel_keys_id) = channel_keys_id { @@ -7432,11 +7459,27 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch let mut secp_ctx = Secp256k1::new(); secp_ctx.seeded_randomize(&entropy_source.get_secure_random_bytes()); - // If we weren't able to load the next_per_commitment_point, ask the signer for it now. - let next_per_commitment_point = holder_signer.get_per_commitment_point( - cur_holder_commitment_transaction_number, &secp_ctx - ).map(|point| Some(point)).map_err(|_| DecodeError::Io(io::ErrorKind::Other))?; - + // If we weren't able to load the next_per_commitment_point or prev_commitment_secret, ask the + // signer for them now: this can happen when restoring a pre-v11x channel that did not have + // these serialized. + // + // N.B. that this can also happen if a post-v11x channel was serialized with an asynchronous + // remote signer. Specifically, if the channel was created but had not yet received its first + // commitment point, a None value will have been serialized. Given that the channel is so + // nascent and isn't fully open or accepted, we'll simply fail here and let the user retry the + // open. + if next_per_commitment_point.is_none() { + next_per_commitment_point = Some( + holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number, &secp_ctx) + .map_err(|_| DecodeError::Io(io::ErrorKind::Other))?); + } + + if prev_commitment_secret.is_none() && cur_holder_commitment_transaction_number <= INITIAL_COMMITMENT_NUMBER - 2 { + prev_commitment_secret = Some( + holder_signer.release_commitment_secret(cur_holder_commitment_transaction_number + 2) + .map_err(|_| DecodeError::Io(io::ErrorKind::Other))?); + } + // `user_id` used to be a single u64 value. In order to remain backwards // compatible with versions prior to 0.0.113, the u128 is serialized as two // separate u64 values. @@ -7490,6 +7533,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch cur_holder_commitment_transaction_number, next_per_commitment_point, + prev_commitment_secret, cur_counterparty_commitment_transaction_number, value_to_self_msat, diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index b4f045412e7..a25d3e99f7d 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -183,7 +183,7 @@ impl EcdsaChannelSigner for TestChannelSigner { let state = self.state.lock().unwrap(); let commitment_number = trusted_tx.commitment_number(); - if state.last_holder_revoked_commitment - 1 != commitment_number && state.last_holder_revoked_commitment - 2 != commitment_number { + if state.last_holder_revoked_commitment != commitment_number && state.last_holder_revoked_commitment - 1 != commitment_number { if !self.disable_revocation_policy_check { panic!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}", state.last_holder_revoked_commitment, commitment_number, self.inner.commitment_seed[0]) From b13e4e8fcaee52d8c6c178a41c4000db0d8023d3 Mon Sep 17 00:00:00 2001 From: Chris Waterson Date: Tue, 5 Sep 2023 15:29:57 -0700 Subject: [PATCH 10/10] Handle outbound channel creation, too. --- lightning/src/ln/channel.rs | 11 ++++++++ lightning/src/ln/channelmanager.rs | 43 ++++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index a761c94e652..62e025ca249 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5857,6 +5857,17 @@ impl OutboundV1Channel where SP::Target: SignerProvider { }) } + /// Sets the first holder per-commitment point by retrieving it from the signer. + pub fn set_first_holder_per_commitment_point(&mut self) -> Result<(), ChannelError> { + assert!(self.context.next_per_commitment_point.is_none()); + let transaction_number = self.context.cur_holder_commitment_transaction_number; + assert_eq!(transaction_number, INITIAL_COMMITMENT_NUMBER); + let point = self.context.holder_signer.as_ref().get_per_commitment_point(transaction_number, &self.context.secp_ctx) + .map_err(|_| ChannelError::Retry("Channel signer is unavailable; try again later".to_owned()))?; + self.context.next_per_commitment_point = Some(point); + Ok(()) + } + /// 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 { let counterparty_keys = self.context.build_remote_transaction_keys(); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index c41c97b63ae..3323fd6016d 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -669,6 +669,7 @@ pub(super) enum ChannelRetryState { ChannelReestablish(msgs::ChannelReestablish), CommitmentSigned(msgs::CommitmentSigned), CompleteAcceptingInboundV1Channel(), + CompleteCreatingOutboundV1Channel(), } @@ -2296,9 +2297,18 @@ where }, } }; - let res = channel.get_open_channel(self.genesis_hash.clone()); let temporary_channel_id = channel.context.channel_id(); + if channel.context.has_first_holder_per_commitment_point() { + peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel { + node_id: their_network_key, + msg: channel.get_open_channel(self.genesis_hash.clone()), + }); + } else { + log_info!(self.logger, "First per-commitment point is not available for {temporary_channel_id}, scheduling retry."); + peer_state.retry_state_by_id.insert(temporary_channel_id.clone(), ChannelRetryState::CompleteCreatingOutboundV1Channel()); + } + match peer_state.outbound_v1_channel_by_id.entry(temporary_channel_id) { hash_map::Entry::Occupied(_) => { if cfg!(fuzzing) { @@ -2310,13 +2320,35 @@ where hash_map::Entry::Vacant(entry) => { entry.insert(channel); } } - peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel { - node_id: their_network_key, - msg: res, - }); Ok(temporary_channel_id) } + fn complete_creating_outbound_v1_channel(&self, counterparty_node_id: &PublicKey, temporary_channel_id: &ChannelId) { + log_info!(self.logger, "Retrying create for channel {temporary_channel_id}"); + self.with_mut_peer_state(counterparty_node_id, |peer_state: &mut PeerState| { + let channel_opt = peer_state.outbound_v1_channel_by_id.get_mut(temporary_channel_id); + if channel_opt.is_none() { + log_error!(self.logger, "Cannot find outbound channel with temporary ID {temporary_channel_id} for which to complete creation"); + return; + } + + log_info!(self.logger, "Found outbound channel with temporary ID {temporary_channel_id}"); + + let channel = channel_opt.unwrap(); + if channel.set_first_holder_per_commitment_point().is_err() { + log_error!(self.logger, "Commitment point for outbound channel with temporary ID {temporary_channel_id} was not available on retry"); + return; + } + + log_info!(self.logger, "Set first per-commitment point for outbound channel {temporary_channel_id}"); + + peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel { + node_id: *counterparty_node_id, + msg: channel.get_open_channel(self.genesis_hash.clone()), + }); + }); + } + fn list_funded_channels_with_filter)) -> bool + Copy>(&self, f: Fn) -> Vec { // Allocate our best estimate of the number of channels we have in the `res` // Vec. Sadly the `short_to_chan_info` map doesn't cover channels without @@ -6945,6 +6977,7 @@ where ChannelRetryState::ChannelReestablish(ref msg) => self.handle_channel_reestablish(counterparty_node_id, msg), ChannelRetryState::CommitmentSigned(ref msg) => self.handle_commitment_signed(counterparty_node_id, msg), ChannelRetryState::CompleteAcceptingInboundV1Channel() => self.retry_accepting_inbound_v1_channel(counterparty_node_id, channel_id), + ChannelRetryState::CompleteCreatingOutboundV1Channel() => self.complete_creating_outbound_v1_channel(counterparty_node_id, channel_id), }; Ok(())