From 2b8a4d296e2d2b5136b1c421837db4884495c3ab Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 8 Sep 2021 13:25:47 +0200 Subject: [PATCH 1/7] lnwallet: use availableBalance in max fee calc In this commit we ensure that the max fee calculated in the MaxFeeRate function takes the local reserve amount into account along with any pending HTLCs. This is done by calling the avaialbeBalance function. --- lnwallet/channel.go | 26 +++++++++++++++----------- lnwallet/channel_test.go | 10 +++++----- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index b914ac789..ea3a9ca56 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6998,9 +6998,10 @@ 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. We also take a fee rate cap that should be used for +// anchor type channels. // // NOTE: This should only be used for channels in which the local commitment is // the initiator. @@ -7010,16 +7011,19 @@ func (lc *LightningChannel) MaxFeeRate(maxAllocation float64, 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( math.Max(maxFeeRate, float64(chainfee.FeePerKwFloor)), diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 0ef5c8514..a490b9502 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -8024,9 +8024,9 @@ 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, 1.0, 0, 676794154) + assertMaxFeeRate(aliceChannel, 0.001, 0, 676794) + assertMaxFeeRate(aliceChannel, 0.000001, 0, 676) assertMaxFeeRate(aliceChannel, 0.0000001, 0, chainfee.FeePerKwFloor) // Check that anchor channels are capped at their max fee rate. @@ -8044,9 +8044,9 @@ func TestChannelMaxFeeRate(t *testing.T) { anchorChannel, 1.0, chainfee.FeePerKwFloor, chainfee.FeePerKwFloor, ) - assertMaxFeeRate(anchorChannel, 0.001, 1000000, 444839) + assertMaxFeeRate(anchorChannel, 0.001, 1000000, 435941) assertMaxFeeRate(anchorChannel, 0.001, 300000, 300000) - assertMaxFeeRate(anchorChannel, 0.000001, 700, 444) + assertMaxFeeRate(anchorChannel, 0.000001, 700, 435) assertMaxFeeRate( anchorChannel, 0.0000001, 1000000, chainfee.FeePerKwFloor, ) From 67125956183ac4bc38421100bb1c81895e96a928 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 8 Sep 2021 13:21:12 +0200 Subject: [PATCH 2/7] lnwallet: fix validateFeeRate The validateFeeRate function uses the availableBalance function to get the current spendable balance of a channel, adds the old fee and then ensures that the new fee is not larger than the amount we have available to spend. This commit also removes the local reserve check in the validateFeeRate function since the balance returned from availableBalance already takes the local reserve into acccount. --- lnwallet/channel.go | 16 ++++---------- lnwallet/channel_test.go | 47 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index ea3a9ca56..a4e9c36f5 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 diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index a490b9502..e626a22db 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" @@ -8014,6 +8016,25 @@ func TestChannelMaxFeeRate(t *testing.T) { "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, + anchorMax fee) bool { + + return func(ma maxAlloc, anchorMax fee) bool { + maxFeeRate := c.MaxFeeRate( + float64(ma), chainfee.SatPerKWeight(anchorMax), + ) + + return c.validateFeeRate(maxFeeRate) == nil + } } aliceChannel, _, cleanUp, err := CreateTestChannels( @@ -8024,6 +8045,10 @@ func TestChannelMaxFeeRate(t *testing.T) { } defer cleanUp() + 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) @@ -8038,6 +8063,10 @@ func TestChannelMaxFeeRate(t *testing.T) { } 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( @@ -8052,6 +8081,24 @@ func TestChannelMaxFeeRate(t *testing.T) { ) } +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 // received using chainfee.FeePerKwFloor as the initiator's fee rate. func TestChannelFeeRateFloor(t *testing.T) { From f667683e6cecf733a90a494cd08391b5a8142eed Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 23 Jun 2021 14:28:25 +0200 Subject: [PATCH 3/7] 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 | 38 +++++--- htlcswitch/link_test.go | 48 +++++++-- htlcswitch/mock.go | 9 +- lnwallet/channel.go | 79 +++++++++++++-- lnwallet/channel_test.go | 203 +++++++++++++++++++++++++++++++++------ 5 files changed, 317 insertions(+), 60 deletions(-) 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 From c01699853d36cf7c8ce1187ff4a7137dcadd90f8 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Fri, 25 Jun 2021 16:19:34 +0200 Subject: [PATCH 4/7] chainfee: add minFeeManager This commit adds a minFeeManager which holds a copy of minFeePerKW and updates this fee every few calls. --- lnwallet/chainfee/minfeemanager.go | 74 +++++++++++++++++++++++++ lnwallet/chainfee/minfeemanager_test.go | 54 ++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 lnwallet/chainfee/minfeemanager.go create mode 100644 lnwallet/chainfee/minfeemanager_test.go diff --git a/lnwallet/chainfee/minfeemanager.go b/lnwallet/chainfee/minfeemanager.go new file mode 100644 index 000000000..490b020e2 --- /dev/null +++ b/lnwallet/chainfee/minfeemanager.go @@ -0,0 +1,74 @@ +package chainfee + +import ( + "sync" + "time" +) + +// 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) +} From 55077d940433ae0d596d3ac6c3237864dd464af4 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 23 Jun 2021 14:55:39 +0200 Subject: [PATCH 5/7] chainfee: fetch new relay fee from bitcoind This commit adds a fetchMinMempoolFee function to the BitcoindEstimator that fetches the current min mempool fee from the bitcoind backend. The commit then also updates the BitcoindEstimator to use a minFeeManager for it's minFeeManager member and uses the fetchMinMempoolFee function to initialise this. --- lnwallet/chainfee/estimator.go | 69 ++++++++++++++++-------------- lnwallet/chainfee/minfeemanager.go | 2 + 2 files changed, 39 insertions(+), 32 deletions(-) diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index eee261488..3a8a2bfea 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -280,11 +280,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 +324,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 +410,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 +457,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 index 490b020e2..1f377d2cf 100644 --- a/lnwallet/chainfee/minfeemanager.go +++ b/lnwallet/chainfee/minfeemanager.go @@ -5,6 +5,8 @@ import ( "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. From ad2859c8638d81a3a8c2df109e29b5363ac7f129 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 23 Jun 2021 15:14:48 +0200 Subject: [PATCH 6/7] chainfee: fetch fresh relay fee for btcd This commit adds a function to the BtcdEstimator that fetches the current min relay fee from the btcd node. --- lnwallet/chainfee/estimator.go | 56 ++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index 3a8a2bfea..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", From f782df67681aefa44a2bd1652baf41d324bb9eb0 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 5 Aug 2021 14:45:58 +0200 Subject: [PATCH 7/7] docs: update 0.14 release notes --- docs/release-notes/release-notes-0.14.0.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/release-notes/release-notes-0.14.0.md b/docs/release-notes/release-notes-0.14.0.md index c9c079e65..d7666b467 100644 --- a/docs/release-notes/release-notes-0.14.0.md +++ b/docs/release-notes/release-notes-0.14.0.md @@ -332,6 +332,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)