diff --git a/docs/release-notes/release-notes-0.17.0.md b/docs/release-notes/release-notes-0.17.0.md index 7bb17bc2d..7245a2ed8 100644 --- a/docs/release-notes/release-notes-0.17.0.md +++ b/docs/release-notes/release-notes-0.17.0.md @@ -148,6 +148,9 @@ * [Cancel rebroadcasting of a transaction when abandoning a channel](https://github.com/lightningnetwork/lnd/pull/7819). +* [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 diff --git a/funding/commitment_type_negotiation.go b/funding/commitment_type_negotiation.go index 475f59a6f..461b6c51d 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. @@ -22,40 +16,63 @@ 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, - remote *lnwire.FeatureVector, mustBeExplicit bool) (bool, - *lnwire.ChannelType, lnwallet.CommitmentType, error) { +// 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) (*lnwire.ChannelType, + lnwallet.CommitmentType, error) { - if channelType != nil { - // If the peer does know explicit negotiation, let's attempt - // that now. - if hasFeatures( - local, remote, lnwire.ExplicitChannelTypeOptional, - ) { + // BOLT#2 specifies we MUST use explicit negotiation if both peers + // signal for it. + explicitNegotiation := hasFeatures( + local, remote, lnwire.ExplicitChannelTypeOptional, + ) - chanType, err := explicitNegotiateCommitmentType( - *channelType, local, remote, - ) - return true, channelType, chanType, err + chanTypeRequested := desiredChanType != nil + + switch { + case explicitNegotiation && chanTypeRequested: + commitType, err := explicitNegotiateCommitmentType( + *desiredChanType, local, remote, + ) + + return desiredChanType, commitType, err + + // 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, + ) + + expected := lnwire.RawFeatureVector(*desiredChanType) + actual := lnwire.RawFeatureVector(*implicitChanType) + if !expected.Equals(&actual) { + return nil, 0, errUnsupportedChannelType } - // 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 - } + return nil, commitType, nil + + default: // !explicitNegotiation && !chanTypeRequested + _, commitType := implicitNegotiateCommitmentType(local, remote) + + return nil, commitType, nil } - - chanType, commitType := implicitNegotiateCommitmentType(local, remote) - return false, chanType, 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 c640eb625..90341c6df 100644 --- a/funding/commitment_type_negotiation_test.go +++ b/funding/commitment_type_negotiation_test.go @@ -15,12 +15,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) { testCases := []struct { name string - mustBeExplicit bool channelFeatures *lnwire.RawFeatureVector localFeatures *lnwire.RawFeatureVector remoteFeatures *lnwire.RawFeatureVector expectsCommitType lnwallet.CommitmentType - expectsChanType lnwire.ChannelType + expectsChanType *lnwire.ChannelType zeroConf bool scidAlias bool expectsErr error @@ -41,30 +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", @@ -106,8 +83,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 +114,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 +147,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 +178,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 +205,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 +229,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,14 +250,16 @@ 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, - // 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, @@ -289,10 +272,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,10 +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", @@ -320,7 +303,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.AnchorsZeroFeeHtlcTxOptional, ), expectsCommitType: lnwallet.CommitmentTypeLegacy, - expectsChanType: lnwire.ChannelType(*lnwire.NewRawFeatureVector()), + expectsChanType: nil, expectsErr: nil, }, } @@ -343,9 +326,8 @@ func TestCommitmentTypeNegotiation(t *testing.T) { ) } - _, lChan, lCommit, err := negotiateCommitmentType( + lChan, lCommit, err := negotiateCommitmentType( channelType, localFeatures, remoteFeatures, - testCase.mustBeExplicit, ) var ( @@ -369,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 { @@ -402,11 +383,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, ) }) diff --git a/funding/manager.go b/funding/manager.go index ddd92491d..9f78aac06 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1504,9 +1504,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 @@ -1525,19 +1524,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) @@ -1773,7 +1769,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, } @@ -1846,7 +1842,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, } @@ -1926,18 +1922,16 @@ 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. - _, implicitChannelType := implicitNegotiateCommitmentType( + // 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(), ) - // 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, + peer.RemoteFeatures(), ) if err != nil { err := errors.New("received unexpected channel type") @@ -1945,11 +1939,7 @@ 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 implicitChannelType != negotiatedChannelType { + if implicitCommitType != negotiatedCommitType { err := errors.New("negotiated unexpected channel type") f.failFundingFlow(peer, cid, err) return @@ -4152,9 +4142,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) @@ -4167,20 +4157,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 @@ -4338,7 +4331,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 73b6fd1d3..df6d8cf22 100644 --- a/funding/manager_test.go +++ b/funding/manager_test.go @@ -4507,3 +4507,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") +}