htlcswitch: respect minimum relay fee

When channels fee rates are being considered for an update, the minimum
relay fee should also be considered.

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.
+// network fee is +/- 10% to our commitment fee or if our current commitment
-func shouldAdjustCommitFee(netFee, chanFee chainfee.SatPerKWeight) bool {
+// 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
+			minRelayFee := l.cfg.FeeEstimator.RelayFeePerKW()
-			// based on our current set fee rate. We'll cap the new
+
-			// fee rate to our max fee allocation.
+			newCommitFee := l.channel.IdealCommitFeeRate(
-			commitFee := l.channel.CommitFeeRate()
+				netFee, minRelayFee,
-			maxFee := l.channel.MaxFeeRate(
-				l.cfg.MaxFeeAllocation,
 				l.cfg.MaxAnchorsCommitFeeRate,
+				l.cfg.MaxFeeAllocation,
 			)
-			newCommitFee := chainfee.SatPerKWeight(
+
-				math.Min(float64(netFee), float64(maxFee)),
+			// We determine if we should adjust the commitment fee
-			)
+			// based on the current commitment fee, the suggested
-			if !shouldAdjustCommitFee(newCommitFee, commitFee) {
+			// new commitment fee and the current minimum relay fee
+			// rate.
+			commitFee := l.channel.CommitFeeRate()
+			if !shouldAdjustCommitFee(
+				newCommitFee, commitFee, minRelayFee,
+			) {
 				continue
 			}
 

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,
+	triggerFeeUpdate := func(feeEstimate, minRelayFee,
-		shouldUpdate bool) {
+		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
+	const maxFeeRate chainfee.SatPerKWeight = 207180182
-	triggerFeeUpdate(maxFeeRate+1, maxFeeRate, true)
+	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

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 {

lnwallet/channel.go

@@ -6992,14 +6992,11 @@ 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 along with the local reserve amount. 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
+// our maximum allocation.
-// anchor type channels.
 //
 // NOTE: This should only be used for channels in which the local commitment is
 // the initiator.
-func (lc *LightningChannel) MaxFeeRate(maxAllocation float64,
+func (lc *LightningChannel) MaxFeeRate(maxAllocation float64) chainfee.SatPerKWeight {
-	maxAnchorFeeRate chainfee.SatPerKWeight) chainfee.SatPerKWeight {
-
 	lc.RLock()
 	defer lc.RUnlock()
 
@@ -7017,16 +7014,78 @@ func (lc *LightningChannel) MaxFeeRate(maxAllocation float64,
 
 	// Ensure the fee rate doesn't dip below the fee floor.
 	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
 }
 
-	return feeRate
+// 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)),
+		)
+	}
+
+	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.

lnwallet/channel_test.go

@@ -8005,12 +8005,10 @@ func TestForceCloseBorkedState(t *testing.T) {
 func TestChannelMaxFeeRate(t *testing.T) {
 	t.Parallel()
 
-	assertMaxFeeRate := func(c *LightningChannel,
+	assertMaxFeeRate := func(c *LightningChannel, maxAlloc float64,
-		maxAlloc float64, anchorMax, expFeeRate chainfee.SatPerKWeight) {
+		expFeeRate chainfee.SatPerKWeight) {
 
-		t.Helper()
+		maxFeeRate := c.MaxFeeRate(maxAlloc)
-
-		maxFeeRate := c.MaxFeeRate(maxAlloc, anchorMax)
 		if maxFeeRate != expFeeRate {
 			t.Fatalf("expected max fee rate of %v with max "+
 				"allocation of %v, got %v", expFeeRate,
@@ -8025,14 +8023,9 @@ func TestChannelMaxFeeRate(t *testing.T) {
 	// 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,
+	propertyTest := func(c *LightningChannel) func(alloc maxAlloc) bool {
-		anchorMax fee) bool {
+		return func(ma maxAlloc) bool {
-
+			maxFeeRate := c.MaxFeeRate(float64(ma))
-		return func(ma maxAlloc, anchorMax fee) bool {
-			maxFeeRate := c.MaxFeeRate(
-				float64(ma), chainfee.SatPerKWeight(anchorMax),
-			)
-
 			return c.validateFeeRate(maxFeeRate) == nil
 		}
 	}
@@ -8045,18 +8038,19 @@ func TestChannelMaxFeeRate(t *testing.T) {
 	}
 	defer cleanUp()
 
-	if err = quick.Check(propertyTest(aliceChannel), nil); err != nil {
+	if err := quick.Check(propertyTest(aliceChannel), nil); err != nil {
 		t.Fatal(err)
 	}
 
-	assertMaxFeeRate(aliceChannel, 1.0, 0, 676794154)
+	assertMaxFeeRate(aliceChannel, 1.0, 676794154)
-	assertMaxFeeRate(aliceChannel, 0.001, 0, 676794)
+	assertMaxFeeRate(aliceChannel, 0.001, 676794)
-	assertMaxFeeRate(aliceChannel, 0.000001, 0, 676)
+	assertMaxFeeRate(aliceChannel, 0.000001, 676)
-	assertMaxFeeRate(aliceChannel, 0.0000001, 0, chainfee.FeePerKwFloor)
+	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)
@@ -8069,16 +8063,167 @@ func TestChannelMaxFeeRate(t *testing.T) {
 
 	// Anchor commitments are heavier, hence will the same allocation lead
 	// to slightly lower fee rates.
-	assertMaxFeeRate(
+	assertMaxFeeRate(anchorChannel, 1.0, 435941555)
-		anchorChannel, 1.0, chainfee.FeePerKwFloor,
+	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,
 		)
-	assertMaxFeeRate(anchorChannel, 0.001, 1000000, 435941)
+
-	assertMaxFeeRate(anchorChannel, 0.001, 300000, 300000)
+		// If the maximum anchor commit fee rate is less than the
-	assertMaxFeeRate(anchorChannel, 0.000001, 700, 435)
+		// maximum fee, use the max anchor commit fee since this is an
-	assertMaxFeeRate(
+		// anchor channel.
-		anchorChannel, 0.0000001, 1000000, chainfee.FeePerKwFloor,
+		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