Skip to content

Commit d4b6f58

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #1025 from TheBlueMatt/2021-07-detect-htlcs-on-local-commitment
2 parents 6a94ff9 + 6bfab9d commit d4b6f58

File tree

3 files changed

+166
-122
lines changed

3 files changed

+166
-122
lines changed

‎lightning/src/chain/channelmonitor.rs‎

Lines changed: 82 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,9 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
516516
on_holder_tx_csv: u16,
517517

518518
commitment_secrets: CounterpartyCommitmentSecrets,
519+
/// The set of outpoints in each counterparty commitment transaction. We always need at least
520+
/// the payment hash from `HTLCOutputInCommitment` to claim even a revoked commitment
521+
/// transaction broadcast as we need to be able to construct the witness script in all cases.
519522
counterparty_claimable_outpoints: HashMap<Txid, Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,
520523
/// We cannot identify HTLC-Success or HTLC-Timeout transactions by themselves on the chain.
521524
/// Nor can we figure out their commitment numbers without the commitment transaction they are
@@ -1208,6 +1211,81 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
12081211
}
12091212
}
12101213

1214+
/// Compares a broadcasted commitment transaction's HTLCs with those in the latest state,
1215+
/// failing any HTLCs which didn't make it into the broadcasted commitment transaction back
1216+
/// after ANTI_REORG_DELAY blocks.
1217+
///
1218+
/// We always compare against the set of HTLCs in counterparty commitment transactions, as those
1219+
/// are the commitment transactions which are generated by us. The off-chain state machine in
1220+
/// `Channel` will automatically resolve any HTLCs which were never included in a commitment
1221+
/// transaction when it detects channel closure, but it is up to us to ensure any HTLCs which were
1222+
/// included in a remote commitment transaction are failed back if they are not present in the
1223+
/// broadcasted commitment transaction.
1224+
///
1225+
/// Specifically, the removal process for HTLCs in `Channel` is always based on the counterparty
1226+
/// sending a `revoke_and_ack`, which causes us to clear `prev_counterparty_commitment_txid`. Thus,
1227+
/// as long as we examine both the current counterparty commitment transaction and, if it hasn't
1228+
/// been revoked yet, the previous one, we we will never "forget" to resolve an HTLC.
1229+
macro_rules! fail_unbroadcast_htlcs {
1230+
($self: expr, $commitment_tx_type: expr, $commitment_tx_conf_height: expr, $confirmed_htlcs_list: expr, $logger: expr) => { {
1231+
macro_rules! check_htlc_fails {
1232+
($txid: expr, $commitment_tx: expr) => {
1233+
if let Some(ref latest_outpoints) = $self.counterparty_claimable_outpoints.get($txid) {
1234+
for &(ref htlc, ref source_option) in latest_outpoints.iter() {
1235+
if let &Some(ref source) = source_option {
1236+
// Check if the HTLC is present in the commitment transaction that was
1237+
// broadcast, but not if it was below the dust limit, which we should
1238+
// fail backwards immediately as there is no way for us to learn the
1239+
// payment_preimage.
1240+
// Note that if the dust limit were allowed to change between
1241+
// commitment transactions we'd want to be check whether *any*
1242+
// broadcastable commitment transaction has the HTLC in it, but it
1243+
// cannot currently change after channel initialization, so we don't
1244+
// need to here.
1245+
let confirmed_htlcs_iter: &mut Iterator<Item = (&HTLCOutputInCommitment, Option<&HTLCSource>)> = &mut $confirmed_htlcs_list;
1246+
let mut matched_htlc = false;
1247+
for (ref broadcast_htlc, ref broadcast_source) in confirmed_htlcs_iter {
1248+
if broadcast_htlc.transaction_output_index.is_some() && Some(&**source) == *broadcast_source {
1249+
matched_htlc = true;
1250+
break;
1251+
}
1252+
}
1253+
if matched_htlc { continue; }
1254+
$self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
1255+
if entry.height != $commitment_tx_conf_height { return true; }
1256+
match entry.event {
1257+
OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
1258+
*update_source != **source
1259+
},
1260+
_ => true,
1261+
}
1262+
});
1263+
let entry = OnchainEventEntry {
1264+
txid: *$txid,
1265+
height: $commitment_tx_conf_height,
1266+
event: OnchainEvent::HTLCUpdate {
1267+
source: (**source).clone(),
1268+
payment_hash: htlc.payment_hash.clone(),
1269+
onchain_value_satoshis: Some(htlc.amount_msat / 1000),
1270+
},
1271+
};
1272+
log_trace!($logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of {} commitment transaction, waiting for confirmation (at height {})",
1273+
log_bytes!(htlc.payment_hash.0), $commitment_tx, $commitment_tx_type, entry.confirmation_threshold());
1274+
$self.onchain_events_awaiting_threshold_conf.push(entry);
1275+
}
1276+
}
1277+
}
1278+
}
1279+
}
1280+
if let Some(ref txid) = $self.current_counterparty_commitment_txid {
1281+
check_htlc_fails!(txid, "current");
1282+
}
1283+
if let Some(ref txid) = $self.prev_counterparty_commitment_txid {
1284+
check_htlc_fails!(txid, "previous");
1285+
}
1286+
} }
1287+
}
1288+
12111289
impl<Signer: Sign> ChannelMonitorImpl<Signer> {
12121290
/// Inserts a revocation secret into this channel monitor. Prunes old preimages if neither
12131291
/// needed by holder commitment transactions HTCLs nor by counterparty ones. Unless we haven't already seen
@@ -1571,43 +1649,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
15711649
}
15721650
self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
15731651

1574-
macro_rules! check_htlc_fails {
1575-
($txid: expr, $commitment_tx: expr) => {
1576-
if let Some(ref outpoints) = self.counterparty_claimable_outpoints.get($txid) {
1577-
for &(ref htlc, ref source_option) in outpoints.iter() {
1578-
if let &Some(ref source) = source_option {
1579-
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
1580-
if entry.height != height { return true; }
1581-
match entry.event {
1582-
OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
1583-
*update_source != **source
1584-
},
1585-
_ => true,
1586-
}
1587-
});
1588-
let entry = OnchainEventEntry {
1589-
txid: *$txid,
1590-
height,
1591-
event: OnchainEvent::HTLCUpdate {
1592-
source: (**source).clone(),
1593-
payment_hash: htlc.payment_hash.clone(),
1594-
onchain_value_satoshis: Some(htlc.amount_msat / 1000),
1595-
},
1596-
};
1597-
log_info!(logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of revoked counterparty commitment transaction, waiting for confirmation (at height {})", log_bytes!(htlc.payment_hash.0), $commitment_tx, entry.confirmation_threshold());
1598-
self.onchain_events_awaiting_threshold_conf.push(entry);
1599-
}
1600-
}
1601-
}
1602-
}
1603-
}
1604-
if let Some(ref txid) = self.current_counterparty_commitment_txid {
1605-
check_htlc_fails!(txid, "current");
1606-
}
1607-
if let Some(ref txid) = self.prev_counterparty_commitment_txid {
1608-
check_htlc_fails!(txid, "counterparty");
1609-
}
1610-
// No need to check holder commitment txn, symmetric HTLCSource must be present as per-htlc data on counterparty commitment tx
1652+
fail_unbroadcast_htlcs!(self, "revoked counterparty", height, [].iter().map(|a| *a), logger);
16111653
}
16121654
} else if let Some(per_commitment_data) = per_commitment_option {
16131655
// While this isn't useful yet, there is a potential race where if a counterparty
@@ -1623,56 +1665,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
16231665
self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
16241666

16251667
log_info!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid);
1626-
1627-
macro_rules! check_htlc_fails {
1628-
($txid: expr, $commitment_tx: expr, $id: tt) => {
1629-
if let Some(ref latest_outpoints) = self.counterparty_claimable_outpoints.get($txid) {
1630-
$id: for &(ref htlc, ref source_option) in latest_outpoints.iter() {
1631-
if let &Some(ref source) = source_option {
1632-
// Check if the HTLC is present in the commitment transaction that was
1633-
// broadcast, but not if it was below the dust limit, which we should
1634-
// fail backwards immediately as there is no way for us to learn the
1635-
// payment_preimage.
1636-
// Note that if the dust limit were allowed to change between
1637-
// commitment transactions we'd want to be check whether *any*
1638-
// broadcastable commitment transaction has the HTLC in it, but it
1639-
// cannot currently change after channel initialization, so we don't
1640-
// need to here.
1641-
for &(ref broadcast_htlc, ref broadcast_source) in per_commitment_data.iter() {
1642-
if broadcast_htlc.transaction_output_index.is_some() && Some(source) == broadcast_source.as_ref() {
1643-
continue $id;
1644-
}
1645-
}
1646-
log_trace!(logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of counterparty commitment transaction", log_bytes!(htlc.payment_hash.0), $commitment_tx);
1647-
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
1648-
if entry.height != height { return true; }
1649-
match entry.event {
1650-
OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
1651-
*update_source != **source
1652-
},
1653-
_ => true,
1654-
}
1655-
});
1656-
self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry {
1657-
txid: *$txid,
1658-
height,
1659-
event: OnchainEvent::HTLCUpdate {
1660-
source: (**source).clone(),
1661-
payment_hash: htlc.payment_hash.clone(),
1662-
onchain_value_satoshis: Some(htlc.amount_msat / 1000),
1663-
},
1664-
});
1665-
}
1666-
}
1667-
}
1668-
}
1669-
}
1670-
if let Some(ref txid) = self.current_counterparty_commitment_txid {
1671-
check_htlc_fails!(txid, "current", 'current_loop);
1672-
}
1673-
if let Some(ref txid) = self.prev_counterparty_commitment_txid {
1674-
check_htlc_fails!(txid, "previous", 'prev_loop);
1675-
}
1668+
fail_unbroadcast_htlcs!(self, "counterparty", height, per_commitment_data.iter().map(|(a, b)| (a, b.as_ref().map(|b| b.as_ref()))), logger);
16761669

16771670
let htlc_claim_reqs = self.get_counterparty_htlc_output_claim_reqs(commitment_number, commitment_txid, Some(tx));
16781671
for req in htlc_claim_reqs {
@@ -1815,52 +1808,19 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
18151808
let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
18161809
let mut to_watch = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, tx);
18171810
append_onchain_update!(res, to_watch);
1811+
fail_unbroadcast_htlcs!(self, "latest holder", height, self.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
18181812
} else if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
18191813
if holder_tx.txid == commitment_txid {
18201814
is_holder_tx = true;
18211815
log_info!(logger, "Got broadcast of previous holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
18221816
let res = self.get_broadcasted_holder_claims(holder_tx, height);
18231817
let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_tx, tx);
18241818
append_onchain_update!(res, to_watch);
1825-
}
1826-
}
1827-
1828-
macro_rules! fail_dust_htlcs_after_threshold_conf {
1829-
($holder_tx: expr, $commitment_tx: expr) => {
1830-
for &(ref htlc, _, ref source) in &$holder_tx.htlc_outputs {
1831-
if htlc.transaction_output_index.is_none() {
1832-
if let &Some(ref source) = source {
1833-
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
1834-
if entry.height != height { return true; }
1835-
match entry.event {
1836-
OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
1837-
update_source != source
1838-
},
1839-
_ => true,
1840-
}
1841-
});
1842-
let entry = OnchainEventEntry {
1843-
txid: commitment_txid,
1844-
height,
1845-
event: OnchainEvent::HTLCUpdate {
1846-
source: source.clone(), payment_hash: htlc.payment_hash,
1847-
onchain_value_satoshis: Some(htlc.amount_msat / 1000)
1848-
},
1849-
};
1850-
log_trace!(logger, "Failing HTLC with payment_hash {} from {} holder commitment tx due to broadcast of transaction, waiting confirmation (at height{})",
1851-
log_bytes!(htlc.payment_hash.0), $commitment_tx, entry.confirmation_threshold());
1852-
self.onchain_events_awaiting_threshold_conf.push(entry);
1853-
}
1854-
}
1855-
}
1819+
fail_unbroadcast_htlcs!(self, "previous holder", height, holder_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
18561820
}
18571821
}
18581822

18591823
if is_holder_tx {
1860-
fail_dust_htlcs_after_threshold_conf!(self.current_holder_commitment_tx, "latest");
1861-
if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
1862-
fail_dust_htlcs_after_threshold_conf!(holder_tx, "previous");
1863-
}
18641824
}
18651825

18661826
(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
@@ -53,6 +53,9 @@ mod reorg_tests;
5353
#[cfg(test)]
5454
#[allow(unused_mut)]
5555
mod onion_route_tests;
56+
#[cfg(test)]
57+
#[allow(unused_mut)]
58+
mod monitor_tests;
5659

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

‎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)