Skip to content

Correctly detect missing HTLCs when a local commitment tx was broadcast #1025

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
merged 2 commits into from
Aug 10, 2021
Merged

Correctly detect missing HTLCs when a local commitment tx was broadcast #1025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
925e642
DRY HTLC failure code in check_spend_counterparty_transaction
TheBlueMatt Jul 31, 2021
6bfab9d
Correctly detect missing HTLCs when a local commitment tx was broadcast
TheBlueMatt Jul 31, 2021
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
204 changes: 82 additions & 122 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,9 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
on_holder_tx_csv: u16,

commitment_secrets: CounterpartyCommitmentSecrets,
/// The set of outpoints in each counterparty commitment transaction. We always need at least
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a true statement: counterparty_claimable_outpoints is the set of (outpoints_in_prev_counterparty_commit_tx_if_not_revoked, outpoints_in_curr_counterparty_commit_tx)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

counterparty_claimable_outpoints, somewhat confusingly named, is a map from a single counterparty commitment transaction (by txid) to the set of HTLCs which were included in that transaction.

/// the payment hash from `HTLCOutputInCommitment` to claim even a revoked commitment
/// transaction broadcast as we need to be able to construct the witness script in all cases.
counterparty_claimable_outpoints: HashMap<Txid, Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>,
/// We cannot identify HTLC-Success or HTLC-Timeout transactions by themselves on the chain.
/// Nor can we figure out their commitment numbers without the commitment transaction they are
Expand Down Expand Up @@ -1201,6 +1204,81 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
}
}

/// Compares a broadcasted commitment transaction's HTLCs with those in the latest state,
/// failing any HTLCs which didn't make it into the broadcasted commitment transaction back
/// after ANTI_REORG_DELAY blocks.
///
/// We always compare against the set of HTLCs in counterparty commitment transactions, as those
/// are the commitment transactions which are generated by us. The off-chain state machine in
/// `Channel` will automatically resolve any HTLCs which were never included in a commitment
/// transaction when it detects channel closure, but it is up to us to ensure any HTLCs which were
/// included in a remote commitment transaction are failed back if they are not present in the
/// broadcasted commitment transaction.
///
/// Specifically, the removal process for HTLCs in `Channel` is always based on the counterparty
/// sending a `revoke_and_ack`, which causes us to clear `prev_counterparty_commitment_txid`. Thus,
/// as long as we examine both the current counterparty commitment transaction and, if it hasn't
/// been revoked yet, the previous one, we we will never "forget" to resolve an HTLC.
macro_rules! fail_unbroadcast_htlcs {
($self: expr, $commitment_tx_type: expr, $commitment_tx_conf_height: expr, $confirmed_htlcs_list: expr, $logger: expr) => { {
macro_rules! check_htlc_fails {
($txid: expr, $commitment_tx: expr) => {
if let Some(ref latest_outpoints) = $self.counterparty_claimable_outpoints.get($txid) {
for &(ref htlc, ref source_option) in latest_outpoints.iter() {
if let &Some(ref source) = source_option {
// Check if the HTLC is present in the commitment transaction that was
// broadcast, but not if it was below the dust limit, which we should
// fail backwards immediately as there is no way for us to learn the
// payment_preimage.
// Note that if the dust limit were allowed to change between
// commitment transactions we'd want to be check whether *any*
// broadcastable commitment transaction has the HTLC in it, but it
// cannot currently change after channel initialization, so we don't
// need to here.
let confirmed_htlcs_iter: &mut Iterator<Item = (&HTLCOutputInCommitment, Option<&HTLCSource>)> = &mut $confirmed_htlcs_list;
let mut matched_htlc = false;
for (ref broadcast_htlc, ref broadcast_source) in confirmed_htlcs_iter {
if broadcast_htlc.transaction_output_index.is_some() && Some(&**source) == *broadcast_source {
matched_htlc = true;
break;
}
}
if matched_htlc { continue; }
$self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
if entry.height != $commitment_tx_conf_height { return true; }
match entry.event {
OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
*update_source != **source
},
_ => true,
}
});
let entry = OnchainEventEntry {
txid: *$txid,
height: $commitment_tx_conf_height,
event: OnchainEvent::HTLCUpdate {
source: (**source).clone(),
payment_hash: htlc.payment_hash.clone(),
onchain_value_satoshis: Some(htlc.amount_msat / 1000),
},
};
log_trace!($logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of {} commitment transaction, waiting for confirmation (at height {})",
log_bytes!(htlc.payment_hash.0), $commitment_tx, $commitment_tx_type, entry.confirmation_threshold());
$self.onchain_events_awaiting_threshold_conf.push(entry);
}
}
}
}
}
if let Some(ref txid) = $self.current_counterparty_commitment_txid {
check_htlc_fails!(txid, "current");
}
if let Some(ref txid) = $self.prev_counterparty_commitment_txid {
check_htlc_fails!(txid, "previous");
}
} }
}

impl<Signer: Sign> ChannelMonitorImpl<Signer> {
/// Inserts a revocation secret into this channel monitor. Prunes old preimages if neither
/// needed by holder commitment transactions HTCLs nor by counterparty ones. Unless we haven't already seen
Expand Down Expand Up @@ -1558,43 +1636,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
}
self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);

macro_rules! check_htlc_fails {
($txid: expr, $commitment_tx: expr) => {
if let Some(ref outpoints) = self.counterparty_claimable_outpoints.get($txid) {
for &(ref htlc, ref source_option) in outpoints.iter() {
if let &Some(ref source) = source_option {
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
if entry.height != height { return true; }
match entry.event {
OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
*update_source != **source
},
_ => true,
}
});
let entry = OnchainEventEntry {
txid: *$txid,
height,
event: OnchainEvent::HTLCUpdate {
source: (**source).clone(),
payment_hash: htlc.payment_hash.clone(),
onchain_value_satoshis: Some(htlc.amount_msat / 1000),
},
};
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());
self.onchain_events_awaiting_threshold_conf.push(entry);
}
}
}
}
}
if let Some(ref txid) = self.current_counterparty_commitment_txid {
check_htlc_fails!(txid, "current");
}
if let Some(ref txid) = self.prev_counterparty_commitment_txid {
check_htlc_fails!(txid, "counterparty");
}
// No need to check holder commitment txn, symmetric HTLCSource must be present as per-htlc data on counterparty commitment tx
fail_unbroadcast_htlcs!(self, "revoked counterparty", height, [].iter().map(|a| *a), logger);
}
} else if let Some(per_commitment_data) = per_commitment_option {
// While this isn't useful yet, there is a potential race where if a counterparty
Expand All @@ -1610,56 +1652,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);

log_info!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid);

macro_rules! check_htlc_fails {
($txid: expr, $commitment_tx: expr, $id: tt) => {
if let Some(ref latest_outpoints) = self.counterparty_claimable_outpoints.get($txid) {
$id: for &(ref htlc, ref source_option) in latest_outpoints.iter() {
if let &Some(ref source) = source_option {
// Check if the HTLC is present in the commitment transaction that was
// broadcast, but not if it was below the dust limit, which we should
// fail backwards immediately as there is no way for us to learn the
// payment_preimage.
// Note that if the dust limit were allowed to change between
// commitment transactions we'd want to be check whether *any*
// broadcastable commitment transaction has the HTLC in it, but it
// cannot currently change after channel initialization, so we don't
// need to here.
for &(ref broadcast_htlc, ref broadcast_source) in per_commitment_data.iter() {
if broadcast_htlc.transaction_output_index.is_some() && Some(source) == broadcast_source.as_ref() {
continue $id;
}
}
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);
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
if entry.height != height { return true; }
match entry.event {
OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
*update_source != **source
},
_ => true,
}
});
self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry {
txid: *$txid,
height,
event: OnchainEvent::HTLCUpdate {
source: (**source).clone(),
payment_hash: htlc.payment_hash.clone(),
onchain_value_satoshis: Some(htlc.amount_msat / 1000),
},
});
}
}
}
}
}
if let Some(ref txid) = self.current_counterparty_commitment_txid {
check_htlc_fails!(txid, "current", 'current_loop);
}
if let Some(ref txid) = self.prev_counterparty_commitment_txid {
check_htlc_fails!(txid, "previous", 'prev_loop);
}
fail_unbroadcast_htlcs!(self, "counterparty", height, per_commitment_data.iter().map(|(a, b)| (a, b.as_ref().map(|b| b.as_ref()))), logger);

let htlc_claim_reqs = self.get_counterparty_htlc_output_claim_reqs(commitment_number, commitment_txid, Some(tx));
for req in htlc_claim_reqs {
Expand Down Expand Up @@ -1802,52 +1795,19 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
let mut to_watch = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, tx);
append_onchain_update!(res, to_watch);
fail_unbroadcast_htlcs!(self, "latest holder", height, self.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
} else if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
if holder_tx.txid == commitment_txid {
is_holder_tx = true;
log_info!(logger, "Got broadcast of previous holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
let res = self.get_broadcasted_holder_claims(holder_tx, height);
let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_tx, tx);
append_onchain_update!(res, to_watch);
}
}

macro_rules! fail_dust_htlcs_after_threshold_conf {
($holder_tx: expr, $commitment_tx: expr) => {
for &(ref htlc, _, ref source) in &$holder_tx.htlc_outputs {
if htlc.transaction_output_index.is_none() {
if let &Some(ref source) = source {
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| {
if entry.height != height { return true; }
match entry.event {
OnchainEvent::HTLCUpdate { source: ref update_source, .. } => {
update_source != source
},
_ => true,
}
});
let entry = OnchainEventEntry {
txid: commitment_txid,
height,
event: OnchainEvent::HTLCUpdate {
source: source.clone(), payment_hash: htlc.payment_hash,
onchain_value_satoshis: Some(htlc.amount_msat / 1000)
},
};
log_trace!(logger, "Failing HTLC with payment_hash {} from {} holder commitment tx due to broadcast of transaction, waiting confirmation (at height{})",
log_bytes!(htlc.payment_hash.0), $commitment_tx, entry.confirmation_threshold());
self.onchain_events_awaiting_threshold_conf.push(entry);
}
}
}
fail_unbroadcast_htlcs!(self, "previous holder", height, holder_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify: is the main difference in logic that before we would only consider the relevant commitment tx's outputs but now we consider all of counterparty_claimable_outpoints?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, so the trick here is that the counterparty commitment transaction is the "latest" one, because its the one that we generated. The local commitment transaction is the one our counterparty generated, so isn't always the HTLC state that matches our ChannelMonitor state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add some docs (including the info in your comment^) to counterparty_claimable_outpoints? I had to search around the file to get some context about it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some docs to counterparty_claimable_outpoints, and expanded the docs on fail_unbroadcast_htlcs to describe my reasoning.

}
}

if is_holder_tx {
fail_dust_htlcs_after_threshold_conf!(self.current_holder_commitment_tx, "latest");
if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
fail_dust_htlcs_after_threshold_conf!(holder_tx, "previous");
}
}

(claim_requests, (commitment_txid, watch_outputs))
Expand Down
3 changes: 3 additions & 0 deletions lightning/src/ln/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ mod reorg_tests;
#[cfg(test)]
#[allow(unused_mut)]
mod onion_route_tests;
#[cfg(test)]
#[allow(unused_mut)]
mod monitor_tests;

pub use self::peer_channel_encryptor::LN_MAX_MSG_LEN;

Expand Down
81 changes: 81 additions & 0 deletions lightning/src/ln/monitor_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// This file is Copyright its original authors, visible in version control
// history.
//
// This file is licensed under the Apache License, Version 2.0 <LICENSE-APACHE
// or http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your option.
// You may not use this file except in accordance with one or both of these
// licenses.

//! Further functional tests which test blockchain reorganizations.

use chain::channelmonitor::ANTI_REORG_DELAY;
use ln::{PaymentPreimage, PaymentHash};
use ln::features::InitFeatures;
use ln::msgs::{ChannelMessageHandler, HTLCFailChannelUpdate, ErrorAction};
use util::events::{Event, MessageSendEvent, MessageSendEventsProvider};
use routing::router::get_route;

use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hashes::Hash;

use prelude::*;

use ln::functional_test_utils::*;

#[test]
fn chanmon_fail_from_stale_commitment() {
// 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, previously, we did not do this, ignoring the HTLC entirely.
//
// This could lead to channel closure if the sender we received the HTLC from decides to go on
// chain to get their HTLC back before it times out.
//
// Here, we check exactly this case, forwarding a payment from A, through B, to C, before B
// broadcasts its latest commitment transaction, which should result in it eventually failing
// the HTLC back off-chain to A.
let chanmon_cfgs = create_chanmon_cfgs(3);
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary mut

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Old rustc incorrectly rejected this as missing-mut, see comment in lightning/src/ln/mod.rs. I added an ignore for this.

error[E0596]: cannot borrow `nodes` as mutable, as it is not declared as mutable
  --> lightning/src/ln/monitor_tests.rs:51:87
   |
46 |     let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
   |         ----- help: consider changing this to be mutable: `mut nodes`
...
51 |     let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
   |                                                                                          ^^^^^ cannot borrow as mutable


create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
let (update_a, _, chan_id_2, _) = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());

let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
check_added_monitors!(nodes[0], 1);

let bs_txn = get_local_commitment_txn!(nodes[1], chan_id_2);

let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
commitment_signed_dance!(nodes[1], nodes[0], updates.commitment_signed, false);

expect_pending_htlcs_forwardable!(nodes[1]);
get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
check_added_monitors!(nodes[1], 1);

// Don't bother delivering the new HTLC add/commits, instead confirming the pre-HTLC commitment
// transaction for nodes[1].
mine_transaction(&nodes[1], &bs_txn[0]);
check_added_monitors!(nodes[1], 1);
check_closed_broadcast!(nodes[1], true);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
expect_pending_htlcs_forwardable!(nodes[1]);
check_added_monitors!(nodes[1], 1);
let fail_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());

nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[0], nodes[1], fail_updates.commitment_signed, true, true);
expect_payment_failed!(nodes[0], payment_hash, false);
expect_payment_failure_chan_update!(nodes[0], update_a.contents.short_channel_id, true);
}