multi: stop opening legacy channels - #11212
Conversation
0e630a8 to
de8dcb6
Compare
ziggie1984
added
backport-v0.20.x-branch
de8dcb6 to
b3994cf
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (5 files)
🟡 Medium (3 files)
🟢 Low (5 files)
AnalysisThis PR changes how To override, add a |
b3994cf to
5828b7f
Compare
Lrifton92
left a comment
Lrifton92
left a comment
There was a problem hiding this comment.
Read through the negotiation change and its call sites; this looks right to me.
A few things I checked:
negotiateCommitmentTypereally is the only place a new channel's type gets picked: its only callers arefundeeProcessOpenChannelandhandleInitFundingMsg, andOpenChannel,OpenChannelSyncandBatchOpenChannelall go throughparseOpenChannelReq, so the RPC check covers every way in.ErrNoAnchorSupportbeing a plain error rather than aFundingErroris fine: the responder already rejects an open withoutchannel_typewithErrChanTypeRequired(manager.go:1578), so implicit negotiation, and with it this error, can only happen on the initiator side, where it goes back to our own caller.- The explicit table has exactly two pre-anchor branches,
StaticRemoteKeyalone and the empty vector, and both are gated now. Every other branch requires anchors or taproot. --protocol.no-anchors:configToFlatMapflags ahiddenfield only when it's non-zero, so the startup warning fires only for configs that actually set it.legacy.committweakis still dev-only, so production can't drop below anchors any more.
go test ./funding -run TestCommitmentTypeNegotiation passes locally.
One small test gap: the new cases cover "default no downgrade to tweakless", but not the case where neither side signals static remote key. The noDeprecated check comes before both fallbacks, so the code already handles it; a case with that feature vector would just make sure the legacy fallback can't come back unnoticed if the check is ever moved.
5828b7f to
2fb854a
Compare
ziggie1984
changed the title
| )) | ||
|
|
||
| return &chanType, lnwallet.CommitmentTypeTweakless | ||
| return &chanType, lnwallet.CommitmentTypeTweakless, nil |
There was a problem hiding this comment.
Do we want to go further and also start rejecting non anchor chans?
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Some nits.
I propose to add this historical context of channel generations to the PR description somewhere:
LEGACY → static remote key → anchors → taproot.
Should we test continued operation of existing legacy channels after upgrading? Itests switch to static remote key. That makes sense for creating new channels, but removes their legacy integration coverage. An older-version or persisted-state fixture should exercise reconnection, payments, closure, and watchtower protection using an already-created legacy channel. Including a pending legacy channel across restart would also protect the upgrade boundary. Lower-level legacy tests remain, so this is a coverage limitation, not evidence those operations are broken.
| using the legacy commitment type, which was | ||
| [removed](https://github.com/lightning/bolts/commit/91f4bd2383cc2fc7a0a43b697e209f9eb9f5183c) | ||
| from the spec in 2024. Its tweaked `to_remote` output is why funds in such a | ||
| channel cannot be recovered after data loss. |
There was a problem hiding this comment.
It cannot be recovered unilaterally. If the peer supplies the appropriate commitment point, it is still possible.
|
|
||
| // ErrChanTypeDeprecated is returned by a remote peer that receives a | ||
| // FundingOpen request for a channel type it no longer opens, namely | ||
| // the legacy and the static remote key commitment types. It is kept |
There was a problem hiding this comment.
and the static remote key commitment types
This PR continues accepting static remote key. This appears to be leftover wording from the broader proposal.
| // The returned ChannelType is always non-nil and is always signaled on the | ||
| // wire. An error is only returned if desiredChanType is not supported. |
There was a problem hiding this comment.
I think this part needs updating.
Suggested comment:
// On success, the returned ChannelType is non-nil and is signaled on the
// wire. An error is returned if the requested type is unsupported or
// deprecated, or if no supported default type can be selected.| case lnrpc.CommitmentType_LEGACY: | ||
| channelType = new(lnwire.ChannelType) | ||
| *channelType = lnwire.ChannelType(*lnwire.NewRawFeatureVector()) | ||
| return nil, funding.ErrDeprecatedChanType |
There was a problem hiding this comment.
Could we also document this restriction in lightning.proto? The LEGACY enum and the commitment_type fields in OpenChannelRequest and BatchOpenChannel don't mention that legacy is now rejected for new openings. The enum should remain for reporting existing channels, but callers should be able to discover the input restriction from the API documentation.
| // The only type left to fall back on is the legacy one, which we never | ||
| // open. This is reachable when we do not signal static remote key | ||
| // ourselves, which no released configuration can produce any more. | ||
| return nil, 0, ErrDeprecatedChanType |
There was a problem hiding this comment.
This branch is also reachable when we advertise static remote key but the peer does not. Could we describe the condition as a lack of mutually supported channel types? The current wording focuses on a local configuration that is no longer possible, which makes this look unreachable in production.
| // ErrDeprecatedChanType is returned when the caller of our own RPC asks | ||
| // for the legacy commitment type. We keep operating the legacy channels | ||
| // we already have, but no longer open new ones. |
There was a problem hiding this comment.
selectDefaultChannelType also returns this error when automatic selection would otherwise fall back to legacy, even if the caller requested no particular type. Could we mention that case here and adjust "who asked for the type" below? The distinction between the operator-facing error and the wire error still makes sense.
| // With no static remote key on either side there is | ||
| // nothing left to fall back on but the legacy type. | ||
| name: "default legacy rejected", | ||
| channelFeatures: nil, | ||
| localFeatures: lnwire.NewRawFeatureVector(), | ||
| remoteFeatures: lnwire.NewRawFeatureVector( | ||
| lnwire.StaticRemoteKeyOptional, | ||
| lnwire.AnchorsZeroFeeHtlcTxOptional, | ||
| ), | ||
| expectsCommitType: lnwallet.CommitmentTypeLegacy, | ||
| expectsChanType: (*lnwire.ChannelType)( | ||
| lnwire.NewRawFeatureVector(), | ||
| ), | ||
| expectsErr: nil, | ||
| expectsErr: ErrDeprecatedChanType, |
There was a problem hiding this comment.
Only one side lacks static-remote-key support in this test; the other advertises both static remote key and anchors. Could this say "Without mutual support for static remote key, there is nothing left to fall back on but legacy"? That also describes the reversed peer ordering exercised by the test.
2fb854a to
6c96f1d
Compare
Good question, so I went and checked what was actually there before this PR. There was exactly one deliberate end to end legacy test: the Three other places look like legacy coverage but are not:
The lower level coverage you mention is untouched, and it is where the real So the scenarios you list, reconnection, payments, closure, watchtower I also added the generation history to the PR description, thanks for the |
In this commit, we add ErrChanTypeDeprecated so that a peer whose channel type we reject learns the actual reason. failFundingFlow only forwards the text of whitelisted error types and flattens everything else into a generic "funding failed due to internal error", which tells the remote nothing. The string is kept as terse as the ones around it. What the remote needs to know is that the type isn't acceptable; which type to use instead is our local policy, and spelling it out on the wire would only add another fingerprint for no gain.
In this commit, we turn the dev build only committweak option into a no-op. It stopped the node from signalling option_static_remotekey, which left the legacy commitment type as the only one it could negotiate. Since we no longer open channels of that type, such a node would be unable to negotiate any commitment type at all. The field is kept so that configs which still set it continue to parse.
6c96f1d to
4510e69
Compare
Lrifton92
left a comment
Lrifton92
left a comment
There was a problem hiding this comment.
Re-reviewed at 4510e69 (a rebase of 6c96f1d; range-diff shows only the release-notes contributor list changed), since my earlier approval was on 5828b7f, before the scope was narrowed to legacy only.
- Legacy is now refused unconditionally on both paths: an explicit
LEGACYinparseOpenChannelReq, an emptychannel_typefrom the peer (ErrChanTypeDeprecatedon the wire), and the default selection when there is no mutually supported type above legacy (ErrDeprecatedChanType). Static remote key stays negotiable, and--channel_type tweaklessis back in lncli, which matches that. - No leftovers from the broader proposal:
ErrNoAnchorSupport,NoDeprecatedChanTypesand the "legacy and static remote key" wording are all gone. - Locally:
go vet(incl.-tags="integration dev"on itest/lntest) is clean;./funding,./lnwire,./lncfgpass, includingTestCommitmentTypeNegotiationand theTestFundingManagerReject{Legacy,Missing}ChanTypecases.
LGTM.
|
@Lrifton92 could you please stop spamming the repos with your bot! |
|
Understood, and sorry for the noise. I'll stop posting reviews on this and the other lightningnetwork repos. |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 🎉
In PR description:
funds in such a channel cannot be recovered after data loss,
Should be "cannot be recovered unilaterally"
Also the same fix is needed to commit "funding: never open a legacy channel" description.
In this commit, we stop opening and accepting new channels using the legacy commitment type, which was removed from the spec in 2024. Its tweaked to_remote output is why funds in such a channel cannot be recovered unilaterally after data loss. Channels that already use it are untouched: they keep working and can be operated, force closed and cooperatively closed exactly as before. An empty channel_type asks for precisely this type, and that branch performed no feature check whatsoever, since the type predates feature bits entirely. Any peer could obtain a legacy channel from us no matter what either side signalled, which is what TestFundingManagerRejectLegacyChanType now covers. Implicit negotiation keeps falling back to the static remote key type, and only fails once there is nothing left below it. The funding manager's own test nodes signalled no features at all, which means they were negotiating legacy channels among themselves. They now default to static remote key.
In this commit, we turn down a request for the legacy commitment type before we touch the wallet or the peer, so the caller gets a clear error instead of one from deep inside the funding flow. The LEGACY enum value itself has to stay: CommitmentType is also the reporting type for ListChannels, ClosedChannels, PendingChannels and the channel acceptor, and channels we already have still report it.
In this commit, we move the two tests that still opened legacy channels over to static remote key. Both produce the same commitment shape and the same fees, so no assertion moves. The watchtower case opted into legacy deliberately. The remote signer one did not: neither of its nodes runs with committweak, so both support static remote key, and the channel only came out legacy because an empty channel_type was accepted without any check. That is the bug this PR fixes, sitting in our own suite. The legacy node arguments and the unreferenced CfgLegacy go away with them, since committweak no longer does anything.
In this commit, we spell out that existing legacy channels are not affected and can still be operated and closed, since that is the question an operator reading these notes will have. Only opening new ones is refused.
In this commit, we add the same entries as the 0.20.5 notes, kept in a commit of their own so that each backport carries only the release notes of the branch it lands on.
In this commit, we document the restriction on the API itself, so callers can discover it without reading the release notes. The LEGACY enum value notes that it is only ever reported for channels that already exist, and the commitment_type fields of OpenChannelRequest and BatchOpenChannel note that it is turned down as an input. Worth spelling out on those fields that an empty channel type on the wire asks for the legacy type as well, so there is no way to request it at all. The generated code carries no change beyond the comments.
4510e69 to
9b634bf
Compare
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-11212-to-v0.20.x-branch
git worktree add --checkout .worktree/backport-11212-to-v0.20.x-branch backport-11212-to-v0.20.x-branch
cd .worktree/backport-11212-to-v0.20.x-branch
git reset --hard HEAD^
git cherry-pick -x 1e3b0747d9b640e817b02a2062b73313f8da15e6 98a0d345a2301629c73c473450864aeeee5d646e 6ea22314c3ad5cc246471e93eb1e6f4856451867 a03747961b0b97ca12bae39452383116cf4ad5ec 03d8b09dca48caf5a6361d05c40c46672853d35e 76c18acbd63bf2868be7663f15d4d30f9471ac70 a5556e0525c2a105778d7e515faf161f535dcd93 9b634bfd0929ecc0f0e0ddc0b6aa85a5234a06b6
git push --force-with-lease |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-11212-to-v0.21.x-branch
git worktree add --checkout .worktree/backport-11212-to-v0.21.x-branch backport-11212-to-v0.21.x-branch
cd .worktree/backport-11212-to-v0.21.x-branch
git reset --hard HEAD^
git cherry-pick -x 1e3b0747d9b640e817b02a2062b73313f8da15e6 98a0d345a2301629c73c473450864aeeee5d646e 6ea22314c3ad5cc246471e93eb1e6f4856451867 a03747961b0b97ca12bae39452383116cf4ad5ec 03d8b09dca48caf5a6361d05c40c46672853d35e 76c18acbd63bf2868be7663f15d4d30f9471ac70 a5556e0525c2a105778d7e515faf161f535dcd93 9b634bfd0929ecc0f0e0ddc0b6aa85a5234a06b6
git push --force-with-lease |
ziggie1984
removed
the
backport-v0.20.x-branch
…21.x-branch [v0.21.x-branch] Backport #11212: multi: stop opening legacy channels
4 participants
Change Description
In this PR, we stop opening and accepting new channels that use the legacy
commitment type. Channels that already use it are untouched: they keep working,
and can be operated, force closed and cooperatively closed exactly as before.
Only opening new ones is refused. The code that drives the type stays in place
and gets removed separately in 0.22, which keeps this small enough to backport
to 0.21 and 0.20.
Fixes #11195.
The legacy type was removed from the
spec
in 2024, and lnd is the last implementation still creating them. Its tweaked
to_remoteoutput is why funds in such a channel cannot be recoveredunilaterally after data loss, which is the actual harm reported in the issue.
Where this sits
Channel commitments have gone through four generations, each superseding the
one before it:
LEGACYtweaks theto_remoteoutput with a per-commitment point. Staticremote key (
option_static_remotekey) leaves that output untweaked, which iswhat makes it recoverable from the seed alone. Anchors
(
option_anchors_zero_fee_htlc_tx) adds the anchor outputs that allow feebumping after a force close. Taproot moves the funding output to musig2.
This PR removes the first generation from the set we will open, and nothing
else. The second is dealt with in #11216.
The hole
An empty
channel_typeTLV inopen_channelasks for exactly the legacy type,and the branch handling it performed no feature check whatsoever, since the
type predates feature bits entirely. Any peer could obtain a legacy channel
from us regardless of what either side signalled, and we would do the same to
them. That is what
TestFundingManagerRejectLegacyChanTypenow covers,adapted from the reproduction in #11195: a peer advertising both
option_static_remotekeyandoption_anchors_zero_fee_htlc_txsends an emptychannel type, and we reply with an
errorinstead of echoing it back inaccept_channel.negotiateCommitmentTypeinfunding/commitment_type_negotiation.gois theonly place a commitment type for a new channel gets chosen, for both the
initiator (
handleInitFundingMsg) and the responder(
fundeeProcessOpenChannel), so the change lives there. Implicit negotiationstill falls back to the static remote key type and only fails once there is
nothing left below it.
We also found that the funding manager's own test nodes signalled no features
at all, which means they had been negotiating legacy channels among
themselves. They now default to static remote key.
Scope
Only the legacy type. The static remote key type is still spec-valid, it is
what replaced legacy, and its
to_remoteis untweaked so the recovery problemabove does not apply to it. Refusing it as well would mean refusing channels
from any peer that cannot do anchors, which is not something to do in a patch
release. That belongs with the code removal in 0.22.
The rejection is a
lnwire.FundingError(ErrChanTypeDeprecated) rather thana plain error, because
failFundingFlowonly forwards the text of whitelistederror types and flattens everything else into "funding failed due to internal
error", which would tell the remote nothing. Its string is kept as terse as
the ones beside it: what the remote needs to know is that the type is not
acceptable, and naming our replacement on the wire would only add another
fingerprint.
OpenChannelrejectscommitment_typeLEGACYup front so the caller gets aclear error before we touch the wallet or the peer. The enum value itself has
to stay, since
CommitmentTypeis also the reporting type forListChannels,ClosedChannels,PendingChannelsand the channel acceptor, and channels wealready have still report it.
The restriction is documented on the API itself: the
LEGACYenum value notesthat it is only reported for channels that already exist, and the
commitment_typefields ofOpenChannelRequestandBatchOpenChannelnotethat it is turned down as an input. The regenerated code carries no change
beyond the comments.
The dev build only
protocol.legacy.committweakoption is deprecated and madea no-op. It stopped the node from signalling
option_static_remotekey, whichnow leaves no commitment type left to negotiate at all.
See each commit message for a detailed description w.r.t the incremental
changes.
Steps to Test
Three code paths could produce a legacy channel, and each has a test:
channel_typeTestFundingManagerRejectLegacyChanTypeTestCommitmentTypeNegotiation/default_legacy_rejectedlegacy_chan_type_rejected,TestCommitmentTypeNegotiation/explicit_legacy_rejectedTwo tests used to open legacy channels and now use static remote key instead.
The watchtower one opted in deliberately; the remote signer one did not, and
was itself an instance of the bug above. Both produce the same commitment
shape and the same fees, so no assertion moved. Re-ran those plus everything
else the funding path touches:
All nine pass.
make lintis clean, unit tests pass, and each commit builds onits own under both the default and
devbuild tags.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.