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") +}