diff --git a/docs/release-notes/release-notes-0.15.3.md b/docs/release-notes/release-notes-0.15.3.md index d2ec24dfe..897913d7f 100644 --- a/docs/release-notes/release-notes-0.15.3.md +++ b/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 diff --git a/lntest/itest/lnd_wallet_import_test.go b/lntest/itest/lnd_wallet_import_test.go index 5fa79989d..e544d680a 100644 --- a/lntest/itest/lnd_wallet_import_test.go +++ b/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 { diff --git a/lnwallet/chancloser/chancloser.go b/lnwallet/chancloser/chancloser.go index 1af162f7d..2ddf47950 100644 --- a/lnwallet/chancloser/chancloser.go +++ b/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) diff --git a/lnwallet/chancloser/chancloser_test.go b/lnwallet/chancloser/chancloser_test.go index 6d194398c..a51ebfa19 100644 --- a/lnwallet/chancloser/chancloser_test.go +++ b/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, } diff --git a/lnwallet/channel.go b/lnwallet/channel.go index da93b3d2c..dd202aac2 100644 --- a/lnwallet/channel.go +++ b/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)) } diff --git a/peer/brontide.go b/peer/brontide.go index b9741fae2..c5f057c54 100644 --- a/peer/brontide.go +++ b/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,