From f630cfd4d7e89b9018d04f82696a1b2a8c4bc250 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 24 Nov 2021 12:15:08 -0600 Subject: [PATCH 1/2] funding: ensure a local funding w/ explicit type can't be downgraded In this commit, we fix an API inconsistency introduced by a recent fix. Without this commit, it would be possible for a local user to attempt an explicit funding type, but then _fallback_ to implicit negotiation if the remote party didn't have the bit set. We resolve this by adding a new bit of information to ensure that if we're the funder and want explicit chan negotiation, we don't settle for anything less. A new test case has been added to exercise this behavior. To recap for posterity, the following issues were found and fixed in the negotiation logic: 1. If the remote party sent a channel type (in accept channel), but we didn't, then we would error out. We no longer error out and instead ensure the channel type they sent matches what we derived via implicit funding. 2. If the remote party _did not_ have the bit set, but they sent a chan type (in open channel), we would error out. We no longer error out, but instead will fall back to implicit negotiation. Ultimately we want to eventually flip the explicit funding bit to _required_ to eliminate any other future ambiguities and also ensure that it isn't possible to inadvertently fall back to implicit funding, when the _user_ expects explicit funding. --- funding/commitment_type_negotiation.go | 23 +++++++++++++++--- funding/commitment_type_negotiation_test.go | 26 +++++++++++++++++++-- funding/manager.go | 24 ++++++++++++++----- 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/funding/commitment_type_negotiation.go b/funding/commitment_type_negotiation.go index 392b7c824..b559d6b66 100644 --- a/funding/commitment_type_negotiation.go +++ b/funding/commitment_type_negotiation.go @@ -8,6 +8,12 @@ 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. @@ -20,19 +26,30 @@ var ( // will be attempted if the set of both local and remote features support it. // Otherwise, implicit negotiation will be attempted. func negotiateCommitmentType(channelType *lnwire.ChannelType, - local, remote *lnwire.FeatureVector) (lnwallet.CommitmentType, error) { + local, remote *lnwire.FeatureVector, + mustBeExplicit bool) (bool, lnwallet.CommitmentType, error) { if channelType != nil { // If the peer does know explicit negotiation, let's attempt // that now. if hasFeatures(local, remote, lnwire.ExplicitChannelTypeOptional) { - return explicitNegotiateCommitmentType( + chanType, err := explicitNegotiateCommitmentType( *channelType, local, remote, ) + return true, chanType, err + } + + // If we're the funder, and we are attempting to use an + // explicit channel type, but the remote party doesn't signal + // the bit, then we actually want to exit here, to ensure the + // user doesn't end up with an unexpected channel type via + // implicit negotiation. + if mustBeExplicit { + return false, 0, errUnsupportedExplicitNegotiation } } - return implicitNegotiateCommitmentType(local, remote), nil + return false, implicitNegotiateCommitmentType(local, remote), nil } // explicitNegotiateCommitmentType attempts to explicitly negotiate for a diff --git a/funding/commitment_type_negotiation_test.go b/funding/commitment_type_negotiation_test.go index c3032685c..6973026ee 100644 --- a/funding/commitment_type_negotiation_test.go +++ b/funding/commitment_type_negotiation_test.go @@ -15,6 +15,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) { testCases := []struct { name string + mustBeExplicit bool channelFeatures *lnwire.RawFeatureVector localFeatures *lnwire.RawFeatureVector remoteFeatures *lnwire.RawFeatureVector @@ -39,6 +40,25 @@ func TestCommitmentTypeNegotiation(t *testing.T) { expectsRes: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx, expectsErr: nil, }, + { + name: "local funder wants explicit, remote doesn't " + + "support so fall back", + mustBeExplicit: true, + channelFeatures: lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyRequired, + lnwire.AnchorsZeroFeeHtlcTxRequired, + ), + localFeatures: lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyOptional, + lnwire.AnchorsZeroFeeHtlcTxOptional, + lnwire.ExplicitChannelTypeOptional, + ), + remoteFeatures: lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyOptional, + lnwire.AnchorsZeroFeeHtlcTxOptional, + ), + expectsErr: errUnsupportedExplicitNegotiation, + }, { name: "explicit missing remote commitment feature", channelFeatures: lnwire.NewRawFeatureVector( @@ -168,13 +188,15 @@ func TestCommitmentTypeNegotiation(t *testing.T) { *testCase.channelFeatures, ) } - localType, err := negotiateCommitmentType( + _, localType, err := negotiateCommitmentType( channelType, localFeatures, remoteFeatures, + testCase.mustBeExplicit, ) require.Equal(t, testCase.expectsErr, err) - remoteType, err := negotiateCommitmentType( + _, remoteType, err := negotiateCommitmentType( channelType, remoteFeatures, localFeatures, + testCase.mustBeExplicit, ) require.Equal(t, testCase.expectsErr, err) diff --git a/funding/manager.go b/funding/manager.go index 9a50685c8..405291a05 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1274,8 +1274,9 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer, // the remote peer are signaling the proper feature bit if we're using // implicit negotiation, and simply the channel type sent over if we're // using explicit negotiation. - commitType, err := negotiateCommitmentType( + wasExplicit, commitType, err := negotiateCommitmentType( msg.ChannelType, peer.LocalFeatures(), peer.RemoteFeatures(), + false, ) if err != nil { // TODO(roasbeef): should be using soft errors @@ -1284,6 +1285,13 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer, return } + // Only echo back a channel type in AcceptChannel if we actually used + // explicit negotiation above. + var chanTypeFeatureBits *lnwire.ChannelType + if wasExplicit { + chanTypeFeatureBits = msg.ChannelType + } + chainHash := chainhash.Hash(msg.ChainHash) req := &lnwallet.InitFundingReserveMsg{ ChainHash: &chainHash, @@ -1520,7 +1528,7 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer, HtlcPoint: ourContribution.HtlcBasePoint.PubKey, FirstCommitmentPoint: ourContribution.FirstCommitmentPoint, UpfrontShutdownScript: ourContribution.UpfrontShutdown, - ChannelType: msg.ChannelType, + ChannelType: chanTypeFeatureBits, LeaseExpiry: msg.LeaseExpiry, } @@ -1594,9 +1602,13 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, implicitChannelType := implicitNegotiateCommitmentType( peer.LocalFeatures(), peer.RemoteFeatures(), ) - negotiatedChannelType, err := negotiateCommitmentType( + + // We pass in false here as the funder since at this point, we + // didn't set a chan type ourselves, so falling back to + // implicit funding is acceptable. + _, negotiatedChannelType, err := negotiateCommitmentType( msg.ChannelType, peer.LocalFeatures(), - peer.RemoteFeatures(), + peer.RemoteFeatures(), false, ) if err != nil { err := errors.New("received unexpected channel type") @@ -3246,9 +3258,9 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) { // Before we init the channel, we'll also check to see what commitment // format we can use with this peer. This is dependent on *both* us and // the remote peer are signaling the proper feature bit. - commitType, err := negotiateCommitmentType( + _, commitType, err := negotiateCommitmentType( msg.ChannelType, msg.Peer.LocalFeatures(), - msg.Peer.RemoteFeatures(), + msg.Peer.RemoteFeatures(), true, ) if err != nil { log.Errorf("channel type negotiation failed: %v", err) From ed854cf492430ccfc3a92adedb05e88809ab4608 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 24 Nov 2021 13:31:23 -0600 Subject: [PATCH 2/2] docs/release-notes: add entry for funding API downgrade protection --- 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 655629e26..6d6ec56f6 100644 --- a/docs/release-notes/release-notes-0.14.1.md +++ b/docs/release-notes/release-notes-0.14.1.md @@ -22,6 +22,10 @@ incompatibilities when opening channels with the latest versions of c-lightning and eclair](https://github.com/lightningnetwork/lnd/pull/6026). +* [Ensure that if a user specifies explicit channel funding on the API level, + then it can't be + downgraded](https://github.com/lightningnetwork/lnd/pull/6027). + # Contributors (Alphabetical Order) * Jamie Turley