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/channel.go b/lnwallet/channel.go index a4e9c36f5..55db9ed51 100644 --- a/lnwallet/channel.go +++ b/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 -// anchor type channels. +// 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() @@ -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 +// 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 e626a22db..b22156567 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -8005,12 +8005,10 @@ 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, @@ -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, - anchorMax fee) bool { - - return func(ma maxAlloc, anchorMax fee) bool { - maxFeeRate := c.MaxFeeRate( - float64(ma), chainfee.SatPerKWeight(anchorMax), - ) - + propertyTest := func(c *LightningChannel) func(alloc maxAlloc) bool { + return func(ma maxAlloc) bool { + maxFeeRate := c.MaxFeeRate(float64(ma)) 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, 0.001, 0, 676794) - assertMaxFeeRate(aliceChannel, 0.000001, 0, 676) - assertMaxFeeRate(aliceChannel, 0.0000001, 0, chainfee.FeePerKwFloor) + 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) @@ -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( - anchorChannel, 1.0, chainfee.FeePerKwFloor, - chainfee.FeePerKwFloor, - ) - assertMaxFeeRate(anchorChannel, 0.001, 1000000, 435941) - assertMaxFeeRate(anchorChannel, 0.001, 300000, 300000) - assertMaxFeeRate(anchorChannel, 0.000001, 700, 435) - 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