From ec85b5be72adc52b62b17f99a2f04d4ffac2f9f8 Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Mon, 16 Jan 2023 13:39:14 -0600 Subject: [PATCH 1/7] funding: rename channelType parameter This prevents us from confusing the channelType parameter with the chanType local. --- funding/commitment_type_negotiation.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/funding/commitment_type_negotiation.go b/funding/commitment_type_negotiation.go index 475f59a6f..49f2cb336 100644 --- a/funding/commitment_type_negotiation.go +++ b/funding/commitment_type_negotiation.go @@ -22,16 +22,16 @@ var ( ) // negotiateCommitmentType negotiates the commitment type of a newly opened -// channel. If a channelType is provided, explicit negotiation for said type +// channel. If a desiredChanType is provided, explicit negotiation for said type // will be attempted if the set of both local and remote features support it. // Otherwise, implicit negotiation will be attempted. Two booleans are // returned letting the caller know if the option-scid-alias or zero-conf // channel types were negotiated. -func negotiateCommitmentType(channelType *lnwire.ChannelType, local, +func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local, remote *lnwire.FeatureVector, mustBeExplicit bool) (bool, *lnwire.ChannelType, lnwallet.CommitmentType, error) { - if channelType != nil { + if desiredChanType != nil { // If the peer does know explicit negotiation, let's attempt // that now. if hasFeatures( @@ -39,9 +39,9 @@ func negotiateCommitmentType(channelType *lnwire.ChannelType, local, ) { chanType, err := explicitNegotiateCommitmentType( - *channelType, local, remote, + *desiredChanType, local, remote, ) - return true, channelType, chanType, err + return true, desiredChanType, chanType, err } // If we're the funder, and we are attempting to use an From e59cd7f91dee7312fce6e26d21604ec50785b329 Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Mon, 16 Jan 2023 13:50:26 -0600 Subject: [PATCH 2/7] funding: stop naming commit types `chanType` negotiateCommitmentType returns both a chanType and a commitType, so it is really confusing when the commitType is called chanType instead. --- funding/commitment_type_negotiation.go | 4 ++-- funding/manager.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/funding/commitment_type_negotiation.go b/funding/commitment_type_negotiation.go index 49f2cb336..b77effc01 100644 --- a/funding/commitment_type_negotiation.go +++ b/funding/commitment_type_negotiation.go @@ -38,10 +38,10 @@ func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local, local, remote, lnwire.ExplicitChannelTypeOptional, ) { - chanType, err := explicitNegotiateCommitmentType( + commitType, err := explicitNegotiateCommitmentType( *desiredChanType, local, remote, ) - return true, desiredChanType, chanType, err + return true, desiredChanType, commitType, err } // If we're the funder, and we are attempting to use an diff --git a/funding/manager.go b/funding/manager.go index 043ea4eb9..76f66eedc 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1883,14 +1883,14 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, // 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( + _, implicitCommitType := implicitNegotiateCommitmentType( peer.LocalFeatures(), peer.RemoteFeatures(), ) // 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( + _, _, negotiatedCommitType, err := negotiateCommitmentType( msg.ChannelType, peer.LocalFeatures(), peer.RemoteFeatures(), false, ) @@ -1904,7 +1904,7 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, // 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 { + if implicitCommitType != negotiatedCommitType { err := errors.New("negotiated unexpected channel type") f.failFundingFlow(peer, msg.PendingChannelID, err) return From 34186dee191af054726b971d2ed0da2b0fdb0673 Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Mon, 16 Jan 2023 13:58:47 -0600 Subject: [PATCH 3/7] funding: remove incorrect and repetitive comments We don't actually do the logging described in the first comment, and the second comment block repeats much of the first comment block. --- funding/manager.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/funding/manager.go b/funding/manager.go index 76f66eedc..fd1de2426 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1881,8 +1881,9 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, } else if msg.ChannelType != nil { // 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. + // explicitly set it in the open_channel message. For now, we + // check that it's the same type we'd have arrived through + // implicit negotiation. If it's another type, we fail the flow. _, implicitCommitType := implicitNegotiateCommitmentType( peer.LocalFeatures(), peer.RemoteFeatures(), ) @@ -1900,10 +1901,6 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, 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 implicitCommitType != negotiatedCommitType { err := errors.New("negotiated unexpected channel type") f.failFundingFlow(peer, msg.PendingChannelID, err) From 0ae9c63d6472ad60485674b8bb97410283330daa Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Mon, 16 Jan 2023 14:52:23 -0600 Subject: [PATCH 4/7] funding: make expectsChanType a pointer This allows us to expect chanType to be nil, which will be possible after the next commit. --- funding/commitment_type_negotiation_test.go | 80 ++++++++++++--------- 1 file changed, 47 insertions(+), 33 deletions(-) diff --git a/funding/commitment_type_negotiation_test.go b/funding/commitment_type_negotiation_test.go index c640eb625..a874bfcf4 100644 --- a/funding/commitment_type_negotiation_test.go +++ b/funding/commitment_type_negotiation_test.go @@ -20,7 +20,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) { localFeatures *lnwire.RawFeatureVector remoteFeatures *lnwire.RawFeatureVector expectsCommitType lnwallet.CommitmentType - expectsChanType lnwire.ChannelType + expectsChanType *lnwire.ChannelType zeroConf bool scidAlias bool expectsErr error @@ -41,10 +41,12 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.AnchorsZeroFeeHtlcTxOptional, ), expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx, - expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector( - lnwire.StaticRemoteKeyRequired, - lnwire.AnchorsZeroFeeHtlcTxRequired, - )), + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyRequired, + lnwire.AnchorsZeroFeeHtlcTxRequired, + ), + ), expectsErr: nil, }, { @@ -106,8 +108,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.ExplicitChannelTypeOptional, ), expectsCommitType: lnwallet.CommitmentTypeScriptEnforcedLease, - expectsChanType: lnwire.ChannelType( - *lnwire.NewRawFeatureVector( + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( lnwire.ZeroConfRequired, lnwire.StaticRemoteKeyRequired, lnwire.AnchorsZeroFeeHtlcTxRequired, @@ -137,8 +139,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.ExplicitChannelTypeOptional, ), expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx, - expectsChanType: lnwire.ChannelType( - *lnwire.NewRawFeatureVector( + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( lnwire.ZeroConfRequired, lnwire.StaticRemoteKeyRequired, lnwire.AnchorsZeroFeeHtlcTxRequired, @@ -170,8 +172,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.ExplicitChannelTypeOptional, ), expectsCommitType: lnwallet.CommitmentTypeScriptEnforcedLease, - expectsChanType: lnwire.ChannelType( - *lnwire.NewRawFeatureVector( + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( lnwire.ScidAliasRequired, lnwire.StaticRemoteKeyRequired, lnwire.AnchorsZeroFeeHtlcTxRequired, @@ -201,8 +203,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.ExplicitChannelTypeOptional, ), expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx, - expectsChanType: lnwire.ChannelType( - *lnwire.NewRawFeatureVector( + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( lnwire.ScidAliasRequired, lnwire.StaticRemoteKeyRequired, lnwire.AnchorsZeroFeeHtlcTxRequired, @@ -228,10 +230,12 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.ExplicitChannelTypeOptional, ), expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx, - expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector( - lnwire.StaticRemoteKeyRequired, - lnwire.AnchorsZeroFeeHtlcTxRequired, - )), + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyRequired, + lnwire.AnchorsZeroFeeHtlcTxRequired, + ), + ), expectsErr: nil, }, { @@ -250,9 +254,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.ExplicitChannelTypeOptional, ), expectsCommitType: lnwallet.CommitmentTypeTweakless, - expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector( - lnwire.StaticRemoteKeyRequired, - )), + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyRequired, + ), + ), expectsErr: nil, }, { @@ -269,8 +275,10 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.ExplicitChannelTypeOptional, ), expectsCommitType: lnwallet.CommitmentTypeLegacy, - expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector()), - expectsErr: nil, + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector(), + ), + expectsErr: nil, }, // Both sides signal the explicit chan type bit, so we expect // that we return the corresponding chan type feature bits, @@ -289,10 +297,12 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.ExplicitChannelTypeOptional, ), expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx, - expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector( - lnwire.StaticRemoteKeyRequired, - lnwire.AnchorsZeroFeeHtlcTxRequired, - )), + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyRequired, + lnwire.AnchorsZeroFeeHtlcTxRequired, + ), + ), expectsErr: nil, }, { @@ -306,9 +316,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.StaticRemoteKeyOptional, ), expectsCommitType: lnwallet.CommitmentTypeTweakless, - expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector( - lnwire.StaticRemoteKeyRequired, - )), + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyRequired, + ), + ), expectsErr: nil, }, { @@ -320,8 +332,10 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.AnchorsZeroFeeHtlcTxOptional, ), expectsCommitType: lnwallet.CommitmentTypeLegacy, - expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector()), - expectsErr: nil, + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector(), + ), + expectsErr: nil, }, } @@ -402,11 +416,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) { ) require.Equal( - t, testCase.expectsChanType, *lChan, + t, testCase.expectsChanType, lChan, testCase.name, ) require.Equal( - t, testCase.expectsChanType, *rChan, + t, testCase.expectsChanType, rChan, testCase.name, ) }) From a8a50f32f5a688c4d49088b798fdae529fb6e83f Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Mon, 16 Jan 2023 14:18:54 -0600 Subject: [PATCH 5/7] funding: fix channel type negotiation bug The bug manifests when a nil ChannelType is passed to the funding manager in InitFundingMsg. A default value for ChannelType is selected and sent in the OpenChannel message. However, a nil ChannelType is stored in the reservation context. This causes our ChannelType checks in handleFundingAccept to be bypassed. Usually this makes us end up in the "peer unexpectedly sent explicit ChannelType" case, where we can still recover by reselecting a default ChannelType and verifying it matches the one the peer sent. But if the peer sends a nil ChannelType, we miss it. While fixing the bug, I also tried to simplify the negotiation logic, as the complexity is likely what hid the bug in the first place. Now negotiateCommitmentType only returns the ChannelType to be used in OpenChannel/AcceptChannel and the CommitmentType to pass to the wallet. It will even return a nil ChannelType when we're supposed to use implicit negotiation, so we don't need to manually set it to nil for OpenChannel and AcceptChannel. --- funding/commitment_type_negotiation.go | 59 ++++++++++--------- funding/commitment_type_negotiation_test.go | 53 ++++------------- funding/manager.go | 64 ++++++++++----------- funding/manager_test.go | 59 +++++++++++++++++++ 4 files changed, 130 insertions(+), 105 deletions(-) diff --git a/funding/commitment_type_negotiation.go b/funding/commitment_type_negotiation.go index b77effc01..d48061868 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. @@ -24,38 +18,47 @@ var ( // negotiateCommitmentType negotiates the commitment type of a newly opened // channel. If a desiredChanType is provided, explicit negotiation for said type // will be attempted if the set of both local and remote features support it. -// Otherwise, implicit negotiation will be attempted. Two booleans are -// returned letting the caller know if the option-scid-alias or zero-conf -// channel types were negotiated. +// Otherwise, implicit negotiation will be attempted. +// +// The returned ChannelType is nil when implicit negotiation is used. An error +// is only returned if desiredChanType is not supported. func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local, - remote *lnwire.FeatureVector, mustBeExplicit bool) (bool, - *lnwire.ChannelType, lnwallet.CommitmentType, error) { - - if desiredChanType != nil { - // If the peer does know explicit negotiation, let's attempt - // that now. - if hasFeatures( - local, remote, lnwire.ExplicitChannelTypeOptional, - ) { + remote *lnwire.FeatureVector) (*lnwire.ChannelType, + lnwallet.CommitmentType, error) { + // BOLT#2 specifies we MUST use explicit negotiation if both peers + // signal for it. + if hasFeatures(local, remote, lnwire.ExplicitChannelTypeOptional) { + if desiredChanType != nil { commitType, err := explicitNegotiateCommitmentType( *desiredChanType, local, remote, ) - return true, desiredChanType, commitType, err + + return desiredChanType, commitType, 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, nil, 0, errUnsupportedExplicitNegotiation + // Explicitly signal the "implicit" negotiation commitment type + // as default when a desired channel type isn't specified. + chanType, commitType := implicitNegotiateCommitmentType(local, + remote) + + return chanType, commitType, nil + } + + // Otherwise, we'll use implicit negotiation. In this case, we are + // restricted to the newest channel type advertised. If the passed-in + // channelType doesn't match what was advertised, we fail. + chanType, commitType := implicitNegotiateCommitmentType(local, remote) + + if desiredChanType != nil { + expected := lnwire.RawFeatureVector(*desiredChanType) + actual := lnwire.RawFeatureVector(*chanType) + if !expected.Equals(&actual) { + return nil, 0, errUnsupportedChannelType } } - chanType, commitType := implicitNegotiateCommitmentType(local, remote) - return false, chanType, commitType, nil + return nil, commitType, 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 a874bfcf4..90341c6df 100644 --- a/funding/commitment_type_negotiation_test.go +++ b/funding/commitment_type_negotiation_test.go @@ -15,7 +15,6 @@ func TestCommitmentTypeNegotiation(t *testing.T) { testCases := []struct { name string - mustBeExplicit bool channelFeatures *lnwire.RawFeatureVector localFeatures *lnwire.RawFeatureVector remoteFeatures *lnwire.RawFeatureVector @@ -41,32 +40,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.AnchorsZeroFeeHtlcTxOptional, ), expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx, - expectsChanType: (*lnwire.ChannelType)( - lnwire.NewRawFeatureVector( - lnwire.StaticRemoteKeyRequired, - lnwire.AnchorsZeroFeeHtlcTxRequired, - ), - ), - 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, + expectsChanType: nil, + expectsErr: nil, }, { name: "explicit missing remote commitment feature", @@ -282,9 +257,9 @@ func TestCommitmentTypeNegotiation(t *testing.T) { }, // Both sides signal the explicit chan type bit, so we expect // that we return the corresponding chan type feature bits, - // even though we didn't set an explicit channel type. + // even though we didn't set a desired channel type. { - name: "implicit anchors", + name: "default explicit anchors", channelFeatures: nil, localFeatures: lnwire.NewRawFeatureVector( lnwire.StaticRemoteKeyOptional, @@ -316,12 +291,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.StaticRemoteKeyOptional, ), expectsCommitType: lnwallet.CommitmentTypeTweakless, - expectsChanType: (*lnwire.ChannelType)( - lnwire.NewRawFeatureVector( - lnwire.StaticRemoteKeyRequired, - ), - ), - expectsErr: nil, + expectsChanType: nil, + expectsErr: nil, }, { name: "implicit legacy", @@ -332,10 +303,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.AnchorsZeroFeeHtlcTxOptional, ), expectsCommitType: lnwallet.CommitmentTypeLegacy, - expectsChanType: (*lnwire.ChannelType)( - lnwire.NewRawFeatureVector(), - ), - expectsErr: nil, + expectsChanType: nil, + expectsErr: nil, }, } @@ -357,9 +326,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { ) } - _, lChan, lCommit, err := negotiateCommitmentType( + lChan, lCommit, err := negotiateCommitmentType( channelType, localFeatures, remoteFeatures, - testCase.mustBeExplicit, ) var ( @@ -383,9 +351,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { require.Equal(t, testCase.scidAlias, localScid) require.Equal(t, testCase.expectsErr, err) - _, rChan, rCommit, err := negotiateCommitmentType( + rChan, rCommit, err := negotiateCommitmentType( channelType, remoteFeatures, localFeatures, - testCase.mustBeExplicit, ) if rChan != nil { diff --git a/funding/manager.go b/funding/manager.go index fd1de2426..2cc9e8de7 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1452,9 +1452,8 @@ 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. - wasExplicit, _, commitType, err := negotiateCommitmentType( + chanType, commitType, err := negotiateCommitmentType( msg.ChannelType, peer.LocalFeatures(), peer.RemoteFeatures(), - false, ) if err != nil { // TODO(roasbeef): should be using soft errors @@ -1473,19 +1472,16 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer, } var ( - chanTypeFeatureBits *lnwire.ChannelType - zeroConf bool - scid bool + zeroConf bool + scid bool ) - if wasExplicit { - // Only echo back a channel type in AcceptChannel if we actually - // used explicit negotiation above. - chanTypeFeatureBits = msg.ChannelType - + // Only echo back a channel type in AcceptChannel if we actually used + // explicit negotiation above. + if chanType != nil { // Check if the channel type includes the zero-conf or // scid-alias bits. - featureVec := lnwire.RawFeatureVector(*chanTypeFeatureBits) + featureVec := lnwire.RawFeatureVector(*chanType) zeroConf = featureVec.IsSet(lnwire.ZeroConfRequired) scid = featureVec.IsSet(lnwire.ScidAliasRequired) @@ -1727,7 +1723,7 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer, remoteMaxHtlcs: maxHtlcs, remoteChanReserve: chanReserve, maxLocalCsv: f.cfg.MaxLocalCSVDelay, - channelType: msg.ChannelType, + channelType: chanType, err: make(chan error, 1), peer: peer, } @@ -1800,7 +1796,7 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer, HtlcPoint: ourContribution.HtlcBasePoint.PubKey, FirstCommitmentPoint: ourContribution.FirstCommitmentPoint, UpfrontShutdownScript: ourContribution.UpfrontShutdown, - ChannelType: chanTypeFeatureBits, + ChannelType: chanType, LeaseExpiry: msg.LeaseExpiry, } @@ -1888,12 +1884,9 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, peer.LocalFeatures(), peer.RemoteFeatures(), ) - // 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. - _, _, negotiatedCommitType, err := negotiateCommitmentType( + _, negotiatedCommitType, err := negotiateCommitmentType( msg.ChannelType, peer.LocalFeatures(), - peer.RemoteFeatures(), false, + peer.RemoteFeatures(), ) if err != nil { err := errors.New("received unexpected channel type") @@ -4055,9 +4048,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. - _, chanType, commitType, err := negotiateCommitmentType( + chanType, commitType, err := negotiateCommitmentType( msg.ChannelType, msg.Peer.LocalFeatures(), - msg.Peer.RemoteFeatures(), true, + msg.Peer.RemoteFeatures(), ) if err != nil { log.Errorf("channel type negotiation failed: %v", err) @@ -4070,20 +4063,23 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) { scid bool ) - // Check if the returned chanType includes either the zero-conf or - // scid-alias bits. - featureVec := lnwire.RawFeatureVector(*chanType) - zeroConf = featureVec.IsSet(lnwire.ZeroConfRequired) - scid = featureVec.IsSet(lnwire.ScidAliasRequired) + if chanType != nil { + // Check if the returned chanType includes either the zero-conf + // or scid-alias bits. + featureVec := lnwire.RawFeatureVector(*chanType) + zeroConf = featureVec.IsSet(lnwire.ZeroConfRequired) + scid = featureVec.IsSet(lnwire.ScidAliasRequired) - // The option-scid-alias channel type for a public channel is - // disallowed. - if scid && !msg.Private { - err = fmt.Errorf("option-scid-alias chantype for public " + - "channel") - log.Error(err) - msg.Err <- err - return + // The option-scid-alias channel type for a public channel is + // disallowed. + if scid && !msg.Private { + err = fmt.Errorf("option-scid-alias chantype for " + + "public channel") + log.Error(err) + msg.Err <- err + + return + } } // First, we'll query the fee estimator for a fee that should get the @@ -4239,7 +4235,7 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) { remoteMaxHtlcs: maxHtlcs, remoteChanReserve: chanReserve, maxLocalCsv: maxCSV, - channelType: msg.ChannelType, + channelType: chanType, reservation: reservation, peer: msg.Peer, updates: msg.Updates, diff --git a/funding/manager_test.go b/funding/manager_test.go index 58e0c05fe..532fef770 100644 --- a/funding/manager_test.go +++ b/funding/manager_test.go @@ -4493,3 +4493,62 @@ func TestCommitmentTypeFundmaxSanityCheck(t *testing.T) { } } } + +// TestFundingManagerNoEchoChanType tests that the funding flow is aborted if +// the peer fails to echo back the channel type in AcceptChannel. +func TestFundingManagerNoEchoChanType(t *testing.T) { + t.Parallel() + + alice, bob := setupFundingManagers(t) + t.Cleanup(func() { + tearDownFundingManagers(t, alice, bob) + }) + + // Alice and Bob will have the same set of feature bits in our test. + featureBits := []lnwire.FeatureBit{ + lnwire.ExplicitChannelTypeOptional, + lnwire.StaticRemoteKeyOptional, + lnwire.AnchorsZeroFeeHtlcTxOptional, + } + alice.localFeatures = featureBits + alice.remoteFeatures = featureBits + bob.localFeatures = featureBits + bob.remoteFeatures = featureBits + + expectedChanType := (*lnwire.ChannelType)(lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyRequired, + lnwire.AnchorsZeroFeeHtlcTxRequired, + )) + + // Create a funding request and start the workflow. + updateChan := make(chan *lnrpc.OpenStatusUpdate) + errChan := make(chan error, 1) + initReq := &InitFundingMsg{ + Peer: bob, + TargetPubkey: bob.privKey.PubKey(), + ChainHash: *fundingNetParams.GenesisHash, + LocalFundingAmt: 500000, + Updates: updateChan, + Err: errChan, + } + + alice.fundingMgr.InitFundingWorkflow(initReq) + + // Alice should have sent the OpenChannel message to Bob. + openChanMsg := expectOpenChannelMsg(t, alice.msgChan) + + require.Equal(t, expectedChanType, openChanMsg.ChannelType) + + // Let Bob handle the OpenChannel message. + bob.fundingMgr.ProcessFundingMsg(openChanMsg, alice) + + acceptChanMsg, _ := assertFundingMsgSent(t, bob.msgChan, + "AcceptChannel").(*lnwire.AcceptChannel) + + require.Equal(t, expectedChanType, acceptChanMsg.ChannelType) + + // Drop the channel type and ensure Alice responds with an error. + acceptChanMsg.ChannelType = nil + alice.fundingMgr.ProcessFundingMsg(acceptChanMsg, bob) + assertFundingMsgSent(t, alice.msgChan, "Error") +} From c50d7048d64917c7c9ff02df32fa99d9109784cc Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Wed, 17 May 2023 11:31:21 -0500 Subject: [PATCH 6/7] docs: release note for #7177 --- docs/release-notes/release-notes-0.17.0.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/release-notes/release-notes-0.17.0.md b/docs/release-notes/release-notes-0.17.0.md index 00132fdc0..28164aeb9 100644 --- a/docs/release-notes/release-notes-0.17.0.md +++ b/docs/release-notes/release-notes-0.17.0.md @@ -53,6 +53,9 @@ unlock or create. all macaroon DB root keys on `ChangePassword`/`GenerateNewRootKey` respectively. +* [Fixed a validation bug](https://github.com/lightningnetwork/lnd/pull/7177) in + `channel_type` negotiation. + ## Code Health * Updated [our fork for serializing protobuf as JSON to be based on the From e6a2167f38c73e3f74c12de31c57f123fca824af Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Thu, 10 Aug 2023 10:37:01 -0500 Subject: [PATCH 7/7] funding: refactor negotiateCommitmentType - use a switch instead of nested ifs to improve readability - improve some variable names - reword comments --- funding/commitment_type_negotiation.go | 56 ++++++++++++++++---------- 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/funding/commitment_type_negotiation.go b/funding/commitment_type_negotiation.go index d48061868..461b6c51d 100644 --- a/funding/commitment_type_negotiation.go +++ b/funding/commitment_type_negotiation.go @@ -28,37 +28,51 @@ func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local, // BOLT#2 specifies we MUST use explicit negotiation if both peers // signal for it. - if hasFeatures(local, remote, lnwire.ExplicitChannelTypeOptional) { - if desiredChanType != nil { - commitType, err := explicitNegotiateCommitmentType( - *desiredChanType, local, remote, - ) + explicitNegotiation := hasFeatures( + local, remote, lnwire.ExplicitChannelTypeOptional, + ) - return desiredChanType, commitType, err - } + chanTypeRequested := desiredChanType != nil - // Explicitly signal the "implicit" negotiation commitment type - // as default when a desired channel type isn't specified. - chanType, commitType := implicitNegotiateCommitmentType(local, - remote) + switch { + case explicitNegotiation && chanTypeRequested: + commitType, err := explicitNegotiateCommitmentType( + *desiredChanType, local, remote, + ) - return chanType, commitType, nil - } + return desiredChanType, commitType, err - // Otherwise, we'll use implicit negotiation. In this case, we are - // restricted to the newest channel type advertised. If the passed-in - // channelType doesn't match what was advertised, we fail. - chanType, commitType := implicitNegotiateCommitmentType(local, remote) + // We don't have a specific channel type requested, so we select a + // default type as if implicit negotiation were used, and then we + // explicitly signal that default type. + case explicitNegotiation && !chanTypeRequested: + defaultChanType, commitType := implicitNegotiateCommitmentType( + local, remote, + ) + + return defaultChanType, commitType, nil + + // A specific channel type was requested, but we can't explicitly signal + // it. So if implicit negotiation wouldn't select the desired channel + // type, we must return an error. + case !explicitNegotiation && chanTypeRequested: + implicitChanType, commitType := implicitNegotiateCommitmentType( + local, remote, + ) - if desiredChanType != nil { expected := lnwire.RawFeatureVector(*desiredChanType) - actual := lnwire.RawFeatureVector(*chanType) + actual := lnwire.RawFeatureVector(*implicitChanType) if !expected.Equals(&actual) { return nil, 0, errUnsupportedChannelType } - } - return nil, commitType, nil + return nil, commitType, nil + + default: // !explicitNegotiation && !chanTypeRequested + _, commitType := implicitNegotiateCommitmentType(local, remote) + + return nil, commitType, nil + } } // explicitNegotiateCommitmentType attempts to explicitly negotiate for a