Merge pull request #6957 from Roasbeef/co-op-close-fee-estimation

lnwallet/chancloser: properly compute initial fee of cop close txn

docs/release-notes/release-notes-0.15.3.md

@@ -17,6 +17,12 @@
 * [A bug has been fixed that caused fee estimation to be incorrect for taproot
   inputs when using the `SendOutputs` call.](https://github.com/lightningnetwork/lnd/pull/6941)
 
+
+* [A bug has been fixed that could cause lnd to underpay for co-op close
+  transaction when or both of the outputs used a P2TR
+  addresss.](https://github.com/lightningnetwork/lnd/pull/6957)
+
+
 ## Taproot
 
 * [Add `p2tr` address type to account

lntest/itest/lnd_wallet_import_test.go

@@ -561,7 +561,7 @@ func fundChanAndCloseFromImportedAccount(t *harnessTest, srcNode, destNode,
 	// they must've been redeemed after the close. Without a pre-negotiated
 	// close address, the funds will go into the source node's wallet
 	// instead of the imported account.
-	const chanCloseTxFee = 9050
+	const chanCloseTxFee = 9650
 	balanceFromClosedChan := chanSize - invoiceAmt - chanCloseTxFee
 
 	if account == defaultImportedAccount {

lnwallet/chancloser/chancloser.go

@@ -10,6 +10,7 @@ import (
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/davecgh/go-spew/spew"
+	"github.com/lightningnetwork/lnd/channeldb"
 	"github.com/lightningnetwork/lnd/htlcswitch"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/labels"
@@ -97,9 +98,6 @@ const (
 // 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
 
@@ -118,6 +116,16 @@ type Channel interface {
 	// an error should be returned.
 	AbsoluteThawHeight() (uint32, error)
 
+	// LocalBalanceDust returns true if when creating a co-op close
+	// transaction, the balance of the local party will be dust after
+	// accounting for any anchor outputs.
+	LocalBalanceDust() bool
+
+	// RemoteBalanceDust returns true if when creating a co-op close
+	// transaction, the balance of the remote party will be dust after
+	// accounting for any anchor outputs.
+	RemoteBalanceDust() bool
+
 	// 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.
@@ -137,6 +145,18 @@ type Channel interface {
 		proposedFee btcutil.Amount) (*wire.MsgTx, btcutil.Amount, error)
 }
 
+// CoopFeeEstimator is used to estimate the fee of a co-op close transaction.
+type CoopFeeEstimator interface {
+	// EstimateFee estimates an _absolute_ fee for a co-op close transaction
+	// given the local+remote tx outs (for the co-op close transaction),
+	// channel type, and ideal fee rate. If a passed TxOut is nil, then
+	// that indicates that an output is dust on the co-op close transaction
+	// _before_ fees are accounted for.
+	EstimateFee(chanType channeldb.ChannelType,
+		localTxOut, remoteTxOut *wire.TxOut,
+		idealFeeRate chainfee.SatPerKWeight) btcutil.Amount
+}
+
 // ChanCloseCfg holds all the items that a ChanCloser requires to carry out its
 // duties.
 type ChanCloseCfg struct {
@@ -163,6 +183,10 @@ type ChanCloseCfg struct {
 	// Quit is a channel that should be sent upon in the occasion the state
 	// machine should cease all progress and shutdown.
 	Quit chan struct{}
+
+	// FeeEstimator is used to estimate the absolute starting co-op close
+	// fee.
+	FeeEstimator CoopFeeEstimator
 }
 
 // ChanCloser is a state machine that handles the cooperative channel closure
@@ -198,6 +222,9 @@ type ChanCloser struct {
 	// multiplier based of the initial starting ideal fee.
 	maxFee btcutil.Amount
 
+	// idealFeeRate is our ideal fee rate.
+	idealFeeRate chainfee.SatPerKWeight
+
 	// 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.
@@ -231,6 +258,45 @@ type ChanCloser struct {
 	locallyInitiated bool
 }
 
+// calcCoopCloseFee computes an "ideal" absolute co-op close fee given the
+// delivery scripts of both parties and our ideal fee rate.
+func calcCoopCloseFee(localOutput, remoteOutput *wire.TxOut,
+	idealFeeRate chainfee.SatPerKWeight) btcutil.Amount {
+
+	var weightEstimator input.TxWeightEstimator
+
+	weightEstimator.AddWitnessInput(input.MultiSigWitnessSize)
+
+	// One of these outputs might be dust, so we'll skip adding it to our
+	// mock transaction, so the fees are more accurate.
+	if localOutput != nil {
+		weightEstimator.AddTxOutput(localOutput)
+	}
+	if remoteOutput != nil {
+		weightEstimator.AddTxOutput(remoteOutput)
+	}
+
+	totalWeight := int64(weightEstimator.Weight())
+
+	return idealFeeRate.FeeForWeight(totalWeight)
+}
+
+// SimpleCoopFeeEstimator is the default co-op close fee estimator. It assumes
+// a normal segwit v0 channel, and that no outputs on the closing transaction
+// are dust.
+type SimpleCoopFeeEstimator struct {
+}
+
+// EstimateFee estimates an _absolute_ fee for a co-op close transaction given
+// the local+remote tx outs (for the co-op close transaction), channel type,
+// and ideal fee rate.
+func (d *SimpleCoopFeeEstimator) EstimateFee(chanType channeldb.ChannelType,
+	localTxOut, remoteTxOut *wire.TxOut,
+	idealFeeRate chainfee.SatPerKWeight) btcutil.Amount {
+
+	return calcCoopCloseFee(localTxOut, remoteTxOut, idealFeeRate)
+}
+
 // NewChanCloser creates a new instance of the channel closure given the passed
 // configuration, and delivery+fee preference. The final argument should only
 // be populated iff, we're the initiator of this closing request.
@@ -238,21 +304,6 @@ 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.
-	idealFeeSat := cfg.Channel.CalcFee(idealFeePerKw)
-
-	// 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",
-		cfg.Channel.ChannelPoint(), int64(idealFeeSat))
-
 	cid := lnwire.NewChanIDFromOutPoint(cfg.Channel.ChannelPoint())
 	return &ChanCloser{
 		closeReq:            closeReq,
@@ -261,14 +312,53 @@ func NewChanCloser(cfg ChanCloseCfg, deliveryScript []byte,
 		cid:                 cid,
 		cfg:                 cfg,
 		negotiationHeight:   negotiationHeight,
-		idealFeeSat:         idealFeeSat,
-		maxFee:              maxFee,
+		idealFeeRate:        idealFeePerKw,
 		localDeliveryScript: deliveryScript,
 		priorFeeOffers:      make(map[btcutil.Amount]*lnwire.ClosingSigned),
 		locallyInitiated:    locallyInitiated,
 	}
 }
 
+// initFeeBaseline computes our ideal fee rate, and also the largest fee we'll
+// accept given information about the delivery script of the remote party.
+func (c *ChanCloser) initFeeBaseline() {
+	// Depending on if a balance ends up being dust or not, we'll pass a
+	// nil TxOut into the EstimateFee call which can handle it.
+	var localTxOut, remoteTxOut *wire.TxOut
+	if !c.cfg.Channel.LocalBalanceDust() {
+		localTxOut = &wire.TxOut{
+			PkScript: c.localDeliveryScript,
+			Value:    0,
+		}
+	}
+	if !c.cfg.Channel.RemoteBalanceDust() {
+		remoteTxOut = &wire.TxOut{
+			PkScript: c.remoteDeliveryScript,
+			Value:    0,
+		}
+	}
+
+	// Given the target fee-per-kw, we'll compute what our ideal _total_
+	// fee will be starting at for this fee negotiation.
+	c.idealFeeSat = c.cfg.FeeEstimator.EstimateFee(
+		0, localTxOut, remoteTxOut, c.idealFeeRate,
+	)
+
+	// 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.
+	c.maxFee = c.idealFeeSat * defaultMaxFeeMultiplier
+	if c.cfg.MaxFee > 0 {
+		c.maxFee = c.cfg.FeeEstimator.EstimateFee(
+			0, localTxOut, remoteTxOut, c.cfg.MaxFee,
+		)
+	}
+
+	chancloserLog.Infof("Ideal fee for closure of ChannelPoint(%v) "+
+		"is: %v sat (max_fee=%v sat)", c.cfg.Channel.ChannelPoint(),
+		int64(c.idealFeeSat), int64(c.maxFee))
+}
+
 // initChanShutdown begins the shutdown process by un-registering the channel,
 // and creating a valid shutdown message to our target delivery address.
 func (c *ChanCloser) initChanShutdown() (*lnwire.Shutdown, error) {
@@ -470,6 +560,10 @@ func (c *ChanCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message,
 		// use this when we craft the closure transaction.
 		c.remoteDeliveryScript = shutdownMsg.Address
 
+		// Now that we know their desried delivery script, we can
+		// compute what our max/ideal fee will be.
+		c.initFeeBaseline()
+
 		// We'll generate a shutdown message of our own to send across the
 		// wire.
 		localShutdown, err := c.initChanShutdown()
@@ -534,6 +628,10 @@ func (c *ChanCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message,
 		// closing transaction should look like.
 		c.state = closeFeeNegotiation
 
+		// Now that we know their desried delivery script, we can
+		// compute what our max/ideal fee will be.
+		c.initFeeBaseline()
+
 		chancloserLog.Infof("ChannelPoint(%v): shutdown response received, "+
 			"entering fee negotiation", c.chanPoint)
 

lnwallet/chancloser/chancloser_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
+	"github.com/lightningnetwork/lnd/channeldb"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
 	"github.com/lightningnetwork/lnd/lnwire"
@@ -131,14 +132,9 @@ 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
+	chanPoint wire.OutPoint
+	initiator bool
+	scid      lnwire.ShortChannelID
 }
 
 func (m *mockChannel) ChannelPoint() *wire.OutPoint {
@@ -179,12 +175,34 @@ func (m *mockChannel) CompleteCooperativeClose(localSig,
 	return nil, 0, nil
 }
 
+func (m *mockChannel) LocalBalanceDust() bool {
+	return false
+}
+
+func (m *mockChannel) RemoteBalanceDust() bool {
+	return false
+}
+
+type mockCoopFeeEstimator struct {
+	targetFee btcutil.Amount
+}
+
+func (m *mockCoopFeeEstimator) EstimateFee(chanType channeldb.ChannelType,
+	localTxOut, remoteTxOut *wire.TxOut,
+	idealFeeRate chainfee.SatPerKWeight) btcutil.Amount {
+
+	return m.targetFee
+}
+
 // 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)
+	const (
+		absoluteFeeOneSatByte = 126
+		absoluteFeeTenSatByte = 1265
+	)
 
 	tests := []struct {
 		name string
@@ -199,33 +217,40 @@ func TestMaxFeeClamp(t *testing.T) {
 			name: "no max fee",
 
 			idealFee: chainfee.SatPerKWeight(253),
-			maxFee:   absoluteFee * defaultMaxFeeMultiplier,
-		}, {
+			maxFee:   absoluteFeeOneSatByte * 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,
+			// We should get the resulting absolute fee based on a
+			// factor of 10 sat/byte (our new max fee).
+			maxFee: absoluteFeeTenSatByte,
 		},
 	}
 	for _, test := range tests {
+		test := test
+
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 
-			channel := mockChannel{
-				absoluteFee: absoluteFee,
-			}
+			channel := mockChannel{}
 
 			chanCloser := NewChanCloser(
 				ChanCloseCfg{
-					Channel: &channel,
-					MaxFee:  test.inputMaxFee,
+					Channel:      &channel,
+					MaxFee:       test.inputMaxFee,
+					FeeEstimator: &SimpleCoopFeeEstimator{},
 				}, nil, test.idealFee, 0, nil, false,
 			)
 
+			// We'll call initFeeBaseline early here since we need
+			// the populate these internal variables.
+			chanCloser.initFeeBaseline()
+
 			require.Equal(t, test.maxFee, chanCloser.maxFee)
 		})
 	}
@@ -243,6 +268,8 @@ func TestMaxFeeBailOut(t *testing.T) {
 	)
 
 	for _, isInitiator := range []bool{true, false} {
+		isInitiator := isInitiator
+
 		t.Run(fmt.Sprintf("initiator=%v", isInitiator), func(t *testing.T) {
 			t.Parallel()
 
@@ -250,8 +277,10 @@ func TestMaxFeeBailOut(t *testing.T) {
 			// instantiate our channel closer.
 			closeCfg := ChanCloseCfg{
 				Channel: &mockChannel{
-					absoluteFee: absoluteFee,
-					initiator:   isInitiator,
+					initiator: isInitiator,
+				},
+				FeeEstimator: &mockCoopFeeEstimator{
+					targetFee: absoluteFee,
 				},
 				MaxFee: idealFee * 2,
 			}

lnwallet/channel.go

@@ -7214,8 +7214,48 @@ func CreateCooperativeCloseTx(fundingTxIn wire.TxIn,
 	return closeTx
 }
 
-// CalcFee returns the commitment fee to use for the given
-// fee rate (fee-per-kw).
+// LocalBalanceDust returns true if when creating a co-op close transaction,
+// the balance of the local party will be dust after accounting for any anchor
+// outputs.
+func (lc *LightningChannel) LocalBalanceDust() bool {
+	lc.RLock()
+	defer lc.RUnlock()
+
+	chanState := lc.channelState
+	localBalance := chanState.LocalCommitment.LocalBalance.ToSatoshis()
+
+	// If this is an anchor channel, and we're the initiator, then we'll
+	// regain the stats allocated to the anchor outputs with the co-op
+	// close transaction.
+	if chanState.ChanType.HasAnchors() && chanState.IsInitiator {
+		localBalance += 2 * anchorSize
+	}
+
+	return localBalance <= chanState.LocalChanCfg.DustLimit
+}
+
+// RemoteBalanceDust returns true if when creating a co-op close transaction,
+// the balance of the remote party will be dust after accounting for any anchor
+// outputs.
+func (lc *LightningChannel) RemoteBalanceDust() bool {
+	lc.RLock()
+	defer lc.RUnlock()
+
+	chanState := lc.channelState
+	remoteBalance := chanState.RemoteCommitment.RemoteBalance.ToSatoshis()
+
+	// If this is an anchor channel, and they're the initiator, then we'll
+	// regain the stats allocated to the anchor outputs with the co-op
+	// close transaction.
+	if chanState.ChanType.HasAnchors() && !chanState.IsInitiator {
+		remoteBalance += 2 * anchorSize
+	}
+
+	return remoteBalance <= chanState.RemoteChanCfg.DustLimit
+}
+
+// CalcFee returns the commitment fee to use for the given fee rate
+// (fee-per-kw).
 func (lc *LightningChannel) CalcFee(feeRate chainfee.SatPerKWeight) btcutil.Amount {
 	return feeRate.FeeForWeight(CommitWeight(lc.channelState.ChanType))
 }

peer/brontide.go

@@ -2719,8 +2719,9 @@ func (p *Brontide) createChanCloser(channel *lnwallet.LightningChannel,
 
 	chanCloser := chancloser.NewChanCloser(
 		chancloser.ChanCloseCfg{
-			Channel:     channel,
-			BroadcastTx: p.cfg.Wallet.PublishTransaction,
+			Channel:      channel,
+			FeeEstimator: &chancloser.SimpleCoopFeeEstimator{},
+			BroadcastTx:  p.cfg.Wallet.PublishTransaction,
 			DisableChannel: func(op wire.OutPoint) error {
 				return p.cfg.ChanStatusMgr.RequestDisable(
 					op, false,