Skip to content

Commit 6bfab9d

Browse files
committed
Correctly detect missing HTLCs when a local commitment tx was broadcast
If we forward an HTLC to our counterparty, but we force-closed the channel before our counterparty provides us an updated commitment transaction, we'll end up with a commitment transaction that does not contain the HTLC which we attempted to forward. In this case, we need to wait `ANTI_REORG_DELAY` blocks and then fail back the HTLC as there is no way for us to learn the preimage and the confirmed commitment transaction paid us the value of the HTLC. However, check_spend_holder_transaction did not do this - it instead only looked for dust HTLCs in the confirmed commitment transaction, paying no attention to what other HTLCs may exist that are missed. This will eventually lead to channel force-closure as the channel on which we received the inbound HTLC to forward will be closed in time for the initial sender to claim the HTLC on-chain.
1 parent 925e642 commit 6bfab9d

File tree

3 files changed

+101
-35
lines changed

3 files changed

+101
-35
lines changed

lightning/src/chain/channelmonitor.rs

Lines changed: 17 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,9 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
510510
on_holder_tx_csv: u16,
511511

512512
commitment_secrets: CounterpartyCommitmentSecrets,
513+
/// The set of outpoints in each counterparty commitment transaction. We always need at least
514+
/// the payment hash from `HTLCOutputInCommitment` to claim even a revoked commitment
515+
/// transaction broadcast as we need to be able to construct the witness script in all cases.
513516
counterparty_claimable_outpoints: HashMap<Txid, Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,
514517
/// We cannot identify HTLC-Success or HTLC-Timeout transactions by themselves on the chain.
515518
/// Nor can we figure out their commitment numbers without the commitment transaction they are
@@ -1204,6 +1207,18 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
12041207
/// Compares a broadcasted commitment transaction's HTLCs with those in the latest state,
12051208
/// failing any HTLCs which didn't make it into the broadcasted commitment transaction back
12061209
/// after ANTI_REORG_DELAY blocks.
1210+
///
1211+
/// We always compare against the set of HTLCs in counterparty commitment transactions, as those
1212+
/// are the commitment transactions which are generated by us. The off-chain state machine in
1213+
/// `Channel` will automatically resolve any HTLCs which were never included in a commitment
1214+
/// transaction when it detects channel closure, but it is up to us to ensure any HTLCs which were
1215+
/// included in a remote commitment transaction are failed back if they are not present in the
1216+
/// broadcasted commitment transaction.
1217+
///
1218+
/// Specifically, the removal process for HTLCs in `Channel` is always based on the counterparty
1219+
/// sending a `revoke_and_ack`, which causes us to clear `prev_counterparty_commitment_txid`. Thus,
1220+
/// as long as we examine both the current counterparty commitment transaction and, if it hasn't
1221+
/// been revoked yet, the previous one, we we will never "forget" to resolve an HTLC.
12071222
macro_rules! fail_unbroadcast_htlcs {
12081223
($self: expr, $commitment_tx_type: expr, $commitment_tx_conf_height: expr, $confirmed_htlcs_list: expr, $logger: expr) => { {
12091224
macro_rules! check_htlc_fails {
@@ -1780,52 +1795,19 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
17801795
let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
17811796
let mut to_watch = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, tx);
17821797
append_onchain_update!(res, to_watch);
1798+
fail_unbroadcast_htlcs!(self, "latest holder", height, self.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
17831799
} else if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
17841800
if holder_tx.txid == commitment_txid {
17851801
is_holder_tx = true;
17861802
log_info!(logger, "Got broadcast of previous holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
17871803
let res = self.get_broadcasted_holder_claims(holder_tx, height);
17881804
let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_tx, tx);
17891805
append_onchain_update!(res, to_watch);
1790-
}
1791-
}
1792-
1793-
macro_rules! fail_dust_htlcs_after_threshold_conf {
1794-
($holder_tx: expr, $commitment_tx: expr) => {
1795-
for &(ref htlc, _, ref source) in &$holder_tx.htlc_outputs {
1796-
if htlc.transaction_output_index.is_none() {
1797-
if let &Some(ref source) = source {
1798-
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
1799-
if entry.height != height { return true; }
1800-
match entry.event {
1801-
OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
1802-
update_source != source
1803-
},
1804-
_ => true,
1805-
}
1806-
});
1807-
let entry = OnchainEventEntry {
1808-
txid: commitment_txid,
1809-
height,
1810-
event: OnchainEvent::HTLCUpdate {
1811-
source: source.clone(), payment_hash: htlc.payment_hash,
1812-
onchain_value_satoshis: Some(htlc.amount_msat / 1000)
1813-
},
1814-
};
1815-
log_trace!(logger, "Failing HTLC with payment_hash {} from {} holder commitment tx due to broadcast of transaction, waiting confirmation (at height{})",
1816-
log_bytes!(htlc.payment_hash.0), $commitment_tx, entry.confirmation_threshold());
1817-
self.onchain_events_awaiting_threshold_conf.push(entry);
1818-
}
1819-
}
1820-
}
1806+
fail_unbroadcast_htlcs!(self, "previous holder", height, holder_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
18211807
}
18221808
}
18231809

18241810
if is_holder_tx {
1825-
fail_dust_htlcs_after_threshold_conf!(self.current_holder_commitment_tx, "latest");
1826-
if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
1827-
fail_dust_htlcs_after_threshold_conf!(holder_tx, "previous");
1828-
}
18291811
}
18301812

18311813
(claim_requests, (commitment_txid, watch_outputs))

lightning/src/ln/mod.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@ mod reorg_tests;
5252
#[cfg(test)]
5353
#[allow(unused_mut)]
5454
mod onion_route_tests;
55+
#[cfg(test)]
56+
#[allow(unused_mut)]
57+
mod monitor_tests;
5558

5659
pub use self::peer_channel_encryptor::LN_MAX_MSG_LEN;
5760

lightning/src/ln/monitor_tests.rs

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// This file is Copyright its original authors, visible in version control
2+
// history.
3+
//
4+
// This file is licensed under the Apache License, Version 2.0 <LICENSE-APACHE
5+
// or http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
6+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your option.
7+
// You may not use this file except in accordance with one or both of these
8+
// licenses.
9+
10+
//! Further functional tests which test blockchain reorganizations.
11+
12+
use chain::channelmonitor::ANTI_REORG_DELAY;
13+
use ln::{PaymentPreimage, PaymentHash};
14+
use ln::features::InitFeatures;
15+
use ln::msgs::{ChannelMessageHandler, HTLCFailChannelUpdate, ErrorAction};
16+
use util::events::{Event, MessageSendEvent, MessageSendEventsProvider};
17+
use routing::router::get_route;
18+
19+
use bitcoin::hashes::sha256::Hash as Sha256;
20+
use bitcoin::hashes::Hash;
21+
22+
use prelude::*;
23+
24+
use ln::functional_test_utils::*;
25+
26+
#[test]
27+
fn chanmon_fail_from_stale_commitment() {
28+
// If we forward an HTLC to our counterparty, but we force-closed the channel before our
29+
// counterparty provides us an updated commitment transaction, we'll end up with a commitment
30+
// transaction that does not contain the HTLC which we attempted to forward. In this case, we
31+
// need to wait `ANTI_REORG_DELAY` blocks and then fail back the HTLC as there is no way for us
32+
// to learn the preimage and the confirmed commitment transaction paid us the value of the
33+
// HTLC.
34+
//
35+
// However, previously, we did not do this, ignoring the HTLC entirely.
36+
//
37+
// This could lead to channel closure if the sender we received the HTLC from decides to go on
38+
// chain to get their HTLC back before it times out.
39+
//
40+
// Here, we check exactly this case, forwarding a payment from A, through B, to C, before B
41+
// broadcasts its latest commitment transaction, which should result in it eventually failing
42+
// the HTLC back off-chain to A.
43+
let chanmon_cfgs = create_chanmon_cfgs(3);
44+
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
45+
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
46+
let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
47+
48+
create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
49+
let (update_a, _, chan_id_2, _) = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
50+
51+
let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
52+
nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
53+
check_added_monitors!(nodes[0], 1);
54+
55+
let bs_txn = get_local_commitment_txn!(nodes[1], chan_id_2);
56+
57+
let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
58+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
59+
commitment_signed_dance!(nodes[1], nodes[0], updates.commitment_signed, false);
60+
61+
expect_pending_htlcs_forwardable!(nodes[1]);
62+
get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
63+
check_added_monitors!(nodes[1], 1);
64+
65+
// Don't bother delivering the new HTLC add/commits, instead confirming the pre-HTLC commitment
66+
// transaction for nodes[1].
67+
mine_transaction(&nodes[1], &bs_txn[0]);
68+
check_added_monitors!(nodes[1], 1);
69+
check_closed_broadcast!(nodes[1], true);
70+
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
71+
72+
connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
73+
expect_pending_htlcs_forwardable!(nodes[1]);
74+
check_added_monitors!(nodes[1], 1);
75+
let fail_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
76+
77+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates.update_fail_htlcs[0]);
78+
commitment_signed_dance!(nodes[0], nodes[1], fail_updates.commitment_signed, true, true);
79+
expect_payment_failed!(nodes[0], payment_hash, false);
80+
expect_payment_failure_chan_update!(nodes[0], update_a.contents.short_channel_id, true);
81+
}

0 commit comments

Comments
 (0)