diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 0e8a027897d..015b3dacfc3 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -635,6 +635,27 @@ where C::Target: chain::Filter, ) } } + + /// Triggers rebroadcasts of pending claims from force-closed channels after a transaction + /// signature generation failure. + /// + /// `monitor_opt` can be used as a filter to only trigger them for a specific channel monitor. + pub fn signer_unblocked(&self, monitor_opt: Option) { + let monitors = self.monitors.read().unwrap(); + if let Some(funding_txo) = monitor_opt { + if let Some(monitor_holder) = monitors.get(&funding_txo) { + monitor_holder.monitor.signer_unblocked( + &*self.broadcaster, &*self.fee_estimator, &self.logger + ) + } + } else { + for (_, monitor_holder) in &*monitors { + monitor_holder.monitor.signer_unblocked( + &*self.broadcaster, &*self.fee_estimator, &self.logger + ) + } + } + } } impl diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index cdbec81abf5..75271637d3c 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -44,7 +44,7 @@ use crate::chain::{BestBlock, WatchedOutput}; use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator}; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, ecdsa::WriteableEcdsaChannelSigner, SignerProvider, EntropySource}; -use crate::chain::onchaintx::{ClaimEvent, OnchainTxHandler}; +use crate::chain::onchaintx::{ClaimEvent, FeerateStrategy, OnchainTxHandler}; use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput}; use crate::chain::Filter; use crate::util::logger::{Logger, Record}; @@ -1562,28 +1562,30 @@ impl ChannelMonitor { self.inner.lock().unwrap().counterparty_node_id } - /// Used by [`ChannelManager`] deserialization to broadcast the latest holder state if its copy - /// of the channel state was out-of-date. - /// - /// You may also use this to broadcast the latest local commitment transaction, either because + /// You may use this to broadcast the latest local commitment transaction, either because /// a monitor update failed or because we've fallen behind (i.e. we've received proof that our /// counterparty side knows a revocation secret we gave them that they shouldn't know). /// - /// Broadcasting these transactions in the second case is UNSAFE, as they allow counterparty + /// Broadcasting these transactions in this manner is UNSAFE, as they allow counterparty /// side to punish you. Nevertheless you may want to broadcast them if counterparty doesn't /// close channel with their commitment transaction after a substantial amount of time. Best /// may be to contact the other node operator out-of-band to coordinate other options available /// to you. - /// - /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager - pub fn get_latest_holder_commitment_txn(&self, logger: &L) -> Vec - where L::Target: Logger { + pub fn broadcast_latest_holder_commitment_txn( + &self, broadcaster: &B, fee_estimator: &F, logger: &L + ) + where + B::Target: BroadcasterInterface, + F::Target: FeeEstimator, + L::Target: Logger + { let mut inner = self.inner.lock().unwrap(); + let fee_estimator = LowerBoundedFeeEstimator::new(&**fee_estimator); let logger = WithChannelMonitor::from_impl(logger, &*inner); - inner.get_latest_holder_commitment_txn(&logger) + inner.queue_latest_holder_commitment_txn_for_broadcast(broadcaster, &fee_estimator, &logger); } - /// Unsafe test-only version of get_latest_holder_commitment_txn used by our test framework + /// Unsafe test-only version of `broadcast_latest_holder_commitment_txn` used by our test framework /// to bypass HolderCommitmentTransaction state update lockdown after signature and generate /// revoked commitment transaction. #[cfg(any(test, feature = "unsafe_revoked_tx_signing"))] @@ -1763,7 +1765,26 @@ impl ChannelMonitor { let logger = WithChannelMonitor::from_impl(logger, &*inner); let current_height = inner.best_block.height; inner.onchain_tx_handler.rebroadcast_pending_claims( - current_height, &broadcaster, &fee_estimator, &logger, + current_height, FeerateStrategy::HighestOfPreviousOrNew, &broadcaster, &fee_estimator, &logger, + ); + } + + /// Triggers rebroadcasts of pending claims from a force-closed channel after a transaction + /// signature generation failure. + pub fn signer_unblocked( + &self, broadcaster: B, fee_estimator: F, logger: &L, + ) + where + B::Target: BroadcasterInterface, + F::Target: FeeEstimator, + L::Target: Logger, + { + let fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator); + let mut inner = self.inner.lock().unwrap(); + let logger = WithChannelMonitor::from_impl(logger, &*inner); + let current_height = inner.best_block.height; + inner.onchain_tx_handler.rebroadcast_pending_claims( + current_height, FeerateStrategy::RetryPrevious, &broadcaster, &fee_estimator, &logger, ); } @@ -1809,6 +1830,12 @@ impl ChannelMonitor { pub fn set_counterparty_payment_script(&self, script: ScriptBuf) { self.inner.lock().unwrap().counterparty_payment_script = script; } + + #[cfg(test)] + pub fn do_signer_call ()>(&self, mut f: F) { + let inner = self.inner.lock().unwrap(); + f(&inner.onchain_tx_handler.signer); + } } impl ChannelMonitorImpl { @@ -2855,7 +2882,7 @@ impl ChannelMonitorImpl { } else if !self.holder_tx_signed { log_error!(logger, "WARNING: You have a potentially-unsafe holder commitment transaction available to broadcast"); log_error!(logger, " in channel monitor for channel {}!", &self.channel_id()); - log_error!(logger, " Read the docs for ChannelMonitor::get_latest_holder_commitment_txn and take manual action!"); + log_error!(logger, " Read the docs for ChannelMonitor::broadcast_latest_holder_commitment_txn to take manual action!"); } else { // If we generated a MonitorEvent::HolderForceClosed, the ChannelManager // will still give us a ChannelForceClosed event with !should_broadcast, but we @@ -3502,45 +3529,6 @@ impl ChannelMonitorImpl { } } - fn get_latest_holder_commitment_txn( - &mut self, logger: &WithChannelMonitor, - ) -> Vec where L::Target: Logger { - log_debug!(logger, "Getting signed latest holder commitment transaction!"); - self.holder_tx_signed = true; - let commitment_tx = self.onchain_tx_handler.get_fully_signed_holder_tx(&self.funding_redeemscript); - let txid = commitment_tx.txid(); - let mut holder_transactions = vec![commitment_tx]; - // When anchor outputs are present, the HTLC transactions are only valid once the commitment - // transaction confirms. - if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() { - return holder_transactions; - } - for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() { - if let Some(vout) = htlc.0.transaction_output_index { - let preimage = if !htlc.0.offered { - if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(preimage.clone()) } else { - // We can't build an HTLC-Success transaction without the preimage - continue; - } - } else if htlc.0.cltv_expiry > self.best_block.height() + 1 { - // Don't broadcast HTLC-Timeout transactions immediately as they don't meet the - // current locktime requirements on-chain. We will broadcast them in - // `block_confirmed` when `should_broadcast_holder_commitment_txn` returns true. - // Note that we add + 1 as transactions are broadcastable when they can be - // confirmed in the next block. - continue; - } else { None }; - if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx( - &::bitcoin::OutPoint { txid, vout }, &preimage) { - holder_transactions.push(htlc_tx); - } - } - } - // We throw away the generated waiting_first_conf data as we aren't (yet) confirmed and we don't actually know what the caller wants to do. - // The data will be re-generated and tracked in check_spend_holder_transaction if we get a confirmation. - holder_transactions - } - #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] /// Note that this includes possibly-locktimed-in-the-future transactions! fn unsafe_get_latest_holder_commitment_txn( @@ -3563,9 +3551,12 @@ impl ChannelMonitorImpl { continue; } } else { None }; - if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx( - &::bitcoin::OutPoint { txid, vout }, &preimage) { - holder_transactions.push(htlc_tx); + if let Some(htlc_tx) = self.onchain_tx_handler.get_maybe_signed_htlc_tx( + &::bitcoin::OutPoint { txid, vout }, &preimage + ) { + if htlc_tx.is_fully_signed() { + holder_transactions.push(htlc_tx.0); + } } } } diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 05d59431fc3..6c29d147f02 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -31,6 +31,7 @@ use crate::chain::ClaimId; use crate::chain::chaininterface::{ConfirmationTarget, FeeEstimator, BroadcasterInterface, LowerBoundedFeeEstimator}; use crate::chain::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER}; use crate::chain::package::{PackageSolvingData, PackageTemplate}; +use crate::chain::transaction::MaybeSignedTransaction; use crate::util::logger::Logger; use crate::util::ser::{Readable, ReadableArgs, MaybeReadable, UpgradableRequired, Writer, Writeable, VecWriter}; @@ -204,12 +205,23 @@ pub(crate) enum ClaimEvent { /// control) onchain. pub(crate) enum OnchainClaim { /// A finalized transaction pending confirmation spending the output to claim. - Tx(Transaction), + Tx(MaybeSignedTransaction), /// An event yielded externally to signal additional inputs must be added to a transaction /// pending confirmation spending the output to claim. Event(ClaimEvent), } +/// Represents the different feerate strategies a pending request can use when generating a claim. +pub(crate) enum FeerateStrategy { + /// We must reuse the most recently used feerate, if any. + RetryPrevious, + /// We must pick the highest between the most recently used and the current feerate estimate. + HighestOfPreviousOrNew, + /// We must force a bump of the most recently used feerate, either by using the current feerate + /// estimate if it's higher, or manually bumping. + ForceBump, +} + /// OnchainTxHandler receives claiming requests, aggregates them if it's sound, broadcast and /// do RBF bumping if possible. #[derive(Clone)] @@ -474,8 +486,8 @@ impl OnchainTxHandler /// invoking this every 30 seconds, or lower if running in an environment with spotty /// connections, like on mobile. pub(super) fn rebroadcast_pending_claims( - &mut self, current_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator, - logger: &L, + &mut self, current_height: u32, feerate_strategy: FeerateStrategy, broadcaster: &B, + fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) where B::Target: BroadcasterInterface, @@ -488,7 +500,7 @@ impl OnchainTxHandler bump_requests.push((*claim_id, request.clone())); } for (claim_id, request) in bump_requests { - self.generate_claim(current_height, &request, false /* force_feerate_bump */, fee_estimator, logger) + self.generate_claim(current_height, &request, &feerate_strategy, fee_estimator, logger) .map(|(_, new_feerate, claim)| { let mut bumped_feerate = false; if let Some(mut_request) = self.pending_claim_requests.get_mut(&claim_id) { @@ -497,9 +509,13 @@ impl OnchainTxHandler } match claim { OnchainClaim::Tx(tx) => { - let log_start = if bumped_feerate { "Broadcasting RBF-bumped" } else { "Rebroadcasting" }; - log_info!(logger, "{} onchain {}", log_start, log_tx!(tx)); - broadcaster.broadcast_transactions(&[&tx]); + if tx.is_fully_signed() { + let log_start = if bumped_feerate { "Broadcasting RBF-bumped" } else { "Rebroadcasting" }; + log_info!(logger, "{} onchain {}", log_start, log_tx!(tx.0)); + broadcaster.broadcast_transactions(&[&tx.0]); + } else { + log_info!(logger, "Waiting for signature of unsigned onchain transaction {}", tx.0.txid()); + } }, OnchainClaim::Event(event) => { let log_start = if bumped_feerate { "Yielding fee-bumped" } else { "Replaying" }; @@ -528,7 +544,7 @@ impl OnchainTxHandler /// Panics if there are signing errors, because signing operations in reaction to on-chain /// events are not expected to fail, and if they do, we may lose funds. fn generate_claim( - &mut self, cur_height: u32, cached_request: &PackageTemplate, force_feerate_bump: bool, + &mut self, cur_height: u32, cached_request: &PackageTemplate, feerate_strategy: &FeerateStrategy, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Option<(u32, u64, OnchainClaim)> where F::Target: FeeEstimator, @@ -577,7 +593,7 @@ impl OnchainTxHandler if cached_request.is_malleable() { if cached_request.requires_external_funding() { let target_feerate_sat_per_1000_weight = cached_request.compute_package_feerate( - fee_estimator, ConfirmationTarget::OnChainSweep, force_feerate_bump + fee_estimator, ConfirmationTarget::OnChainSweep, feerate_strategy, ); if let Some(htlcs) = cached_request.construct_malleable_package_with_external_funding(self) { return Some(( @@ -597,15 +613,14 @@ impl OnchainTxHandler let predicted_weight = cached_request.package_weight(&self.destination_script); if let Some((output_value, new_feerate)) = cached_request.compute_package_output( predicted_weight, self.destination_script.dust_value().to_sat(), - force_feerate_bump, fee_estimator, logger, + feerate_strategy, fee_estimator, logger, ) { assert!(new_feerate != 0); - let transaction = cached_request.finalize_malleable_package( + let transaction = cached_request.maybe_finalize_malleable_package( cur_height, self, output_value, self.destination_script.clone(), logger ).unwrap(); - log_trace!(logger, "...with timer {} and feerate {}", new_timer, new_feerate); - assert!(predicted_weight >= transaction.weight().to_wu()); + assert!(predicted_weight >= transaction.0.weight().to_wu()); return Some((new_timer, new_feerate, OnchainClaim::Tx(transaction))); } } else { @@ -614,7 +629,7 @@ impl OnchainTxHandler // which require external funding. let mut inputs = cached_request.inputs(); debug_assert_eq!(inputs.len(), 1); - let tx = match cached_request.finalize_untractable_package(self, logger) { + let tx = match cached_request.maybe_finalize_untractable_package(self, logger) { Some(tx) => tx, None => return None, }; @@ -625,19 +640,19 @@ impl OnchainTxHandler // Commitment inputs with anchors support are the only untractable inputs supported // thus far that require external funding. PackageSolvingData::HolderFundingOutput(output) => { - debug_assert_eq!(tx.txid(), self.holder_commitment.trust().txid(), + debug_assert_eq!(tx.0.txid(), self.holder_commitment.trust().txid(), "Holder commitment transaction mismatch"); let conf_target = ConfirmationTarget::OnChainSweep; let package_target_feerate_sat_per_1000_weight = cached_request - .compute_package_feerate(fee_estimator, conf_target, force_feerate_bump); + .compute_package_feerate(fee_estimator, conf_target, feerate_strategy); if let Some(input_amount_sat) = output.funding_amount { - let fee_sat = input_amount_sat - tx.output.iter().map(|output| output.value).sum::(); + let fee_sat = input_amount_sat - tx.0.output.iter().map(|output| output.value).sum::(); let commitment_tx_feerate_sat_per_1000_weight = - compute_feerate_sat_per_1000_weight(fee_sat, tx.weight().to_wu()); + compute_feerate_sat_per_1000_weight(fee_sat, tx.0.weight().to_wu()); if commitment_tx_feerate_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight { - log_debug!(logger, "Pre-signed {} already has feerate {} sat/kW above required {} sat/kW", - log_tx!(tx), commitment_tx_feerate_sat_per_1000_weight, + log_debug!(logger, "Pre-signed commitment {} already has feerate {} sat/kW above required {} sat/kW", + tx.0.txid(), commitment_tx_feerate_sat_per_1000_weight, package_target_feerate_sat_per_1000_weight); return Some((new_timer, 0, OnchainClaim::Tx(tx.clone()))); } @@ -645,7 +660,7 @@ impl OnchainTxHandler // We'll locate an anchor output we can spend within the commitment transaction. let funding_pubkey = &self.channel_transaction_parameters.holder_pubkeys.funding_pubkey; - match chan_utils::get_anchor_output(&tx, funding_pubkey) { + match chan_utils::get_anchor_output(&tx.0, funding_pubkey) { // An anchor output was found, so we should yield a funding event externally. Some((idx, _)) => { // TODO: Use a lower confirmation target when both our and the @@ -655,7 +670,7 @@ impl OnchainTxHandler package_target_feerate_sat_per_1000_weight as u64, OnchainClaim::Event(ClaimEvent::BumpCommitment { package_target_feerate_sat_per_1000_weight, - commitment_tx: tx.clone(), + commitment_tx: tx.0.clone(), anchor_output_idx: idx, }), )) @@ -767,7 +782,7 @@ impl OnchainTxHandler // height timer expiration (i.e in how many blocks we're going to take action). for mut req in preprocessed_requests { if let Some((new_timer, new_feerate, claim)) = self.generate_claim( - cur_height, &req, true /* force_feerate_bump */, &*fee_estimator, &*logger, + cur_height, &req, &FeerateStrategy::ForceBump, &*fee_estimator, &*logger, ) { req.set_timer(new_timer); req.set_feerate(new_feerate); @@ -776,9 +791,13 @@ impl OnchainTxHandler // `OnchainClaim`. let claim_id = match claim { OnchainClaim::Tx(tx) => { - log_info!(logger, "Broadcasting onchain {}", log_tx!(tx)); - broadcaster.broadcast_transactions(&[&tx]); - ClaimId(tx.txid().to_byte_array()) + if tx.is_fully_signed() { + log_info!(logger, "Broadcasting onchain {}", log_tx!(tx.0)); + broadcaster.broadcast_transactions(&[&tx.0]); + } else { + log_info!(logger, "Waiting for signature of unsigned onchain transaction {}", tx.0.txid()); + } + ClaimId(tx.0.txid().to_byte_array()) }, OnchainClaim::Event(claim_event) => { log_info!(logger, "Yielding onchain event to spend inputs {:?}", req.outpoints()); @@ -967,12 +986,17 @@ impl OnchainTxHandler log_trace!(logger, "Bumping {} candidates", bump_candidates.len()); for (claim_id, request) in bump_candidates.iter() { if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim( - cur_height, &request, true /* force_feerate_bump */, &*fee_estimator, &*logger, + cur_height, &request, &FeerateStrategy::ForceBump, &*fee_estimator, &*logger, ) { match bump_claim { OnchainClaim::Tx(bump_tx) => { - log_info!(logger, "Broadcasting RBF-bumped onchain {}", log_tx!(bump_tx)); - broadcaster.broadcast_transactions(&[&bump_tx]); + if bump_tx.is_fully_signed() { + log_info!(logger, "Broadcasting RBF-bumped onchain {}", log_tx!(bump_tx.0)); + broadcaster.broadcast_transactions(&[&bump_tx.0]); + } else { + log_info!(logger, "Waiting for signature of RBF-bumped unsigned onchain transaction {}", + bump_tx.0.txid()); + } }, OnchainClaim::Event(claim_event) => { log_info!(logger, "Yielding RBF-bumped onchain event to spend inputs {:?}", request.outpoints()); @@ -1048,14 +1072,18 @@ impl OnchainTxHandler // `height` is the height being disconnected, so our `current_height` is 1 lower. let current_height = height - 1; if let Some((new_timer, new_feerate, bump_claim)) = self.generate_claim( - current_height, &request, true /* force_feerate_bump */, fee_estimator, logger + current_height, &request, &FeerateStrategy::ForceBump, fee_estimator, logger ) { request.set_timer(new_timer); request.set_feerate(new_feerate); match bump_claim { OnchainClaim::Tx(bump_tx) => { - log_info!(logger, "Broadcasting onchain {}", log_tx!(bump_tx)); - broadcaster.broadcast_transactions(&[&bump_tx]); + if bump_tx.is_fully_signed() { + log_info!(logger, "Broadcasting onchain {}", log_tx!(bump_tx.0)); + broadcaster.broadcast_transactions(&[&bump_tx.0]); + } else { + log_info!(logger, "Waiting for signature of unsigned onchain transaction {}", bump_tx.0.txid()); + } }, OnchainClaim::Event(claim_event) => { log_info!(logger, "Yielding onchain event after reorg to spend inputs {:?}", request.outpoints()); @@ -1108,13 +1136,11 @@ impl OnchainTxHandler &self.holder_commitment.trust().built_transaction().transaction } - //TODO: getting lastest holder transactions should be infallible and result in us "force-closing the channel", but we may - // have empty holder commitment transaction if a ChannelMonitor is asked to force-close just after OutboundV1Channel::get_funding_created, - // before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing - // to monitor before. - pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction { - let sig = self.signer.sign_holder_commitment(&self.holder_commitment, &self.secp_ctx).expect("signing holder commitment"); - self.holder_commitment.add_holder_sig(funding_redeemscript, sig) + pub(crate) fn get_maybe_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> MaybeSignedTransaction { + let tx = self.signer.sign_holder_commitment(&self.holder_commitment, &self.secp_ctx) + .map(|sig| self.holder_commitment.add_holder_sig(funding_redeemscript, sig)) + .unwrap_or_else(|_| self.get_unsigned_holder_commitment_tx().clone()); + MaybeSignedTransaction(tx) } #[cfg(any(test, feature="unsafe_revoked_tx_signing"))] @@ -1123,7 +1149,7 @@ impl OnchainTxHandler self.holder_commitment.add_holder_sig(funding_redeemscript, sig) } - pub(crate) fn get_fully_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option) -> Option { + pub(crate) fn get_maybe_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option) -> Option { let get_signed_htlc_tx = |holder_commitment: &HolderCommitmentTransaction| { let trusted_tx = holder_commitment.trust(); if trusted_tx.txid() != outp.txid { @@ -1151,11 +1177,12 @@ impl OnchainTxHandler preimage: preimage.clone(), counterparty_sig: counterparty_htlc_sig.clone(), }; - let htlc_sig = self.signer.sign_holder_htlc_transaction(&htlc_tx, 0, &htlc_descriptor, &self.secp_ctx).unwrap(); - htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness( - htlc_idx, &counterparty_htlc_sig, &htlc_sig, preimage, - ); - Some(htlc_tx) + if let Ok(htlc_sig) = self.signer.sign_holder_htlc_transaction(&htlc_tx, 0, &htlc_descriptor, &self.secp_ctx) { + htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness( + htlc_idx, &counterparty_htlc_sig, &htlc_sig, preimage, + ); + } + Some(MaybeSignedTransaction(htlc_tx)) }; // Check if the HTLC spends from the current holder commitment first, or the previous. diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index efc32bf7d40..e304b16ef3e 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -28,8 +28,9 @@ use crate::ln::features::ChannelTypeFeatures; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; use crate::ln::msgs::DecodeError; use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT, compute_feerate_sat_per_1000_weight, FEERATE_FLOOR_SATS_PER_KW}; +use crate::chain::transaction::MaybeSignedTransaction; use crate::sign::ecdsa::WriteableEcdsaChannelSigner; -use crate::chain::onchaintx::{ExternalHTLCClaim, OnchainTxHandler}; +use crate::chain::onchaintx::{FeerateStrategy, ExternalHTLCClaim, OnchainTxHandler}; use crate::util::logger::Logger; use crate::util::ser::{Readable, Writer, Writeable, RequiredWrapper}; @@ -633,14 +634,14 @@ impl PackageSolvingData { } true } - fn get_finalized_tx(&self, outpoint: &BitcoinOutPoint, onchain_handler: &mut OnchainTxHandler) -> Option { + fn get_maybe_finalized_tx(&self, outpoint: &BitcoinOutPoint, onchain_handler: &mut OnchainTxHandler) -> Option { match self { PackageSolvingData::HolderHTLCOutput(ref outp) => { debug_assert!(!outp.channel_type_features.supports_anchors_zero_fee_htlc_tx()); - return onchain_handler.get_fully_signed_htlc_tx(outpoint, &outp.preimage); + onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage) } PackageSolvingData::HolderFundingOutput(ref outp) => { - return Some(onchain_handler.get_fully_signed_holder_tx(&outp.funding_redeemscript)); + Some(onchain_handler.get_maybe_signed_holder_tx(&outp.funding_redeemscript)) } _ => { panic!("API Error!"); } } @@ -908,10 +909,10 @@ impl PackageTemplate { } htlcs } - pub(crate) fn finalize_malleable_package( + pub(crate) fn maybe_finalize_malleable_package( &self, current_height: u32, onchain_handler: &mut OnchainTxHandler, value: u64, destination_script: ScriptBuf, logger: &L - ) -> Option { + ) -> Option { debug_assert!(self.is_malleable()); let mut bumped_tx = Transaction { version: 2, @@ -927,19 +928,17 @@ impl PackageTemplate { } for (i, (outpoint, out)) in self.inputs.iter().enumerate() { log_debug!(logger, "Adding claiming input for outpoint {}:{}", outpoint.txid, outpoint.vout); - if !out.finalize_input(&mut bumped_tx, i, onchain_handler) { return None; } + if !out.finalize_input(&mut bumped_tx, i, onchain_handler) { continue; } } - log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid()); - Some(bumped_tx) + Some(MaybeSignedTransaction(bumped_tx)) } - pub(crate) fn finalize_untractable_package( + pub(crate) fn maybe_finalize_untractable_package( &self, onchain_handler: &mut OnchainTxHandler, logger: &L, - ) -> Option { + ) -> Option { debug_assert!(!self.is_malleable()); if let Some((outpoint, outp)) = self.inputs.first() { - if let Some(final_tx) = outp.get_finalized_tx(outpoint, onchain_handler) { + if let Some(final_tx) = outp.get_maybe_finalized_tx(outpoint, onchain_handler) { log_debug!(logger, "Adding claiming input for outpoint {}:{}", outpoint.txid, outpoint.vout); - log_debug!(logger, "Finalized transaction {} ready to broadcast", final_tx.txid()); return Some(final_tx); } return None; @@ -963,7 +962,7 @@ impl PackageTemplate { /// which was used to generate the value. Will not return less than `dust_limit_sats` for the /// value. pub(crate) fn compute_package_output( - &self, predicted_weight: u64, dust_limit_sats: u64, force_feerate_bump: bool, + &self, predicted_weight: u64, dust_limit_sats: u64, feerate_strategy: &FeerateStrategy, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Option<(u64, u64)> where F::Target: FeeEstimator, @@ -974,7 +973,7 @@ impl PackageTemplate { // If old feerate is 0, first iteration of this claim, use normal fee calculation if self.feerate_previous != 0 { if let Some((new_fee, feerate)) = feerate_bump( - predicted_weight, input_amounts, self.feerate_previous, force_feerate_bump, + predicted_weight, input_amounts, self.feerate_previous, feerate_strategy, fee_estimator, logger, ) { return Some((cmp::max(input_amounts as i64 - new_fee as i64, dust_limit_sats as i64) as u64, feerate)); @@ -987,32 +986,32 @@ impl PackageTemplate { None } - /// Computes a feerate based on the given confirmation target. If a previous feerate was used, - /// the new feerate is below it, and `force_feerate_bump` is set, we'll use a 25% increase of - /// the previous feerate instead of the new feerate. + /// Computes a feerate based on the given confirmation target and feerate strategy. pub(crate) fn compute_package_feerate( &self, fee_estimator: &LowerBoundedFeeEstimator, conf_target: ConfirmationTarget, - force_feerate_bump: bool, + feerate_strategy: &FeerateStrategy, ) -> u32 where F::Target: FeeEstimator { let feerate_estimate = fee_estimator.bounded_sat_per_1000_weight(conf_target); if self.feerate_previous != 0 { - // Use the new fee estimate if it's higher than the one previously used. - if feerate_estimate as u64 > self.feerate_previous { - feerate_estimate - } else if !force_feerate_bump { - self.feerate_previous.try_into().unwrap_or(u32::max_value()) - } else { - // Our fee estimate has decreased, but our transaction remains unconfirmed after - // using our previous fee estimate. This may point to an unreliable fee estimator, - // so we choose to bump our previous feerate by 25%, making sure we don't use a - // lower feerate or overpay by a large margin by limiting it to 5x the new fee - // estimate. - let previous_feerate = self.feerate_previous.try_into().unwrap_or(u32::max_value()); - let mut new_feerate = previous_feerate.saturating_add(previous_feerate / 4); - if new_feerate > feerate_estimate * 5 { - new_feerate = cmp::max(feerate_estimate * 5, previous_feerate); - } - new_feerate + let previous_feerate = self.feerate_previous.try_into().unwrap_or(u32::max_value()); + match feerate_strategy { + FeerateStrategy::RetryPrevious => previous_feerate, + FeerateStrategy::HighestOfPreviousOrNew => cmp::max(previous_feerate, feerate_estimate), + FeerateStrategy::ForceBump => if feerate_estimate > previous_feerate { + feerate_estimate + } else { + // Our fee estimate has decreased, but our transaction remains unconfirmed after + // using our previous fee estimate. This may point to an unreliable fee estimator, + // so we choose to bump our previous feerate by 25%, making sure we don't use a + // lower feerate or overpay by a large margin by limiting it to 5x the new fee + // estimate. + let previous_feerate = self.feerate_previous.try_into().unwrap_or(u32::max_value()); + let mut new_feerate = previous_feerate.saturating_add(previous_feerate / 4); + if new_feerate > feerate_estimate * 5 { + new_feerate = cmp::max(feerate_estimate * 5, previous_feerate); + } + new_feerate + }, } } else { feerate_estimate @@ -1128,12 +1127,12 @@ fn compute_fee_from_spent_amounts(input_amounts: u64, predi /// Attempt to propose a bumping fee for a transaction from its spent output's values and predicted /// weight. If feerates proposed by the fee-estimator have been increasing since last fee-bumping -/// attempt, use them. If `force_feerate_bump` is set, we bump the feerate by 25% of the previous -/// feerate, or just use the previous feerate otherwise. If a feerate bump did happen, we also -/// verify that those bumping heuristics respect BIP125 rules 3) and 4) and if required adjust the -/// new fee to meet the RBF policy requirement. +/// attempt, use them. If we need to force a feerate bump, we manually bump the feerate by 25% of +/// the previous feerate. If a feerate bump did happen, we also verify that those bumping heuristics +/// respect BIP125 rules 3) and 4) and if required adjust the new fee to meet the RBF policy +/// requirement. fn feerate_bump( - predicted_weight: u64, input_amounts: u64, previous_feerate: u64, force_feerate_bump: bool, + predicted_weight: u64, input_amounts: u64, previous_feerate: u64, feerate_strategy: &FeerateStrategy, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Option<(u64, u64)> where @@ -1141,20 +1140,29 @@ where { // If old feerate inferior to actual one given back by Fee Estimator, use it to compute new fee... let (new_fee, new_feerate) = if let Some((new_fee, new_feerate)) = compute_fee_from_spent_amounts(input_amounts, predicted_weight, fee_estimator, logger) { - if new_feerate > previous_feerate { - (new_fee, new_feerate) - } else if !force_feerate_bump { - let previous_fee = previous_feerate * predicted_weight / 1000; - (previous_fee, previous_feerate) - } else { - // ...else just increase the previous feerate by 25% (because that's a nice number) - let bumped_feerate = previous_feerate + (previous_feerate / 4); - let bumped_fee = bumped_feerate * predicted_weight / 1000; - if input_amounts <= bumped_fee { - log_warn!(logger, "Can't 25% bump new claiming tx, amount {} is too small", input_amounts); - return None; - } - (bumped_fee, bumped_feerate) + match feerate_strategy { + FeerateStrategy::RetryPrevious => { + let previous_fee = previous_feerate * predicted_weight / 1000; + (previous_fee, previous_feerate) + }, + FeerateStrategy::HighestOfPreviousOrNew => if new_feerate > previous_feerate { + (new_fee, new_feerate) + } else { + let previous_fee = previous_feerate * predicted_weight / 1000; + (previous_fee, previous_feerate) + }, + FeerateStrategy::ForceBump => if new_feerate > previous_feerate { + (new_fee, new_feerate) + } else { + // ...else just increase the previous feerate by 25% (because that's a nice number) + let bumped_feerate = previous_feerate + (previous_feerate / 4); + let bumped_fee = bumped_feerate * predicted_weight / 1000; + if input_amounts <= bumped_fee { + log_warn!(logger, "Can't 25% bump new claiming tx, amount {} is too small", input_amounts); + return None; + } + (bumped_fee, bumped_feerate) + }, } } else { log_warn!(logger, "Can't new-estimation bump new claiming tx, amount {} is too small", input_amounts); diff --git a/lightning/src/chain/transaction.rs b/lightning/src/chain/transaction.rs index 17815207a8d..7a447dd5d90 100644 --- a/lightning/src/chain/transaction.rs +++ b/lightning/src/chain/transaction.rs @@ -58,7 +58,7 @@ pub struct OutPoint { impl OutPoint { /// Converts this OutPoint into the OutPoint field as used by rust-bitcoin /// - /// This is not exported to bindings users as the same type is used universally in the C bindings + /// This is not exported to bindings users as the same type is used universally in the C bindings /// for all outpoints pub fn into_bitcoin_outpoint(self) -> BitcoinOutPoint { BitcoinOutPoint { @@ -76,6 +76,15 @@ impl core::fmt::Display for OutPoint { impl_writeable!(OutPoint, { txid, index }); +#[derive(Debug, Clone)] +pub(crate) struct MaybeSignedTransaction(pub Transaction); + +impl MaybeSignedTransaction { + pub fn is_fully_signed(&self) -> bool { + !self.0.input.iter().any(|input| input.witness.is_empty()) + } +} + #[cfg(test)] mod tests { use crate::chain::transaction::OutPoint; diff --git a/lightning/src/ln/async_signer_tests.rs b/lightning/src/ln/async_signer_tests.rs index 2beb3c2d051..613df570d4e 100644 --- a/lightning/src/ln/async_signer_tests.rs +++ b/lightning/src/ln/async_signer_tests.rs @@ -10,7 +10,12 @@ //! Tests for asynchronous signing. These tests verify that the channel state machine behaves //! properly with a signer implementation that asynchronously derives signatures. -use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider}; +use bitcoin::{Transaction, TxOut, TxIn, Amount}; +use bitcoin::blockdata::locktime::absolute::LockTime; + +use crate::chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS; +use crate::events::bump_transaction::WalletSource; +use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason}; use crate::ln::functional_test_utils::*; use crate::ln::msgs::ChannelMessageHandler; use crate::ln::channelmanager::{PaymentId, RecipientOnionFields}; @@ -321,3 +326,124 @@ fn test_async_commitment_signature_for_peer_disconnect() { }; } } + +fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) { + // Ensures that we can obtain holder signatures for commitment and HTLC transactions + // asynchronously by allowing their retrieval to fail and retrying via + // `ChannelMonitor::signer_unblocked`. + let mut config = test_default_channel_config(); + if anchors { + config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; + config.manually_accept_inbound_channels = true; + } + + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config), Some(config)]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let closing_node = if remote_commitment { &nodes[1] } else { &nodes[0] }; + let coinbase_tx = Transaction { + version: 2, + lock_time: LockTime::ZERO, + input: vec![TxIn { ..Default::default() }], + output: vec![ + TxOut { + value: Amount::ONE_BTC.to_sat(), + script_pubkey: closing_node.wallet_source.get_change_script().unwrap(), + }, + ], + }; + if anchors { + *nodes[0].fee_estimator.sat_per_kw.lock().unwrap() *= 2; + *nodes[1].fee_estimator.sat_per_kw.lock().unwrap() *= 2; + closing_node.wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.txid(), vout: 0 }, coinbase_tx.output[0].value); + } + + // Route an HTLC and set the signer as unavailable. + let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1); + route_payment(&nodes[0], &[&nodes[1]], 1_000_000); + + nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, false); + + if remote_commitment { + // Make the counterparty broadcast its latest commitment. + nodes[1].node.force_close_broadcasting_latest_txn(&chan_id, &nodes[0].node.get_our_node_id()).unwrap(); + check_added_monitors(&nodes[1], 1); + check_closed_broadcast(&nodes[1], 1, true); + check_closed_event(&nodes[1], 1, ClosureReason::HolderForceClosed, false, &[nodes[0].node.get_our_node_id()], 100_000); + } else { + // We'll connect blocks until the sender has to go onchain to time out the HTLC. + connect_blocks(&nodes[0], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + 1); + + // No transaction should be broadcast since the signer is not available yet. + assert!(nodes[0].tx_broadcaster.txn_broadcast().is_empty()); + assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty()); + + // Mark it as available now, we should see the signed commitment transaction. + nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, true); + get_monitor!(nodes[0], chan_id).signer_unblocked(nodes[0].tx_broadcaster, nodes[0].fee_estimator, &nodes[0].logger); + } + + let commitment_tx = { + let mut txn = closing_node.tx_broadcaster.txn_broadcast(); + if anchors || remote_commitment { + assert_eq!(txn.len(), 1); + check_spends!(txn[0], funding_tx); + txn.remove(0) + } else { + assert_eq!(txn.len(), 2); + if txn[0].input[0].previous_output.txid == funding_tx.txid() { + check_spends!(txn[0], funding_tx); + check_spends!(txn[1], txn[0]); + txn.remove(0) + } else { + check_spends!(txn[1], funding_tx); + check_spends!(txn[0], txn[1]); + txn.remove(1) + } + } + }; + + // Mark it as unavailable again to now test the HTLC transaction. We'll mine the commitment such + // that the HTLC transaction is retried. + nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, false); + mine_transaction(&nodes[0], &commitment_tx); + + check_added_monitors(&nodes[0], 1); + check_closed_broadcast(&nodes[0], 1, true); + check_closed_event(&nodes[0], 1, ClosureReason::CommitmentTxConfirmed, false, &[nodes[1].node.get_our_node_id()], 100_000); + + // If the counterparty broadcast its latest commitment, we need to mine enough blocks for the + // HTLC timeout. + if remote_commitment { + connect_blocks(&nodes[0], TEST_FINAL_CLTV); + } + + // No HTLC transaction should be broadcast as the signer is not available yet. + if anchors && !remote_commitment { + handle_bump_htlc_event(&nodes[0], 1); + } + assert!(nodes[0].tx_broadcaster.txn_broadcast().is_empty()); + + // Mark it as available now, we should see the signed HTLC transaction. + nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, true); + get_monitor!(nodes[0], chan_id).signer_unblocked(nodes[0].tx_broadcaster, nodes[0].fee_estimator, &nodes[0].logger); + + if anchors && !remote_commitment { + handle_bump_htlc_event(&nodes[0], 1); + } + { + let txn = nodes[0].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1); + check_spends!(txn[0], commitment_tx, coinbase_tx); + } +} + +#[test] +fn test_async_holder_signatures() { + do_test_async_holder_signatures(false, false); + do_test_async_holder_signatures(false, true); + do_test_async_holder_signatures(true, false); + do_test_async_holder_signatures(true, true); +} diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 0bc9f5e5793..a82f4d41be4 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1492,7 +1492,10 @@ pub(super) struct ChannelContext where SP::Target: SignerProvider { /// The unique identifier used to re-derive the private key material for the channel through /// [`SignerProvider::derive_channel_signer`]. + #[cfg(not(test))] channel_keys_id: [u8; 32], + #[cfg(test)] + pub channel_keys_id: [u8; 32], /// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we /// store it here and only release it to the `ChannelManager` once it asks for it. diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 87ac05ccb2b..243cf741822 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3021,8 +3021,8 @@ where /// the latest local transaction(s). Fails if `channel_id` is unknown to the manager, or if the /// `counterparty_node_id` isn't the counterparty of the corresponding channel. /// - /// You can always get the latest local transaction(s) to broadcast from - /// [`ChannelMonitor::get_latest_holder_commitment_txn`]. + /// You can always broadcast the latest local transaction(s) via + /// [`ChannelMonitor::broadcast_latest_holder_commitment_txn`]. pub fn force_close_without_broadcasting_txn(&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey) -> Result<(), APIError> { self.force_close_sending_error(channel_id, counterparty_node_id, false) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index d2cfce3608a..adf2768b415 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -487,16 +487,38 @@ impl<'a, 'b, 'c> Node<'a, 'b, 'c> { /// `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) { + use crate::sign::ChannelSigner; + log_debug!(self.logger, "Setting channel signer for {} as available={}", chan_id, available); + 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 = (|| { - match chan_lock.channel_by_id.get(chan_id) { - Some(phase) => phase.context().get_signer(), - None => panic!("Couldn't find a channel with id {}", chan_id), + + let mut channel_keys_id = None; + if let Some(chan) = chan_lock.channel_by_id.get(chan_id).map(|phase| phase.context()) { + chan.get_signer().as_ecdsa().unwrap().set_available(available); + channel_keys_id = Some(chan.channel_keys_id); + } + + let mut monitor = None; + for (funding_txo, channel_id) in self.chain_monitor.chain_monitor.list_monitors() { + if *chan_id == channel_id { + monitor = self.chain_monitor.chain_monitor.get_monitor(funding_txo).ok(); } - })(); - log_debug!(self.logger, "Setting channel signer for {} as available={}", chan_id, available); - signer.as_ecdsa().unwrap().set_available(available); + } + if let Some(monitor) = monitor { + monitor.do_signer_call(|signer| { + channel_keys_id = channel_keys_id.or(Some(signer.inner.channel_keys_id())); + signer.set_available(available) + }); + } + + if available { + self.keys_manager.unavailable_signers.lock().unwrap() + .remove(channel_keys_id.as_ref().unwrap()); + } else { + self.keys_manager.unavailable_signers.lock().unwrap() + .insert(channel_keys_id.unwrap()); + } } } diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 260b4d7c0c6..063c8fd9d24 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -2755,7 +2755,9 @@ fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterp (&nodes[0], &nodes[1]) }; - closing_node.node.force_close_broadcasting_latest_txn(&chan_id, &other_node.node.get_our_node_id()).unwrap(); + get_monitor!(closing_node, chan_id).broadcast_latest_holder_commitment_txn( + &closing_node.tx_broadcaster, &closing_node.fee_estimator, &closing_node.logger + ); // The commitment transaction comes first. let commitment_tx = { @@ -2768,7 +2770,7 @@ fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterp mine_transaction(closing_node, &commitment_tx); check_added_monitors!(closing_node, 1); check_closed_broadcast!(closing_node, true); - check_closed_event!(closing_node, 1, ClosureReason::HolderForceClosed, [other_node.node.get_our_node_id()], 1_000_000); + check_closed_event!(closing_node, 1, ClosureReason::CommitmentTxConfirmed, [other_node.node.get_our_node_id()], 1_000_000); mine_transaction(other_node, &commitment_tx); check_added_monitors!(other_node, 1); diff --git a/lightning/src/sign/ecdsa.rs b/lightning/src/sign/ecdsa.rs index 2e98213c182..070f48f122f 100644 --- a/lightning/src/sign/ecdsa.rs +++ b/lightning/src/sign/ecdsa.rs @@ -51,6 +51,13 @@ pub trait EcdsaChannelSigner: ChannelSigner { /// This may be called multiple times for the same transaction. /// /// An external signer implementation should check that the commitment has not been revoked. + /// + /// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid + /// signature and should be retried later. Once the signer is ready to provide a signature after + /// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its + /// monitor. + /// + /// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked // // TODO: Document the things someone using this interface should enforce before signing. fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, @@ -76,6 +83,13 @@ pub trait EcdsaChannelSigner: ChannelSigner { /// revoked the state which they eventually broadcast. It's not a _holder_ secret key and does /// not allow the spending of any funds by itself (you need our holder `revocation_secret` to do /// so). + /// + /// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid + /// signature and should be retried later. Once the signer is ready to provide a signature after + /// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its + /// monitor. + /// + /// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, secp_ctx: &Secp256k1 ) -> Result; @@ -97,6 +111,13 @@ pub trait EcdsaChannelSigner: ChannelSigner { /// /// `htlc` holds HTLC elements (hash, timelock), thus changing the format of the witness script /// (which is committed to in the BIP 143 signatures). + /// + /// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid + /// signature and should be retried later. Once the signer is ready to provide a signature after + /// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its + /// monitor. + /// + /// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked fn sign_justice_revoked_htlc(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1) -> Result; @@ -108,8 +129,14 @@ pub trait EcdsaChannelSigner: ChannelSigner { /// [`ChannelMonitor`] [replica](https://github.com/lightningdevkit/rust-lightning/blob/main/GLOSSARY.md#monitor-replicas) /// broadcasts it before receiving the update for the latest commitment transaction. /// + /// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid + /// signature and should be retried later. Once the signer is ready to provide a signature after + /// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its + /// monitor. + /// /// [`EcdsaSighashType::All`]: bitcoin::sighash::EcdsaSighashType::All /// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor + /// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked fn sign_holder_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, htlc_descriptor: &HTLCDescriptor, secp_ctx: &Secp256k1 ) -> Result; @@ -130,6 +157,13 @@ pub trait EcdsaChannelSigner: ChannelSigner { /// detected onchain. It has been generated by our counterparty and is used to derive /// channel state keys, which are then included in the witness script and committed to in the /// BIP 143 signature. + /// + /// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid + /// signature and should be retried later. Once the signer is ready to provide a signature after + /// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its + /// monitor. + /// + /// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked fn sign_counterparty_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1) -> Result; @@ -141,6 +175,13 @@ pub trait EcdsaChannelSigner: ChannelSigner { secp_ctx: &Secp256k1) -> Result; /// Computes the signature for a commitment transaction's anchor output used as an /// input within `anchor_tx`, which spends the commitment transaction, at index `input`. + /// + /// An `Err` can be returned to signal that the signer is unavailable/cannot produce a valid + /// signature and should be retried later. Once the signer is ready to provide a signature after + /// previously returning an `Err`, [`ChannelMonitor::signer_unblocked`] must be called on its + /// monitor. + /// + /// [`ChannelMonitor::signer_unblocked`]: crate::chain::channelmonitor::ChannelMonitor::signer_unblocked fn sign_holder_anchor_input( &self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1, ) -> Result; diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index a2cbf78b700..bfa3e32c91f 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -117,7 +117,6 @@ impl TestChannelSigner { /// When `true`, methods are forwarded to the underlying signer as normal. When `false`, some /// methods will return `Err` indicating that the signer is unavailable. Intended to be used for /// testing asynchronous signing. - #[cfg(test)] pub fn set_available(&self, available: bool) { *self.available.lock().unwrap() = available; } @@ -210,10 +209,16 @@ impl EcdsaChannelSigner for TestChannelSigner { } fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, secp_ctx: &Secp256k1) -> Result { + if !*self.available.lock().unwrap() { + return Err(()); + } Ok(EcdsaChannelSigner::sign_justice_revoked_output(&self.inner, justice_tx, input, amount, per_commitment_key, secp_ctx).unwrap()) } fn sign_justice_revoked_htlc(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1) -> Result { + if !*self.available.lock().unwrap() { + return Err(()); + } Ok(EcdsaChannelSigner::sign_justice_revoked_htlc(&self.inner, justice_tx, input, amount, per_commitment_key, htlc, secp_ctx).unwrap()) } @@ -221,6 +226,9 @@ impl EcdsaChannelSigner for TestChannelSigner { &self, htlc_tx: &Transaction, input: usize, htlc_descriptor: &HTLCDescriptor, secp_ctx: &Secp256k1 ) -> Result { + if !*self.available.lock().unwrap() { + return Err(()); + } let state = self.state.lock().unwrap(); if state.last_holder_revoked_commitment - 1 != htlc_descriptor.per_commitment_number && state.last_holder_revoked_commitment - 2 != htlc_descriptor.per_commitment_number @@ -254,6 +262,9 @@ impl EcdsaChannelSigner for TestChannelSigner { } fn sign_counterparty_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1) -> Result { + if !*self.available.lock().unwrap() { + return Err(()); + } Ok(EcdsaChannelSigner::sign_counterparty_htlc_transaction(&self.inner, htlc_tx, input, amount, per_commitment_point, htlc, secp_ctx).unwrap()) } @@ -270,6 +281,9 @@ impl EcdsaChannelSigner for TestChannelSigner { // As long as our minimum dust limit is enforced and is greater than our anchor output // value, an anchor output can only have an index within [0, 1]. assert!(anchor_tx.input[input].previous_output.vout == 0 || anchor_tx.input[input].previous_output.vout == 1); + if !*self.available.lock().unwrap() { + return Err(()); + } EcdsaChannelSigner::sign_holder_anchor_input(&self.inner, anchor_tx, input, secp_ctx) } diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 5346f8ec306..09a8c3a2661 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -1164,6 +1164,7 @@ pub struct TestKeysInterface { pub disable_revocation_policy_check: bool, enforcement_states: Mutex>>>, expectations: Mutex>>, + pub unavailable_signers: Mutex>, } impl EntropySource for TestKeysInterface { @@ -1222,7 +1223,11 @@ impl SignerProvider for TestKeysInterface { fn derive_channel_signer(&self, channel_value_satoshis: u64, channel_keys_id: [u8; 32]) -> TestChannelSigner { let keys = self.backing.derive_channel_signer(channel_value_satoshis, channel_keys_id); let state = self.make_enforcement_state_cell(keys.commitment_seed); - TestChannelSigner::new_with_revoked(keys, state, self.disable_revocation_policy_check) + let signer = TestChannelSigner::new_with_revoked(keys, state, self.disable_revocation_policy_check); + if self.unavailable_signers.lock().unwrap().contains(&channel_keys_id) { + signer.set_available(false); + } + signer } fn read_chan_signer(&self, buffer: &[u8]) -> Result { @@ -1260,6 +1265,7 @@ impl TestKeysInterface { disable_revocation_policy_check: false, enforcement_states: Mutex::new(new_hash_map()), expectations: Mutex::new(None), + unavailable_signers: Mutex::new(new_hash_set()), } } @@ -1273,9 +1279,7 @@ impl TestKeysInterface { } pub fn derive_channel_keys(&self, channel_value_satoshis: u64, id: &[u8; 32]) -> TestChannelSigner { - let keys = self.backing.derive_channel_keys(channel_value_satoshis, id); - let state = self.make_enforcement_state_cell(keys.commitment_seed); - TestChannelSigner::new_with_revoked(keys, state, self.disable_revocation_policy_check) + self.derive_channel_signer(channel_value_satoshis, *id) } fn make_enforcement_state_cell(&self, commitment_seed: [u8; 32]) -> Arc> {