Intercept HTLC forwards for JIT channels #1835

valentinewallace · 2022-11-07T16:31:25Z

For context, LSPs need to be able to open 0-conf JIT channels to users upon the first time the user is receiving a payment. To do this, they will put fake route hints in end user invoices that signal to LDK that this is an intercept forward, similar to phantom payments. LDK will then generate an event, giving the LSP the opportunity to open the JIT channel. The LSP can then forward the payment over the newly opened channel, or fail it.

~~Based on #1840~~
Supercedes #1601

lightning/src/ln/channelmanager.rs

codecov-commenter · 2022-11-07T22:58:21Z

Codecov Report

Base: 90.70% // Head: 90.67% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (a5c5d53) compared to base (440c3ee).
Patch coverage: 84.34% of modified lines in pull request are covered.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1835      +/-   ##
==========================================
- Coverage   90.70%   90.67%   -0.03%     
==========================================
  Files          91       91              
  Lines       48090    48303     +213     
  Branches    48090    48303     +213     
==========================================
+ Hits        43618    43800     +182     
- Misses       4472     4503      +31

Impacted Files	Coverage Δ
lightning/src/util/config.rs	`65.90% <0.00%> (-0.76%)`	⬇️
lightning/src/util/events.rs	`24.65% <4.16%> (-1.32%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.20% <83.70%> (-0.16%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.73% <97.74%> (-0.17%)`	⬇️
lightning/src/util/scid_utils.rs	`99.31% <100.00%> (+0.10%)`	⬆️
lightning-block-sync/src/rest.rs	`62.06% <0.00%> (-1.87%)`	⬇️
lightning-block-sync/src/rpc.rs	`74.80% <0.00%> (-1.14%)`	⬇️
lightning-block-sync/src/poll.rs	`86.33% <0.00%> (-0.58%)`	⬇️
lightning-net-tokio/src/lib.rs	`77.03% <0.00%> (-0.28%)`	⬇️
lightning-block-sync/src/lib.rs	`93.33% <0.00%> (-0.27%)`	⬇️
... and 13 more

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.

lightning/src/util/events.rs

TonyGiorgio · 2022-11-10T15:45:12Z

Are there plans for generic HTLC interception at all? Not dependent on the routing hints of someone else's invoice? There can still be logic into determining the type based on SCID but in general all HTLC's could still be intercepted or at least subscribed to based on type.

There are many use cases for generic HTLC interception that shouldn't depend on LDK development, here are a few:

Other approaches to 0 conf JIT channel payments (something double-invoice like instead of routing hint-like)
Escrow (having a router decide to continue routing to destination in order to deliver funds based on a condition)
Whether or not to route payments down certain channels or at all (liquidity management, balance probe protection)
Third party preimages (node not knowing the preimage but waiting for either a third party or a different application handle preimages)
Hodl invoice (not advocating for long running but at least allowing for checks and processes to run for a few seconds)

You could still have SCID logic but I would love to see generic solved for first and then application specific tbh. I think that's worked really well in the LND/CLN ecosystems.

valentinewallace · 2022-11-10T16:52:21Z

Are there plans for generic HTLC interception at all? Not dependent on the routing hints of someone else's invoice?

Yes, the plan is to swiftly follow up to this PR with adding general forward interception :)

(1), (4), and (5) are already supported via our PaymentReceived event, IIUC. LDK generates this event once all MPP parts arrive. Upon receipt, you can get the preimage and do whatever other desired operations you like, then call claim_funds (per the linked docs).

valentinewallace · 2022-11-10T16:56:35Z

Had to rebase to get #1844.

TonyGiorgio · 2022-11-10T17:24:54Z

Yes, the plan is to swiftly follow up to this PR with adding general forward interception :)

Awesome! Concept ACK from me then. I browsed through the code and the API usage sounds good to me but probably can't comment on the rest 👍

ViktorTigerstrom

First look-through generally looks good :).

lightning/src/ln/channelmanager.rs

TheBlueMatt

I think we need a fallback failure on a timer. If the user screws up and loses the intercept event entirely we shouldn't just sit on the HTLC forever and get the channel force-closed.

lightning/src/util/events.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2022-11-11T21:57:29Z

Great feedback once again @ViktorTigerstrom! Working on addressing it

ViktorTigerstrom

This looks good to me. Only real feedback I have left is that it would be valuable to include test coverage to ensure that we don't generate the PaymentIntercepted event + populate pending_intercepted_htlcs when accept_intercept_htlcs is set to false.

ViktorTigerstrom · 2022-11-28T22:02:59Z

lightning/src/ln/payment_tests.rs

+	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
+	commitment_signed_dance!(nodes[1], nodes[0], &payment_event.commitment_msg, false, true);
+
+	// Check that we generate the PaymentIntercepted event when an intercept forward is detected.


Would be valuable to also have test coverage ensuring that the PaymentIntercepted event isn't generated + pending_intercepted_htlcs isn't populated when accept_intercept_htlcs is false.

Gonna save that for follow-up for the sake of the size of this PR, but you can manually check that the test fails when the config change lines are commented out. FWIW, that would set a new standard of testing rigor for us since i.e. the zero-conf PR didn't/doesn't test its config.

Ok fair enough :)

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2022-11-28T23:15:31Z

lightning/src/ln/channelmanager.rs

+						_ => unreachable!(),
+					};
+					timed_out_htlcs.push((prev_hop_data, htlc.forward_info.payment_hash,
+							HTLCFailReason::Reason { failure_code: 0x1000 | 14, data: Vec::new() },


14 is definitely right here, but its an UPDATE error, so we need to include a channel_update for the outbound channel, which is obviously....unclear? I guess we could deliberately include the wrong channel's update here and use the previous-hop channel, but that's pretty nonsensical. Maybe we just temporary_node_failure?

TheBlueMatt · 2022-11-29T00:22:06Z

LGTM, I think.

ViktorTigerstrom · 2022-11-29T00:42:37Z

Looks good to me as well!

dunxen

LGTM, just a tiny nit and question :)

dunxen · 2022-11-30T09:15:36Z

lightning/src/util/events.rs

+		payment_hash: PaymentHash,
+		/// How many msats were received on the inbound edge of this HTLC.
+		inbound_amount_msat: u64,
+		/// How many msats the payer intended to route to the next node. Depending on the reason you are


new rust style team pls save us

lightning/src/util/events.rs

dunxen · 2022-11-30T09:24:35Z

lightning/src/ln/channelmanager.rs

+										});
+
+										failed_intercept_forwards.push((htlc_source, forward_info.payment_hash,
+												HTLCFailReason::Reason { failure_code: 0x4000 | 10, data: Vec::new() },


not for this PR, but I've wondered whether we should collect failure code consts somewhere so it's easy to see what it is. I can create an issue if we'd want that, unless I missed some prior reasoning not to.

Yeah, I think that would be nice to have!

dunxen · 2022-11-30T09:35:37Z

lightning/src/ln/channelmanager.rs

+	/// [`HTLCIntercepted`]: events::Event::HTLCIntercepted
+	// TODO: when we move to deciding the best outbound channel at forward time, only take
+	// `next_node_id` and not `next_hop_channel_id`
+	pub fn forward_intercepted_htlc(&self, intercept_id: InterceptId, next_hop_channel_id: &[u8; 32], _next_node_id: PublicKey, amt_to_forward_msat: u64) -> Result<(), APIError> {


just a question to check my understanding:

Is this basically the procedure for an LDK user?

Calls ChannelManager::get_intercept_scid

Generates invoice with intercept scid in route hints for end-user

Receives an HTLCIntercepted event when the HTLC is intercepted

At this point the LDK user can create a channel

Then call this method to forward over that JIT channel

Yes that's my understanding, where the LDK user is the LSP in your example (nodes[1] in the test), and the end-user is nodes[2] in the test. I guess step (2) in your example doesn't necessarily need to be on the LDK user's side, as long as the Intercept scid is passed to the end-user.

Ah, I should have read the test.

Yeah I think I phrased that weirdly for (2). End user generates invoice but just shoves the scid they got from the LSP in the route hints.

No htlcs are intercepted yet, that will be added in upcoming commit(s) Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

This is useful for LSPs who wish to create a just-in-time channel for end users receiving a lightning payment. These fake scids will be encoded into route hints in end user invoices, and signal to LDK to create an event triggering the JIT channel, after which the payment will be received. Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

Used in upcoming commit(s) so users can intercept forwarded HTLCs Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

And store the pending intercepted HTLC in pending_intercepted_htlcs Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

See ChannelManager::forward_intercepted_htlc and ChannelManager::get_intercept_scid for details Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

valentinewallace · 2022-11-30T17:52:54Z

Rebased due to conflicts

ariard

ACK acff8f6

Reviewed code changes since last diff, and also checked test coverage. Remaining comments are minor.

ariard · 2022-11-30T22:59:22Z

lightning/src/ln/channelmanager.rs

+		let next_hop_scid = match self.channel_state.lock().unwrap().by_id.get(next_hop_channel_id) {
+			Some(chan) => {
+				if !chan.is_usable() {
+					return Err(APIError::APIMisuseError {


We have APIError::ChannelUnavailable in fact, could be used.

ariard · 2022-11-30T22:59:48Z

lightning/src/ln/channelmanager.rs

+		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
+
+		let payment = self.pending_intercepted_htlcs.lock().unwrap().remove(&intercept_id)
+			.ok_or_else(|| APIError::APIMisuseError {


Mutating this line does not break intercepted_payment

TheBlueMatt

LGTM, gonna land to get the conflicts out of the way but please address the two issues @ariard noted and the below one in a followup.

TheBlueMatt · 2022-12-01T00:01:50Z

lightning/src/ln/payment_tests.rs

+		};
+		connect_block(&nodes[0], &block);
+		connect_block(&nodes[1], &block);
+		let block_count = 183; // find_route adds a random CLTV offset, so hardcode rather than summing consts


It adds a random value in a range, though, so we should still be able to sum consts. Please change it to summing consts cause otherwise our entire test suite fails any time we change any constant :(

Note that if you use the get_route method it won't add the random value, as well.

valentinewallace force-pushed the 2022-11-jit-chan-htlc-intercept branch 5 times, most recently from beae304 to 90c4d75 Compare November 7, 2022 18:42

valentinewallace mentioned this pull request Nov 7, 2022

intercept htlcs #1601

Closed

valentinewallace force-pushed the 2022-11-jit-chan-htlc-intercept branch from 8e3a664 to b166af0 Compare November 7, 2022 22:30

valentinewallace commented Nov 7, 2022

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

ariard reviewed Nov 8, 2022

View reviewed changes

lightning/src/util/events.rs Outdated Show resolved Hide resolved

valentinewallace mentioned this pull request Nov 8, 2022

Pre-refactor for HTLC Interception #1840

Merged

valentinewallace added the blocked on dependent pr label Nov 8, 2022

valentinewallace force-pushed the 2022-11-jit-chan-htlc-intercept branch 4 times, most recently from 5e8146c to 638a5d1 Compare November 9, 2022 18:48

valentinewallace removed the blocked on dependent pr label Nov 9, 2022

valentinewallace force-pushed the 2022-11-jit-chan-htlc-intercept branch 2 times, most recently from 2b60d57 to d763bb5 Compare November 10, 2022 01:22

valentinewallace added the Seeking Code Review label Nov 10, 2022

valentinewallace force-pushed the 2022-11-jit-chan-htlc-intercept branch from d763bb5 to 099533d Compare November 10, 2022 16:55

ViktorTigerstrom reviewed Nov 10, 2022

View reviewed changes

TheBlueMatt reviewed Nov 11, 2022

View reviewed changes

valentinewallace force-pushed the 2022-11-jit-chan-htlc-intercept branch 2 times, most recently from 0759af8 to ee70d50 Compare November 14, 2022 20:23

valentinewallace force-pushed the 2022-11-jit-chan-htlc-intercept branch from 72087f8 to 225a22b Compare November 28, 2022 20:13

ViktorTigerstrom reviewed Nov 28, 2022

View reviewed changes

TheBlueMatt reviewed Nov 28, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Nov 28, 2022

Ensure UPDATE errors have an update #1879

Closed

valentinewallace force-pushed the 2022-11-jit-chan-htlc-intercept branch from 225a22b to a5c5d53 Compare November 28, 2022 23:49

TheBlueMatt assigned TheBlueMatt, dunxen, jkczyz, ViktorTigerstrom and ariard Nov 29, 2022

TheBlueMatt added this to the 0.0.113 milestone Nov 29, 2022

dunxen reviewed Nov 30, 2022

View reviewed changes

valentinewallace and others added 9 commits November 30, 2022 12:43

Persist pending intercepted htlcs in ChannelManager

129e1f6

No htlcs are intercepted yet, that will be added in upcoming commit(s) Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

Add HTLCIntercepted event

5efc197

Used in upcoming commit(s) so users can intercept forwarded HTLCs Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

Generate HTLCIntercepted event upon interceptable forward

8fe7cbe

And store the pending intercepted HTLC in pending_intercepted_htlcs Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

Utils for forwarding intercepted htlcs + getting intercept scids

c1f1b78

See ChannelManager::forward_intercepted_htlc and ChannelManager::get_intercept_scid for details Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

Allow failing back intercepted HTLCs

f79ad2e

Co-authored-by: John Cantrell <[email protected]> Co-authored-by: Valentine Wallace <[email protected]>

Add config knob for forwarding intercept payments

ddcd9b0

Automatically fail intercepts back on timeout

7809c55

Don't forward HTLC intercepts over unestablished channels

acff8f6

valentinewallace force-pushed the 2022-11-jit-chan-htlc-intercept branch from a5c5d53 to acff8f6 Compare November 30, 2022 17:52

ariard approved these changes Nov 30, 2022

View reviewed changes

TheBlueMatt approved these changes Dec 1, 2022

View reviewed changes

TheBlueMatt merged commit fb6e018 into lightningdevkit:main Dec 1, 2022

TheBlueMatt mentioned this pull request Dec 1, 2022

#1835 followups #1890

Closed

joostjager mentioned this pull request Dec 15, 2022

htlcswitch: add interceptor channel filter lightningnetwork/lnd#6874

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intercept HTLC forwards for JIT channels #1835

Intercept HTLC forwards for JIT channels #1835

valentinewallace commented Nov 7, 2022 •

edited

Loading

codecov-commenter commented Nov 7, 2022 •

edited

Loading

TonyGiorgio commented Nov 10, 2022 •

edited

Loading

valentinewallace commented Nov 10, 2022

valentinewallace commented Nov 10, 2022

TonyGiorgio commented Nov 10, 2022

ViktorTigerstrom left a comment

TheBlueMatt left a comment

valentinewallace commented Nov 11, 2022

ViktorTigerstrom left a comment

ViktorTigerstrom Nov 28, 2022

valentinewallace Nov 28, 2022 •

edited

Loading

ViktorTigerstrom Nov 28, 2022

TheBlueMatt Nov 28, 2022

TheBlueMatt commented Nov 29, 2022

ViktorTigerstrom commented Nov 29, 2022

dunxen left a comment

dunxen Nov 30, 2022

dunxen Nov 30, 2022

valentinewallace Nov 30, 2022

dunxen Nov 30, 2022

ViktorTigerstrom Nov 30, 2022 •

edited

Loading

dunxen Nov 30, 2022 •

edited

Loading

valentinewallace commented Nov 30, 2022

ariard left a comment

ariard Nov 30, 2022

ariard Nov 30, 2022

TheBlueMatt left a comment

TheBlueMatt Dec 1, 2022

Intercept HTLC forwards for JIT channels #1835

Intercept HTLC forwards for JIT channels #1835

Conversation

valentinewallace commented Nov 7, 2022 • edited Loading

codecov-commenter commented Nov 7, 2022 • edited Loading

Codecov Report

TonyGiorgio commented Nov 10, 2022 • edited Loading

valentinewallace commented Nov 10, 2022

valentinewallace commented Nov 10, 2022

TonyGiorgio commented Nov 10, 2022

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

valentinewallace commented Nov 11, 2022

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 29, 2022

ViktorTigerstrom commented Nov 29, 2022

dunxen left a 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

ViktorTigerstrom Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

dunxen Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

valentinewallace commented Nov 30, 2022

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Nov 7, 2022 •

edited

Loading

codecov-commenter commented Nov 7, 2022 •

edited

Loading

TonyGiorgio commented Nov 10, 2022 •

edited

Loading

valentinewallace Nov 28, 2022 •

edited

Loading

ViktorTigerstrom Nov 30, 2022 •

edited

Loading

dunxen Nov 30, 2022 •

edited

Loading