Support HTLC interception by source channel #4338
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While its nice to document things, the lockorder comment at the top of `ChannelManager` is just annoying to always update and doesn't add all that much value. Developers likely shouldn't be checking it while writing code, our automated lockorder issue detection framework more than suffices to catch any bugs in test-reachable code. That makes it basically write-only which isn't exactly a useful comment.
When we added support for async payments (which requires holding HTLCs until we receive an onion message), we added the hold logic to `ChannelManager::forward_htlcs`. This made sense as we reused the forwarding datastructure in the holding logic so already had the right types in place, but it turns out only a single call of `forward_htlcs` should ever result in an HTLC being held. All of the other calls (un-holding an HTLC, forwarding an intercepted HTLC, forwarding an HTLC decoded by LDK prior to 0.2, or processing a phantom receive) should never result in an HTLC being held. Instead, HTLCs should actually only ever be held when the HTLC is decoded in `process_pending_update_add_htlcs` before forwarding. Because of this, and because we want to move the interception (and thus also the holding logic) out of `forward_htlcs`, here we move the holding logic into `process_pending_update_add_htlcs`.
If a peer is offline, but only recently went offline and thus the channel has not yet been marked disabled in our gossip, we should be returning `LocalHTLCFailureReason::PeerOffline` rather than `LocalHTLCFailureReason::ChannelNotReady`. Here we fix the error returned and tweak documentation to make the cases clearer.
In the next commit we'll substantially expand the types of HTLCs which can be intercepted. In order to do so, we want to make forwarding decisions with access to the (specified) destination channel. Sadly, this isn't available in `forward_htlcs`, so here we move interception decisions out of `forward_htlcs` and into `process_pending_update_add_htlcs` and `handle_release_held_htlc`. Note that we do not handle HTLC interception when forwarding an HTLC which was decoded in LDK versions prior to 0.2, which is noted in a suggested release note. This is due to a gap where such HTLC might have had its routing decision made already and be waiting for an interception decision in `forward_htlcs`, but now we will only make an interception decision when decoding the onion.
At various points we've had requests to support more generic HTLC interception in LDK. In most cases, full HTLC interception was not, in fact, the right way to accomplish what the developer wanted, but there have been various times when it might have been. Here, we finally add full HTLC interception support, doing so with a configurable bitfield to allow developers to intercept only certain classes of HTLCs. Specifically, we currently support intercepting HTLCs: * which were to be forwarded to intercept SCIDs (as was already supported), * which were to be forwarded to offline private channels (for LSPs to accept HTLCs for offline clients so that they can attempt to wake them before failing the HTLC), * which were to be forwarded to online private channels (for LSPs to take additional fees or enforce certain policies), * which were to be forwarded over public channels (for general forwarding policy enforcement), * which were to be forwarded to unknown SCIDs (for everything else).
In the previous commit our interception documentation noted that in some cases devs may wish to validate the CLTV expiry of HTLCs before forwarding. Here we enable that by exposing the next-hop's CLTV value in `Event::HTLCIntercepted`
We jump through some hoops in order to pass a small list of objects to `forward_htlcs` on a per-channel basis rather than per-HTLC. Then, `forward_htlcs` builds a `PendingAddHTLCInfo` for each HTLC for insertion. Worse, in some `forward_htlcs` callsites we're actually starting with a `PendingAddHTLCInfo`, converting it to a tuple, then back inside `forward_htlcs`. Instead, here we just pass a list of built `PendingAddHTLCInfo`s to `forward_htlcs`, cleaning up a good bit of code and even avoiding an allocation of the HTLCs vec in many cases.
It may be useful in some situations to select HTLCs for interception based on the source channel in addition to the sink. Here we add the ability to do so by adding new flags to `HTLCInterceptionFlags`.
|
👋 I see @tnull was un-assigned. |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just a few more flags and a small cleanup.
Based on #4300