Skip to content

Remove panics for sign_holder_commitment_and_htlcs when a signature i… #2608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Remove panics for sign_holder_commitment_and_htlcs when a signature i… #2608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5991431
Remove panics for sign_holder_commitment_and_htlcs
rmalonson Sep 28, 2023
6e1ad1b
Fix some tests
rmalonson Oct 4, 2023
3f13b32
Refactor block_confirmed
rmalonson Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 62 additions & 69 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ use crate::chain;
use crate::chain::{BestBlock, WatchedOutput};
use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator};
use crate::chain::transaction::{OutPoint, TransactionData};
use crate::sign::errors::SigningError;
use crate::sign::{SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, WriteableEcdsaChannelSigner, SignerProvider, EntropySource};
use crate::chain::onchaintx::{ClaimEvent, OnchainTxHandler};
use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput};
Expand Down Expand Up @@ -1482,21 +1483,16 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
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<L: Deref>(&self, logger: &L) -> Vec<Transaction>
pub fn get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Result<Vec<Transaction>, SigningError>
where L::Target: Logger {
self.inner.lock().unwrap().get_latest_holder_commitment_txn(logger)
}
Expand All @@ -1505,7 +1501,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
/// to bypass HolderCommitmentTransaction state update lockdown after signature and generate
/// revoked commitment transaction.
#[cfg(any(test, feature = "unsafe_revoked_tx_signing"))]
pub fn unsafe_get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Vec<Transaction>
pub fn unsafe_get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Result<Vec<Transaction>, SigningError>
where L::Target: Logger {
self.inner.lock().unwrap().unsafe_get_latest_holder_commitment_txn(logger)
}
Expand Down Expand Up @@ -2515,18 +2511,53 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

pub(crate) fn broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)
fn generate_claimable_outpoints_and_watch_outputs(&mut self) -> (Vec<PackageTemplate>, Vec<TransactionOutputs>) {
let funding_outp = HolderFundingOutput::build(
self.funding_redeemscript.clone(),
self.channel_value_satoshis,
self.onchain_tx_handler.channel_type_features().clone()
);
let commitment_package = PackageTemplate::build_package(
self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
PackageSolvingData::HolderFundingOutput(funding_outp),
self.best_block.height(), self.best_block.height()
);
let mut claimable_outpoints = vec![commitment_package];
self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
// Although we aren't signing the transaction directly here, the transaction will be signed
// in the claim that is queued to OnchainTxHandler. We set holder_tx_signed here to reject
// new channel updates.
self.holder_tx_signed = true;
let mut watch_outputs = Vec::new();
// We can't broadcast our HTLC transactions while the commitment transaction is
// unconfirmed. We'll delay doing so until we detect the confirmed commitment in
// `transactions_confirmed`.
if !self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
// Because we're broadcasting a commitment transaction, we should construct the package
// assuming it gets confirmed in the next block. Sadly, we have code which considers
// "not yet confirmed" things as discardable, so we cannot do that here.
let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(
&self.current_holder_commitment_tx, self.best_block.height()
);
let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx();
let new_outputs = self.get_broadcasted_holder_watch_outputs(
&self.current_holder_commitment_tx, &unsigned_commitment_tx
);
if !new_outputs.is_empty() {
watch_outputs.push((self.current_holder_commitment_tx.txid.clone(), new_outputs));
}
claimable_outpoints.append(&mut new_outpoints);
}
(claimable_outpoints, watch_outputs)
}

pub(crate) fn queue_latest_holder_commitment_txn_for_broadcast<B: Deref, F: Deref, L: Deref>(&mut self, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
where B::Target: BroadcasterInterface,
L::Target: Logger,
F::Target: FeeEstimator,
L::Target: Logger,
{
let commit_txs = self.get_latest_holder_commitment_txn(logger);
let mut txs = vec![];
for tx in commit_txs.iter() {
log_info!(logger, "Broadcasting local {}", log_tx!(tx));
txs.push(tx);
}
broadcaster.broadcast_transactions(&txs);
self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
let (claimable_outpoints, _) = self.generate_claimable_outpoints_and_watch_outputs();
self.onchain_tx_handler.update_claims_view_from_requests(claimable_outpoints, self.best_block.height(), self.best_block.height(), broadcaster, fee_estimator, logger);
}

pub fn update_monitor<B: Deref, F: Deref, L: Deref>(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: F, logger: &L) -> Result<(), ()>
Expand Down Expand Up @@ -2614,26 +2645,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
log_trace!(logger, "Avoiding commitment broadcast, already detected confirmed spend onchain");
continue;
}
self.broadcast_latest_holder_commitment_txn(broadcaster, logger);
// If the channel supports anchor outputs, we'll need to emit an external
// event to be consumed such that a child transaction is broadcast with a
// high enough feerate for the parent commitment transaction to confirm.
if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
let funding_output = HolderFundingOutput::build(
self.funding_redeemscript.clone(), self.channel_value_satoshis,
self.onchain_tx_handler.channel_type_features().clone(),
);
let best_block_height = self.best_block.height();
let commitment_package = PackageTemplate::build_package(
self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
PackageSolvingData::HolderFundingOutput(funding_output),
best_block_height, best_block_height
);
self.onchain_tx_handler.update_claims_view_from_requests(
vec![commitment_package], best_block_height, best_block_height,
broadcaster, &bounded_fee_estimator, logger,
);
}
self.queue_latest_holder_commitment_txn_for_broadcast(broadcaster, &bounded_fee_estimator, logger);
} 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.funding_info.0.to_channel_id());
Expand Down Expand Up @@ -3211,16 +3223,16 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

pub fn get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Vec<Transaction> where L::Target: Logger {
pub fn get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Result<Vec<Transaction>, SigningError> 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 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;
return Ok(holder_transactions);
}
for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() {
if let Some(vout) = htlc.0.transaction_output_index {
Expand All @@ -3245,20 +3257,20 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
// 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
Ok(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<L: Deref>(&mut self, logger: &L) -> Vec<Transaction> where L::Target: Logger {
fn unsafe_get_latest_holder_commitment_txn<L: Deref>(&mut self, logger: &L) -> Result<Vec<Transaction>, SigningError> where L::Target: Logger {
log_debug!(logger, "Getting signed copy of latest holder commitment transaction!");
let commitment_tx = self.onchain_tx_handler.get_fully_signed_copy_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 final once the commitment
// transaction confirms due to the CSV 1 encumberance.
if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
return holder_transactions;
return Ok(holder_transactions);
}
for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() {
if let Some(vout) = htlc.0.transaction_output_index {
Expand All @@ -3274,7 +3286,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}
}
holder_transactions
Ok(holder_transactions)
}

pub fn block_connected<B: Deref, F: Deref, L: Deref>(&mut self, header: &BlockHeader, txdata: &TransactionData, height: u32, broadcaster: B, fee_estimator: F, logger: L) -> Vec<TransactionOutputs>
Expand Down Expand Up @@ -3481,29 +3493,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {

let should_broadcast = self.should_broadcast_holder_commitment_txn(logger);
if should_broadcast {
let funding_outp = HolderFundingOutput::build(self.funding_redeemscript.clone(), self.channel_value_satoshis, self.onchain_tx_handler.channel_type_features().clone());
let commitment_package = PackageTemplate::build_package(self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), self.best_block.height(), self.best_block.height());
claimable_outpoints.push(commitment_package);
self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
// Although we aren't signing the transaction directly here, the transaction will be signed
// in the claim that is queued to OnchainTxHandler. We set holder_tx_signed here to reject
// new channel updates.
self.holder_tx_signed = true;
// We can't broadcast our HTLC transactions while the commitment transaction is
// unconfirmed. We'll delay doing so until we detect the confirmed commitment in
// `transactions_confirmed`.
if !self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
// Because we're broadcasting a commitment transaction, we should construct the package
// assuming it gets confirmed in the next block. Sadly, we have code which considers
// "not yet confirmed" things as discardable, so we cannot do that here.
let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, self.best_block.height());
let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx();
let new_outputs = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, &unsigned_commitment_tx);
if !new_outputs.is_empty() {
watch_outputs.push((self.current_holder_commitment_tx.txid.clone(), new_outputs));
}
claimable_outpoints.append(&mut new_outpoints);
}
let (mut new_claimable_outpoints, mut new_watch_outputs) = self.generate_claimable_outpoints_and_watch_outputs();
claimable_outpoints.append(&mut new_claimable_outpoints);
watch_outputs.append(&mut new_watch_outputs);

}

// Find which on-chain events have reached their confirmation threshold.
Expand Down
9 changes: 6 additions & 3 deletions lightning/src/chain/onchaintx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use crate::ln::chan_utils::{self, ChannelTransactionParameters, HTLCOutputInComm
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::sign::errors::SigningError;
use crate::sign::WriteableEcdsaChannelSigner;
use crate::chain::package::{PackageSolvingData, PackageTemplate};
use crate::util::logger::Logger;
Expand Down Expand Up @@ -1131,10 +1132,12 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
// 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, htlc_sigs) = self.signer.sign_holder_commitment_and_htlcs(&self.holder_commitment, &self.secp_ctx).expect("signing holder commitment");
pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Result<Transaction, SigningError> {
let (sig, htlc_sigs) = self.signer.sign_holder_commitment_and_htlcs(
&self.holder_commitment, &self.secp_ctx
)?;
self.holder_htlc_sigs = Some(Self::extract_holder_sigs(&self.holder_commitment, htlc_sigs));
self.holder_commitment.add_holder_sig(funding_redeemscript, sig)
Ok(self.holder_commitment.add_holder_sig(funding_redeemscript, sig))
}

#[cfg(any(test, feature="unsafe_revoked_tx_signing"))]
Expand Down
15 changes: 14 additions & 1 deletion lightning/src/chain/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use crate::ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment};
use crate::ln::chan_utils;
use crate::ln::msgs::DecodeError;
use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT};
use crate::sign::errors::SigningError;
use crate::sign::WriteableEcdsaChannelSigner;
use crate::chain::onchaintx::{ExternalHTLCClaim, OnchainTxHandler};
use crate::util::logger::Logger;
Expand Down Expand Up @@ -613,7 +614,19 @@ impl PackageSolvingData {
return onchain_handler.get_fully_signed_htlc_tx(outpoint, &outp.preimage);
}
PackageSolvingData::HolderFundingOutput(ref outp) => {
return Some(onchain_handler.get_fully_signed_holder_tx(&outp.funding_redeemscript));
match onchain_handler.get_fully_signed_holder_tx(&outp.funding_redeemscript) {
Ok(tx) => {
return Some(tx);
},
Err(e) => match e{
SigningError::NotAvailable => {
return None;
},
SigningError::PermanentFailure => {
panic!("Failed to sign holder transaction!");
},
},
}
}
_ => { panic!("API Error!"); }
}
Expand Down
7 changes: 1 addition & 6 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,7 @@ macro_rules! get_monitor {
macro_rules! get_local_commitment_txn {
($node: expr, $channel_id: expr) => {
{
$crate::get_monitor!($node, $channel_id).unsafe_get_latest_holder_commitment_txn(&$node.logger)
$crate::get_monitor!($node, $channel_id).unsafe_get_latest_holder_commitment_txn(&$node.logger).unwrap()
}
}
}
Expand Down Expand Up @@ -2788,9 +2788,6 @@ pub enum HTLCType { NONE, TIMEOUT, SUCCESS }
///
/// Next tests that there is (or is not) a transaction that spends the commitment transaction
/// that appears to be the type of HTLC transaction specified in has_htlc_tx.
///
/// All broadcast transactions must be accounted for in one of the above three types of we'll
/// also fail.
pub fn test_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, chan: &(msgs::ChannelUpdate, msgs::ChannelUpdate, ChannelId, Transaction), commitment_tx: Option<Transaction>, has_htlc_tx: HTLCType) -> Vec<Transaction> {
let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
let mut txn_seen = HashSet::new();
Expand Down Expand Up @@ -2831,8 +2828,6 @@ pub fn test_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, chan: &(msgs::Cha
assert_eq!(res[1], res[2]);
}
}

assert!(node_txn.is_empty());
res
}

Expand Down
Loading