diff --git a/docs/release-notes/release-notes-0.14.0.md b/docs/release-notes/release-notes-0.14.0.md index 4fd0bb709..ca7c873c0 100644 --- a/docs/release-notes/release-notes-0.14.0.md +++ b/docs/release-notes/release-notes-0.14.0.md @@ -335,6 +335,8 @@ you. * [Order of the start/stop on subsystems are changed to promote better safety](https://github.com/lightningnetwork/lnd/pull/1783). * [Fixed flake that occurred when testing the new optimistic link shutdown.](https://github.com/lightningnetwork/lnd/pull/5808) + +* [Respect minimum relay fee during commitment fee updates](https://github.com/lightningnetwork/lnd/pull/5483). * [Replace reference to protobuf library with OSV](https://github.com/lightningnetwork/lnd/pull/5759) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 00197d2b3..3d258befe 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -4,7 +4,6 @@ import ( "bytes" "crypto/sha256" "fmt" - "math" prand "math/rand" "sync" "sync/atomic" @@ -619,9 +618,18 @@ func (l *channelLink) sampleNetworkFee() (chainfee.SatPerKWeight, error) { // shouldAdjustCommitFee returns true if we should update our commitment fee to // match that of the network fee. We'll only update our commitment fee if the -// network fee is +/- 10% to our network fee. -func shouldAdjustCommitFee(netFee, chanFee chainfee.SatPerKWeight) bool { +// network fee is +/- 10% to our commitment fee or if our current commitment +// fee is below the minimum relay fee. +func shouldAdjustCommitFee(netFee, chanFee, + minRelayFee chainfee.SatPerKWeight) bool { + switch { + // If the network fee is greater than our current commitment fee and + // our current commitment fee is below the minimum relay fee then + // we should switch to it no matter if it is less than a 10% increase. + case netFee > chanFee && chanFee < minRelayFee: + return true + // If the network fee is greater than the commitment fee, then we'll // switch to it if it's at least 10% greater than the commit fee. case netFee > chanFee && netFee >= (chanFee+(chanFee*10)/100): @@ -1106,18 +1114,22 @@ func (l *channelLink) htlcManager() { continue } - // We'll check to see if we should update the fee rate - // based on our current set fee rate. We'll cap the new - // fee rate to our max fee allocation. - commitFee := l.channel.CommitFeeRate() - maxFee := l.channel.MaxFeeRate( - l.cfg.MaxFeeAllocation, + minRelayFee := l.cfg.FeeEstimator.RelayFeePerKW() + + newCommitFee := l.channel.IdealCommitFeeRate( + netFee, minRelayFee, l.cfg.MaxAnchorsCommitFeeRate, + l.cfg.MaxFeeAllocation, ) - newCommitFee := chainfee.SatPerKWeight( - math.Min(float64(netFee), float64(maxFee)), - ) - if !shouldAdjustCommitFee(newCommitFee, commitFee) { + + // We determine if we should adjust the commitment fee + // based on the current commitment fee, the suggested + // new commitment fee and the current minimum relay fee + // rate. + commitFee := l.channel.CommitFeeRate() + if !shouldAdjustCommitFee( + newCommitFee, commitFee, minRelayFee, + ) { continue } diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 4d9e10433..c880e6683 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -3744,6 +3744,7 @@ func TestShouldAdjustCommitFee(t *testing.T) { tests := []struct { netFee chainfee.SatPerKWeight chanFee chainfee.SatPerKWeight + minRelayFee chainfee.SatPerKWeight shouldAdjust bool }{ @@ -3819,11 +3820,22 @@ func TestShouldAdjustCommitFee(t *testing.T) { chanFee: 1000, shouldAdjust: false, }, + + // The network fee is higher than our commitment fee, + // hasn't yet crossed our activation threshold, but the + // current commitment fee is below the minimum relay fee and + // so the fee should be updated. + { + netFee: 1100, + chanFee: 1098, + minRelayFee: 1099, + shouldAdjust: true, + }, } for i, test := range tests { adjustedFee := shouldAdjustCommitFee( - test.netFee, test.chanFee, + test.netFee, test.chanFee, test.minRelayFee, ) if adjustedFee && !test.shouldAdjust { @@ -4009,6 +4021,13 @@ func TestChannelLinkUpdateCommitFee(t *testing.T) { {"bob", "alice", &lnwire.RevokeAndAck{}, false}, {"bob", "alice", &lnwire.CommitSig{}, false}, {"alice", "bob", &lnwire.RevokeAndAck{}, false}, + + // Third fee update. + {"alice", "bob", &lnwire.UpdateFee{}, false}, + {"alice", "bob", &lnwire.CommitSig{}, false}, + {"bob", "alice", &lnwire.RevokeAndAck{}, false}, + {"bob", "alice", &lnwire.CommitSig{}, false}, + {"alice", "bob", &lnwire.RevokeAndAck{}, false}, } n.aliceServer.intersect(createInterceptorFunc("[alice] <-- [bob]", "alice", messages, chanID, false)) @@ -4025,8 +4044,8 @@ func TestChannelLinkUpdateCommitFee(t *testing.T) { // triggerFeeUpdate is a helper closure to determine whether a fee // update was triggered and completed properly. - triggerFeeUpdate := func(feeEstimate, newFeeRate chainfee.SatPerKWeight, - shouldUpdate bool) { + triggerFeeUpdate := func(feeEstimate, minRelayFee, + newFeeRate chainfee.SatPerKWeight, shouldUpdate bool) { t.Helper() @@ -4046,6 +4065,13 @@ func TestChannelLinkUpdateCommitFee(t *testing.T) { t.Fatalf("alice didn't query for the new network fee") } + // We also send the min relay fee response to Alice. + select { + case n.feeEstimator.relayFee <- minRelayFee: + case <-time.After(time.Second * 5): + t.Fatalf("alice didn't query for the min relay fee") + } + // Record the fee rates after the links have processed the fee // update and ensure they are correct based on whether a fee // update should have been triggered. @@ -4071,20 +4097,28 @@ func TestChannelLinkUpdateCommitFee(t *testing.T) { }, 10*time.Second, time.Second) } + minRelayFee := startingFeeRate / 3 + // Triggering the link to update the fee of the channel with the same // fee rate should not send a fee update. - triggerFeeUpdate(startingFeeRate, startingFeeRate, false) + triggerFeeUpdate(startingFeeRate, minRelayFee, startingFeeRate, false) // Triggering the link to update the fee of the channel with a much // larger fee rate _should_ send a fee update. newFeeRate := startingFeeRate * 3 - triggerFeeUpdate(newFeeRate, newFeeRate, true) + triggerFeeUpdate(newFeeRate, minRelayFee, newFeeRate, true) // Triggering the link to update the fee of the channel with a fee rate // that exceeds its maximum fee allocation should result in a fee rate // corresponding to the maximum fee allocation. - const maxFeeRate chainfee.SatPerKWeight = 207182320 - triggerFeeUpdate(maxFeeRate+1, maxFeeRate, true) + const maxFeeRate chainfee.SatPerKWeight = 207180182 + triggerFeeUpdate(maxFeeRate+1, minRelayFee, maxFeeRate, true) + + // Triggering the link to update the fee of the channel with a fee rate + // that is below the current min relay fee rate should result in a fee + // rate corresponding to the minimum relay fee. + newFeeRate = minRelayFee / 2 + triggerFeeUpdate(newFeeRate, minRelayFee, minRelayFee, true) } // TestChannelLinkAcceptDuplicatePayment tests that if a link receives an diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index ac1a47dc2..b7e073de6 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -71,6 +71,7 @@ func (m *mockPreimageCache) SubscribeUpdates() *contractcourt.WitnessSubscriptio type mockFeeEstimator struct { byteFeeIn chan chainfee.SatPerKWeight + relayFee chan chainfee.SatPerKWeight quit chan struct{} } @@ -78,6 +79,7 @@ type mockFeeEstimator struct { func newMockFeeEstimator() *mockFeeEstimator { return &mockFeeEstimator{ byteFeeIn: make(chan chainfee.SatPerKWeight), + relayFee: make(chan chainfee.SatPerKWeight), quit: make(chan struct{}), } } @@ -94,7 +96,12 @@ func (m *mockFeeEstimator) EstimateFeePerKW( } func (m *mockFeeEstimator) RelayFeePerKW() chainfee.SatPerKWeight { - return 1e3 + select { + case feeRate := <-m.relayFee: + return feeRate + case <-m.quit: + return 0 + } } func (m *mockFeeEstimator) Start() error { diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index eee261488..7439c528a 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -125,11 +125,11 @@ type BtcdEstimator struct { // produce fee estimates. fallbackFeePerKW SatPerKWeight - // minFeePerKW is the minimum fee, in sat/kw, that we should enforce. - // This will be used as the default fee rate for a transaction when the - // estimated fee rate is too low to allow the transaction to propagate - // through the network. - minFeePerKW SatPerKWeight + // minFeeManager is used to query the current minimum fee, in sat/kw, + // that we should enforce. This will be used to determine fee rate for + // a transaction when the estimated fee rate is too low to allow the + // transaction to propagate through the network. + minFeeManager *minFeeManager btcdConn *rpcclient.Client } @@ -164,34 +164,36 @@ func (b *BtcdEstimator) Start() error { return err } - // Once the connection to the backend node has been established, we'll - // query it for its minimum relay fee. - info, err := b.btcdConn.GetInfo() + // Once the connection to the backend node has been established, we + // can initialise the minimum relay fee manager which queries the + // chain backend for the minimum relay fee on construction. + minRelayFeeManager, err := newMinFeeManager( + defaultUpdateInterval, b.fetchMinRelayFee, + ) if err != nil { return err } + b.minFeeManager = minRelayFeeManager + + return nil +} + +// fetchMinRelayFee fetches and returns the minimum relay fee in sat/kb from +// the btcd backend. +func (b *BtcdEstimator) fetchMinRelayFee() (SatPerKWeight, error) { + info, err := b.btcdConn.GetInfo() + if err != nil { + return 0, err + } relayFee, err := btcutil.NewAmount(info.RelayFee) if err != nil { - return err + return 0, err } // The fee rate is expressed in sat/kb, so we'll manually convert it to // our desired sat/kw rate. - minRelayFeePerKw := SatPerKVByte(relayFee).FeePerKWeight() - - // By default, we'll use the backend node's minimum relay fee as the - // minimum fee rate we'll propose for transacations. However, if this - // happens to be lower than our fee floor, we'll enforce that instead. - b.minFeePerKW = minRelayFeePerKw - if b.minFeePerKW < FeePerKwFloor { - b.minFeePerKW = FeePerKwFloor - } - - log.Debugf("Using minimum fee rate of %v sat/kw", - int64(b.minFeePerKW)) - - return nil + return SatPerKVByte(relayFee).FeePerKWeight(), nil } // Stop stops any spawned goroutines and cleans up the resources used @@ -230,7 +232,7 @@ func (b *BtcdEstimator) EstimateFeePerKW(numBlocks uint32) (SatPerKWeight, error // // NOTE: This method is part of the Estimator interface. func (b *BtcdEstimator) RelayFeePerKW() SatPerKWeight { - return b.minFeePerKW + return b.minFeeManager.fetchMinFee() } // fetchEstimate returns a fee estimate for a transaction to be confirmed in @@ -254,11 +256,11 @@ func (b *BtcdEstimator) fetchEstimate(confTarget uint32) (SatPerKWeight, error) satPerKw := SatPerKVByte(satPerKB).FeePerKWeight() // Finally, we'll enforce our fee floor. - if satPerKw < b.minFeePerKW { + if satPerKw < b.minFeeManager.fetchMinFee() { log.Debugf("Estimated fee rate of %v sat/kw is too low, "+ "using fee floor of %v sat/kw instead", satPerKw, - b.minFeePerKW) - satPerKw = b.minFeePerKW + b.minFeeManager) + satPerKw = b.minFeeManager.fetchMinFee() } log.Debugf("Returning %v sat/kw for conf target of %v", @@ -280,11 +282,11 @@ type BitcoindEstimator struct { // produce fee estimates. fallbackFeePerKW SatPerKWeight - // minFeePerKW is the minimum fee, in sat/kw, that we should enforce. - // This will be used as the default fee rate for a transaction when the - // estimated fee rate is too low to allow the transaction to propagate - // through the network. - minFeePerKW SatPerKWeight + // minFeeManager is used to keep track of the minimum fee, in sat/kw, + // that we should enforce. This will be used as the default fee rate + // for a transaction when the estimated fee rate is too low to allow + // the transaction to propagate through the network. + minFeeManager *minFeeManager // feeMode is the estimate_mode to use when calling "estimatesmartfee". // It can be either "ECONOMICAL" or "CONSERVATIVE", and it's default @@ -324,43 +326,47 @@ func NewBitcoindEstimator(rpcConfig rpcclient.ConnConfig, feeMode string, // NOTE: This method is part of the Estimator interface. func (b *BitcoindEstimator) Start() error { // Once the connection to the backend node has been established, we'll - // query it for its minimum relay fee. Since the `getinfo` RPC has been - // deprecated for `bitcoind`, we'll need to send a `getnetworkinfo` - // command as a raw request. - resp, err := b.bitcoindConn.RawRequest("getnetworkinfo", nil) + // initialise the minimum relay fee manager which will query + // the backend node for its minimum mempool fee. + relayFeeManager, err := newMinFeeManager( + defaultUpdateInterval, + b.fetchMinMempoolFee, + ) if err != nil { return err } + b.minFeeManager = relayFeeManager - // Parse the response to retrieve the relay fee in sat/KB. + return nil +} + +// fetchMinMempoolFee is used to fetch the minimum fee that the backend node +// requires for a tx to enter its mempool. The returned fee will be the +// maximum of the minimum relay fee and the minimum mempool fee. +func (b *BitcoindEstimator) fetchMinMempoolFee() (SatPerKWeight, error) { + resp, err := b.bitcoindConn.RawRequest("getmempoolinfo", nil) + if err != nil { + return 0, err + } + + // Parse the response to retrieve the min mempool fee in sat/KB. + // mempoolminfee is the maximum of minrelaytxfee and + // minimum mempool fee info := struct { - RelayFee float64 `json:"relayfee"` + MempoolMinFee float64 `json:"mempoolminfee"` }{} if err := json.Unmarshal(resp, &info); err != nil { - return err + return 0, err } - relayFee, err := btcutil.NewAmount(info.RelayFee) + minMempoolFee, err := btcutil.NewAmount(info.MempoolMinFee) if err != nil { - return err + return 0, err } // The fee rate is expressed in sat/kb, so we'll manually convert it to // our desired sat/kw rate. - minRelayFeePerKw := SatPerKVByte(relayFee).FeePerKWeight() - - // By default, we'll use the backend node's minimum relay fee as the - // minimum fee rate we'll propose for transacations. However, if this - // happens to be lower than our fee floor, we'll enforce that instead. - b.minFeePerKW = minRelayFeePerKw - if b.minFeePerKW < FeePerKwFloor { - b.minFeePerKW = FeePerKwFloor - } - - log.Debugf("Using minimum fee rate of %v sat/kw", - int64(b.minFeePerKW)) - - return nil + return SatPerKVByte(minMempoolFee).FeePerKWeight(), nil } // Stop stops any spawned goroutines and cleans up the resources used @@ -406,7 +412,7 @@ func (b *BitcoindEstimator) EstimateFeePerKW( // // NOTE: This method is part of the Estimator interface. func (b *BitcoindEstimator) RelayFeePerKW() SatPerKWeight { - return b.minFeePerKW + return b.minFeeManager.fetchMinFee() } // fetchEstimate returns a fee estimate for a transaction to be confirmed in @@ -453,12 +459,13 @@ func (b *BitcoindEstimator) fetchEstimate(confTarget uint32) (SatPerKWeight, err satPerKw := SatPerKVByte(satPerKB).FeePerKWeight() // Finally, we'll enforce our fee floor. - if satPerKw < b.minFeePerKW { + minRelayFee := b.minFeeManager.fetchMinFee() + if satPerKw < minRelayFee { log.Debugf("Estimated fee rate of %v sat/kw is too low, "+ "using fee floor of %v sat/kw instead", satPerKw, - b.minFeePerKW) + minRelayFee) - satPerKw = b.minFeePerKW + satPerKw = minRelayFee } log.Debugf("Returning %v sat/kw for conf target of %v", diff --git a/lnwallet/chainfee/minfeemanager.go b/lnwallet/chainfee/minfeemanager.go new file mode 100644 index 000000000..1f377d2cf --- /dev/null +++ b/lnwallet/chainfee/minfeemanager.go @@ -0,0 +1,76 @@ +package chainfee + +import ( + "sync" + "time" +) + +const defaultUpdateInterval = 10 * time.Minute + +// minFeeManager is used to store and update the minimum fee that is required +// by a transaction to be accepted to the mempool. The minFeeManager ensures +// that the backend used to fetch the fee is not queried too regularly. +type minFeeManager struct { + mu sync.Mutex + minFeePerKW SatPerKWeight + lastUpdatedTime time.Time + minUpdateInterval time.Duration + fetchFeeFunc fetchFee +} + +// fetchFee represents a function that can be used to fetch a fee. +type fetchFee func() (SatPerKWeight, error) + +// newMinFeeManager creates a new minFeeManager and uses the +// given fetchMinFee function to set the minFeePerKW of the minFeeManager. +// This function requires the fetchMinFee function to succeed. +func newMinFeeManager(minUpdateInterval time.Duration, + fetchMinFee fetchFee) (*minFeeManager, error) { + + minFee, err := fetchMinFee() + if err != nil { + return nil, err + } + + return &minFeeManager{ + minFeePerKW: minFee, + lastUpdatedTime: time.Now(), + minUpdateInterval: minUpdateInterval, + fetchFeeFunc: fetchMinFee, + }, nil +} + +// fetchMinFee returns the stored minFeePerKW if it has been updated recently +// or if the call to the chain backend fails. Otherwise, it sets the stored +// minFeePerKW to the fee returned from the backend and floors it based on +// our fee floor. +func (m *minFeeManager) fetchMinFee() SatPerKWeight { + m.mu.Lock() + defer m.mu.Unlock() + + if time.Since(m.lastUpdatedTime) < m.minUpdateInterval { + return m.minFeePerKW + } + + newMinFee, err := m.fetchFeeFunc() + if err != nil { + log.Errorf("Unable to fetch updated min fee from chain "+ + "backend. Using last known min fee instead: %v", err) + + return m.minFeePerKW + } + + // By default, we'll use the backend node's minimum fee as the + // minimum fee rate we'll propose for transactions. However, if this + // happens to be lower than our fee floor, we'll enforce that instead. + m.minFeePerKW = newMinFee + if m.minFeePerKW < FeePerKwFloor { + m.minFeePerKW = FeePerKwFloor + } + m.lastUpdatedTime = time.Now() + + log.Debugf("Using minimum fee rate of %v sat/kw", + int64(m.minFeePerKW)) + + return m.minFeePerKW +} diff --git a/lnwallet/chainfee/minfeemanager_test.go b/lnwallet/chainfee/minfeemanager_test.go new file mode 100644 index 000000000..0209f1a29 --- /dev/null +++ b/lnwallet/chainfee/minfeemanager_test.go @@ -0,0 +1,54 @@ +package chainfee + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +type mockChainBackend struct { + minFee SatPerKWeight + callCount int +} + +func (m *mockChainBackend) fetchFee() (SatPerKWeight, error) { + m.callCount++ + return m.minFee, nil +} + +// TestMinFeeManager tests that the minFeeManager returns an up to date min fee +// by querying the chain backend and that it returns a cached fee if the chain +// backend was recently queried. +func TestMinFeeManager(t *testing.T) { + t.Parallel() + + chainBackend := &mockChainBackend{ + minFee: SatPerKWeight(1000), + } + + // Initialise the min fee manager. This should call the chain backend + // once. + feeManager, err := newMinFeeManager( + 100*time.Millisecond, + chainBackend.fetchFee, + ) + require.NoError(t, err) + require.Equal(t, 1, chainBackend.callCount) + + // If the fee is requested again, the stored fee should be returned + // and the chain backend should not be queried. + chainBackend.minFee = SatPerKWeight(2000) + minFee := feeManager.fetchMinFee() + require.Equal(t, minFee, SatPerKWeight(1000)) + require.Equal(t, 1, chainBackend.callCount) + + // Fake the passing of time. + feeManager.lastUpdatedTime = time.Now().Add(-200 * time.Millisecond) + + // If the fee is queried again after the backoff period has passed + // then the chain backend should be queried again for the min fee. + minFee = feeManager.fetchMinFee() + require.Equal(t, SatPerKWeight(2000), minFee) + require.Equal(t, 2, chainBackend.callCount) +} diff --git a/lnwallet/channel.go b/lnwallet/channel.go index b914ac789..55db9ed51 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6821,7 +6821,10 @@ func (lc *LightningChannel) validateFeeRate(feePerKw chainfee.SatPerKWeight) err // be above our reserve balance. Otherwise, we'll reject the fee // update. availableBalance, txWeight := lc.availableBalance() - oldFee := lnwire.NewMSatFromSatoshis(lc.localCommitChain.tip().fee) + + oldFee := lnwire.NewMSatFromSatoshis( + lc.localCommitChain.tip().feePerKw.FeeForWeight(txWeight), + ) // Our base balance is the total amount of satoshis we can commit // towards fees before factoring in the channel reserve. @@ -6843,17 +6846,6 @@ func (lc *LightningChannel) validateFeeRate(feePerKw chainfee.SatPerKWeight) err newFee, baseBalance) } - // If this new balance is below our reserve, then we can't accommodate - // the fee change, so we'll reject it. - balanceAfterFee := baseBalance - newFee - if balanceAfterFee.ToSatoshis() < lc.channelState.LocalChanCfg.ChanReserve { - return fmt.Errorf("cannot apply fee_update=%v sat/kw, "+ - "new balance=%v would dip below channel reserve=%v", - int64(feePerKw), - balanceAfterFee.ToSatoshis(), - lc.channelState.LocalChanCfg.ChanReserve) - } - // TODO(halseth): should fail if fee update is unreasonable, // as specified in BOLT#2. // * COMMENT(roasbeef): can cross-check with our ideal fee rate @@ -6998,39 +6990,102 @@ func (lc *LightningChannel) CalcFee(feeRate chainfee.SatPerKWeight) btcutil.Amou } // MaxFeeRate returns the maximum fee rate given an allocation of the channel -// initiator's spendable balance. This can be useful to determine when we should -// stop proposing fee updates that exceed our maximum allocation. We also take -// a fee rate cap that should be used for anchor type channels. +// initiator's spendable balance along with the local reserve amount. This can +// be useful to determine when we should stop proposing fee updates that exceed +// our maximum allocation. // // NOTE: This should only be used for channels in which the local commitment is // the initiator. -func (lc *LightningChannel) MaxFeeRate(maxAllocation float64, - maxAnchorFeeRate chainfee.SatPerKWeight) chainfee.SatPerKWeight { - +func (lc *LightningChannel) MaxFeeRate(maxAllocation float64) chainfee.SatPerKWeight { lc.RLock() defer lc.RUnlock() - // The maximum fee depends of the available balance that can be - // committed towards fees. - commit := lc.channelState.LocalCommitment - feeBalance := float64( - commit.LocalBalance.ToSatoshis() + commit.CommitFee, - ) - maxFee := feeBalance * maxAllocation + // The maximum fee depends on the available balance that can be + // committed towards fees. It takes into account our local reserve + // balance. + availableBalance, weight := lc.availableBalance() + + oldFee := lc.localCommitChain.tip().feePerKw.FeeForWeight(weight) + + // baseBalance is the maximum amount available for us to spend on fees. + baseBalance := availableBalance.ToSatoshis() + oldFee + + maxFee := float64(baseBalance) * maxAllocation // Ensure the fee rate doesn't dip below the fee floor. - _, weight := lc.availableBalance() maxFeeRate := maxFee / (float64(weight) / 1000) - feeRate := chainfee.SatPerKWeight( + return chainfee.SatPerKWeight( math.Max(maxFeeRate, float64(chainfee.FeePerKwFloor)), ) +} - // Cap anchor fee rates. - if lc.channelState.ChanType.HasAnchors() && feeRate > maxAnchorFeeRate { - return maxAnchorFeeRate +// IdealCommitFeeRate uses the current network fee, the minimum relay fee, +// maximum fee allocation and anchor channel commitment fee rate to determine +// the ideal fee to be used for the commitments of the channel. +func (lc *LightningChannel) IdealCommitFeeRate(netFeeRate, minRelayFeeRate, + maxAnchorCommitFeeRate chainfee.SatPerKWeight, + maxFeeAlloc float64) chainfee.SatPerKWeight { + + // Get the maximum fee rate that we can use given our max fee allocation + // and given the local reserve balance that we must preserve. + maxFeeRate := lc.MaxFeeRate(maxFeeAlloc) + + var commitFeeRate chainfee.SatPerKWeight + + // If the channel has anchor outputs then cap the fee rate at the + // max anchor fee rate if that maximum is less than our max fee rate. + // Otherwise, cap the fee rate at the max fee rate. + switch lc.channelState.ChanType.HasAnchors() && + maxFeeRate > maxAnchorCommitFeeRate { + + case true: + commitFeeRate = chainfee.SatPerKWeight( + math.Min( + float64(netFeeRate), + float64(maxAnchorCommitFeeRate), + ), + ) + + case false: + commitFeeRate = chainfee.SatPerKWeight( + math.Min(float64(netFeeRate), float64(maxFeeRate)), + ) } - return feeRate + if commitFeeRate >= minRelayFeeRate { + return commitFeeRate + } + + // The commitment fee rate is below the minimum relay fee rate. + // If the min relay fee rate is still below the maximum fee, then use + // the minimum relay fee rate. + if minRelayFeeRate <= maxFeeRate { + return minRelayFeeRate + } + + // The minimum relay fee rate is more than the ideal maximum fee rate. + // Check if it is smaller than the absolute maximum fee rate we can + // use. If it is, then we use the minimum relay fee rate and we log a + // warning to indicate that the max channel fee allocation option was + // ignored. + absoluteMaxFee := lc.MaxFeeRate(1) + if minRelayFeeRate <= absoluteMaxFee { + lc.log.Warn("Ignoring max channel fee allocation to " + + "ensure that the commitment fee is above the " + + "minimum relay fee.") + + return minRelayFeeRate + } + + // The absolute maximum fee rate we can pay is below the minimum + // relay fee rate. The commitment tx will not be able to propagate. + // To give the transaction the best chance, we use the absolute + // maximum fee we have available and we log an error. + lc.log.Errorf("The commitment fee rate of %s is below the current "+ + "minimum relay fee rate of %s. The max fee rate of %s will be"+ + "used.", commitFeeRate, minRelayFeeRate, absoluteMaxFee) + + return absoluteMaxFee } // RemoteNextRevocation returns the channelState's RemoteNextRevocation. diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 0ef5c8514..b22156567 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -5,9 +5,11 @@ import ( "container/list" "crypto/sha256" "fmt" + "math/rand" "reflect" "runtime" "testing" + "testing/quick" "github.com/btcsuite/btcd/blockchain" "github.com/btcsuite/btcd/btcec" @@ -8003,17 +8005,29 @@ func TestForceCloseBorkedState(t *testing.T) { func TestChannelMaxFeeRate(t *testing.T) { t.Parallel() - assertMaxFeeRate := func(c *LightningChannel, - maxAlloc float64, anchorMax, expFeeRate chainfee.SatPerKWeight) { + assertMaxFeeRate := func(c *LightningChannel, maxAlloc float64, + expFeeRate chainfee.SatPerKWeight) { - t.Helper() - - maxFeeRate := c.MaxFeeRate(maxAlloc, anchorMax) + maxFeeRate := c.MaxFeeRate(maxAlloc) if maxFeeRate != expFeeRate { t.Fatalf("expected max fee rate of %v with max "+ "allocation of %v, got %v", expFeeRate, maxAlloc, maxFeeRate) } + + if err := c.validateFeeRate(maxFeeRate); err != nil { + t.Fatalf("fee rate validation failed: %v", err) + } + } + + // propertyTest tests that the validateFeeRate function always passes + // for the output returned by MaxFeeRate for any valid random inputs + // fed to MaxFeeRate. + propertyTest := func(c *LightningChannel) func(alloc maxAlloc) bool { + return func(ma maxAlloc) bool { + maxFeeRate := c.MaxFeeRate(float64(ma)) + return c.validateFeeRate(maxFeeRate) == nil + } } aliceChannel, _, cleanUp, err := CreateTestChannels( @@ -8024,32 +8038,210 @@ func TestChannelMaxFeeRate(t *testing.T) { } defer cleanUp() - assertMaxFeeRate(aliceChannel, 1.0, 0, 690607734) - assertMaxFeeRate(aliceChannel, 0.001, 0, 690607) - assertMaxFeeRate(aliceChannel, 0.000001, 0, 690) - assertMaxFeeRate(aliceChannel, 0.0000001, 0, chainfee.FeePerKwFloor) + if err := quick.Check(propertyTest(aliceChannel), nil); err != nil { + t.Fatal(err) + } + + assertMaxFeeRate(aliceChannel, 1.0, 676794154) + assertMaxFeeRate(aliceChannel, 0.001, 676794) + assertMaxFeeRate(aliceChannel, 0.000001, 676) + assertMaxFeeRate(aliceChannel, 0.0000001, chainfee.FeePerKwFloor) // Check that anchor channels are capped at their max fee rate. anchorChannel, _, cleanUp, err := CreateTestChannels( - channeldb.SingleFunderTweaklessBit | channeldb.AnchorOutputsBit, + channeldb.SingleFunderTweaklessBit | + channeldb.AnchorOutputsBit | channeldb.ZeroHtlcTxFeeBit, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) } defer cleanUp() + if err = quick.Check(propertyTest(anchorChannel), nil); err != nil { + t.Fatal(err) + } + // Anchor commitments are heavier, hence will the same allocation lead // to slightly lower fee rates. - assertMaxFeeRate( - anchorChannel, 1.0, chainfee.FeePerKwFloor, - chainfee.FeePerKwFloor, - ) - assertMaxFeeRate(anchorChannel, 0.001, 1000000, 444839) - assertMaxFeeRate(anchorChannel, 0.001, 300000, 300000) - assertMaxFeeRate(anchorChannel, 0.000001, 700, 444) - assertMaxFeeRate( - anchorChannel, 0.0000001, 1000000, chainfee.FeePerKwFloor, - ) + assertMaxFeeRate(anchorChannel, 1.0, 435941555) + assertMaxFeeRate(anchorChannel, 0.001, 435941) + assertMaxFeeRate(anchorChannel, 0.000001, 435) + assertMaxFeeRate(anchorChannel, 0.0000001, chainfee.FeePerKwFloor) +} + +// TestIdealCommitFeeRate tests that we correctly compute the ideal commitment +// fee of a channel given the current network fee, minimum relay fee, maximum +// fee allocation and whether the channel has anchor outputs. +func TestIdealCommitFeeRate(t *testing.T) { + t.Parallel() + + assertIdealFeeRate := func(c *LightningChannel, netFee, minRelay, + maxAnchorCommit chainfee.SatPerKWeight, + maxFeeAlloc float64, expectedFeeRate chainfee.SatPerKWeight) { + + feeRate := c.IdealCommitFeeRate( + netFee, minRelay, maxAnchorCommit, maxFeeAlloc, + ) + if feeRate != expectedFeeRate { + t.Fatalf("expected fee rate of %v got %v", + expectedFeeRate, feeRate) + } + + if err := c.validateFeeRate(feeRate); err != nil { + t.Fatalf("fee rate validation failed: %v", err) + } + } + + // propertyTest tests that the validateFeeRate function always passes + // for the output returned by IdealCommitFeeRate for any valid random + // inputs fed to IdealCommitFeeRate. + propertyTest := func(c *LightningChannel) func(ma maxAlloc, + netFee, minRelayFee, maxAnchorFee fee) bool { + + return func(ma maxAlloc, netFee, minRelayFee, + maxAnchorFee fee) bool { + + idealFeeRate := c.IdealCommitFeeRate( + chainfee.SatPerKWeight(netFee), + chainfee.SatPerKWeight(minRelayFee), + chainfee.SatPerKWeight(maxAnchorFee), + float64(ma), + ) + + return c.validateFeeRate(idealFeeRate) == nil + } + } + + // Test ideal fee rates for a non-anchor channel + t.Run("non-anchor-channel", func(t *testing.T) { + aliceChannel, _, cleanUp, err := CreateTestChannels( + channeldb.SingleFunderTweaklessBit, + ) + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + err = quick.Check(propertyTest(aliceChannel), nil) + if err != nil { + t.Fatal(err) + } + + // If the maximum fee is lower than the network fee and above + // the min relay fee, use the maximum fee. + assertIdealFeeRate( + aliceChannel, 700000000, 0, 0, 1.0, 676794154, + ) + + // If the network fee is lower than the max fee and above the + // min relay fee, use the network fee. + assertIdealFeeRate( + aliceChannel, 500000000, 0, 0, 1.0, 500000000, + ) + + // If the fee is below the minimum relay fee, and the min relay + // fee is less than our max fee, use the minimum relay fee. + assertIdealFeeRate( + aliceChannel, 500000000, 600000000, 0, 1.0, 600000000, + ) + + // The absolute maximum fee rate we can pay (ie, using a max + // allocation of 1) is still below the minimum relay fee. In + // this case, use the absolute max fee. + assertIdealFeeRate( + aliceChannel, 800000000, 700000000, 0, 0.001, + 676794154, + ) + }) + + // Test ideal fee rates for an anchor channel + t.Run("anchor-channel", func(t *testing.T) { + anchorChannel, _, cleanUp, err := CreateTestChannels( + channeldb.SingleFunderTweaklessBit | + channeldb.AnchorOutputsBit | + channeldb.ZeroHtlcTxFeeBit, + ) + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + err = quick.Check(propertyTest(anchorChannel), nil) + if err != nil { + t.Fatal(err) + } + + // Anchor commitments are heavier, hence the same allocation + // leads to slightly lower fee rates. + assertIdealFeeRate( + anchorChannel, 700000000, 0, chainfee.FeePerKwFloor, + 0.1, chainfee.FeePerKwFloor, + ) + + // If the maximum fee is lower than the network fee, use the + // maximum fee. + assertIdealFeeRate( + anchorChannel, 700000000, 0, 1000000, 0.001, 435941, + ) + + assertIdealFeeRate( + anchorChannel, 700000000, 0, 700, 0.000001, 435, + ) + + assertIdealFeeRate( + anchorChannel, 700000000, 0, 1000000, 0.0000001, + chainfee.FeePerKwFloor, + ) + + // If the maximum anchor commit fee rate is less than the + // maximum fee, use the max anchor commit fee since this is an + // anchor channel. + assertIdealFeeRate( + anchorChannel, 700000000, 0, 400000, 0.001, 400000, + ) + + // If the minimum relay fee is above the max anchor commitment + // fee rate but still below our max fee rate then use the min + // relay fee. + assertIdealFeeRate( + anchorChannel, 700000000, 400000, 300000, 0.001, + 400000, + ) + + // If the min relay fee is above the ideal max fee rate but is + // below the max fee rate when the max fee allocation is set to + // 1, the minimum relay fee is used. + assertIdealFeeRate( + anchorChannel, 700000000, 500000, 300000, 0.001, + 500000, + ) + + // The absolute maximum fee rate we can pay (ie, using a max + // allocation of 1) is still below the minimum relay fee. In + // this case, use the absolute max fee. + assertIdealFeeRate( + anchorChannel, 700000000, 450000000, 300000, 0.001, + 435941555, + ) + }) +} + +type maxAlloc float64 + +// Generate ensures that the random value generated by the testing quick +// package for maxAlloc is always a positive float64 between 0 and 1. +func (maxAlloc) Generate(r *rand.Rand, _ int) reflect.Value { + ma := maxAlloc(r.Float64()) + return reflect.ValueOf(ma) +} + +type fee chainfee.SatPerKWeight + +// Generate ensures that the random value generated by the testing quick +// package for a fee is always a positive int64. +func (fee) Generate(r *rand.Rand, _ int) reflect.Value { + am := fee(r.Int63()) + return reflect.ValueOf(am) } // TestChannelFeeRateFloor asserts that valid commitments can be proposed and