Skip to content

Allow holder commitment and HTLC signature requests to fail#2816

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

Merged
TheBlueMatt merged 3 commits into from
Feb 13, 2024
Merged

Allow holder commitment and HTLC signature requests to fail #2816

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
71b4bd0
Rework get_latest_holder_commitment_txn to broadcast for users
wpaulino Jan 30, 2024
043ab75
Introduce FeerateStrategy enum for onchain claims
wpaulino Jan 30, 2024
e38dcca
Allow holder commitment and HTLC signature requests to fail
wpaulino Jan 30, 2024
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
21 changes: 21 additions & 0 deletions lightning/src/chain/chainmonitor.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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<OutPoint>) {
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<ChannelSigner: WriteableEcdsaChannelSigner, C: Deref, T: Deref, F: Deref, L: Deref, P: Deref>
Expand Down
103 changes: 47 additions & 56 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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};
Expand DownExpand Up@@ -1562,28 +1562,30 @@ 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>
where L::Target: Logger {
pub fn broadcast_latest_holder_commitment_txn<B: Deref, F: Deref, L: Deref>(
&self, broadcaster: &B, fee_estimator: &F, logger: &L
)
where
B::Target: BroadcasterInterface,
F::Target: FeeEstimator,
L::Target: Logger
{
Comment on lines +1565 to +1581

@coderabbitaicoderabbitaiBotFeb 6, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The broadcast_latest_holder_commitment_txn method is introduced with detailed documentation on its unsafe nature and intended use. Ensure that all calls to this method are carefully reviewed to prevent misuse, given its potential to cause loss of funds if used incorrectly.

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"))]
Expand DownExpand Up@@ -1763,7 +1765,26 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
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<B: Deref, F: Deref, L: Deref>(
Comment thread
wpaulino marked this conversation as resolved.
&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,
);
}

Expand DownExpand Up@@ -1809,6 +1830,12 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
pub fn set_counterparty_payment_script(&self, script: ScriptBuf) {
self.inner.lock().unwrap().counterparty_payment_script = script;
}

#[cfg(test)]
pub fn do_signer_call<F: FnMut(&Signer) -> ()>(&self, mut f: F) {
let inner = self.inner.lock().unwrap();
f(&inner.onchain_tx_handler.signer);
}
}

impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
Expand DownExpand Up@@ -2855,7 +2882,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
} 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!");

@coderabbitaicoderabbitaiBotFeb 6, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging statements are added to warn about the availability of a potentially-unsafe holder commitment transaction. Ensure that these logs provide clear guidance to operators on how to proceed safely.

} else {
// If we generated a MonitorEvent::HolderForceClosed, the ChannelManager
// will still give us a ChannelForceClosed event with !should_broadcast, but we
Expand DownExpand Up@@ -3502,45 +3529,6 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

fn get_latest_holder_commitment_txn<L: Deref>(
&mut self, logger: &WithChannelMonitor<L>,
) -> Vec<Transaction> 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<L: Deref>(
Expand All@@ -3563,9 +3551,12 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
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);
}
}
}
}
Expand Down
Loading