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

In this commit, we modify the way we compute the starting ideal fee for
the co-op close transaction. Before thsi commit, channel.CalcFee was
used, which'll compute the fee based on the commitment transaction
itself, rathern than the co-op close transaction. As the co-op close
transaction is potentailly bigger (two P2TR outputs) than the commitment
transaction, this can cause us to under estimate the fee, which can
result in the fee rate being too low to propagate.

To remedy this, we now compute a fee estimate from scratch, based on the
delivery fees of the two parties.

We also add a bug fix in the chancloser unit tests that wasn't caught
due to loop variable shadowing.

The wallet import itest has been updated as well, since we'll now pay
600 extra saothis to close the channel, since we're accounting for the
added weight of the P2TR outputs.

Fixes #6953

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)
 		})
 	}
@@ -250,8 +275,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,