[DRAFT] Generic HTLC Interception #3843
Conversation
|
👋 I see @wpaulino was un-assigned. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3843 +/- ##
==========================================
- Coverage 89.88% 89.76% -0.12%
==========================================
Files 160 162 +2
Lines 129654 130361 +707
Branches 129654 130361 +707
==========================================
+ Hits 116534 117014 +480
- Misses 10425 10646 +221
- Partials 2695 2701 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Concept ACK, this def. goes in the direction I had in mind for #2855.
Might be good to get early feedback from others too though (cc @TheBlueMatt @wpaulino)
| >, | ||
| Arc<test_utils::TestLogger>, | ||
| >; | ||
| type ChannelManager = channelmanager::ChannelManager< |
There was a problem hiding this comment.
nit: It's tabs, not spaces across LDK-related projects. Please also just run cargo +1.63 fmt after each commit.
| /// HTLCInterceptAction::Forward { | ||
| /// next_hop_channel_id: htlc_info.next_channel_id, | ||
| /// next_node_id: htlc_info.next_node_id, | ||
| /// amount_to_forward_msat: htlc_info.expected_outbound_amount_msat - fee, |
There was a problem hiding this comment.
While this might work, I wonder if it would be better if we'd find a way to still restrict the possible changes that could happen as part of an HTLCInterceptionAction to safe values and/or values that are feasible to override. For example, we could define a struct ModifyForwardAction or similar that is held in HTLCInterceptAction::Forward and that would expose certain setters that still employ certain safety checks, etc.
There was a problem hiding this comment.
Makes sense. Would the idea be that the handler is called with an instance of ModifyForwardAction initialized with the current next hop information and that represents a no-op essentially? Or were you thinking it would be something the end-user constructs themselves if they want to modify the forward?
aylagreystone
force-pushed
the
htlc-interception
branch
from
331b37b to
87648d8
Compare
|
One concern I had with this approach compared to the scid/event based approach that currently exists is around use-cases like LSPS5 where the end-user will want to hold the HTLC while they attempt to wake-up the recipient before attempting the forward. This inline handler approach means this entire code-path would block in that scenario. Seems potentially dangerous to block in this hot-path but is something I would love some feedback on. |
Yeah this was my immediate concern as well. The handler approach requires implementers to immediately act on the intercepted HTLC to not delay unrelated HTLCs in other channels, given this is interrupting the node's forwarding pipeline. If we really intend for generic interception then the flow needs to be async as we can't possible predict/enforce the type of code that would be run in the handler. The event approach would help solve this, and also allows for flexibility on the user's end to let them decide which specific channels they're interested in receiving intercept events for. |
Thanks for the feedback! I had opened this PR previously (#3839) that extends the current scid + event based interception that exists today to allow users to opt-in specific channels to interception by creating the channel's scid alias using the Intercept namespace. That approach works for my use cases so I'm happy to pick that back up if the team deems it a viable path forward. |
Let's wait for @TheBlueMatt to chime in, but that approach doesn't seem flexible enough (assuming that's the goal) because it'd require you to set interception during channel creation and it'd remain fixed throughout the lifetime of the channel. We'd probably want something more configurable that lets you specify the channels you're interested in at runtime. |
Yes, while I had hoped we can find an interface that is a bit sleeker I agree that we probably have to go the callback route, at least as one option. FWIW, we could have "I'll call back in" as one
I mentioned this on Discord also, but I don't expect many users to filter the channels before they intercept. I rather expect them to intercept all and then decide in their logic whether just to shortcut to forwarding, or take additional steps before continuing the flow. In particular, we also had user requests for a generalized interface that currently simply use HTLC interception (in LND) as some way to get additional data on all HTLCs forwarded, and to hook into more general 'HTLC firewalling' code. All that is to say that IMO we'll need to find a solution for the interface that scales to the degree so a large forwarding node could in theory use it generally to hook all HTLCs they see. While I agree that we'll probably need a callback architecture to not block the flow, I don't think (ab)using the event queue for this is an option, as it would be really spammy, and would introduce at least two (?) additional unnecessary IOps. TLDR: Building out a separate, callback & |
There was a problem hiding this comment.
Indeed, it certainly seems like no matter what we do we need to support callbacks to decide what to do with an HTLC later. If we don't want to use the event pipeline for those (which I think I agree with, though we do already push an event per forwarded HTLC, so its not entirely out of the realm of possibility), then indeed having some trait decide which HTLCs to intercept makes sense to me.
Rather than having the trait return a "what to do with this HTLC", we could instead consider the trait returning "do we want to intercept this" and then either push it out via the normal intercept pipeline (without a notification event) or forward it normally. This would also make the patch pretty small, assuming this is the API we want. Of course it might be nice for some to have the "what to do with this HTLC" option in the API, but we could similarly implement that by just relying on the existing intercept logic, I assume.
| /// Default value: `false` | ||
| /// | ||
| /// [`HTLCInterceptHandler`]: crate::ln::channelmanager::HTLCInterceptHandler | ||
| pub accept_generic_htlc_intercept: bool, |
There was a problem hiding this comment.
Dunno if we need to bother with a config flag if we're setting a trait for this. We can just use the Default trait impl and call it always, no?
Adds a
HTLCInterceptHandlertrait with a default implementation that forwards the HTLC as expected. Opened as a draft to get some initial feedback on the overall direction because I'm not really sure if this is the right approach.