Skip to content

Commit d8f9b9b

Browse files
committed
Rework get_latest_holder_commitment_txn to broadcast for users
This method is meant to be used as a last resort when a user is forced to broadcast the current state, even if it is stale, in an attempt to claim their funds in the channel. Previously, we'd return the commitment and HTLC transactions such that they broadcast them themselves. Doing so required a different code path, one which was not tested, to obtain these transactions than our usual path when force closing. It's not worth maintaining both, and it's much simpler for us to broadcast instead.
1 parent 15c9f5b commit d8f9b9b

File tree

3 files changed

+22
-57
lines changed

3 files changed

+22
-57
lines changed

lightning/src/chain/channelmonitor.rs

Lines changed: 16 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1396,8 +1396,8 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
13961396
/// Loads the funding txo and outputs to watch into the given `chain::Filter` by repeatedly
13971397
/// calling `chain::Filter::register_output` and `chain::Filter::register_tx` until all outputs
13981398
/// have been registered.
1399-
pub fn load_outputs_to_watch<F: Deref, L: Deref>(&self, filter: &F, logger: &L)
1400-
where
1399+
pub fn load_outputs_to_watch<F: Deref, L: Deref>(&self, filter: &F, logger: &L)
1400+
where
14011401
F::Target: chain::Filter, L::Target: Logger,
14021402
{
14031403
let lock = self.inner.lock().unwrap();
@@ -1542,28 +1542,30 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
15421542
self.inner.lock().unwrap().counterparty_node_id
15431543
}
15441544

1545-
/// Used by [`ChannelManager`] deserialization to broadcast the latest holder state if its copy
1546-
/// of the channel state was out-of-date.
1547-
///
1548-
/// You may also use this to broadcast the latest local commitment transaction, either because
1545+
/// You may use this to broadcast the latest local commitment transaction, either because
15491546
/// a monitor update failed or because we've fallen behind (i.e. we've received proof that our
15501547
/// counterparty side knows a revocation secret we gave them that they shouldn't know).
15511548
///
1552-
/// Broadcasting these transactions in the second case is UNSAFE, as they allow counterparty
1549+
/// Broadcasting these transactions in this manner is UNSAFE, as they allow counterparty
15531550
/// side to punish you. Nevertheless you may want to broadcast them if counterparty doesn't
15541551
/// close channel with their commitment transaction after a substantial amount of time. Best
15551552
/// may be to contact the other node operator out-of-band to coordinate other options available
15561553
/// to you.
1557-
///
1558-
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
1559-
pub fn get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Vec<Transaction>
1560-
where L::Target: Logger {
1554+
pub fn broadcast_latest_holder_commitment_txn<B: Deref, F: Deref, L: Deref>(
1555+
&self, broadcaster: &B, fee_estimator: &F, logger: &L
1556+
)
1557+
where
1558+
B::Target: BroadcasterInterface,
1559+
F::Target: FeeEstimator,
1560+
L::Target: Logger
1561+
{
15611562
let mut inner = self.inner.lock().unwrap();
1563+
let fee_estimator = LowerBoundedFeeEstimator::new(&**fee_estimator);
15621564
let logger = WithChannelMonitor::from_impl(logger, &*inner);
1563-
inner.get_latest_holder_commitment_txn(&logger)
1565+
inner.queue_latest_holder_commitment_txn_for_broadcast(broadcaster, &fee_estimator, &logger);
15641566
}
15651567

1566-
/// Unsafe test-only version of get_latest_holder_commitment_txn used by our test framework
1568+
/// Unsafe test-only version of `broadcast_latest_holder_commitment_txn` used by our test framework
15671569
/// to bypass HolderCommitmentTransaction state update lockdown after signature and generate
15681570
/// revoked commitment transaction.
15691571
#[cfg(any(test, feature = "unsafe_revoked_tx_signing"))]
@@ -2835,7 +2837,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
28352837
} else if !self.holder_tx_signed {
28362838
log_error!(logger, "WARNING: You have a potentially-unsafe holder commitment transaction available to broadcast");
28372839
log_error!(logger, " in channel monitor for channel {}!", &self.funding_info.0.to_channel_id());
2838-
log_error!(logger, " Read the docs for ChannelMonitor::get_latest_holder_commitment_txn and take manual action!");
2840+
log_error!(logger, " Read the docs for ChannelMonitor::broadcast_latest_holder_commitment_txn and take manual action!");
28392841
} else {
28402842
// If we generated a MonitorEvent::HolderForceClosed, the ChannelManager
28412843
// will still give us a ChannelForceClosed event with !should_broadcast, but we
@@ -3460,45 +3462,6 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
34603462
}
34613463
}
34623464

3463-
fn get_latest_holder_commitment_txn<L: Deref>(
3464-
&mut self, logger: &WithChannelMonitor<L>,
3465-
) -> Vec<Transaction> where L::Target: Logger {
3466-
log_debug!(logger, "Getting signed latest holder commitment transaction!");
3467-
self.holder_tx_signed = true;
3468-
let commitment_tx = self.onchain_tx_handler.get_fully_signed_holder_tx(&self.funding_redeemscript);
3469-
let txid = commitment_tx.txid();
3470-
let mut holder_transactions = vec![commitment_tx];
3471-
// When anchor outputs are present, the HTLC transactions are only valid once the commitment
3472-
// transaction confirms.
3473-
if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
3474-
return holder_transactions;
3475-
}
3476-
for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() {
3477-
if let Some(vout) = htlc.0.transaction_output_index {
3478-
let preimage = if !htlc.0.offered {
3479-
if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(preimage.clone()) } else {
3480-
// We can't build an HTLC-Success transaction without the preimage
3481-
continue;
3482-
}
3483-
} else if htlc.0.cltv_expiry > self.best_block.height() + 1 {
3484-
// Don't broadcast HTLC-Timeout transactions immediately as they don't meet the
3485-
// current locktime requirements on-chain. We will broadcast them in
3486-
// `block_confirmed` when `should_broadcast_holder_commitment_txn` returns true.
3487-
// Note that we add + 1 as transactions are broadcastable when they can be
3488-
// confirmed in the next block.
3489-
continue;
3490-
} else { None };
3491-
if let Some(htlc_tx) = self.onchain_tx_handler.get_fully_signed_htlc_tx(
3492-
&::bitcoin::OutPoint { txid, vout }, &preimage) {
3493-
holder_transactions.push(htlc_tx);
3494-
}
3495-
}
3496-
}
3497-
// 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.
3498-
// The data will be re-generated and tracked in check_spend_holder_transaction if we get a confirmation.
3499-
holder_transactions
3500-
}
3501-
35023465
#[cfg(any(test,feature = "unsafe_revoked_tx_signing"))]
35033466
/// Note that this includes possibly-locktimed-in-the-future transactions!
35043467
fn unsafe_get_latest_holder_commitment_txn<L: Deref>(

lightning/src/ln/channelmanager.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2997,8 +2997,8 @@ where
29972997
/// the latest local transaction(s). Fails if `channel_id` is unknown to the manager, or if the
29982998
/// `counterparty_node_id` isn't the counterparty of the corresponding channel.
29992999
///
3000-
/// You can always get the latest local transaction(s) to broadcast from
3001-
/// [`ChannelMonitor::get_latest_holder_commitment_txn`].
3000+
/// You can always broadcast the latest local transaction(s) via
3001+
/// [`ChannelMonitor::broadcast_latest_holder_commitment_txn`].
30023002
pub fn force_close_without_broadcasting_txn(&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey)
30033003
-> Result<(), APIError> {
30043004
self.force_close_sending_error(channel_id, counterparty_node_id, false)

lightning/src/ln/monitor_tests.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2755,7 +2755,9 @@ fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterp
27552755
(&nodes[0], &nodes[1])
27562756
};
27572757

2758-
closing_node.node.force_close_broadcasting_latest_txn(&chan_id, &other_node.node.get_our_node_id()).unwrap();
2758+
get_monitor!(closing_node, chan_id).broadcast_latest_holder_commitment_txn(
2759+
&closing_node.tx_broadcaster, &closing_node.fee_estimator, &closing_node.logger
2760+
);
27592761

27602762
// The commitment transaction comes first.
27612763
let commitment_tx = {
@@ -2768,7 +2770,7 @@ fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterp
27682770
mine_transaction(closing_node, &commitment_tx);
27692771
check_added_monitors!(closing_node, 1);
27702772
check_closed_broadcast!(closing_node, true);
2771-
check_closed_event!(closing_node, 1, ClosureReason::HolderForceClosed, [other_node.node.get_our_node_id()], 1_000_000);
2773+
check_closed_event!(closing_node, 1, ClosureReason::CommitmentTxConfirmed, [other_node.node.get_our_node_id()], 1_000_000);
27722774

27732775
mine_transaction(other_node, &commitment_tx);
27742776
check_added_monitors!(other_node, 1);

0 commit comments

Comments
 (0)