diff --git a/funding/commitment_type_negotiation.go b/funding/commitment_type_negotiation.go index 080f4c8695..690cd5615f 100644 --- a/funding/commitment_type_negotiation.go +++ b/funding/commitment_type_negotiation.go @@ -18,61 +18,30 @@ 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. +// Otherwise, a default type is selected based on feature compatibility, +// particularly when the RPC caller does not request a specific channel type. // -// The returned ChannelType is nil when implicit negotiation is used. An error -// is only returned if desiredChanType is not supported. +// The returned ChannelType is always non-nil. An error is only returned if +// desiredChanType is not supported. func negotiateCommitmentType(desiredChanType *lnwire.ChannelType, local, remote *lnwire.FeatureVector) (*lnwire.ChannelType, lnwallet.CommitmentType, error) { - // BOLT#2 specifies we MUST use explicit negotiation if both peers - // signal for it. - explicitNegotiation := hasFeatures( - local, remote, lnwire.ExplicitChannelTypeOptional, - ) - - chanTypeRequested := desiredChanType != nil - - switch { - case explicitNegotiation && chanTypeRequested: + // If a specific channel type was provided, verify it's supported. + if desiredChanType != nil { 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 - } - - return nil, commitType, nil - - default: // !explicitNegotiation && !chanTypeRequested - _, commitType := implicitNegotiateCommitmentType(local, remote) + // No specific channel type was requested. Select a default type based + // on locally-known feature compatibility. This default is then sent + // explicitly over the wire. + defaultChanType, commitType := selectDefaultChannelType(local, remote) - return nil, commitType, nil - } + return defaultChanType, commitType, nil } // explicitNegotiateCommitmentType attempts to explicitly negotiate for a @@ -454,15 +423,14 @@ func explicitNegotiateCommitmentType(channelType lnwire.ChannelType, local, } } -// implicitNegotiateCommitmentType negotiates the commitment type of a channel -// implicitly by choosing the latest non-taproot type supported by the local and -// remote features. Taproot channels must be requested explicitly, keeping -// implicit opens on channel types that can be used for both public and private -// channels. +// selectDefaultChannelType selects a default channel type by choosing the +// latest non-taproot type supported by the local and remote features. +// Taproot channels must be requested explicitly, keeping default selections +// on channel types that can be used for both public and private channels. // -// TODO(yy): Revisit implicit taproot negotiation once public taproot channel +// TODO(yy): Revisit taproot channel selection once public taproot channel // announcements are supported. -func implicitNegotiateCommitmentType(local, +func selectDefaultChannelType(local, remote *lnwire.FeatureVector) (*lnwire.ChannelType, lnwallet.CommitmentType) { diff --git a/funding/commitment_type_negotiation_test.go b/funding/commitment_type_negotiation_test.go index 75907dfce1..8973e55c7e 100644 --- a/funding/commitment_type_negotiation_test.go +++ b/funding/commitment_type_negotiation_test.go @@ -39,10 +39,14 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.StaticRemoteKeyOptional, lnwire.AnchorsZeroFeeHtlcTxOptional, ), - //nolint:ll expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx, - expectsChanType: nil, - expectsErr: nil, + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyRequired, + lnwire.AnchorsZeroFeeHtlcTxRequired, + ), + ), + expectsErr: nil, }, { name: "explicit missing remote commitment feature", @@ -282,7 +286,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) { expectsErr: nil, }, { - name: "implicit tweakless", + name: "default tweakless", channelFeatures: nil, localFeatures: lnwire.NewRawFeatureVector( lnwire.StaticRemoteKeyRequired, @@ -292,11 +296,15 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.StaticRemoteKeyOptional, ), expectsCommitType: lnwallet.CommitmentTypeTweakless, - expectsChanType: nil, - expectsErr: nil, + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector( + lnwire.StaticRemoteKeyRequired, + ), + ), + expectsErr: nil, }, { - name: "implicit legacy", + name: "default legacy", channelFeatures: nil, localFeatures: lnwire.NewRawFeatureVector(), remoteFeatures: lnwire.NewRawFeatureVector( @@ -304,8 +312,10 @@ func TestCommitmentTypeNegotiation(t *testing.T) { lnwire.AnchorsZeroFeeHtlcTxOptional, ), expectsCommitType: lnwallet.CommitmentTypeLegacy, - expectsChanType: nil, - expectsErr: nil, + expectsChanType: (*lnwire.ChannelType)( + lnwire.NewRawFeatureVector(), + ), + expectsErr: nil, }, // Test cases for final taproot channels with explicit @@ -432,11 +442,11 @@ func TestCommitmentTypeNegotiation(t *testing.T) { expectsErr: errUnsupportedChannelType, }, - // Test cases for implicit negotiation ignoring taproot feature + // Test cases for default negotiation ignoring taproot feature // bits. Taproot channels require an explicit channel type. { //nolint:ll - name: "implicit anchors preferred over taproot", + name: "default anchors preferred over taproot", channelFeatures: nil, localFeatures: lnwire.NewRawFeatureVector( lnwire.AnchorsZeroFeeHtlcTxOptional, @@ -461,7 +471,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) { }, { //nolint:ll - name: "implicit ignores staging taproot without anchors", + name: "default ignores staging taproot without anchors", channelFeatures: nil, localFeatures: lnwire.NewRawFeatureVector( lnwire.SimpleTaprootChannelsOptionalFinal, @@ -480,7 +490,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) { }, { //nolint:ll - name: "implicit ignores final taproot without anchors", + name: "default ignores final taproot without anchors", channelFeatures: nil, localFeatures: lnwire.NewRawFeatureVector( lnwire.SimpleTaprootChannelsOptionalFinal, diff --git a/funding/manager.go b/funding/manager.go index 40ac99aba2..3865838281 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1582,6 +1582,15 @@ func (f *Manager) fundeeProcessOpenChannel(peer lnpeer.Peer, return } + // Enforce BOLT-02: The funder MUST set the channel_type in + // open_channel. Reject if it's omitted. + if msg.ChannelType == nil { + err := errors.New("channel type required but not provided") + f.failFundingFlow(peer, cid, err) + + return + } + log.Infof("Recv'd fundingRequest(amt=%v, push=%v, delay=%v, "+ "pendingId=%x) from peer(%x)", amt, msg.PushAmount, msg.CsvDelay, msg.PendingChannelID, @@ -1594,10 +1603,8 @@ func (f *Manager) fundeeProcessOpenChannel(peer lnpeer.Peer, // funds to the channel ourselves. // // 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 if we're using - // implicit negotiation, and simply the channel type sent over if we're - // using explicit negotiation. + // format we can use with this peer. This is dependent on the channel + // type sent by the funder and the feature bits both peers are signaling chanType, commitType, err := negotiateCommitmentType( msg.ChannelType, peer.LocalFeatures(), peer.RemoteFeatures(), ) @@ -1617,52 +1624,49 @@ func (f *Manager) fundeeProcessOpenChannel(peer lnpeer.Peer, scidFeatureVal = true } + // Since we always negotiate an explicit channel type now, chanType is + // guaranteed to be non-nil. var ( zeroConf bool scid bool ) - // 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(*chanType) - zeroConf = featureVec.IsSet(lnwire.ZeroConfRequired) - scid = featureVec.IsSet(lnwire.ScidAliasRequired) - - // If the zero-conf channel type was negotiated, ensure that - // the acceptor allows it. - if zeroConf && !acceptorResp.ZeroConf { + // Check if the channel type includes the zero-conf or scid-alias bits. + featureVec := lnwire.RawFeatureVector(*chanType) + zeroConf = featureVec.IsSet(lnwire.ZeroConfRequired) + scid = featureVec.IsSet(lnwire.ScidAliasRequired) + + // If the zero-conf channel type was negotiated, ensure that the + // acceptor allows it. + if zeroConf && !acceptorResp.ZeroConf { + // Fail the funding flow. + flowErr := fmt.Errorf("channel acceptor blocked zero-conf " + + "channel negotiation") + log.Errorf("Cancelling funding flow for %v based on channel "+ + "acceptor response: %v", cid, flowErr) + f.failFundingFlow(peer, cid, flowErr) + + return + } + + // If the zero-conf channel type wasn't negotiated and the fundee still + // wants a zero-conf channel, perform more checks. Require that both + // sides have the scid-alias feature bit set. We don't require anchors + // here - this is for compatibility with LDK. + if !zeroConf && acceptorResp.ZeroConf { + if !scidFeatureVal { // Fail the funding flow. - flowErr := fmt.Errorf("channel acceptor blocked " + - "zero-conf channel negotiation") - log.Errorf("Cancelling funding flow for %v based on "+ - "channel acceptor response: %v", cid, flowErr) + flowErr := fmt.Errorf("scid-alias feature must be " + + "negotiated for zero-conf") + log.Errorf("Cancelling funding flow for zero-conf "+ + "channel %v: %v", cid, + flowErr) f.failFundingFlow(peer, cid, flowErr) return } - // If the zero-conf channel type wasn't negotiated and the - // fundee still wants a zero-conf channel, perform more checks. - // Require that both sides have the scid-alias feature bit set. - // We don't require anchors here - this is for compatibility - // with LDK. - if !zeroConf && acceptorResp.ZeroConf { - if !scidFeatureVal { - // Fail the funding flow. - flowErr := fmt.Errorf("scid-alias feature " + - "must be negotiated for zero-conf") - log.Errorf("Cancelling funding flow for "+ - "zero-conf channel %v: %v", cid, - flowErr) - f.failFundingFlow(peer, cid, flowErr) - return - } - - // Set zeroConf to true to enable the zero-conf flow. - zeroConf = true - } + // Set zeroConf to true to enable the zero-conf flow. + zeroConf = true } public := msg.ChannelFlags&lnwire.FFAnnounceChannel != 0 @@ -2063,65 +2067,42 @@ func (f *Manager) funderProcessAcceptChannel(peer lnpeer.Peer, // Create the channel identifier. cid := newChanIdentifier(msg.PendingChannelID) - // Perform some basic validation of any custom TLV records included. - // - // TODO: Return errors as funding.Error to give context to remote peer? - if resCtx.channelType != nil { - // We'll want to quickly check that the ChannelType echoed by - // the channel request recipient matches what we proposed. - if msg.ChannelType == nil { - err := errors.New("explicit channel type not echoed " + - "back") - f.failFundingFlow(peer, cid, err) - return - } - proposedFeatures := lnwire.RawFeatureVector(*resCtx.channelType) - ackedFeatures := lnwire.RawFeatureVector(*msg.ChannelType) - if !proposedFeatures.Equals(&ackedFeatures) { - err := errors.New("channel type mismatch") - f.failFundingFlow(peer, cid, err) - return - } - - // We'll want to do the same with the LeaseExpiry if one should - // be set. - if resCtx.reservation.LeaseExpiry() != 0 { - if msg.LeaseExpiry == nil { - err := errors.New("lease expiry not echoed " + - "back") - f.failFundingFlow(peer, cid, err) - return - } - if uint32(*msg.LeaseExpiry) != - resCtx.reservation.LeaseExpiry() { + // Channel type in our reservation should never be nil since we always + // negotiate the channel type explicitly and fall back to the default + // only when the RPC caller does not request one. + if resCtx.channelType == nil { + err := errors.New("channel type not set by funder") + f.failFundingFlow(peer, cid, err) + return + } - err := errors.New("lease expiry mismatch") - f.failFundingFlow(peer, cid, err) - return - } - } - } 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, 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'll want to quickly check that the ChannelType echoed by the + // channel request recipient matches what we proposed. + // TODO: Return errors as funding.Error to give context to remote peer? + if msg.ChannelType == nil { + err := errors.New("explicit channel type not echoed back") + f.failFundingFlow(peer, cid, err) + return + } + proposedFeatures := lnwire.RawFeatureVector(*resCtx.channelType) + ackedFeatures := lnwire.RawFeatureVector(*msg.ChannelType) + if !proposedFeatures.Equals(&ackedFeatures) { + err := errors.New("channel type mismatch") + f.failFundingFlow(peer, cid, err) + return + } - _, negotiatedCommitType, err := negotiateCommitmentType( - msg.ChannelType, peer.LocalFeatures(), - peer.RemoteFeatures(), - ) - if err != nil { - err := errors.New("received unexpected channel type") + // We'll want to do the same with the LeaseExpiry if one should be set. + if resCtx.reservation.LeaseExpiry() != 0 { + if msg.LeaseExpiry == nil { + err := errors.New("lease expiry not echoed back") f.failFundingFlow(peer, cid, err) return } + if uint32(*msg.LeaseExpiry) != + resCtx.reservation.LeaseExpiry() { - if implicitCommitType != negotiatedCommitType { - err := errors.New("negotiated unexpected channel type") + err := errors.New("lease expiry mismatch") f.failFundingFlow(peer, cid, err) return } @@ -4994,28 +4975,27 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) { return } + // Since we always negotiate an explicit channel type now, chanType is + // guaranteed to be non-nil. var ( zeroConf bool scid bool ) - 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 + // 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) - 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 } // The current variant of taproot channels can only be used with diff --git a/funding/manager_test.go b/funding/manager_test.go index ae7058c038..2f5b4317b6 100644 --- a/funding/manager_test.go +++ b/funding/manager_test.go @@ -5089,6 +5089,106 @@ func TestFundingManagerNoEchoChanType(t *testing.T) { assertFundingMsgSent(t, alice.msgChan, "Error") } +// TestFundingManagerRejectMissingChanType verifies that the fundee rejects an +// OpenChannel message that omits the ChannelType field. +func TestFundingManagerRejectMissingChanType(t *testing.T) { + t.Parallel() + + alice, bob := setupFundingManagers(t) + t.Cleanup(func() { + tearDownFundingManagers(t, alice, bob) + }) + + // Build an OpenChannel with the ChannelType field omitted. + openChannelReq := &lnwire.OpenChannel{ + ChainHash: *fundingNetParams.GenesisHash, + PendingChannelID: [32]byte{0x01}, + FundingAmount: btcutil.Amount(10000000), + PushAmount: 0, + DustLimit: btcutil.Amount(546), + MaxValueInFlight: lnwire.MilliSatoshi(100000000), + ChannelReserve: btcutil.Amount(10000), + HtlcMinimum: lnwire.MilliSatoshi(1000), + FeePerKiloWeight: 15000, + CsvDelay: 144, + MaxAcceptedHTLCs: 483, + ChannelType: nil, + } + bob.fundingMgr.ProcessFundingMsg(openChannelReq, alice) + + // Bob should reject the OpenChannel message with an Error. + errMsg := assertFundingMsgSent(t, bob.msgChan, "Error") + err, ok := errMsg.(*lnwire.Error) + require.True(t, ok) + require.ErrorContains( + t, err, "funding failed due to internal error", + ) + assertNumPendingReservations(t, bob, alicePubKey, 0) +} + +// TestFundingManagerAcceptChanType verifies that the fundee accepts an +// OpenChannel message that includes the ChannelType field and echoes it back +// in AcceptChannel. +func TestFundingManagerAcceptChanType(t *testing.T) { + t.Parallel() + + alice, bob := setupFundingManagers(t) + t.Cleanup(func() { + tearDownFundingManagers(t, alice, bob) + }) + + // Set up feature bits for channel type negotiation. + 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, + )) + + // Build an OpenChannel with the ChannelType field properly set. + openChannelReq := &lnwire.OpenChannel{ + ChainHash: *fundingNetParams.GenesisHash, + PendingChannelID: [32]byte{0x01}, + FundingAmount: btcutil.Amount(10000000), + PushAmount: 0, + DustLimit: btcutil.Amount(546), + MaxValueInFlight: lnwire.MilliSatoshi(100000000), + ChannelReserve: btcutil.Amount(10000), + HtlcMinimum: lnwire.MilliSatoshi(1000), + FeePerKiloWeight: 15000, + CsvDelay: 144, + MaxAcceptedHTLCs: 483, + FundingKey: alice.privKey.PubKey(), + RevocationPoint: alice.privKey.PubKey(), + PaymentPoint: alice.privKey.PubKey(), + DelayedPaymentPoint: alice.privKey.PubKey(), + HtlcPoint: alice.privKey.PubKey(), + FirstCommitmentPoint: alice.privKey.PubKey(), + ChannelType: expectedChanType, + } + bob.fundingMgr.ProcessFundingMsg(openChannelReq, alice) + + // Bob should accept the OpenChannel message and send AcceptChannel. + acceptChannelResponse, ok := assertFundingMsgSent( + t, bob.msgChan, "AcceptChannel", + ).(*lnwire.AcceptChannel) + require.True(t, ok) + + // Verify the channel type is echoed back. + require.Equal(t, expectedChanType, acceptChannelResponse.ChannelType) + + // Bob should have a new pending reservation. + assertNumPendingReservations(t, bob, alicePubKey, 1) +} + // TestFundingManagerZeroConf tests that the fundingmanager properly handles // the whole flow for zero-conf channels. func TestFundingManagerZeroConf(t *testing.T) {