Skip to content

Commit 68cd71c

Browse files
wpaulinowpaulino
committed
Detect commitment transaction confirmation in ChannelMonitor instead
Previously, the `ChannelManager` would assume a `Channel` was closed the moment it saw a spend for its funding input. With splicing, this will no longer be the case. Since the `ChannelMonitor` is already responsible for reliably tracking each onchain transaction relevant to a channel, we now produce a `MonitorEvent::CommitmentTxConfirmed` event to inform the `ChannelManager` the channel can be considered closed and removed. As a result of this change, many tests failed now that we rely on handling the `MonitorEvent::CommitmentTxConfirmed` first before seeing the `ChannelMonitorUpdateStep::ChannelForceClosed` go out.
1 parent ac8f897 commit 68cd71c

File tree

11 files changed

+176
-173
lines changed

11 files changed

+176
-173
lines changed

‎lightning/src/chain/chainmonitor.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1490,8 +1490,8 @@ mod tests {
14901490
assert_eq!(close_tx.len(), 1);
14911491

14921492
mine_transaction(&nodes[2], &close_tx[0]);
1493-
check_added_monitors(&nodes[2], 1);
14941493
check_closed_broadcast(&nodes[2], 1, true);
1494+
check_added_monitors(&nodes[2], 1);
14951495
let closure_reason = ClosureReason::CommitmentTxConfirmed;
14961496
check_closed_event!(&nodes[2], 1, closure_reason, false, [node_a_id], 1000000);
14971497

‎lightning/src/chain/channelmonitor.rs‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,10 @@ pub enum MonitorEvent {
205205
/// channel.
206206
HolderForceClosed(OutPoint),
207207

208+
/// Indicates that we've detected a commitment transaction (either holder's or counterparty's)
209+
/// be included in a block and should consider the channel closed.
210+
CommitmentTxConfirmed(()),
211+
208212
/// Indicates a [`ChannelMonitor`] update has completed. See
209213
/// [`ChannelMonitorUpdateStatus::InProgress`] for more information on how this is used.
210214
///
@@ -236,6 +240,7 @@ impl_writeable_tlv_based_enum_upgradable_legacy!(MonitorEvent,
236240
(4, channel_id, required),
237241
},
238242
;
243+
(1, CommitmentTxConfirmed),
239244
(2, HTLCEvent),
240245
(4, HolderForceClosed),
241246
// 6 was `UpdateFailed` until LDK 0.0.117
@@ -5088,6 +5093,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
50885093
);
50895094
log_info!(logger, "Channel {} closed by funding output spend in txid {txid}",
50905095
self.channel_id());
5096+
if !self.funding_spend_seen {
5097+
self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(()));
5098+
}
50915099
self.funding_spend_seen = true;
50925100

50935101
let mut balance_spendable_csv = None;
@@ -6371,6 +6379,7 @@ mod tests {
63716379
use bitcoin::{Sequence, Witness};
63726380

63736381
use crate::chain::chaininterface::LowerBoundedFeeEstimator;
6382+
use crate::events::ClosureReason;
63746383

63756384
use super::ChannelMonitorUpdateStep;
63766385
use crate::chain::channelmonitor::{ChannelMonitor, WithChannelMonitor};
@@ -6468,22 +6477,26 @@ mod tests {
64686477
// Build a new ChannelMonitorUpdate which contains both the failing commitment tx update
64696478
// and provides the claim preimages for the two pending HTLCs. The first update generates
64706479
// an error, but the point of this test is to ensure the later updates are still applied.
6471-
let monitor_updates = nodes[1].chain_monitor.monitor_updates.lock().unwrap();
6472-
let mut replay_update = monitor_updates.get(&channel.2).unwrap().iter().next_back().unwrap().clone();
6473-
assert_eq!(replay_update.updates.len(), 1);
6474-
if let ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } = replay_update.updates[0] {
6475-
} else { panic!(); }
6476-
replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage {
6477-
payment_preimage: payment_preimage_1, payment_info: None,
6478-
});
6479-
replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage {
6480-
payment_preimage: payment_preimage_2, payment_info: None,
6481-
});
6480+
let replay_update = {
6481+
let monitor_updates = nodes[1].chain_monitor.monitor_updates.lock().unwrap();
6482+
let mut replay_update = monitor_updates.get(&channel.2).unwrap().iter().next_back().unwrap().clone();
6483+
assert_eq!(replay_update.updates.len(), 1);
6484+
if let ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } = replay_update.updates[0] {
6485+
} else { panic!(); }
6486+
replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage {
6487+
payment_preimage: payment_preimage_1, payment_info: None,
6488+
});
6489+
replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage {
6490+
payment_preimage: payment_preimage_2, payment_info: None,
6491+
});
6492+
replay_update
6493+
};
64826494

64836495
let broadcaster = TestBroadcaster::with_blocks(Arc::clone(&nodes[1].blocks));
64846496
assert!(
64856497
pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &&chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
64866498
.is_err());
6499+
64876500
// Even though we error'd on the first update, we should still have generated an HTLC claim
64886501
// transaction
64896502
let txn_broadcasted = broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
@@ -6495,7 +6508,12 @@ mod tests {
64956508
assert_eq!(htlc_txn.len(), 2);
64966509
check_spends!(htlc_txn[0], broadcast_tx);
64976510
check_spends!(htlc_txn[1], broadcast_tx);
6511+
6512+
check_closed_broadcast(&nodes[1], 1, true);
6513+
check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false, &[nodes[0].node.get_our_node_id()], 100000);
6514+
check_added_monitors(&nodes[1], 1);
64986515
}
6516+
64996517
#[test]
65006518
fn test_funding_spend_refuses_updates() {
65016519
do_test_funding_spend_refuses_updates(true);

‎lightning/src/ln/async_signer_tests.rs‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,7 +1010,7 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {
10101010

10111011
// Route an HTLC and set the signer as unavailable.
10121012
let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1);
1013-
route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
1013+
let (_, payment_hash, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
10141014

10151015
if remote_commitment {
10161016
let message = "Channel force-closed".to_owned();
@@ -1086,16 +1086,14 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {
10861086
nodes[0].disable_channel_signer_op(&node_b_id, &chan_id, sign_htlc_op);
10871087
mine_transaction(&nodes[0], &commitment_tx);
10881088

1089-
check_added_monitors(&nodes[0], 1);
10901089
check_closed_broadcast(&nodes[0], 1, true);
1091-
check_closed_event(
1092-
&nodes[0],
1093-
1,
1094-
ClosureReason::CommitmentTxConfirmed,
1095-
false,
1096-
&[node_b_id],
1097-
100_000,
1098-
);
1090+
check_added_monitors(&nodes[0], 1);
1091+
let closure_reason = if remote_commitment {
1092+
ClosureReason::CommitmentTxConfirmed
1093+
} else {
1094+
ClosureReason::HTLCsTimedOut { payment_hash: Some(payment_hash) }
1095+
};
1096+
check_closed_event(&nodes[0], 1, closure_reason, false, &[node_b_id], 100_000);
10991097

11001098
// If the counterparty broadcast its latest commitment, we need to mine enough blocks for the
11011099
// HTLC timeout.

‎lightning/src/ln/chanmon_update_fail_tests.rs‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3895,6 +3895,11 @@ fn do_test_durable_preimages_on_closed_channel(
38953895
persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
38963896
}
38973897
}
3898+
if !close_chans_before_reload {
3899+
check_closed_broadcast(&nodes[1], 1, true);
3900+
let reason = ClosureReason::CommitmentTxConfirmed;
3901+
check_closed_event(&nodes[1], 1, reason, false, &[node_a_id], 100000);
3902+
}
38983903
nodes[1].node.timer_tick_occurred();
38993904
check_added_monitors(&nodes[1], mons_added);
39003905

@@ -3907,12 +3912,6 @@ fn do_test_durable_preimages_on_closed_channel(
39073912
.unwrap();
39083913
check_spends!(bs_preimage_tx, as_closing_tx[0]);
39093914

3910-
if !close_chans_before_reload {
3911-
check_closed_broadcast(&nodes[1], 1, true);
3912-
let reason = ClosureReason::CommitmentTxConfirmed;
3913-
check_closed_event(&nodes[1], 1, reason, false, &[node_a_id], 100000);
3914-
}
3915-
39163915
mine_transactions(&nodes[0], &[&as_closing_tx[0], bs_preimage_tx]);
39173916
check_closed_broadcast(&nodes[0], 1, true);
39183917
expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
@@ -4048,7 +4047,7 @@ fn do_test_reload_mon_update_completion_actions(close_during_reload: bool) {
40484047
let mut events = nodes[1].node.get_and_clear_pending_events();
40494048
assert_eq!(events.len(), if close_during_reload { 2 } else { 1 });
40504049
expect_payment_forwarded(
4051-
events.pop().unwrap(),
4050+
events.remove(0),
40524051
&nodes[1],
40534052
&nodes[0],
40544053
&nodes[2],
@@ -4491,9 +4490,9 @@ fn test_claim_to_closed_channel_blocks_forwarded_preimage_removal() {
44914490
assert_eq!(as_commit_tx.len(), 1);
44924491

44934492
mine_transaction(&nodes[1], &as_commit_tx[0]);
4493+
check_closed_broadcast!(nodes[1], true);
44944494
check_added_monitors!(nodes[1], 1);
44954495
check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [node_a_id], 1000000);
4496-
check_closed_broadcast!(nodes[1], true);
44974496

44984497
// Now that B has a pending forwarded payment across it with the inbound edge on-chain, claim
44994498
// the payment on C and give B the preimage for it.
@@ -4567,9 +4566,9 @@ fn test_claim_to_closed_channel_blocks_claimed_event() {
45674566
assert_eq!(as_commit_tx.len(), 1);
45684567

45694568
mine_transaction(&nodes[1], &as_commit_tx[0]);
4569+
check_closed_broadcast!(nodes[1], true);
45704570
check_added_monitors!(nodes[1], 1);
45714571
check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [node_a_id], 1000000);
4572-
check_closed_broadcast!(nodes[1], true);
45734572

45744573
// Now that B has a pending payment with the inbound HTLC on a closed channel, claim the
45754574
// payment on disk, but don't let the `ChannelMonitorUpdate` complete. This should prevent the

‎lightning/src/ln/channel.rs‎

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -5775,35 +5775,6 @@ where
57755775
Ok(false)
57765776
}
57775777

5778-
#[rustfmt::skip]
5779-
fn check_for_funding_tx_spent<L: Deref>(
5780-
&mut self, funding: &FundingScope, tx: &Transaction, logger: &L,
5781-
) -> Result<(), ClosureReason>
5782-
where
5783-
L::Target: Logger,
5784-
{
5785-
let funding_txo = match funding.get_funding_txo() {
5786-
Some(funding_txo) => funding_txo,
5787-
None => {
5788-
debug_assert!(false);
5789-
return Ok(());
5790-
},
5791-
};
5792-
5793-
for input in tx.input.iter() {
5794-
if input.previous_output == funding_txo.into_bitcoin_outpoint() {
5795-
log_info!(
5796-
logger, "Detected channel-closing tx {} spending {}:{}, closing channel {}",
5797-
tx.compute_txid(), input.previous_output.txid, input.previous_output.vout,
5798-
&self.channel_id(),
5799-
);
5800-
return Err(ClosureReason::CommitmentTxConfirmed);
5801-
}
5802-
}
5803-
5804-
Ok(())
5805-
}
5806-
58075778
/// Returns SCIDs that have been associated with the channel's funding transactions.
58085779
pub fn historical_scids(&self) -> &[u64] {
58095780
&self.historical_scids[..]
@@ -10005,12 +9976,6 @@ where
100059976
}
100069977

100079978
if let Some(channel_ready) = self.check_get_channel_ready(height, logger) {
10008-
for &(idx, tx) in txdata.iter() {
10009-
if idx > index_in_block {
10010-
self.context.check_for_funding_tx_spent(&self.funding, tx, logger)?;
10011-
}
10012-
}
10013-
100149979
log_info!(logger, "Sending a channel_ready to our peer for channel {}", &self.context.channel_id);
100159980
let announcement_sigs = self.get_announcement_sigs(node_signer, chain_hash, user_config, height, logger);
100169981
return Ok((Some(FundingConfirmedMessage::Establishment(channel_ready)), announcement_sigs));
@@ -10051,12 +10016,6 @@ where
1005110016
let funding = self.pending_funding.get(confirmed_funding_index).unwrap();
1005210017

1005310018
if let Some(splice_locked) = pending_splice.check_get_splice_locked(&self.context, funding, height) {
10054-
for &(idx, tx) in txdata.iter() {
10055-
if idx > index_in_block {
10056-
self.context.check_for_funding_tx_spent(funding, tx, logger)?;
10057-
}
10058-
}
10059-
1006010019
log_info!(
1006110020
logger,
1006210021
"Sending splice_locked txid {} to our peer for channel {}",
@@ -10076,13 +10035,6 @@ where
1007610035
return Ok((Some(FundingConfirmedMessage::Splice(splice_locked, funding_txo, monitor_update)), announcement_sigs));
1007710036
}
1007810037
}
10079-
10080-
self.context.check_for_funding_tx_spent(&self.funding, tx, logger)?;
10081-
#[cfg(splicing)]
10082-
for funding in self.pending_funding.iter() {
10083-
self.context.check_for_funding_tx_spent(funding, tx, logger)?;
10084-
}
10085-
1008610038
}
1008710039

1008810040
Ok((None, None))

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11316,6 +11316,22 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1131611316
}
1131711317
}
1131811318
},
11319+
MonitorEvent::CommitmentTxConfirmed(_) => {
11320+
let per_peer_state = self.per_peer_state.read().unwrap();
11321+
if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) {
11322+
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
11323+
let peer_state = &mut *peer_state_lock;
11324+
if let hash_map::Entry::Occupied(chan_entry) =
11325+
peer_state.channel_by_id.entry(channel_id)
11326+
{
11327+
let reason = ClosureReason::CommitmentTxConfirmed;
11328+
let err = ChannelError::Close((reason.to_string(), reason));
11329+
let mut chan = chan_entry.remove();
11330+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
11331+
failed_channels.push((Err(e), counterparty_node_id));
11332+
}
11333+
}
11334+
},
1131911335
MonitorEvent::Completed { channel_id, monitor_update_id, .. } => {
1132011336
self.channel_monitor_updated(
1132111337
&channel_id,

0 commit comments

Comments
 (0)