Skip to content

htlcswitch: add interceptor channel filter #6874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Aug 31, 2022

In #6232, a new command line flag --requireinterceptor was added. This is an essential flag for certain types of applications that use the htlc interceptor api. But at the same time, it is also a flag which may have too much power. If set and there's no interceptor registered, all forwards on the node will halt.

Applications that intercept traffic to a virtual channel really only need to be notified of htlcs that are bound for that channel. Regular forwards over real channels can proceed as normal.

This PR adds a channel filter flag htlc-interceptor-channel-filter to the htlc interceptor api so that the traffic that is intercepted can be limited. The goal is to decouple the availability of the node from the intercepting application as much as possible.

An alternative to this PR is to add a filter that makes the interceptor only intercept htlcs to unknown channels. This seems to be a more complex option, because the interceptable switch needs to be aware of all real channels. It also introduces edge cases where a channel doesn't exist yet, but when an htlc is replayed later, the channel does exist. This could mean that the htlc isn't offered to the interceptor anymore on the replay. Also with scid alias, a counterparty can open channels with scids in the full uint64 range. There is no reserved space as far as I know.

Todo:

  • Add test coverage

@joostjager joostjager requested a review from Crypt-iQ September 1, 2022 08:11
@joostjager joostjager force-pushed the interceptor-chan-filter branch from e4cde5a to 3463fcb Compare September 7, 2022 12:12
@joostjager
Copy link
Contributor Author

Looking for concept ack

@Crypt-iQ
Copy link
Collaborator

Also with scid alias, a counterparty can open channels with scids in the full uint64 range. There is no reserved space as far as I know.

the counterparty's scid alias isn't used for forwarding, the switch is only aware of our own scid aliases. lnd uses an scid starting with a block height of 16_000_000 -> max block height for a 24-bit number

@@ -383,6 +383,8 @@ type Config struct {
// registered regardless of whether the RPC is called or not.
RequireInterceptor bool `long:"requireinterceptor" description:"Whether to always intercept HTLCs, even if no stream is attached"`

HTLCInterceptorChannelFilter uint64 `long:"htlc-interceptor-channel-filter" description:"Only intercept htlcs for the specified outgoing channel"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this instead be a list? Or are applications instead using a virtual scid for many channels such that this single uint64 is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure honestly. For my application, a single channel is sufficient. But I don't know about others. Maybe it could be a way to keep certain types of traffic apart. Making it a list is fairly cheap though, so I could do that.

@joostjager
Copy link
Contributor Author

the counterparty's scid alias isn't used for forwarding, the switch is only aware of our own scid aliases. lnd uses an scid starting with a block height of 16_000_000 -> max block height for a 24-bit number

Ah ok. In code I also found this:

the starting ShortChannelID is 16000000:0:0 and the ending ShortChannelID is 16250000:16777215:65535.

With that we could define a range for virtual channels and make the filter a boolean flag indicating to only intercept htlcs destined for channels in the virtual channel range.

But not sure if that is better or worse than an explicit list of channels to intercept?

@Crypt-iQ
Copy link
Collaborator

With that we could define a range for virtual channels and make the filter a boolean flag indicating to only intercept htlcs destined for channels in the virtual channel range.

If these virtual channels are using scid's that are not allocated via the existing scid-alias mechanism, then the switch logic would need to be changed to account for this. This would be in handlePacketForward when it calls getLinkByMapping. Also the existing range is the scid-alias range for channels with peers that have negotiated the bit, so unless this channel is one of those, it should use a different range (maybe 16250001:0:0 and higher)

But not sure if that is better or worse than an explicit list of channels to intercept?

I think it's less complex to intercept an explicit list of channels, but here we cannot change the list of channels on the fly. I don't know if that prohibits certain use cases or not.

@joostjager
Copy link
Contributor Author

If these virtual channels are using scid's that are not allocated via the existing scid-alias mechanism, then the switch logic would need to be changed to account for this.

Is this really necessary? I'd think that forwards over virtual channels will never even reach the switch. InterceptableSwitch will handle them, and if not, those forwards are supposed to fail?

I think it's less complex to intercept an explicit list of channels, but here we cannot change the list of channels on the fly. I don't know if that prohibits certain use cases or not.

Hard to say. For our use case, we just need to intercept a single scid. So either a single value or list works. Perhaps just go with that, because guessing at future requirements is usually not very successful?

@joostjager
Copy link
Contributor Author

It seems that LDK is using a specific channel range as an interception condition: lightningdevkit/rust-lightning#1835

@lightninglabs-deploy
Copy link

@joostjager, remember to re-request review from reviewers when ready

@joostjager
Copy link
Contributor Author

!lightninglabs-deploy mute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants