Skip to content

Fix duplicate HTLC fail-back on stale force-close #4010

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+232 −7
Draft

Fix duplicate HTLC fail-back on stale force-close #4010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
91 changes: 86 additions & 5 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1356,7 +1356,7 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
/// a downstream channel force-close remaining unconfirmed by the time the upstream timeout
/// expires. This is used to tell us we already generated an event to fail this HTLC back
/// during a previous block scan.
failed_back_htlc_ids: HashSet<SentHTLCId>,
failed_back_htlc_ids: HashMap<SentHTLCId, u64>,

// The auxiliary HTLC data associated with a holder commitment transaction. This includes
// non-dust HTLC sources, along with dust HTLCs and their sources. Note that this assumes any
Expand Down Expand Up @@ -1956,7 +1956,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
initial_counterparty_commitment_tx: None,
balances_empty_height: None,

failed_back_htlc_ids: new_hash_set(),
failed_back_htlc_ids: new_hash_map(),

// There are never any HTLCs in the initial commitment transaction
current_holder_htlc_data: CommitmentHTLCData::new(),
Expand Down Expand Up @@ -5523,7 +5523,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
let mut matured_htlcs = Vec::new();

// Produce actionable events from on-chain events having reached their threshold.
for entry in onchain_events_reaching_threshold_conf {
for entry in onchain_events_reaching_threshold_conf.clone() {
match entry.event {
OnchainEvent::HTLCUpdate { source, payment_hash, htlc_value_satoshis, commitment_tx_output_idx } => {
// Check for duplicate HTLC resolutions.
Expand Down Expand Up @@ -5587,6 +5587,85 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
});
}
}

// Only act on stale force-closes where the confirmed funding spend is the
// previous counterparty commitment transaction.
let prev_txid = match self.funding.prev_counterparty_commitment_txid {
Some(txid) => {
if txid != entry.txid { continue; }
txid
},
None => { continue; }
};

// Process only HTLCs from that previous counterparty commitment.
let prev_outputs = if let Some(outputs) =
self.funding.counterparty_claimable_outpoints.get(&prev_txid) {
outputs
} else { continue; };

// Build the set of HTLC ids present in the stale (previous) commitment.
let mut prev_ids: HashSet<SentHTLCId> = new_hash_set();
for &(_, ref source_opt) in prev_outputs.iter() {
if let Some(source) = source_opt.as_ref() {
prev_ids.insert(SentHTLCId::from_source(&**source));
}
}

// Avoid duplicate fail-back if an HTLCUpdate for this source has already been
// generated (either matured or still awaiting maturity).
let mut seen_monitor_htlc_ids: HashSet<SentHTLCId> = new_hash_set();
for e in onchain_events_reaching_threshold_conf.iter() {
if let OnchainEvent::HTLCUpdate { source, .. } = &e.event {
seen_monitor_htlc_ids.insert(SentHTLCId::from_source(source));
}
}
for e in self.onchain_events_awaiting_threshold_conf.iter() {
if let OnchainEvent::HTLCUpdate { source, .. } = &e.event {
seen_monitor_htlc_ids.insert(SentHTLCId::from_source(source));
}
}

// Consider current live HTLCs (holder + current counterparty) with sources,
// and fail back only those NOT present in the stale (previous) commitment.
let current_counterparty_iter = if let Some(txid) = self.funding.current_counterparty_commitment_txid {
if let Some(htlc_outputs) = self.funding.counterparty_claimable_outpoints.get(&txid) {
Some(htlc_outputs.iter().map(|&(ref a, ref b)| (a, b.as_ref().map(|boxed| &**boxed))))
} else { None }
} else { None }.into_iter().flatten();
let current_htlcs = holder_commitment_htlcs!(self, CURRENT_WITH_SOURCES)
.chain(current_counterparty_iter);

// Count expected duplicates per id from the current view to bound emissions.
let mut expected_current_counts: HashMap<SentHTLCId, u64> = new_hash_map();
for (_, source_opt) in current_htlcs.clone() {
if let Some(source) = source_opt { *expected_current_counts.entry(SentHTLCId::from_source(source)).or_default() += 1; }
}

for (htlc, source_opt) in current_htlcs {
if let Some(source) = source_opt {

let sent_id = SentHTLCId::from_source(source);
if prev_ids.contains(&sent_id) { continue; }
if seen_monitor_htlc_ids.contains(&sent_id) { continue; }

let already_emitted_count = *self.failed_back_htlc_ids.get(&sent_id).unwrap_or(&0);
let expected_count = *expected_current_counts.get(&sent_id).unwrap_or(&0);

if already_emitted_count < expected_count {
log_info!(logger,
"Detected stale force-close. Failing back all HTLCs for hash {}.",
&htlc.payment_hash);
self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
source: source.clone(),
payment_preimage: None,
payment_hash: htlc.payment_hash,
htlc_value_satoshis: Some(htlc.amount_msat / 1000),
}));
*self.failed_back_htlc_ids.entry(sent_id).or_default() += 1;
}
}
}
},
OnchainEvent::AlternativeFundingConfirmation {} => {
// An alternative funding transaction has irrevocably confirmed and we're no
Expand Down Expand Up @@ -5646,7 +5725,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
if duplicate_event {
continue;
}
if !self.failed_back_htlc_ids.insert(SentHTLCId::from_source(source)) {
let htlc_id = SentHTLCId::from_source(source);
if *self.failed_back_htlc_ids.get(&htlc_id).unwrap_or(&0) > 0 {
continue;
}
if !duplicate_event {
Expand All @@ -5659,6 +5739,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
payment_hash: htlc.payment_hash,
htlc_value_satoshis: Some(htlc.amount_msat / 1000),
}));
*self.failed_back_htlc_ids.entry(htlc_id).or_default() += 1;
}
}
}
Expand Down Expand Up @@ -6650,7 +6731,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
initial_counterparty_commitment_info,
initial_counterparty_commitment_tx,
balances_empty_height,
failed_back_htlc_ids: new_hash_set(),
failed_back_htlc_ids: new_hash_map(),

current_holder_htlc_data,
prev_holder_htlc_data,
Expand Down
148 changes: 146 additions & 2 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ use crate::ln::channel::{
MIN_CHAN_DUST_LIMIT_SATOSHIS, UNFUNDED_CHANNEL_AGE_LIMIT_TICKS,
};
use crate::ln::channelmanager::{
PaymentId, RAACommitmentOrder, RecipientOnionFields, BREAKDOWN_TIMEOUT, DISABLE_GOSSIP_TICKS,
ENABLE_GOSSIP_TICKS, MIN_CLTV_EXPIRY_DELTA,
PaymentId, RAACommitmentOrder, RecipientOnionFields, Retry, BREAKDOWN_TIMEOUT,
DISABLE_GOSSIP_TICKS, ENABLE_GOSSIP_TICKS, MIN_CLTV_EXPIRY_DELTA,
};
use crate::ln::msgs;
use crate::ln::msgs::{
Expand Down Expand Up @@ -9703,3 +9703,147 @@ pub fn test_multi_post_event_actions() {
do_test_multi_post_event_actions(true);
do_test_multi_post_event_actions(false);
}

#[xtest(feature = "_externalize_tests")]
fn test_stale_force_close_with_identical_htlcs() {
// Test that when two identical HTLCs are relayed and force-closes
// with a stale state, that we fail both HTLCs back immediately.
let chanmon_cfgs = create_chanmon_cfgs(4);
let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs);

let chan_a_b = create_announced_chan_between_nodes(&nodes, 0, 1);
let _chan_b_c = create_announced_chan_between_nodes(&nodes, 1, 2);
let _chan_d_b = create_announced_chan_between_nodes(&nodes, 3, 1);

let (_payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]);

let payment_params = PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), 100);
let scorer = test_utils::TestScorer::new();
let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes();
let route_params = RouteParameters::from_payment_params_and_value(payment_params, 1_000_000);

let route = get_route(
&nodes[0].node.get_our_node_id(),
&route_params,
&nodes[0].network_graph.read_only(),
None,
nodes[0].logger,
&scorer,
&Default::default(),
&random_seed_bytes,
)
.unwrap();

nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
nodes[0]
.node
.send_payment(
payment_hash,
RecipientOnionFields::secret_only(payment_secret),
PaymentId([1; 32]),
route_params.clone(),
Retry::Attempts(0),
)
.unwrap();

let ev1 = remove_first_msg_event_to_node(
&nodes[1].node.get_our_node_id(),
&mut nodes[0].node.get_and_clear_pending_msg_events(),
);
let mut send_ev1 = SendEvent::from_event(ev1);

nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &send_ev1.msgs[0]);
nodes[1].node.handle_commitment_signed_batch_test(
nodes[0].node.get_our_node_id(),
&send_ev1.commitment_msg,
);

let mut b_events = nodes[1].node.get_and_clear_pending_msg_events();
for ev in b_events.drain(..) {
match ev {
MessageSendEvent::SendRevokeAndACK { node_id, msg } => {
assert_eq!(node_id, nodes[0].node.get_our_node_id());
nodes[0].node.handle_revoke_and_ack(nodes[1].node.get_our_node_id(), &msg);
},
MessageSendEvent::UpdateHTLCs { node_id, updates, .. } => {
assert_eq!(node_id, nodes[0].node.get_our_node_id());
nodes[0].node.handle_commitment_signed_batch_test(
nodes[1].node.get_our_node_id(),
&updates.commitment_signed,
);
let mut a_events = nodes[0].node.get_and_clear_pending_msg_events();
for a_ev in a_events.drain(..) {
if let MessageSendEvent::SendRevokeAndACK { node_id, msg } = a_ev {
assert_eq!(node_id, nodes[1].node.get_our_node_id());
nodes[1].node.handle_revoke_and_ack(nodes[0].node.get_our_node_id(), &msg);
}
}
},
_ => {},
}
}

nodes[1].node.process_pending_htlc_forwards();
let _ = nodes[1].node.get_and_clear_pending_msg_events();

let stale_commitment_tx = get_local_commitment_txn!(nodes[0], chan_a_b.2)[0].clone();

*nodes[0].network_payment_count.borrow_mut() -= 1;
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
nodes[0]
.node
.send_payment(
payment_hash,
RecipientOnionFields::secret_only(payment_secret),
PaymentId([2; 32]),
route_params.clone(),
Retry::Attempts(0),
)
.unwrap();

let ev2 = remove_first_msg_event_to_node(
&nodes[1].node.get_our_node_id(),
&mut nodes[0].node.get_and_clear_pending_msg_events(),
);
let mut send_ev2 = SendEvent::from_event(ev2);

nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &send_ev2.msgs[0]);
nodes[1].node.handle_commitment_signed_batch_test(
nodes[0].node.get_our_node_id(),
&send_ev2.commitment_msg,
);

let mut b2_events = nodes[1].node.get_and_clear_pending_msg_events();
for ev in b2_events.drain(..) {
match ev {
MessageSendEvent::SendRevokeAndACK { node_id, msg } => {
assert_eq!(node_id, nodes[0].node.get_our_node_id());
nodes[0].node.handle_revoke_and_ack(nodes[1].node.get_our_node_id(), &msg);
},
MessageSendEvent::UpdateHTLCs { node_id, updates, .. } => {
assert_eq!(node_id, nodes[0].node.get_our_node_id());
nodes[0].node.handle_commitment_signed_batch_test(
nodes[1].node.get_our_node_id(),
&updates.commitment_signed,
);
},
_ => {},
}
}

nodes[1].node.process_pending_htlc_forwards();
let _ = nodes[1].node.get_and_clear_pending_msg_events();

mine_transaction(&nodes[1], &stale_commitment_tx);
connect_blocks(&nodes[1], ANTI_REORG_DELAY);

let events = nodes[1].node.get_and_clear_pending_events();
let failed_count =
events.iter().filter(|e| matches!(e, Event::HTLCHandlingFailed { .. })).count();
assert_eq!(failed_count, 2);

check_added_monitors!(&nodes[1], 1);
nodes[1].node.get_and_clear_pending_msg_events();
}
Loading