From 655ce002203923196fa0b09e1d470682253d7b41 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 7 Dec 2021 19:51:16 +0800 Subject: [PATCH 1/3] chainfee: handle conf target not found in cache --- lnwallet/chainfee/estimator.go | 92 ++++++++++++++++++++++++----- lnwallet/chainfee/estimator_test.go | 61 +++++++++++++++++++ 2 files changed, 138 insertions(+), 15 deletions(-) diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index 7439c528a..0eedee64a 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -2,8 +2,10 @@ package chainfee import ( "encoding/json" + "errors" "fmt" "io" + "math" prand "math/rand" "net" "net/http" @@ -35,6 +37,15 @@ const ( maxFeeUpdateTimeout = 20 * time.Minute ) +var ( + // errNoFeeRateFound is used when a given conf target cannot be found + // from the fee estimator. + errNoFeeRateFound = errors.New("no fee estimation for block target") + + // errEmptyCache is used when the fee rate cache is empty. + errEmptyCache = errors.New("fee rate cache is empty") +) + // Estimator provides the ability to estimate on-chain transaction fees for // various combinations of transaction sizes and desired confirmation time // (measured by number of blocks). @@ -579,7 +590,9 @@ func NewWebAPIEstimator(api WebAPIFeeSource, noCache bool) *WebAPIEstimator { // confirmation and returns the estimated fee expressed in sat/kw. // // NOTE: This method is part of the Estimator interface. -func (w *WebAPIEstimator) EstimateFeePerKW(numBlocks uint32) (SatPerKWeight, error) { +func (w *WebAPIEstimator) EstimateFeePerKW(numBlocks uint32) ( + SatPerKWeight, error) { + if numBlocks > maxBlockTarget { numBlocks = maxBlockTarget } else if numBlocks < minBlockTarget { @@ -593,8 +606,12 @@ func (w *WebAPIEstimator) EstimateFeePerKW(numBlocks uint32) (SatPerKWeight, err } feePerKb, err := w.getCachedFee(numBlocks) + + // If the estimator returns an error, a zero value fee rate will be + // returned. We will log the error and return the fall back fee rate + // instead. if err != nil { - return 0, err + log.Errorf("unable to query estimator: %v", err) } // If the result is too low, then we'll clamp it to our current fee @@ -672,31 +689,76 @@ func (w *WebAPIEstimator) randomFeeUpdateTimeout() time.Duration { return time.Duration(prand.Int63n(upper-lower) + lower) } -// getCachedFee takes in a target for the number of blocks until an initial -// confirmation and returns an estimated fee (if one was returned by the API). If -// the fee was not previously cached, we cache it here. +// getCachedFee takes a conf target and returns the cached fee rate. When the +// fee rate cannot be found, it will search the cache by decrementing the conf +// target until a fee rate is found. If still not found, it will return the fee +// rate of the minimum conf target cached, in other words, the most expensive +// fee rate it knows of. func (w *WebAPIEstimator) getCachedFee(numBlocks uint32) (uint32, error) { w.feesMtx.Lock() defer w.feesMtx.Unlock() - // Search our cached fees for the desired block target. If the target is - // not cached, then attempt to extrapolate it from the next lowest target - // that *is* cached. If we successfully extrapolate, then cache the - // target's fee. + // If the cache is empty, return an error. + if len(w.feeByBlockTarget) == 0 { + return 0, fmt.Errorf("web API error: %w", errEmptyCache) + } + + // Search the conf target from the cache. We expect a query to the web + // API has been made and the result has been cached at this point. + fee, ok := w.feeByBlockTarget[numBlocks] + + // If the conf target can be found, exit early. + if ok { + return fee, nil + } + + // The conf target cannot be found. We will first search the cache + // using a lower conf target. This is a conservative approach as the + // fee rate returned will be larger than what's requested. for target := numBlocks; target >= minBlockTarget; target-- { fee, ok := w.feeByBlockTarget[target] if !ok { continue } - _, ok = w.feeByBlockTarget[numBlocks] - if !ok { - w.feeByBlockTarget[numBlocks] = fee - } + log.Warnf("Web API does not have a fee rate for target=%d, "+ + "using the fee rate for target=%d instead", + numBlocks, target) + + // Return the fee rate found, which will be more expensive than + // requested. We will not cache the fee rate here in the hope + // that the web API will later populate this value. return fee, nil } - return 0, fmt.Errorf("web API does not include a fee estimation for "+ - "block target of %v", numBlocks) + + // There are no lower conf targets cached, which is likely when the + // requested conf target is 1. We will search the cache using a higher + // conf target, which gives a fee rate that's cheaper than requested. + // + // NOTE: we can only get here iff the requested conf target is smaller + // than the minimum conf target cached, so we return the minimum conf + // target from the cache. + minTargetCached := uint32(math.MaxUint32) + for target := range w.feeByBlockTarget { + if target < minTargetCached { + minTargetCached = target + } + } + + fee, ok = w.feeByBlockTarget[minTargetCached] + if !ok { + // We should never get here, just a vanity check. + return 0, fmt.Errorf("web API error: %w, conf target: %d", + errNoFeeRateFound, numBlocks) + } + + // Log an error instead of a warning as a cheaper fee rate may delay + // the confirmation for some important transactions. + log.Errorf("Web API does not have a fee rate for target=%d, "+ + "using the fee rate for target=%d instead", + numBlocks, minTargetCached) + + return fee, nil } // updateFeeEstimates re-queries the API for fresh fees and caches them. diff --git a/lnwallet/chainfee/estimator_test.go b/lnwallet/chainfee/estimator_test.go index 2900ff174..575b763a8 100644 --- a/lnwallet/chainfee/estimator_test.go +++ b/lnwallet/chainfee/estimator_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/btcsuite/btcutil" + "github.com/stretchr/testify/require" ) type mockSparseConfFeeSource struct { @@ -233,3 +234,63 @@ func TestWebAPIFeeEstimator(t *testing.T) { }) } } + +// TestGetCachedFee checks that the fee caching logic works as expected. +func TestGetCachedFee(t *testing.T) { + target := uint32(2) + fee := uint32(100) + + // Create a dummy estimator without WebAPIFeeSource. + estimator := NewWebAPIEstimator(nil, false) + + // When the cache is empty, an error should be returned. + cachedFee, err := estimator.getCachedFee(target) + require.Zero(t, cachedFee) + require.ErrorIs(t, err, errEmptyCache) + + // Store a fee rate inside the cache. + estimator.feeByBlockTarget[target] = fee + + testCases := []struct { + name string + confTarget uint32 + expectedFee uint32 + expectErr error + }{ + { + // When the target is cached, return it. + name: "return cached fee", + confTarget: target, + expectedFee: fee, + expectErr: nil, + }, + { + // When the target is not cached, return the next + // lowest target that's cached. + name: "return next cached fee", + confTarget: target + 1, + expectedFee: fee, + expectErr: nil, + }, + { + // When the target is not cached, and the next lowest + // target is not cached, return the nearest fee rate. + name: "return highest cached fee", + confTarget: target - 1, + expectedFee: fee, + expectErr: nil, + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + cachedFee, err := estimator.getCachedFee(tc.confTarget) + + require.Equal(t, tc.expectedFee, cachedFee) + require.ErrorIs(t, err, tc.expectErr) + }) + } + +} From 7e7fca78c4a2bda05dbec72ca7e69e9804c15014 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 7 Dec 2021 20:14:06 +0800 Subject: [PATCH 2/3] chainfee: update test TestWebAPIFeeEstimator --- lnwallet/chainfee/estimator_test.go | 53 ++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/lnwallet/chainfee/estimator_test.go b/lnwallet/chainfee/estimator_test.go index 575b763a8..0ac92a562 100644 --- a/lnwallet/chainfee/estimator_test.go +++ b/lnwallet/chainfee/estimator_test.go @@ -164,8 +164,9 @@ func TestSparseConfFeeSource(t *testing.T) { // as expected. func TestWebAPIFeeEstimator(t *testing.T) { t.Parallel() - feeFloor := uint32(FeePerKwFloor.FeePerKVByte()) + testFeeRate := feeFloor * 100 + testCases := []struct { name string target uint32 @@ -173,11 +174,41 @@ func TestWebAPIFeeEstimator(t *testing.T) { est uint32 err string }{ - {"target_below_min", 0, 12345, 12345, "too low, minimum"}, - {"target_w_too-low_fee", 10, 42, feeFloor, ""}, - {"API-omitted_target", 2, 0, 0, "web API does not include"}, - {"valid_target", 20, 54321, 54321, ""}, - {"valid_target_extrapolated_fee", 25, 0, 54321, ""}, + { + name: "target_below_min", + target: 0, + apiEst: 0, + est: 0, + err: "too low, minimum", + }, + { + name: "target_w_too-low_fee", + target: 100, + apiEst: 42, + est: feeFloor, + err: "", + }, + { + name: "API-omitted_target", + target: 2, + apiEst: 0, + est: testFeeRate, + err: "", + }, + { + name: "valid_target", + target: 20, + apiEst: testFeeRate, + est: testFeeRate, + err: "", + }, + { + name: "valid_target_extrapolated_fee", + target: 25, + apiEst: 0, + est: testFeeRate, + err: "", + }, } // Construct mock fee source for the Estimator to pull fees from. @@ -196,12 +227,10 @@ func TestWebAPIFeeEstimator(t *testing.T) { estimator := NewWebAPIEstimator(feeSource, false) // Test that requesting a fee when no fees have been cached fails. - _, err := estimator.EstimateFeePerKW(5) - if err == nil || - !strings.Contains(err.Error(), "web API does not include") { - - t.Fatalf("expected fee estimation to fail, instead got: %v", err) - } + feeRate, err := estimator.EstimateFeePerKW(5) + require.NoErrorf(t, err, "expected no error") + require.Equalf(t, FeePerKwFloor, feeRate, "expected fee rate floor "+ + "returned when no cached fee rate found") if err := estimator.Start(); err != nil { t.Fatalf("unable to start fee estimator, got: %v", err) From 7f3a146216ab8b610144ddc569344cd86449b359 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 7 Dec 2021 20:16:11 +0800 Subject: [PATCH 3/3] docs: update release note for fee rate fix --- docs/release-notes/release-notes-0.14.2.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/release-notes/release-notes-0.14.2.md b/docs/release-notes/release-notes-0.14.2.md index c97031984..62e660ac4 100644 --- a/docs/release-notes/release-notes-0.14.2.md +++ b/docs/release-notes/release-notes-0.14.2.md @@ -1,5 +1,10 @@ # Release Notes +## Bug Fixes + +* [Return the nearest known fee rate when a given conf target cannot be found + from Web API fee estimator.](https://github.com/lightningnetwork/lnd/pull/6062) + ## Build System * [Clean up Makefile by using go