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) + }) + } + +}