Merge pull request #6062 from yyforyongyu/6056-fix-webapi

chainfee: return floor fee rate when a given target is not found

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

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.

lnwallet/chainfee/estimator_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/btcsuite/btcutil"
+	"github.com/stretchr/testify/require"
 )
 
 type mockSparseConfFeeSource struct {
@@ -163,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
@@ -172,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.
@@ -195,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)
@@ -233,3 +263,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)
+		})
+	}
+
+}