Intercept HTLC forwards for JIT channels

[valentinewallace]

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

[codecov-commenter]

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.

[AnthonyRonning]

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:

1. Other approaches to 0 conf JIT channel payments (something double-invoice like instead of routing hint-like)
2. Escrow (having a router decide to continue routing to destination in order to deliver funds based on a condition)
3. Whether or not to route payments down certain channels or at all (liquidity management, balance probe protection)
4. Third party preimages (node not knowing the preimage but waiting for either a third party or a different application handle preimages)
5. 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]

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]

Had to rebase to get #1844.

[AnthonyRonning]

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 👍

[ViktorT-11]

First look-through generally looks good :).

[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.

[valentinewallace]

Great feedback once again @ViktorTigerstrom! Working on addressing it

[ViktorT-11]

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.

[ViktorT-11]

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.

[valentinewallace]

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.

[ViktorT-11]

Ok fair enough :)

[TheBlueMatt]

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]

LGTM, I think.

[ViktorT-11]

Looks good to me as well!

[dunxen]

LGTM, just a tiny nit and question :)

[dunxen]

new rust style team pls save us

[dunxen]

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.

[valentinewallace]

Yeah, I think that would be nice to have!

[dunxen]

just a question to check my understanding:

Is this basically the procedure for an LDK user?

1. Calls ChannelManager::get_intercept_scid
2. Generates invoice with intercept scid in route hints for end-user
3. Receives an HTLCIntercepted event when the HTLC is intercepted
4. At this point the LDK user can create a channel
5. Then call this method to forward over that JIT channel

[ViktorT-11]

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.

[dunxen]

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.

[valentinewallace]

Rebased due to conflicts

[ariard]

ACK acff8f6

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

[ariard]

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

[ariard]

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]

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]

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

[codecov-commenter]

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.

[AnthonyRonning]

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:

1. Other approaches to 0 conf JIT channel payments (something double-invoice like instead of routing hint-like)
2. Escrow (having a router decide to continue routing to destination in order to deliver funds based on a condition)
3. Whether or not to route payments down certain channels or at all (liquidity management, balance probe protection)
4. Third party preimages (node not knowing the preimage but waiting for either a third party or a different application handle preimages)
5. 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]

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]

Had to rebase to get #1844.

[AnthonyRonning]

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 👍

[ViktorT-11]

First look-through generally looks good :).

[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.

[valentinewallace]

Great feedback once again @ViktorTigerstrom! Working on addressing it

[ViktorT-11]

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.

[ViktorT-11]

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.

[valentinewallace]

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.

[ViktorT-11]

Ok fair enough :)

[TheBlueMatt]

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]

LGTM, I think.

[ViktorT-11]

Looks good to me as well!

[dunxen]

LGTM, just a tiny nit and question :)

[dunxen]

new rust style team pls save us

[dunxen]

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.

[valentinewallace]

Yeah, I think that would be nice to have!

[dunxen]

just a question to check my understanding:

Is this basically the procedure for an LDK user?

1. Calls ChannelManager::get_intercept_scid
2. Generates invoice with intercept scid in route hints for end-user
3. Receives an HTLCIntercepted event when the HTLC is intercepted
4. At this point the LDK user can create a channel
5. Then call this method to forward over that JIT channel

[ViktorT-11]

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.

[dunxen]

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.

[valentinewallace]

Rebased due to conflicts

[ariard]

ACK acff8f6

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

[ariard]

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

[ariard]

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]

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]

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

[codecov-commenter]

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.

[AnthonyRonning]

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:

1. Other approaches to 0 conf JIT channel payments (something double-invoice like instead of routing hint-like)
2. Escrow (having a router decide to continue routing to destination in order to deliver funds based on a condition)
3. Whether or not to route payments down certain channels or at all (liquidity management, balance probe protection)
4. Third party preimages (node not knowing the preimage but waiting for either a third party or a different application handle preimages)
5. 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]

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]

Had to rebase to get #1844.

[AnthonyRonning]

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 👍

[ViktorT-11]

First look-through generally looks good :).

[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.

[valentinewallace]

Great feedback once again @ViktorTigerstrom! Working on addressing it

[ViktorT-11]

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.

[ViktorT-11]

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.

[valentinewallace]

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.

[ViktorT-11]

Ok fair enough :)

[TheBlueMatt]

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]

LGTM, I think.

[ViktorT-11]

Looks good to me as well!

[dunxen]

LGTM, just a tiny nit and question :)

[dunxen]

new rust style team pls save us

[dunxen]

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.

[valentinewallace]

Yeah, I think that would be nice to have!

[dunxen]

just a question to check my understanding:

Is this basically the procedure for an LDK user?

1. Calls ChannelManager::get_intercept_scid
2. Generates invoice with intercept scid in route hints for end-user
3. Receives an HTLCIntercepted event when the HTLC is intercepted
4. At this point the LDK user can create a channel
5. Then call this method to forward over that JIT channel

[ViktorT-11]

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.

[dunxen]

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.

[valentinewallace]

Rebased due to conflicts

[ariard]

ACK acff8f6

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

[ariard]

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

[ariard]

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]

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]

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

[codecov-commenter]

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.

[AnthonyRonning]

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:

1. Other approaches to 0 conf JIT channel payments (something double-invoice like instead of routing hint-like)
2. Escrow (having a router decide to continue routing to destination in order to deliver funds based on a condition)
3. Whether or not to route payments down certain channels or at all (liquidity management, balance probe protection)
4. Third party preimages (node not knowing the preimage but waiting for either a third party or a different application handle preimages)
5. 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]

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]

Had to rebase to get #1844.

[AnthonyRonning]

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 👍

[ViktorT-11]

First look-through generally looks good :).

[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.

[valentinewallace]

Great feedback once again @ViktorTigerstrom! Working on addressing it

[ViktorT-11]

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.

[ViktorT-11]

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.

[valentinewallace]

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.

[ViktorT-11]

Ok fair enough :)

[TheBlueMatt]

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]

LGTM, I think.

[ViktorT-11]

Looks good to me as well!

[dunxen]

LGTM, just a tiny nit and question :)

[dunxen]

new rust style team pls save us

[dunxen]

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.

[valentinewallace]

Yeah, I think that would be nice to have!

[dunxen]

just a question to check my understanding:

Is this basically the procedure for an LDK user?

1. Calls ChannelManager::get_intercept_scid
2. Generates invoice with intercept scid in route hints for end-user
3. Receives an HTLCIntercepted event when the HTLC is intercepted
4. At this point the LDK user can create a channel
5. Then call this method to forward over that JIT channel

[ViktorT-11]

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.

[dunxen]

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.

[valentinewallace]

Rebased due to conflicts

[ariard]

ACK acff8f6

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

[ariard]

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

[ariard]

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]

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]

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

[codecov-commenter]

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.

[AnthonyRonning]

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:

1. Other approaches to 0 conf JIT channel payments (something double-invoice like instead of routing hint-like)
2. Escrow (having a router decide to continue routing to destination in order to deliver funds based on a condition)
3. Whether or not to route payments down certain channels or at all (liquidity management, balance probe protection)
4. Third party preimages (node not knowing the preimage but waiting for either a third party or a different application handle preimages)
5. 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]

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]

Had to rebase to get #1844.

[AnthonyRonning]

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 👍

[ViktorT-11]

First look-through generally looks good :).

[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.

[valentinewallace]

Great feedback once again @ViktorTigerstrom! Working on addressing it

[ViktorT-11]

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.

[ViktorT-11]

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.

[valentinewallace]

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.

[ViktorT-11]

Ok fair enough :)

[TheBlueMatt]

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]

LGTM, I think.

[ViktorT-11]

Looks good to me as well!

[dunxen]

LGTM, just a tiny nit and question :)

[dunxen]

new rust style team pls save us

[dunxen]

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.

[valentinewallace]

Yeah, I think that would be nice to have!

[dunxen]

just a question to check my understanding:

Is this basically the procedure for an LDK user?

1. Calls ChannelManager::get_intercept_scid
2. Generates invoice with intercept scid in route hints for end-user
3. Receives an HTLCIntercepted event when the HTLC is intercepted
4. At this point the LDK user can create a channel
5. Then call this method to forward over that JIT channel

[ViktorT-11]

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.

[dunxen]

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.

[valentinewallace]

Rebased due to conflicts

[ariard]

ACK acff8f6

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

[ariard]

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

[ariard]

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]

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]

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

[codecov-commenter]

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.

[AnthonyRonning]

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:

1. Other approaches to 0 conf JIT channel payments (something double-invoice like instead of routing hint-like)
2. Escrow (having a router decide to continue routing to destination in order to deliver funds based on a condition)
3. Whether or not to route payments down certain channels or at all (liquidity management, balance probe protection)
4. Third party preimages (node not knowing the preimage but waiting for either a third party or a different application handle preimages)
5. 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]

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]

Had to rebase to get #1844.

[AnthonyRonning]

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 👍

[ViktorT-11]

First look-through generally looks good :).

[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.

[valentinewallace]

Great feedback once again @ViktorTigerstrom! Working on addressing it

[ViktorT-11]

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.

[ViktorT-11]

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.

[valentinewallace]

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.

[ViktorT-11]

Ok fair enough :)

[TheBlueMatt]

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]

LGTM, I think.

[ViktorT-11]

Looks good to me as well!

[dunxen]

LGTM, just a tiny nit and question :)

[dunxen]

new rust style team pls save us

[dunxen]

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.

[valentinewallace]

Yeah, I think that would be nice to have!

[dunxen]

just a question to check my understanding:

Is this basically the procedure for an LDK user?

1. Calls ChannelManager::get_intercept_scid
2. Generates invoice with intercept scid in route hints for end-user
3. Receives an HTLCIntercepted event when the HTLC is intercepted
4. At this point the LDK user can create a channel
5. Then call this method to forward over that JIT channel

[ViktorT-11]

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.

[dunxen]

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.

[valentinewallace]

Rebased due to conflicts

[ariard]

ACK acff8f6

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

[ariard]

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

[ariard]

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]

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]

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

[codecov-commenter]

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.

[AnthonyRonning]

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:

1. Other approaches to 0 conf JIT channel payments (something double-invoice like instead of routing hint-like)
2. Escrow (having a router decide to continue routing to destination in order to deliver funds based on a condition)
3. Whether or not to route payments down certain channels or at all (liquidity management, balance probe protection)
4. Third party preimages (node not knowing the preimage but waiting for either a third party or a different application handle preimages)
5. 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]

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]

Had to rebase to get #1844.

[AnthonyRonning]

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 👍

[ViktorT-11]

First look-through generally looks good :).

[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.

[valentinewallace]

Great feedback once again @ViktorTigerstrom! Working on addressing it

[ViktorT-11]

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.

[ViktorT-11]

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.

[valentinewallace]

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.

[ViktorT-11]

Ok fair enough :)

[TheBlueMatt]

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]

LGTM, I think.

[ViktorT-11]

Looks good to me as well!

[dunxen]

LGTM, just a tiny nit and question :)

[dunxen]

new rust style team pls save us

[dunxen]

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.

[valentinewallace]

Yeah, I think that would be nice to have!

[dunxen]

just a question to check my understanding:

Is this basically the procedure for an LDK user?

1. Calls ChannelManager::get_intercept_scid
2. Generates invoice with intercept scid in route hints for end-user
3. Receives an HTLCIntercepted event when the HTLC is intercepted
4. At this point the LDK user can create a channel
5. Then call this method to forward over that JIT channel

[ViktorT-11]

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.

[dunxen]

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.

[valentinewallace]

Rebased due to conflicts

[ariard]

ACK acff8f6

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

[ariard]

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

[ariard]

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]

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]

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

[codecov-commenter]

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.

[AnthonyRonning]

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:

1. Other approaches to 0 conf JIT channel payments (something double-invoice like instead of routing hint-like)
2. Escrow (having a router decide to continue routing to destination in order to deliver funds based on a condition)
3. Whether or not to route payments down certain channels or at all (liquidity management, balance probe protection)
4. Third party preimages (node not knowing the preimage but waiting for either a third party or a different application handle preimages)
5. 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]

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]

Had to rebase to get #1844.

[AnthonyRonning]

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 👍

[ViktorT-11]

First look-through generally looks good :).

[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.

[valentinewallace]

Great feedback once again @ViktorTigerstrom! Working on addressing it

[ViktorT-11]

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.

[ViktorT-11]

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.

[valentinewallace]

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.

[ViktorT-11]

Ok fair enough :)

[TheBlueMatt]

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]

LGTM, I think.

[ViktorT-11]

Looks good to me as well!

[dunxen]

LGTM, just a tiny nit and question :)

[dunxen]

new rust style team pls save us

[dunxen]

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.

[valentinewallace]

Yeah, I think that would be nice to have!

[dunxen]

just a question to check my understanding:

Is this basically the procedure for an LDK user?

1. Calls ChannelManager::get_intercept_scid
2. Generates invoice with intercept scid in route hints for end-user
3. Receives an HTLCIntercepted event when the HTLC is intercepted
4. At this point the LDK user can create a channel
5. Then call this method to forward over that JIT channel

[ViktorT-11]

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.

[dunxen]

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.

[valentinewallace]

Rebased due to conflicts

[ariard]

ACK acff8f6

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

[ariard]

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

[ariard]

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]

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.