Track claimed outbound HTLCs in ChannelMonitors

[TheBlueMatt]

When we receive an update_fulfill_htlc message, we immediately try to "claim" the HTLC against the HTLCSource. If there is one, this works great, we immediately generate a ChannelMonitorUpdate for the corresponding inbound HTLC and persist that before we ever get to processing our counterparty's commitment_signed and persisting the corresponding ChannelMonitorUpdate.

However, if there isn't one (and this is the first successful HTLC for a payment we sent), we immediately generate a PaymentSent event and queue it up for the user. Then, a millisecond later, we receive the commitment_signed from our peer, removing the HTLC from the latest local commitment transaction as a side-effect of the ChannelMonitorUpdate applied.

If the user has processed the PaymentSent event by that point, great, we're done. However, if they have not, and we crash prior to persisting the ChannelManager, on startup we get confused about the state of the payment. We'll force-close the channel for being stale, and see an HTLC which was removed and is no longer present in the latest commitment transaction (which we're broadcasting). Because we claim corresponding inbound HTLCs before updating a ChannelMonitor, we assume such HTLCs have failed - attempting to fail after having claimed should be a noop. However, in the sent-payment case we now generate a PaymentFailed event for the user, allowing an HTLC to complete without giving the user a preimage.

Here we address this issue by storing the payment preimages for claimed outbound HTLCs in the ChannelMonitor, in addition to the existing inbound HTLC preimages already stored there. This allows us to fix the specific issue described by checking for a preimage and switching the type of event generated in response. In addition, it reduces the risk of future confusion by ensuring we don't fail HTLCs which were claimed but not fully committed to before a crash.

It does not, however, full fix the issue here - because the preimages are removed after the HTLC has been fully removed from available commitment transactions if we are substantially delayed in persisting the ChannelManager from the time we receive the update_fulfill_htlc until after a full commitment signed dance completes we may still hit this issue. The full fix for this issue is to delay the persistence of the ChannelMonitorUpdate until after the PaymentSent event has been processed. This avoids the issue entirely, ensuring we process the event before updating the ChannelMonitor, the same as we ensure the upstream HTLC has been claimed before updating the ChannelMonitor for forwarded payments.

The full solution will be implemented in a later work, however this change still makes sense at that point as well - if we were to delay the initial commitment_signed ChannelMonitorUpdate util after the PaymentSent event has been processed (which likely requires a database update on the users' end), we'd hold our commitment_signed + revoke_and_ack response for two DB writes (i.e. fsync() calls), making our commitment transaction processing a full fsync slower. By making this change first, we can instead delay the ChannelMonitorUpdate from the counterparty's final revoke_and_ack message until the event has been processed, giving us a full network roundtrip to do so and avoiding delaying our response as long as an fsync is faster than a network roundtrip.

[dunxen]

Congrats on PR 2^11 :)
Added to my review list for tomorrow.

[codecov-commenter]

Codecov Report

Patch coverage: 82.38% and project coverage change: -0.11 ⚠️

Comparison is base (6ddf69c) 87.19% compared to head (75527db) 87.09%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2048      +/-   ##
==========================================
- Coverage   87.19%   87.09%   -0.11%     
==========================================
  Files         100      100              
  Lines       44560    45110     +550     
  Branches    44560    45110     +550     
==========================================
+ Hits        38853    39287     +434     
- Misses       5707     5823     +116     

Impacted Files	Coverage Δ
lightning/src/sync/nostd_sync.rs	40.90% <0.00%> (-3.00%)	⬇️
lightning/src/chain/channelmonitor.rs	89.08% <74.50%> (-0.51%)	⬇️
lightning/src/ln/channelmanager.rs	86.54% <79.59%> (-0.04%)	⬇️
lightning/src/ln/payment_tests.rs	95.52% <95.55%> (+<0.01%)	⬆️
lightning/src/ln/channel.rs	84.01% <100.00%> (+0.01%)	⬆️
lightning/src/sync/debug_sync.rs	81.61% <100.00%> (+0.41%)	⬆️
lightning-invoice/src/utils.rs	96.14% <0.00%> (+0.64%)	⬆️
lightning/src/ln/peer_handler.rs	64.19% <0.00%> (+5.78%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[naumenkogs]

I was wondering if we could apply any invariant w.r.t option in this case... Should it be assert(is_none) or something? It would be better than just dropping it on the floor in this match condition.

[TheBlueMatt]

Not sure which option you're referring to?

[naumenkogs]

Ahhh i meant match. Inside the PreviousHopData condition, i wondered if anything could be asserted about preimage_opt instead of just dropping it of the floor. This is totally not a blocker, but if that's possible it just helps to understand better what and where is coming from

[TheBlueMatt]

Ohhh, right, yea, its a bit annoying to add cause we end up with a good bit of lookups, so let me save it for a followup, but will do.

[valentinewallace]

Initial pass

[TheBlueMatt]

Sorry for the delay, fixed the fuzzing failure and addressed comments.

[TheBlueMatt]

I believe the CI failure will be fixed by squash - let me know if I can do that reviewers.

[TheBlueMatt]

Squashed and pushed with one cleanup from @douglaz:

$ git diff-tree -U1 f407de1b 743d5639
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 684a1163b..f67d0e330 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2178,13 +2178,9 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			#[cfg(debug_assertions)] {
-				let mut found_matching_pending_htlc = false;
-				for (_, source_opt) in self.counterparty_claimable_outpoints.get(
-					&self.current_counterparty_commitment_txid.unwrap()
-				).unwrap().iter() {
+				let cur_counterparty_htlcs = self.counterparty_claimable_outpoints.get(
+						&self.current_counterparty_commitment_txid.unwrap()).unwrap();
+				assert!(cur_counterparty_htlcs.iter().any(|(_, source_opt)| {
 					if let Some(source) = source_opt {
-						if SentHTLCId::from_source(source) == *claimed_htlc_id {
-							found_matching_pending_htlc = true;
-						}
-					}
-				}
-				assert!(found_matching_pending_htlc);
+						SentHTLCId::from_source(source) == *claimed_htlc_id
+					} else { false }
+				}));
 			}

[wpaulino]

LGTM after squash

[valentinewallace]

LGTM after squash

[TheBlueMatt]

Squashed without further changes.