funding: require explicit channel type in all negotiations - #11064
funding: require explicit channel type in all negotiations #11064NishantBansal2003 wants to merge 2 commits into
Conversation
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟢 Low (1 file)
AnalysisThis PR changes To override, add a |
|
We should remove implicit channel-type negotiation instead of gating this validation on Please require |
|
When removing the implicit negotiation paths, we should keep the backward-compatibility implications in mind. In particular, removing implicit negotiation should be separate from removing the Continuing to advertise bit 44 allows older LND versions that understand explicit channel types to enter their explicit path and send/echo I suggest always requiring, sending, and echoing |
686650d to
f781d03
Compare
Wanted to do that in the first place, since all the other implementations currently do this, but I was unsure why it wasn’t done in: #9637. Anyway, in the latest commit, I did the following:
|
LND allows `open_channel` with an omitted `channel_type`, violating BOLT 2 even though it signals the required `option_channel_type` feature bit. This will be tracked upstream and will be suppressed until fixed. see: lightningnetwork/lnd#11064 Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
LND allows `open_channel` with an omitted `channel_type`, violating BOLT 2 even though it signals the required `option_channel_type` feature bit. This will be tracked upstream and will be suppressed until fixed. see: lightningnetwork/lnd#11064 Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
There was a problem hiding this comment.
Agreed with @ziggie1984 .
The flip side is that the advertisement is now a prerequisite of this change rather than an independent feature flag: dropping that line silently breaks the funding flow against older peers, so a short note there would be cheap insurance:
// NOTE: Funding requires an explicit channel type, and older peers only
// enter their explicit path when we advertise this. Removing this bit
// breaks the funding flow against them until those versions age out.
lnwire.ExplicitChannelTypeRequired: {
SetInit: {}, // I
SetNodeAnn: {}, // N
},| zeroConf bool | ||
| scid bool | ||
| ) | ||
|
|
There was a problem hiding this comment.
nit: the var block only existed because the assignment lived inside the chanType != nil guard.
| var ( | ||
| zeroConf bool | ||
| scid bool | ||
| ) |
There was a problem hiding this comment.
nit: the var block only existed because the assignment lived inside the chanType != nil guard.
|
|
||
| // TestFundingManagerRejectMissingChanType verifies that the fundee rejects an | ||
| // OpenChannel message that omits the ChannelType field. | ||
| func TestFundingManagerRejectMissingChanType(t *testing.T) { |
There was a problem hiding this comment.
nit — about regression robustness, not the change itself.
The message omits all six pubkeys, so this check is the only thing preventing a nil deref further down. Removing it locally doesn't fail the test: it panics in copyPubKey and takes the binary down.
There was a problem hiding this comment.
Thanks, good point, I added those keys in the latest push
|
I've been building a coverage-guided fuzz harness for the funding manager, and added oracles for BOLT 2's
For now these are logged rather than fatal, so the fuzzer can keep running against master — it otherwise stops within seconds and reaches nothing else. The must-reject check for case 1 is commented out as well; the case still surfaces, All three pass with this branch applied. |
Move from optional implicit negotiation to mandatory explicit channel type in OpenChannel and AcceptChannel. The returned ChannelType is now always non-nil. Channel type is required in all negotiations now. We only fallback to a default channel type when the RPC caller does not explicitly specify one. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
f781d03 to
b2b6678
Compare
MPins
left a comment
There was a problem hiding this comment.
LGTM 🎉
Verified the test fix does what it was meant to: with the check removed, the package now fails as expected, instead of panicking in copyPubKey and taking the binary down.
Two non-code leftovers before this lands: missing the release notes and the PR description, which still describe the conditional design "I think we should fail the funding flow if ChannelType is omitted when both peers have negotiated ExplicitChannelType".
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Currently, BOLT assumes
ExplicitChannelType, and LND sends it as required (#9637). However, when receiving anOpenChannelfrom peer, LND doesn’t enforce an explicitChannelTypeand falls back to implicit negotiation, even though both peers have negotiatedExplicitChannelType. According to BOLT 2, I think we should fail the funding flow ifChannelTypeis omitted from the receivedOpenChannelmessage and echo the same channel type (if valid) inAcceptChannel.So, we should remove implicit negotiation entirely. If the RPC caller doesn’t request a channel type, a default should be derived from both peers' features and signaled explicitly.