Do not FC for outgoing payments #4140

elnosh · 2025-10-03T19:50:15Z

Resolves #4048

    Previously `should_broadcast_holder_commitment_txn` would FC a
    channel if an outbound HTLC that hasn't been resolved was
    `LATENCY_GRACE_PERIOD_BLOCKS` past expiry. For outgoing payments,
    we can avoid FC the channel since we are not in a race to claim an
    inbound HTLC. For cases in which a node has been offline for a
    while, this could help to fail the HTLC on reconnection instead
    of causing a FC.

ldk-reviews-bot · 2025-10-03T19:50:18Z

I've assigned @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

lightning/src/ln/reload_tests.rs

valentinewallace · 2025-10-07T21:09:13Z

lightning/src/chain/channelmonitor.rs

+						true
+					};
+					if (htlc_outbound && has_incoming && htlc.0.cltv_expiry + LATENCY_GRACE_PERIOD_BLOCKS <= height) ||
+					   (htlc_outbound && !has_incoming && htlc.0.cltv_expiry + 2016 <= height) ||


I think per the issue, we shouldn't FC at all for these timed out outbound payments. I can't really see a downside to doing that since the user can FC manually or contact their counterparty out-of-band, though I could be missing something. It should be really rare/weird for the counterparty to not just fail the HTLC back on reconnect anyway.

Asked @wpaulino offline and he seemed to confirm this approach

ok, will remove the arbitrary 2016 I added. Yeah I thought about not FC at all but wasn't sure in case something weird happens and counterparty never fails it back and could end up with the HTLC stuck there. But yeah, it can FC manually.

Previously `should_broadcast_holder_commitment_txn` would FC a channel if an outbound HTLC that hasn't been resolved was `LATENCY_GRACE_PERIOD_BLOCKS` past expiry. For outgoing payments, we can avoid FC the channel since we are not in a race to claim an inbound HTLC. For cases in which a node has been offline for a while, this could help to fail the HTLC on reconnection instead of causing a FC.

codecov · 2025-10-12T18:47:33Z

Codecov Report

❌ Patch coverage is 98.31933% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.80%. Comparing base (81495c6) to head (efd46d1).
⚠️ Report is 43 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/async_payments_tests.rs	95.00%	1 Missing and 2 partials ⚠️
lightning/src/chain/channelmonitor.rs	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4140      +/-   ##
==========================================
+ Coverage   88.64%   88.80%   +0.15%     
==========================================
  Files         180      180              
  Lines      135230   136721    +1491     
  Branches   135230   136721    +1491     
==========================================
+ Hits       119874   121414    +1540     
+ Misses      12591    12504      -87     
- Partials     2765     2803      +38

Flag	Coverage Δ
fuzzing	`21.02% <100.00%> (-0.74%)`	⬇️
tests	`88.64% <98.31%> (+0.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

elnosh · 2025-10-12T19:05:03Z

I think this is ready now. I removed the arbitrary 2016 block grace period I had for outbound payments to instead not FC at all.

@valentinewallace Added test for async payment version.

Addressed all the tests that were failing which were expecting a FC on an outbound HTLC. I found this a bit tricky for tests that were depending on the FC after LATENCY_GRACE_PERIOD_BLOCKS. For some tests (like async_signer_tests) I just triggered the broadcast manually since the test is not testing this specifically. For the other tests that actually test the FC on HTLC-Timeout, these mostly had HTLCs going from A -> B and expecting a timeout on the outbound HTLC from A. This won't cause a FC anymore so I changed to test the FC on timeout when it is forwarding an HTLC since this is the functionality that is kept. So in those cases, I added an extra node and channel to route a payment and expect the FC on the node that is forwarding the HTLC.

valentinewallace

Basically LGTM

valentinewallace · 2025-10-14T15:17:43Z

lightning/src/chain/channelmonitor.rs

 					// can still claim the corresponding HTLC. Thus, to avoid needlessly hitting the
 					// chain when our counterparty is waiting for expiration to off-chain fail an HTLC
 					// we give ourselves a few blocks of headroom after expiration before going
 					// on-chain for an expired HTLC.


I think we should update this comment for the new behavior

lightning/src/chain/channelmonitor.rs

valentinewallace · 2025-10-14T15:27:52Z

lightning/src/ln/chanmon_update_fail_tests.rs

 	chain_mon
 		.chain_monitor
 		.block_connected(&create_dummy_block(BlockHash::all_zeros(), 42, Vec::new()), 200);
+	chain_mon.chain_monitor.get_monitor(chan.2).unwrap().broadcast_latest_holder_commitment_txn(


nit: add a comment

valentinewallace · 2025-10-14T15:48:10Z

lightning/src/ln/functional_tests.rs

 	};
 	let block = create_dummy_block(BlockHash::all_zeros(), 42, Vec::new());
-	watchtower_bob.chain_monitor.block_connected(&block, HTLC_TIMEOUT_BROADCAST - 1);
+	watchtower_bob.chain_monitor.block_connected(&block, htlc_timeout - 1);


I think we're connecting an absolute expiry amount of blocks here when we should be connecting a relative number of blocks?

valentinewallace · 2025-10-14T15:52:00Z

lightning/src/ln/reorg_tests.rs

+	let reason = ClosureReason::HTLCsTimedOut { payment_hash: Some(payment_hash) };
 	check_closed_broadcast(&nodes[0], 1, true);
 	check_added_monitors(&nodes[0], 1);
-	let reason = ClosureReason::HTLCsTimedOut { payment_hash: Some(payment_hash) };


nit: unnecessary diff, and below?

tankyleo

Thank you, I just reviewed just channelmonitor.rs so far, will be taking a look at the tests next

tankyleo · 2025-10-14T15:53:11Z

lightning/src/chain/channelmonitor.rs

+					let htlc_outbound = $holder_tx == htlc.0.offered;
+					let has_incoming = if htlc_outbound {
+						if let Some(source) = htlc.1.as_deref() {
+							match *source {
+								HTLCSource::OutboundRoute { .. } => false,
+								HTLCSource::PreviousHopData(_) => true,
+							}
+						} else {
+							panic!("Every offered non-dust HTLC should have a corresponding source");
+						}
+					} else {
+						true
+					};
+					if (htlc_outbound && has_incoming && htlc.0.cltv_expiry + LATENCY_GRACE_PERIOD_BLOCKS <= height) ||
+					   (!htlc_outbound && htlc.0.cltv_expiry <= height + CLTV_CLAIM_BUFFER && self.payment_preimages.contains_key(&htlc.0.payment_hash)) {
+						log_info!(logger, "Force-closing channel due to {} HTLC timeout - HTLC with payment hash {} expires at {}", if htlc_outbound { "outbound" } else { "inbound"}, htlc.0.payment_hash, htlc.0.cltv_expiry);
+						return Some(htlc.0.payment_hash);


I've got this diff below on this section here, seems to me it reads better ? Feel free to edit

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 5b9d7435d1..f364b25f0c 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -5887,7 +5887,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> { let height = self.best_block.height; macro_rules! scan_commitment { ($htlcs: expr, $holder_tx: expr) => { - for ref htlc in $htlcs { + for (ref htlc, ref source) in $htlcs { // For inbound HTLCs which we know the preimage for, we have to ensure we hit the // chain with enough room to claim the HTLC without our counterparty being able to // time out the HTLC first. @@ -5899,23 +5899,13 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> { // chain when our counterparty is waiting for expiration to off-chain fail an HTLC // we give ourselves a few blocks of headroom after expiration before going // on-chain for an expired HTLC. - let htlc_outbound = $holder_tx == htlc.0.offered; - let has_incoming = if htlc_outbound { - if let Some(source) = htlc.1.as_deref() { - match *source { - HTLCSource::OutboundRoute { .. } => false, - HTLCSource::PreviousHopData(_) => true, - } - } else { - panic!("Every offered non-dust HTLC should have a corresponding source"); - } - } else { - true - }; - if (htlc_outbound && has_incoming && htlc.0.cltv_expiry + LATENCY_GRACE_PERIOD_BLOCKS <= height) || - (!htlc_outbound && htlc.0.cltv_expiry <= height + CLTV_CLAIM_BUFFER && self.payment_preimages.contains_key(&htlc.0.payment_hash)) { - log_info!(logger, "Force-closing channel due to {} HTLC timeout - HTLC with payment hash {} expires at {}", if htlc_outbound { "outbound" } else { "inbound"}, htlc.0.payment_hash, htlc.0.cltv_expiry); - return Some(htlc.0.payment_hash); + let htlc_outbound = $holder_tx == htlc.offered; + let is_outbound_and_has_incoming = htlc_outbound + && matches!(source.as_deref().expect("Every outbound HTLC should have a corresponding source"), HTLCSource::PreviousHopData(_)); + if (is_outbound_and_has_incoming && htlc.cltv_expiry + LATENCY_GRACE_PERIOD_BLOCKS <= height) || + (!htlc_outbound && htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER && self.payment_preimages.contains_key(&htlc.payment_hash)) { + log_info!(logger, "Force-closing channel due to {} HTLC timeout - HTLC with payment hash {} expires at {}", if htlc_outbound { "outbound" } else { "inbound"}, htlc.payment_hash, htlc.cltv_expiry); + return Some(htlc.payment_hash); } } }

tankyleo · 2025-10-14T15:59:39Z

lightning/src/chain/channelmonitor.rs

+								HTLCSource::PreviousHopData(_) => true,
+							}
+						} else {
+							panic!("Every offered non-dust HTLC should have a corresponding source");


For this particular spot, dust offered HTLCs on holder_tx also have sources, as well as dust and non-dust accepted HTLCs on !holder_tx ie the counterparty's tx.

So this would include all these cases too: "Every outbound HTLC should have a corresponding source"

TheBlueMatt

I don't think we can simply never FC a channel for holding outbound payments forever, though I admit I'm not entirely sure what the right answer is. For someone not a mobile client, we still want to FC "on time" since they want their money back and their peer is simply misbehaving. For a mobile client, too, we do eventually want to FC if the peer is simply not failing the HTLC even though we're connected (if we're not connected cause the peer is gone the user presumably will eventually manually FC anyway). Maybe there's a way to gate this on connectivity or uptime?

valentinewallace reviewed Oct 7, 2025

View reviewed changes

elnosh force-pushed the no-fc-startup branch from 93388e4 to efd46d1 Compare October 12, 2025 18:36

elnosh marked this pull request as ready for review October 12, 2025 19:05

ldk-reviews-bot requested a review from tankyleo October 12, 2025 19:05

elnosh changed the title ~~Delay FC for outgoing payments~~ Do not FC for outgoing payments Oct 12, 2025

valentinewallace reviewed Oct 14, 2025

View reviewed changes

tankyleo reviewed Oct 14, 2025

View reviewed changes

TheBlueMatt reviewed Oct 14, 2025

View reviewed changes

Do not FC for outgoing payments #4140

Are you sure you want to change the base?

Do not FC for outgoing payments #4140

Uh oh!

Conversation

elnosh commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

elnosh commented Oct 12, 2025

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elnosh commented Oct 3, 2025 •

edited

Loading

ldk-reviews-bot commented Oct 3, 2025 •

edited

Loading

codecov bot commented Oct 12, 2025 •

edited

Loading