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

TheBlueMatt · 2021-07-31T14:19:49Z

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.

This is a semi-important DoS issue, so tagging 0.0.100.

codecov · 2021-07-31T14:59:35Z

Codecov Report

Merging #1025 (c3c0dff) into main (8530078) will increase coverage by 0.80%.
The diff coverage is 67.69%.

❗ Current head c3c0dff differs from pull request most recent head 6bfab9d. Consider uploading reports for the commit 6bfab9d to get more accurate results

@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
+ Coverage   90.80%   91.60%   +0.80%     
==========================================
  Files          61       62       +1     
  Lines       31582    39598    +8016     
==========================================
+ Hits        28678    36274    +7596     
- Misses       2904     3324     +420

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`90.00% <ø> (ø)`
lightning/src/chain/channelmonitor.rs	`93.14% <40.00%> (+2.56%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/util/events.rs	`12.50% <0.00%> (-1.44%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.82% <0.00%> (-0.50%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.25% <0.00%> (-0.49%)`	⬇️
lightning/src/util/ser_macros.rs	`95.91% <0.00%> (-0.40%)`	⬇️
lightning-background-processor/src/lib.rs	`95.58% <0.00%> (ø)`
lightning/src/chain/keysinterface.rs	`95.41% <0.00%> (+0.04%)`	⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8530078...6bfab9d. Read the comment docs.

valentinewallace · 2021-08-03T17:23:55Z

lightning/src/chain/channelmonitor.rs

 		if is_holder_tx {
-			fail_dust_htlcs_after_threshold_conf!(self.current_holder_commitment_tx);
-			if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
-				fail_dust_htlcs_after_threshold_conf!(holder_tx);
-			}
 		}


Empty if block?

Yea, it'll be re-filled in the PR this one grew out of so I left it. I can remove it temporarily if you want.

What PR is that?

I'll let you know when I open it :)

lightning/src/ln/monitor_tests.rs

lightning/src/chain/channelmonitor.rs

valentinewallace · 2021-08-03T20:04:29Z

lightning/src/ln/monitor_tests.rs

+	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);


unnecessary mut

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

valentinewallace · 2021-08-03T22:35:20Z

lightning/src/chain/channelmonitor.rs

-						}
-					}
-				}
+				fail_unbroadcast_htlcs!(self, "previous holder", height, holder_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);


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?

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.

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

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

TheBlueMatt · 2021-08-09T15:47:55Z

Rebased and squashed.

This extracts the HTLC-not-in-broadcasted-commitment-transaction code from check_spend_counterparty_transaction and moves it to a global macro, DRYing up the two very similar codepaths (fixing some minor logging inconsistencies) in the process. This macro will be used for local commitment transaction HTLC failure as well in the next commit. This commit has no functional change outside of logging.

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.

valentinewallace · 2021-08-09T20:18:24Z

lightning/src/chain/channelmonitor.rs

@@ -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


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

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.

valentinewallace

LGTM

TheBlueMatt added this to the 0.0.100 milestone Jul 31, 2021

TheBlueMatt force-pushed the 2021-07-detect-htlcs-on-local-commitment branch from 931b054 to b688365 Compare July 31, 2021 14:34

valentinewallace reviewed Aug 3, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-detect-htlcs-on-local-commitment branch from 0722912 to bb1a050 Compare August 4, 2021 03:13

TheBlueMatt mentioned this pull request Aug 4, 2021

Expose in-flight claim balances #1034

Merged

TheBlueMatt force-pushed the 2021-07-detect-htlcs-on-local-commitment branch from c3c0dff to 3f3aff1 Compare August 9, 2021 15:47

TheBlueMatt added 2 commits August 9, 2021 16:12

TheBlueMatt force-pushed the 2021-07-detect-htlcs-on-local-commitment branch from 3f3aff1 to 6bfab9d Compare August 9, 2021 16:13

jkczyz requested a review from arik-so August 9, 2021 19:11

valentinewallace reviewed Aug 9, 2021

View reviewed changes

arik-so approved these changes Aug 9, 2021

View reviewed changes

valentinewallace approved these changes Aug 10, 2021

View reviewed changes

TheBlueMatt merged commit d4b6f58 into lightningdevkit:main Aug 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TheBlueMatt commented Jul 31, 2021

codecov bot commented Jul 31, 2021 •

edited

Loading

valentinewallace Aug 3, 2021

TheBlueMatt Aug 3, 2021

valentinewallace Aug 3, 2021

TheBlueMatt Aug 3, 2021

TheBlueMatt Aug 4, 2021

valentinewallace Aug 3, 2021

TheBlueMatt Aug 4, 2021

valentinewallace Aug 3, 2021

TheBlueMatt Aug 4, 2021

valentinewallace Aug 4, 2021

TheBlueMatt Aug 4, 2021

TheBlueMatt commented Aug 9, 2021

valentinewallace Aug 9, 2021

TheBlueMatt Aug 9, 2021

valentinewallace left a comment

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

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

Conversation

TheBlueMatt commented Jul 31, 2021

codecov bot commented Jul 31, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 31, 2021 •

edited

Loading