Merge pull request #6770 from Roasbeef/co-op-close-with-more-spec-innit

lnwallet/chancloser: remove the commit fee clamp, introduce max fee

docs/release-notes/release-notes-0.15.1.md

@@ -1,6 +1,10 @@
 # Release Notes
 
-## Protocol Extensions
+## Protocol/Spec Updates
+
+* [We'll now no longer clamp the co-op close fee to the commitment
+ fee](https://github.com/lightningnetwork/lnd/pull/6770). Instead, if users are
+ the initiator, they can now specify a max fee that should be respected.
 
 ### Zero-Conf Channel Opens
 * [Introduces support for zero-conf channel opens and non-zero-conf option_scid_alias channels.](https://github.com/lightningnetwork/lnd/pull/5955)

htlcswitch/switch.go

@@ -106,6 +106,12 @@ type ChanClose struct {
 	// process for the cooperative closure transaction kicks off.
 	TargetFeePerKw chainfee.SatPerKWeight
 
+	// MaxFee is the highest fee the caller is willing to pay.
+	//
+	// NOTE: This field is only respected if the caller is the initiator of
+	// the channel.
+	MaxFee chainfee.SatPerKWeight
+
 	// DeliveryScript is an optional delivery script to pay funds out to.
 	DeliveryScript lnwire.DeliveryAddress
 
@@ -1656,7 +1662,7 @@ func (s *Switch) teardownCircuit(pkt *htlcPacket) error {
 // optional parameter which sets a user specified script to close out to.
 func (s *Switch) CloseLink(chanPoint *wire.OutPoint,
 	closeType contractcourt.ChannelCloseType,
-	targetFeePerKw chainfee.SatPerKWeight,
+	targetFeePerKw, maxFee chainfee.SatPerKWeight,
 	deliveryScript lnwire.DeliveryAddress) (chan interface{}, chan error) {
 
 	// TODO(roasbeef) abstract out the close updates.
@@ -1668,6 +1674,7 @@ func (s *Switch) CloseLink(chanPoint *wire.OutPoint,
 		ChanPoint:      chanPoint,
 		Updates:        updateChan,
 		TargetFeePerKw: targetFeePerKw,
+		MaxFee:         maxFee,
 		DeliveryScript: deliveryScript,
 		Err:            errChan,
 	}

lnrpc/lightning.proto

@@ -1910,6 +1910,11 @@ message CloseChannelRequest {
     // A manual fee rate set in sat/vbyte that should be used when crafting the
     // closure transaction.
     uint64 sat_per_vbyte = 6;
+
+    // The maximum fee rate the closer is willing to pay.
+    //
+    // NOTE: This field is only respected if we're the initiator of the channel.
+    uint64 max_fee_per_vbyte = 7;
 }
 
 message CloseStatusUpdate {

lnrpc/lightning.swagger.json

@@ -811,6 +811,14 @@
             "required": false,
             "type": "string",
             "format": "uint64"
+          },
+          {
+            "name": "max_fee_per_vbyte",
+            "description": "The maximum fee rate the closer is willing to pay.\n\nNOTE: This field is only respected if we're the initiator of the channel.",
+            "in": "query",
+            "required": false,
+            "type": "string",
+            "format": "uint64"
           }
         ],
         "tags": [

lnwallet/chancloser/chancloser.go

@@ -6,10 +6,12 @@ import (
 
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/btcsuite/btcd/chaincfg"
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/davecgh/go-spew/spew"
 	"github.com/lightningnetwork/lnd/htlcswitch"
+	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/labels"
 	"github.com/lightningnetwork/lnd/lnwallet"
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
@@ -35,6 +37,12 @@ var (
 	// shutdown script previously set for that party.
 	ErrUpfrontShutdownScriptMismatch = fmt.Errorf("shutdown script does not " +
 		"match upfront shutdown script")
+
+	// ErrProposalExeceedsMaxFee is returned when as the initiator, the
+	// latest fee proposal sent by the responder exceed our max fee.
+	// responder.
+	ErrProposalExeceedsMaxFee = fmt.Errorf("latest fee proposal exceeds " +
+		"max fee")
 )
 
 // closeState represents all the possible states the channel closer state
@@ -73,11 +81,63 @@ const (
 	closeFinished
 )
 
+const (
+	// defaultMaxFeeMultiplier is a multiplier we'll apply to the ideal fee
+	// of the initiator, to decide when the negotiated fee is too high. By
+	// default, we want to bail out if we attempt to negotiate a fee that's
+	// 3x higher than our max fee.
+	defaultMaxFeeMultiplier = 3
+)
+
+// Channel abstracts away from the core channel state machine by exposing an
+// interface that requires only the methods we need to carry out the channel
+// closing process.
+type Channel interface {
+	// CalcFee returns the absolute fee for the given fee rate.
+	CalcFee(chainfee.SatPerKWeight) btcutil.Amount
+
+	// ChannelPoint returns the channel point of the target channel.
+	ChannelPoint() *wire.OutPoint
+
+	// MarkCoopBroadcasted persistently marks that the channel close
+	// transaction has been broadcast.
+	MarkCoopBroadcasted(*wire.MsgTx, bool) error
+
+	// IsInitiator returns true we are the initiator of the channel.
+	IsInitiator() bool
+
+	// ShortChanID returns the scid of the channel.
+	ShortChanID() lnwire.ShortChannelID
+
+	// AbsoluteThawHeight returns the absolute thaw height of the channel.
+	// If the channel is pending, or an unconfirmed zero conf channel, then
+	// an error should be returned.
+	AbsoluteThawHeight() (uint32, error)
+
+	// RemoteUpfrontShutdownScript returns the upfront shutdown script of
+	// the remote party. If the remote party didn't specify such a script,
+	// an empty delivery address should be returned.
+	RemoteUpfrontShutdownScript() lnwire.DeliveryAddress
+
+	// CreateCloseProposal creates a new co-op close proposal in the form
+	// of a valid signature, the chainhash of the final txid, and our final
+	// balance in the created state.
+	CreateCloseProposal(proposedFee btcutil.Amount, localDeliveryScript []byte,
+		remoteDeliveryScript []byte) (input.Signature, *chainhash.Hash,
+		btcutil.Amount, error)
+
+	// CompleteCooperativeClose persistently "completes" the cooperative
+	// close by producing a fully signed co-op close transaction.
+	CompleteCooperativeClose(localSig, remoteSig input.Signature,
+		localDeliveryScript, remoteDeliveryScript []byte,
+		proposedFee btcutil.Amount) (*wire.MsgTx, btcutil.Amount, error)
+}
+
 // ChanCloseCfg holds all the items that a ChanCloser requires to carry out its
 // duties.
 type ChanCloseCfg struct {
 	// Channel is the channel that should be closed.
-	Channel *lnwallet.LightningChannel
+	Channel Channel
 
 	// BroadcastTx broadcasts the passed transaction to the network.
 	BroadcastTx func(*wire.MsgTx, string) error
@@ -89,6 +149,10 @@ type ChanCloseCfg struct {
 	// Disconnect will disconnect from the remote peer in this close.
 	Disconnect func() error
 
+	// MaxFee, is non-zero represents the highest fee that the initiator is
+	// willing to pay to close the channel.
+	MaxFee chainfee.SatPerKWeight
+
 	// Quit is a channel that should be sent upon in the occasion the state
 	// machine should cease all progress and shutdown.
 	Quit chan struct{}
@@ -122,6 +186,11 @@ type ChanCloser struct {
 	// offer when starting negotiation. This will be used as a baseline.
 	idealFeeSat btcutil.Amount
 
+	// maxFee is the highest fee the initiator is willing to pay to close
+	// out the channel. This is either a use specified value, or a default
+	// multiplier based of the initial starting ideal fee.
+	maxFee btcutil.Amount
+
 	// lastFeeProposal is the last fee that we proposed to the remote party.
 	// We'll use this as a pivot point to ratchet our next offer up, down, or
 	// simply accept the remote party's prior offer.
@@ -162,23 +231,16 @@ func NewChanCloser(cfg ChanCloseCfg, deliveryScript []byte,
 	idealFeePerKw chainfee.SatPerKWeight, negotiationHeight uint32,
 	closeReq *htlcswitch.ChanClose, locallyInitiated bool) *ChanCloser {
 
-	// Given the target fee-per-kw, we'll compute what our ideal _total_ fee
-	// will be starting at for this fee negotiation.
-	//
-	// TODO(roasbeef): should factor in minimal commit
+	// Given the target fee-per-kw, we'll compute what our ideal _total_
+	// fee will be starting at for this fee negotiation.
 	idealFeeSat := cfg.Channel.CalcFee(idealFeePerKw)
 
-	// If this fee is greater than the fee currently present within the
-	// commitment transaction, then we'll clamp it down to be within the proper
-	// range.
-	//
-	// TODO(roasbeef): clamp fee func?
-	channelCommitFee := cfg.Channel.StateSnapshot().CommitFee
-	if idealFeeSat > channelCommitFee {
-		chancloserLog.Infof("Ideal starting fee of %v is greater than commit "+
-			"fee of %v, clamping", int64(idealFeeSat), int64(channelCommitFee))
-
-		idealFeeSat = channelCommitFee
+	// When we're the initiator, we'll want to also factor in the highest
+	// fee we want to pay. This'll either be 3x the ideal fee, or the
+	// specified explicit max fee.
+	maxFee := idealFeeSat * defaultMaxFeeMultiplier
+	if cfg.MaxFee > 0 {
+		maxFee = cfg.Channel.CalcFee(cfg.MaxFee)
 	}
 
 	chancloserLog.Infof("Ideal fee for closure of ChannelPoint(%v) is: %v sat",
@@ -193,6 +255,7 @@ func NewChanCloser(cfg ChanCloseCfg, deliveryScript []byte,
 		cfg:                 cfg,
 		negotiationHeight:   negotiationHeight,
 		idealFeeSat:         idealFeeSat,
+		maxFee:              maxFee,
 		localDeliveryScript: deliveryScript,
 		priorFeeOffers:      make(map[btcutil.Amount]*lnwire.ClosingSigned),
 		locallyInitiated:    locallyInitiated,
@@ -282,9 +345,13 @@ func (c *ChanCloser) CloseRequest() *htlcswitch.ChanClose {
 	return c.closeReq
 }
 
-// Channel returns the channel stored in the config.
+// Channel returns the channel stored in the config as a
+// *lnwallet.LightningChannel.
+//
+// NOTE: This method will PANIC if the underlying channel implementation isn't
+// the desired type.
 func (c *ChanCloser) Channel() *lnwallet.LightningChannel {
-	return c.cfg.Channel
+	return c.cfg.Channel.(*lnwallet.LightningChannel)
 }
 
 // NegotiationHeight returns the negotiation height.
@@ -352,9 +419,8 @@ func (c *ChanCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message,
 		// initiator of the channel opening, then we'll deny their close
 		// attempt.
 		chanInitiator := c.cfg.Channel.IsInitiator()
-		chanState := c.cfg.Channel.State()
 		if !chanInitiator {
-			absoluteThawHeight, err := chanState.AbsoluteThawHeight()
+			absoluteThawHeight, err := c.cfg.Channel.AbsoluteThawHeight()
 			if err != nil {
 				return nil, false, err
 			}
@@ -483,9 +549,10 @@ func (c *ChanCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message,
 			feeProposal := calcCompromiseFee(c.chanPoint, c.idealFeeSat,
 				c.lastFeeProposal, remoteProposedFee,
 			)
-			if feeProposal > c.idealFeeSat*3 {
-				return nil, false, fmt.Errorf("couldn't find" +
-					" compromise fee")
+			if c.cfg.Channel.IsInitiator() && feeProposal > c.maxFee {
+				return nil, false, fmt.Errorf("%w: %v > %v",
+					ErrProposalExeceedsMaxFee, feeProposal,
+					c.maxFee)
 			}
 
 			// With our new fee proposal calculated, we'll craft a new close

lnwallet/chancloser/chancloser_test.go

@@ -2,8 +2,14 @@ package chancloser
 
 import (
 	"crypto/rand"
+	"fmt"
 	"testing"
 
+	"github.com/btcsuite/btcd/btcutil"
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/btcsuite/btcd/wire"
+	"github.com/lightningnetwork/lnd/input"
+	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/stretchr/testify/require"
 )
@@ -23,6 +29,8 @@ func randDeliveryAddress(t *testing.T) lnwire.DeliveryAddress {
 // when an upfront shutdown script is set and the script provided does not
 // match, and does not error in any other case.
 func TestMaybeMatchScript(t *testing.T) {
+	t.Parallel()
+
 	addr1 := randDeliveryAddress(t)
 	addr2 := randDeliveryAddress(t)
 
@@ -62,6 +70,8 @@ func TestMaybeMatchScript(t *testing.T) {
 		test := test
 
 		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
 			err := maybeMatchScript(
 				func() error { return nil }, test.upfrontScript,
 				test.shutdownScript,
@@ -73,3 +83,167 @@ func TestMaybeMatchScript(t *testing.T) {
 		})
 	}
 }
+
+type mockChannel struct {
+	absoluteFee btcutil.Amount
+	chanPoint   wire.OutPoint
+	initiator   bool
+	scid        lnwire.ShortChannelID
+}
+
+func (m *mockChannel) CalcFee(chainfee.SatPerKWeight) btcutil.Amount {
+	return m.absoluteFee
+}
+
+func (m *mockChannel) ChannelPoint() *wire.OutPoint {
+	return &m.chanPoint
+}
+
+func (m *mockChannel) MarkCoopBroadcasted(*wire.MsgTx, bool) error {
+	return nil
+}
+
+func (m *mockChannel) IsInitiator() bool {
+	return m.initiator
+}
+
+func (m *mockChannel) ShortChanID() lnwire.ShortChannelID {
+	return m.scid
+}
+
+func (m *mockChannel) AbsoluteThawHeight() (uint32, error) {
+	return 0, nil
+}
+
+func (m *mockChannel) RemoteUpfrontShutdownScript() lnwire.DeliveryAddress {
+	return lnwire.DeliveryAddress{}
+}
+
+func (m *mockChannel) CreateCloseProposal(fee btcutil.Amount,
+	localScript, remoteScript []byte,
+) (input.Signature, *chainhash.Hash, btcutil.Amount, error) {
+
+	return nil, nil, 0, nil
+}
+
+func (m *mockChannel) CompleteCooperativeClose(localSig,
+	remoteSig input.Signature, localScript, remoteScript []byte,
+	proposedFee btcutil.Amount) (*wire.MsgTx, btcutil.Amount, error) {
+
+	return nil, 0, nil
+}
+
+// TestMaxFeeClamp tests that if a max fee is specified, then it's used instead
+// of the default max fee multiplier.
+func TestMaxFeeClamp(t *testing.T) {
+	t.Parallel()
+
+	const absoluteFee = btcutil.Amount(1000)
+
+	tests := []struct {
+		name string
+
+		idealFee    chainfee.SatPerKWeight
+		inputMaxFee chainfee.SatPerKWeight
+
+		maxFee btcutil.Amount
+	}{
+		{
+			// No max fee specified, we should see 3x the ideal fee.
+			name: "no max fee",
+
+			idealFee: chainfee.SatPerKWeight(253),
+			maxFee:   absoluteFee * defaultMaxFeeMultiplier,
+		}, {
+			// Max fee specified, this should be used in place.
+			name: "max fee clamp",
+
+			idealFee:    chainfee.SatPerKWeight(253),
+			inputMaxFee: chainfee.SatPerKWeight(2530),
+
+			// Our mock just returns the canned absolute fee here.
+			maxFee: absoluteFee,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			channel := mockChannel{
+				absoluteFee: absoluteFee,
+			}
+
+			chanCloser := NewChanCloser(
+				ChanCloseCfg{
+					Channel: &channel,
+					MaxFee:  test.inputMaxFee,
+				}, nil, test.idealFee, 0, nil, false,
+			)
+
+			require.Equal(t, test.maxFee, chanCloser.maxFee)
+		})
+	}
+}
+
+// TestMaxFeeBailOut tests that once the negotiated fee rate rises above our
+// maximum fee, we'll return an error and refuse to process a co-op close
+// message.
+func TestMaxFeeBailOut(t *testing.T) {
+	t.Parallel()
+
+	const (
+		absoluteFee = btcutil.Amount(1000)
+		idealFee    = chainfee.SatPerKWeight(253)
+	)
+
+	for _, isInitiator := range []bool{true, false} {
+		t.Run(fmt.Sprintf("initiator=%v", isInitiator), func(t *testing.T) {
+			t.Parallel()
+
+			// First, we'll make our mock channel, and use that to
+			// instantiate our channel closer.
+			closeCfg := ChanCloseCfg{
+				Channel: &mockChannel{
+					absoluteFee: absoluteFee,
+					initiator:   isInitiator,
+				},
+				MaxFee: idealFee * 2,
+			}
+			chanCloser := NewChanCloser(
+				closeCfg, nil, idealFee, 0, nil, false,
+			)
+
+			// We'll now force the channel state into the
+			// closeFeeNegotiation state so we can skip straight to
+			// the juicy part. We'll also set our last fee sent so
+			// we'll attempt to actually "negotiate" here.
+			chanCloser.state = closeFeeNegotiation
+			chanCloser.lastFeeProposal = absoluteFee
+
+			// Next, we'll make a ClosingSigned message that
+			// proposes a fee that's above the specified max fee.
+			//
+			// NOTE: We use the absoluteFee here since our mock
+			// always returns this fee for the CalcFee method which
+			// is used to translate a fee rate
+			// into an absolute fee amount in sats.
+			closeMsg := &lnwire.ClosingSigned{
+				FeeSatoshis: absoluteFee * 2,
+			}
+
+			_, _, err := chanCloser.ProcessCloseMsg(closeMsg)
+
+			switch isInitiator {
+			// If we're the initiator, then we expect an error at
+			// this point.
+			case true:
+				require.ErrorIs(t, err, ErrProposalExeceedsMaxFee)
+
+			// Otherwise, we expect things to fail for some other
+			// reason (invalid sig, etc).
+			case false:
+				require.NotErrorIs(t, err, ErrProposalExeceedsMaxFee)
+			}
+		})
+	}
+}

lnwallet/channel.go

@@ -5659,6 +5659,15 @@ func (lc *LightningChannel) RemoteUpfrontShutdownScript() lnwire.DeliveryAddress
 	return lc.channelState.RemoteShutdownScript
 }
 
+// AbsoluteThawHeight determines a frozen channel's absolute thaw height. If
+// the channel is not frozen, then 0 is returned.
+//
+// An error is returned if the channel is penidng, or is an unconfirmed zero
+// conf channel.
+func (lc *LightningChannel) AbsoluteThawHeight() (uint32, error) {
+	return lc.channelState.AbsoluteThawHeight()
+}
+
 // getSignedCommitTx function take the latest commitment transaction and
 // populate it with witness data.
 func (lc *LightningChannel) getSignedCommitTx() (*wire.MsgTx, error) {

peer/brontide.go

@@ -2697,6 +2697,12 @@ func (p *Brontide) createChanCloser(channel *lnwallet.LightningChannel,
 		return nil, fmt.Errorf("cannot obtain best block")
 	}
 
+	// The req will only be set if we initaited the co-op closing flow.
+	var maxFee chainfee.SatPerKWeight
+	if req != nil {
+		maxFee = req.MaxFee
+	}
+
 	chanCloser := chancloser.NewChanCloser(
 		chancloser.ChanCloseCfg{
 			Channel:     channel,
@@ -2706,6 +2712,7 @@ func (p *Brontide) createChanCloser(channel *lnwallet.LightningChannel,
 					op, false,
 				)
 			},
+			MaxFee: maxFee,
 			Disconnect: func() error {
 				return p.cfg.DisconnectPeer(p.IdentityKey())
 			},

rpcserver.go

@@ -2508,9 +2508,12 @@ func (r *rpcServer) CloseChannel(in *lnrpc.CloseChannelRequest,
 			}
 		}
 
+		maxFee := chainfee.SatPerKVByte(
+			in.MaxFeePerVbyte * 1000,
+		).FeePerKWeight()
 		updateChan, errChan = r.server.htlcSwitch.CloseLink(
 			chanPoint, contractcourt.CloseRegular, feeRate,
-			deliveryScript,
+			maxFee, deliveryScript,
 		)
 	}
 out:

server.go

@@ -1034,7 +1034,7 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 		// Instruct the switch to close the channel.  Provide no close out
 		// delivery script or target fee per kw because user input is not
 		// available when the remote peer closes the channel.
-		s.htlcSwitch.CloseLink(chanPoint, closureType, 0, nil)
+		s.htlcSwitch.CloseLink(chanPoint, closureType, 0, 0, nil)
 	}
 
 	// We will use the following channel to reliably hand off contract