lightningnetwork / lnd

Lightning Network Daemon ⚡️
MIT License
7.69k stars 2.08k forks source link

[bug]: Channels without zero-conf feature but `minimum_depth` 0 are rejected #8783

Closed tnull closed 4 months ago

tnull commented 5 months ago

Background

As per BOLT 2, a node receiving open_channel can communicate the number of confirmations of the funding transaction it deems reasonable via accept_channel's minimum_depth field:

if channel_type includes option_zeroconf:

    MUST set minimum_depth to zero.

otherwise:

    SHOULD set minimum_depth to a number of blocks it considers reasonable to avoid double-spending of the funding transaction.

However, LND currently does not only check that minimum_depth is 0 if the zero conf. feature is set, but also that minimum_depth is non-zero otherwise.

In particular, it rejects any channel without the feature that sets minimum_depth to 0:

https://github.com/lightningnetwork/lnd/blob/bc6292f8bd37ddcb4283e79418861be37a36a618/funding/manager.go#L2023-L2030

Unfortunately, this means LND is currently unnecessarily failing channel opens on a more permissive counterparty limit.

LDK currently does not set the optional zero-conf feature bit, but just sets minimum_depth to zero in accept_channel via a specific API method via which users can opt into trusting the counterparty for 0conf. However, if we now do this on a per-peer basis and the LND counterparty tries to open a non-zero conf channel to us, it will always be rejected due to non-zero-conf channel has min depth zero.

That is, due to this arbitrary restriction, users will have to know beforehand whether the LND counterparty will open a zero-conf or non-zero-conf channel.

Steps to reproduce

Try to open a non-zero-conf channel from LND to a LDK node that trusts its LND counterparty for zero-conf channels.

Expected behaviour

The channel should be opened.

Actual behaviour

The channel is unnecessarily rejected.

Roasbeef commented 5 months ago

That is, due to this arbitrary restriction, users will have to know beforehand whether the LND counterparty will open a zero-conf or non-zero-conf channel.

Why can't LDK set the channel type feature bit? It was added explicitly for this reason. By setting the bit, both parties know up front the type of channel desired.

tnull commented 5 months ago

Why can't LDK set the channel type feature bit? It was added explicitly for this reason. By setting the bit, both parties know up front the type of channel desired.

I believe this has been discussed quite a bit in the past, so I don't intend to fully restart the discussions around whether or not to set the optional feature bit here. If LND wants to set the feature bit that's fine, but it shouldn't unnecessarily break interop with implementations that do not require it?

Could you explain what benefit the above cited 'early failure' of the channel has to which party?

Roasbeef commented 5 months ago

I believe this has been discussed quite a bit in the past, so I don't intend to fully restart the discussions around whether or not to set the optional feature bit here.

You seem to be doing just that by opening this issue.

break interop with implementations that do not require it?

Has LDK changed it's behavior/API recently? To my knowledge we achieved interop long ago when this feature was first added, and there exist production systems that utilize zero conf channels between the implementations np.

Could you explain what benefit the above cited 'early failure' of the channel has to which party?

If a node expects a zero conf channel (sets the bit), but the responder doesn't want one, then they can reject the channel opening with an error. If a user expects a zero conf channel, but the initiator doesn't, then without this strict behavior, the user's channel would be silently downgraded (which can lead to UX issue if the user expects to be able to recv an HTLC over a zero conf channel).

As you note above, this is a trusted channel type, thus it should be explicit that such a channel is or isn't accepted.

Can you explain what prevents LDK from conditionally setting min_depth based on the incoming feature bit and the user's configured settings? The logic should be straight forward, if you need some inspiration, you can see how lnd does it with the ChannelAcceptor API.

tnull commented 5 months ago

Has LDK changed it's behavior/API recently? To my knowledge we achieved interop long ago when this feature was first added, and there exist production systems that utilize zero conf channels between the implementations np.

No, the behavior hasn't changed, but I assume it was only ever tested if zero-conf channel opens work, not if the zero-conf behavior then breaks non-zero conf channels.

If a node expects a zero conf channel (sets the bit), but the responder doesn't want one, then they can reject the channel opening with an error.

Right, that case is clearly compatible and defined in the BOLT.

If a user expects a zero conf channel, but the initiator doesn't, then without this strict behavior, the user's channel would be silently downgraded (which can lead to UX issue if the user expects to be able to recv an HTLC over a zero conf channel.

Hmm, but I believe when the feature was added it was kept optional? Note that it's only ever described that min depth has to be 0 if the feature is set, not that it can't be zero if it's not set (i.e., feature and minimum_depth == 0 are not equivalent).

As you note above, this is a trusted channel type, thus it should be explicit that such a channel is or isn't accepted.

But isn't the risk only on the acceptor's side, which is why it defines in the lower-bound via minimum_depth?

Can you explain what prevents LDK from conditionally setting min_depth based on the incoming feature bit and the user's configured settings?

Yes, it would simply break interop with implementations who don't require or set the feature (namely LDK itself, not sure about the current state in CLN).

TheBlueMatt commented 5 months ago

Why can't LDK set the channel type feature bit?

Because then the peer would reject the channel if they don't want 0conf? We'd like to open a channel, offering 0conf, but in most cases will happily continue if the counterparty politely declines 0conf by simply setting a higher min_depth.

It was added explicitly for this reason. By setting the bit, both parties know up front the type of channel desired.

No it wasn't. The reason the feature bit was added was so that a channel opener could require a 0conf channel with their peer, telling the peer up front that "0conf or no channel". What's happening here is LND is requiring that the channel initiator demand a 0conf channel in order to open a 0conf channel at all, which seems very strange.

ellemouton commented 5 months ago

makes sense. also looks like the other way around is handled correctly: we do allow a peer to send us an OpenChannel that does not set the channel type to zero conf explicitly and then we will send back a min-depth of 0 if our channelAcceptor interceptor allows it/requires it. So we expect the peer to be ok with this & hence we should be ok when the peer does the same to us

ellemouton commented 5 months ago

^ opened a patch if yall wanna test it out

Roasbeef commented 5 months ago

Because then the peer would reject the channel if they don't want 0conf?

That's ideal behavior imo. You want the channel to be zero conf (vastly diff trust relationship that normal channels), it should be set explicitly at the protocol level what expectations are. This is implicit vs explicit, fast fail vs silently fail.

tnull commented 5 months ago

That's ideal behavior imo. You want the channel to be zero conf (vastly diff trust relationship that normal channels), it should be set explicitly at the protocol level what expectations are. This is implicit vs explicit, fast fail vs silently fail.

But the issue here is not that the channel opener requires zero conf and makes it explicit. The issue is that a non-zero conf channel will be rejected by LND if the acceptor would trust it to be zero-conf. So it's not about making zero-conf explicit, it's about unnecessary failing on a more permissive counterparty requirement.