From 31ae48c59c5c36d5c436bfa9999e13a4806bba87 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Fri, 19 Nov 2021 15:58:56 -0800 Subject: [PATCH 1/7] feature: if a bit is unset, then all other features that dep it should be This fixes an issue where if one tries to unset a feature like anchors, and other feature depend on it, then `lnd` fails to start as it realizes that its dependnacy set is inconsistent. Fixes https://github.com/lightningnetwork/lnd/issues/6002 --- feature/manager.go | 15 +++++++++++++++ feature/manager_internal_test.go | 10 ++++++++++ 2 files changed, 25 insertions(+) diff --git a/feature/manager.go b/feature/manager.go index 50a54d625..9d5d1cad4 100644 --- a/feature/manager.go +++ b/feature/manager.go @@ -91,6 +91,21 @@ func newManager(cfg Config, desc setDesc) (*Manager, error) { if cfg.NoAnchors { raw.Unset(lnwire.AnchorsZeroFeeHtlcTxOptional) raw.Unset(lnwire.AnchorsZeroFeeHtlcTxRequired) + + // If anchors are disabled, then we also need to + // disable all other features that depend on it as + // well, as otherwise we may create an invalid feature + // bit set. + for bit, depFeatures := range deps { + for depFeature := range depFeatures { + switch { + case depFeature == lnwire.AnchorsZeroFeeHtlcTxRequired: + fallthrough + case depFeature == lnwire.AnchorsZeroFeeHtlcTxOptional: + raw.Unset(bit) + } + } + } } if cfg.NoWumbo { raw.Unset(lnwire.WumboChannelsOptional) diff --git a/feature/manager_internal_test.go b/feature/manager_internal_test.go index 9c97bd052..2778a86d9 100644 --- a/feature/manager_internal_test.go +++ b/feature/manager_internal_test.go @@ -52,6 +52,12 @@ var managerTests = []managerTest{ NoStaticRemoteKey: true, }, }, + { + name: "anchors should disable anything dependent on it", + cfg: Config{ + NoAnchors: true, + }, + }, } // TestManager asserts basic initialazation and operation of a feature manager, @@ -104,6 +110,10 @@ func testManager(t *testing.T, test managerTest) { if test.cfg.NoStaticRemoteKey { assertUnset(lnwire.StaticRemoteKeyOptional) } + if test.cfg.NoAnchors { + assertUnset(lnwire.ScriptEnforcedLeaseRequired) + assertUnset(lnwire.ScriptEnforcedLeaseOptional) + } assertUnset(unknownFeature) } From 5a5d535dc577690265746f4685d90b9cf82b9c85 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Fri, 19 Nov 2021 16:08:08 -0800 Subject: [PATCH 2/7] docs/release-notes: add section about transitive feature dep fix --- docs/release-notes/release-notes-0.14.1.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.14.1.md b/docs/release-notes/release-notes-0.14.1.md index 11828dd53..844325d64 100644 --- a/docs/release-notes/release-notes-0.14.1.md +++ b/docs/release-notes/release-notes-0.14.1.md @@ -12,8 +12,12 @@ * [A bug has been fixed in channeldb that uses the return value without checking the returned error first](https://github.com/lightningnetwork/lnd/pull/6012). +* [Fixes a bug that would cause lnd to be unable to start if anchors was + disabled](https://github.com/lightningnetwork/lnd/pull/6007). + # Contributors (Alphabetical Order) * Jamie Turley * nayuta-ueno +* Olaoluwa Osuntokun * Oliver Gugger From 5a285827194f85af10c138da735b35a1faee97aa Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 18 Nov 2021 13:32:27 -0800 Subject: [PATCH 3/7] contractcourt: only supplement resolvers if channel has historical state In this commit, we fix a bug that would cause newly updated nodes to be unable to start up, if they have an older channel that was closed before we started to store all the historical state for each channel. The issue is that we started to write the complete state to disk, but newer channels don't have it, so when we try to supplement the resolvers, we run into this error. Ultimately, we only need this new supplemented information for script enforcement channels. Ideally we would instead check the channel type there instead, but it doesn't appear to be available in this context as is, without further changes. Fixes https://github.com/lightningnetwork/lnd/issues/6001. --- contractcourt/channel_arbitrator.go | 42 +++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index e85e74b34..ff29e6966 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -612,8 +612,17 @@ func (c *ChannelArbitrator) relaunchResolvers(commitSet *CommitSet, // We'll also fetch the historical state of this channel, as it should // have been marked as closed by now, and supplement it to each resolver // such that we can properly resolve our pending contracts. - chanState, err := c.cfg.FetchHistoricalChannel() - if err != nil { + var chanState *channeldb.OpenChannel + chanState, err = c.cfg.FetchHistoricalChannel() + switch { + // If we don't find this channel, then it may be the case that it + // was closed before we started to retain the final state + // information for open channels. + case err == channeldb.ErrChannelNotFound: + log.Warnf("ChannelArbitrator(%v): unable to fetch historical "+ + "state", c.cfg.ChanPoint) + + case err != nil: return err } @@ -621,7 +630,9 @@ func (c *ChannelArbitrator) relaunchResolvers(commitSet *CommitSet, "resolvers", c.cfg.ChanPoint, len(unresolvedContracts)) for _, resolver := range unresolvedContracts { - resolver.SupplementState(chanState) + if chanState != nil { + resolver.SupplementState(chanState) + } htlcResolver, ok := resolver.(htlcContractResolver) if !ok { @@ -1930,8 +1941,17 @@ func (c *ChannelArbitrator) prepContractResolutions( // We'll also fetch the historical state of this channel, as it should // have been marked as closed by now, and supplement it to each resolver // such that we can properly resolve our pending contracts. - chanState, err := c.cfg.FetchHistoricalChannel() - if err != nil { + var chanState *channeldb.OpenChannel + chanState, err = c.cfg.FetchHistoricalChannel() + switch { + // If we don't find this channel, then it may be the case that it + // was closed before we started to retain the final state + // information for open channels. + case err == channeldb.ErrChannelNotFound: + log.Warnf("ChannelArbitrator(%v): unable to fetch historical "+ + "state", c.cfg.ChanPoint) + + case err != nil: return nil, nil, err } @@ -2040,7 +2060,9 @@ func (c *ChannelArbitrator) prepContractResolutions( resolver := newTimeoutResolver( resolution, height, htlc, resolverCfg, ) - resolver.SupplementState(chanState) + if chanState != nil { + resolver.SupplementState(chanState) + } htlcResolvers = append(htlcResolvers, resolver) } @@ -2097,7 +2119,9 @@ func (c *ChannelArbitrator) prepContractResolutions( resolver := newOutgoingContestResolver( resolution, height, htlc, resolverCfg, ) - resolver.SupplementState(chanState) + if chanState != nil { + resolver.SupplementState(chanState) + } htlcResolvers = append(htlcResolvers, resolver) } } @@ -2111,7 +2135,9 @@ func (c *ChannelArbitrator) prepContractResolutions( *contractResolutions.CommitResolution, height, c.cfg.ChanPoint, resolverCfg, ) - resolver.SupplementState(chanState) + if chanState != nil { + resolver.SupplementState(chanState) + } htlcResolvers = append(htlcResolvers, resolver) } From 32d0ecdca1a7052283b1a06002e914b0d926f8de Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 18 Nov 2021 13:40:03 -0800 Subject: [PATCH 4/7] docs/release-notes: add entry for cnct bug fix --- docs/release-notes/release-notes-0.14.1.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/release-notes/release-notes-0.14.1.md b/docs/release-notes/release-notes-0.14.1.md index 844325d64..be68bf769 100644 --- a/docs/release-notes/release-notes-0.14.1.md +++ b/docs/release-notes/release-notes-0.14.1.md @@ -15,6 +15,9 @@ * [Fixes a bug that would cause lnd to be unable to start if anchors was disabled](https://github.com/lightningnetwork/lnd/pull/6007). +* [Fixed a bug that would cause nodes with older channels to be unable to start + up](https://github.com/lightningnetwork/lnd/pull/6003). + # Contributors (Alphabetical Order) * Jamie Turley From 6ee79136622d3ac89d8135bffadc1f111728e241 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 24 Nov 2021 17:44:28 +0100 Subject: [PATCH 5/7] funding: don't negotiate on known types --- funding/commitment_type_negotiation.go | 17 ++++++----------- funding/commitment_type_negotiation_test.go | 13 ++++++++++--- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/funding/commitment_type_negotiation.go b/funding/commitment_type_negotiation.go index 091c6fcbb..392b7c824 100644 --- a/funding/commitment_type_negotiation.go +++ b/funding/commitment_type_negotiation.go @@ -8,12 +8,6 @@ import ( ) var ( - // errUnsupportedExplicitNegotiation is an error returned when explicit - // channel commitment negotiation is attempted but either peer of the - // channel does not support it. - errUnsupportedExplicitNegotiation = errors.New("explicit channel " + - "type negotiation not supported") - // errUnsupportedCommitmentType is an error returned when a specific // channel commitment type is being explicitly negotiated but either // peer of the channel does not support it. @@ -29,12 +23,13 @@ func negotiateCommitmentType(channelType *lnwire.ChannelType, local, remote *lnwire.FeatureVector) (lnwallet.CommitmentType, error) { if channelType != nil { - if !hasFeatures(local, remote, lnwire.ExplicitChannelTypeOptional) { - return 0, errUnsupportedExplicitNegotiation + // If the peer does know explicit negotiation, let's attempt + // that now. + if hasFeatures(local, remote, lnwire.ExplicitChannelTypeOptional) { + return explicitNegotiateCommitmentType( + *channelType, local, remote, + ) } - return explicitNegotiateCommitmentType( - *channelType, local, remote, - ) } return implicitNegotiateCommitmentType(local, remote), nil diff --git a/funding/commitment_type_negotiation_test.go b/funding/commitment_type_negotiation_test.go index c9ac25a7a..c3032685c 100644 --- a/funding/commitment_type_negotiation_test.go +++ b/funding/commitment_type_negotiation_test.go @@ -36,7 +36,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.StaticRemoteKeyOptional, lnwire.AnchorsZeroFeeHtlcTxOptional, ), - expectsErr: errUnsupportedExplicitNegotiation, + expectsRes: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx, + expectsErr: nil, }, { name: "explicit missing remote commitment feature", @@ -181,8 +182,14 @@ func TestCommitmentTypeNegotiation(t *testing.T) { return } - require.Equal(t, testCase.expectsRes, localType) - require.Equal(t, testCase.expectsRes, remoteType) + require.Equal( + t, testCase.expectsRes, localType, + testCase.name, + ) + require.Equal( + t, testCase.expectsRes, remoteType, + testCase.name, + ) }) if !ok { return From 328e3361ffce9646e49740258452e100d6696000 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 24 Nov 2021 17:44:29 +0100 Subject: [PATCH 6/7] funding: don't fail flow on channel type --- funding/manager.go | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/funding/manager.go b/funding/manager.go index b4fad5570..9a50685c8 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1587,9 +1587,32 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, } } } else if msg.ChannelType != nil { - err := errors.New("received unexpected channel type") - f.failFundingFlow(peer, msg.PendingChannelID, err) - return + // The spec isn't too clear about whether it's okay to set the + // channel type in the accept_channel response if we didn't + // explicitly set it in the open_channel message. For now, let's + // just log the problem instead of failing the funding flow. + implicitChannelType := implicitNegotiateCommitmentType( + peer.LocalFeatures(), peer.RemoteFeatures(), + ) + negotiatedChannelType, err := negotiateCommitmentType( + msg.ChannelType, peer.LocalFeatures(), + peer.RemoteFeatures(), + ) + if err != nil { + err := errors.New("received unexpected channel type") + f.failFundingFlow(peer, msg.PendingChannelID, err) + return + } + + // Even though we don't expect a channel type to be set when we + // didn't send one in the first place, we check that it's the + // same type we'd have arrived through implicit negotiation. If + // it's another type, we fail the flow. + if implicitChannelType != negotiatedChannelType { + err := errors.New("negotiated unexpected channel type") + f.failFundingFlow(peer, msg.PendingChannelID, err) + return + } } // The required number of confirmations should not be greater than the From ee015a1d6e224d638c28aedfd187095af6bf4792 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 24 Nov 2021 17:44:31 +0100 Subject: [PATCH 7/7] docs: add release notes for channel negotiation --- docs/release-notes/release-notes-0.14.1.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.14.1.md b/docs/release-notes/release-notes-0.14.1.md index be68bf769..655629e26 100644 --- a/docs/release-notes/release-notes-0.14.1.md +++ b/docs/release-notes/release-notes-0.14.1.md @@ -18,6 +18,10 @@ * [Fixed a bug that would cause nodes with older channels to be unable to start up](https://github.com/lightningnetwork/lnd/pull/6003). +* [Addresses an issue with explicit channel type negotiation that caused + incompatibilities when opening channels with the latest versions of + c-lightning and eclair](https://github.com/lightningnetwork/lnd/pull/6026). + # Contributors (Alphabetical Order) * Jamie Turley