multi: merge DetermineFeePerKw and Estimate

This commit moves `DetermineFeePerKw` into the `Estimate` method on
`FeePreference`. A few callsites previously calling `DetermineFeePerKw`
without the max fee rate is now also temporarily fixed by forcing them
to use `Estimate` with the default sweeper max fee rate.

htlcswitch/mock.go

@@ -81,6 +81,7 @@ func (m *mockPreimageCache) SubscribeUpdates(
 	return nil, nil
 }
 
+// TODO(yy): replace it with chainfee.MockEstimator.
 type mockFeeEstimator struct {
 	byteFeeIn chan chainfee.SatPerKWeight
 	relayFee  chan chainfee.SatPerKWeight

lnrpc/rpc_utils.go

@@ -222,12 +222,12 @@ func CalculateFeeRate(satPerByte, satPerVByte uint64, targetConf uint32,
 
 	// Based on the passed fee related parameters, we'll determine an
 	// appropriate fee rate for this transaction.
-	feeRate, err := sweep.DetermineFeePerKw(
-		estimator, sweep.FeePreference{
+	feePref := sweep.FeePreference{
 		ConfTarget: targetConf,
 		FeeRate:    satPerKw,
-		},
-	)
+	}
+	// TODO(yy): need to pass the configured max fee here.
+	feeRate, err := feePref.Estimate(estimator, 0)
 	if err != nil {
 		return feeRate, err
 	}

lnwallet/chainfee/mocks.go

@@ -1,6 +1,8 @@
 package chainfee
 
-import "github.com/stretchr/testify/mock"
+import (
+	"github.com/stretchr/testify/mock"
+)
 
 type mockFeeSource struct {
 	mock.Mock
@@ -15,3 +17,50 @@ func (m *mockFeeSource) GetFeeMap() (map[uint32]uint32, error) {
 
 	return args.Get(0).(map[uint32]uint32), args.Error(1)
 }
+
+// MockEstimator implements the `Estimator` interface and is used by
+// other packages for mock testing.
+type MockEstimator struct {
+	mock.Mock
+}
+
+// Compile time assertion that MockEstimator implements Estimator.
+var _ Estimator = (*MockEstimator)(nil)
+
+// EstimateFeePerKW takes in a target for the number of blocks until an initial
+// confirmation and returns the estimated fee expressed in sat/kw.
+func (m *MockEstimator) EstimateFeePerKW(
+	numBlocks uint32) (SatPerKWeight, error) {
+
+	args := m.Called(numBlocks)
+
+	if args.Get(0) == nil {
+		return 0, args.Error(1)
+	}
+
+	return args.Get(0).(SatPerKWeight), args.Error(1)
+}
+
+// Start signals the Estimator to start any processes or goroutines it needs to
+// perform its duty.
+func (m *MockEstimator) Start() error {
+	args := m.Called()
+
+	return args.Error(0)
+}
+
+// Stop stops any spawned goroutines and cleans up the resources used by the
+// fee estimator.
+func (m *MockEstimator) Stop() error {
+	args := m.Called()
+
+	return args.Error(0)
+}
+
+// RelayFeePerKW returns the minimum fee rate required for transactions to be
+// relayed. This is also the basis for calculation of the dust limit.
+func (m *MockEstimator) RelayFeePerKW() SatPerKWeight {
+	args := m.Called()
+
+	return args.Get(0).(SatPerKWeight)
+}

rpcserver.go

@@ -1175,11 +1175,13 @@ func (r *rpcServer) EstimateFee(ctx context.Context,
 	// Query the fee estimator for the fee rate for the given confirmation
 	// target.
 	target := in.TargetConf
-	feePerKw, err := sweep.DetermineFeePerKw(
-		r.server.cc.FeeEstimator, sweep.FeePreference{
+	feePref := sweep.FeePreference{
 		ConfTarget: uint32(target),
-		},
-	)
+	}
+
+	// Since we are providing a fee estimation as an RPC response, there's
+	// no need to set a max feerate here, so we use 0.
+	feePerKw, err := feePref.Estimate(r.server.cc.FeeEstimator, 0)
 	if err != nil {
 		return nil, err
 	}

sweep/fee_estimator_mock_test.go

@@ -9,6 +9,8 @@ import (
 // mockFeeEstimator implements a mock fee estimator. It closely resembles
 // lnwallet.StaticFeeEstimator with the addition that fees can be changed for
 // testing purposes in a thread safe manner.
+//
+// TODO(yy): replace it with chainfee.MockEstimator once it's merged.
 type mockFeeEstimator struct {
 	feePerKW chainfee.SatPerKWeight
 

sweep/sweeper.go

@@ -1428,7 +1428,9 @@ func (s *UtxoSweeper) handleUpdateReq(req *updateReq) (
 func (s *UtxoSweeper) CreateSweepTx(inputs []input.Input,
 	feePref FeePreference) (*wire.MsgTx, error) {
 
-	feePerKw, err := DetermineFeePerKw(s.cfg.FeeEstimator, feePref)
+	feePerKw, err := feePref.Estimate(
+		s.cfg.FeeEstimator, s.cfg.MaxFeeRate.FeePerKWeight(),
+	)
 	if err != nil {
 		return nil, err
 	}

sweep/walletsweep.go

@@ -27,6 +27,10 @@ var (
 	// ErrNoFeePreference is returned when we attempt to satisfy a sweep
 	// request from a client whom did not specify a fee preference.
 	ErrNoFeePreference = errors.New("no fee preference specified")
+
+	// ErrFeePreferenceConflict is returned when both a fee rate and a conf
+	// target is set for a fee preference.
+	ErrFeePreferenceConflict = errors.New("fee preference conflict")
 )
 
 // FeePreference allows callers to express their time value for inclusion of a
@@ -50,35 +54,70 @@ func (p FeePreference) String() string {
 }
 
 // Estimate returns a fee rate for the given fee preference. It ensures that
-// the fee rate respects the bounds of the relay fee and the specified max fee
-// rates.
-//
-// TODO(yy): add tests.
+// the fee rate respects the bounds of the relay fee and the max fee rates, if
+// specified.
 func (f FeePreference) Estimate(estimator chainfee.Estimator,
 	maxFeeRate chainfee.SatPerKWeight) (chainfee.SatPerKWeight, error) {
 
+	var (
+		feeRate chainfee.SatPerKWeight
+		err     error
+	)
+
+	switch {
+	// Ensure a type of fee preference is specified to prevent using a
+	// default below.
+	case f.FeeRate == 0 && f.ConfTarget == 0:
+		return 0, ErrNoFeePreference
+
+	// If both values are set, then we'll return an error as we require a
+	// strict directive.
+	case f.FeeRate != 0 && f.ConfTarget != 0:
+		return 0, ErrFeePreferenceConflict
+
+	// If the target number of confirmations is set, then we'll use that to
+	// consult our fee estimator for an adequate fee.
+	case f.ConfTarget != 0:
+		feeRate, err = estimator.EstimateFeePerKW((f.ConfTarget))
+		if err != nil {
+			return 0, fmt.Errorf("unable to query fee "+
+				"estimator: %w", err)
+		}
+
+	// If a manual sat/kw fee rate is set, then we'll use that directly.
+	// We'll need to convert it to sat/kw as this is what we use
+	// internally.
+	case f.FeeRate != 0:
+		feeRate = f.FeeRate
+
+		// Because the user can specify 1 sat/vByte on the RPC
+		// interface, which corresponds to 250 sat/kw, we need to bump
+		// that to the minimum "safe" fee rate which is 253 sat/kw.
+		if feeRate == chainfee.AbsoluteFeePerKwFloor {
+			log.Infof("Manual fee rate input of %d sat/kw is "+
+				"too low, using %d sat/kw instead", feeRate,
+				chainfee.FeePerKwFloor)
+
+			feeRate = chainfee.FeePerKwFloor
+		}
+	}
+
 	// Get the relay fee as the min fee rate.
 	minFeeRate := estimator.RelayFeePerKW()
 
-	// Ensure a type of fee preference is specified to prevent using a
-	// default below.
-	if f.FeeRate == 0 && f.ConfTarget == 0 {
-		return 0, ErrNoFeePreference
-	}
-
-	feeRate, err := DetermineFeePerKw(estimator, f)
-	if err != nil {
-		return 0, err
-	}
-
+	// If that bumped fee rate of at least 253 sat/kw is still lower than
+	// the relay fee rate, we return an error to let the user know. Note
+	// that "Relay fee rate" may mean slightly different things depending
+	// on the backend. For bitcoind, it is effectively max(relay fee, min
+	// mempool fee).
 	if feeRate < minFeeRate {
 		return 0, fmt.Errorf("%w: got %v, minimum is %v",
 			ErrFeePreferenceTooLow, feeRate, minFeeRate)
 	}
 
-	// If the estimated fee rate is above the maximum allowed fee rate,
-	// default to the max fee rate.
-	if feeRate > maxFeeRate {
+	// If a maxFeeRate is specified and the estimated fee rate is above the
+	// maximum allowed fee rate, default to the max fee rate.
+	if maxFeeRate != 0 && feeRate > maxFeeRate {
 		log.Warnf("Estimated fee rate %v exceeds max allowed fee "+
 			"rate %v, using max fee rate instead", feeRate,
 			maxFeeRate)
@@ -89,80 +128,6 @@ func (f FeePreference) Estimate(estimator chainfee.Estimator,
 	return feeRate, nil
 }
 
-// DetermineFeePerKw will determine the fee in sat/kw that should be paid given
-// an estimator, a confirmation target, and a manual value for sat/byte. A
-// value is chosen based on the two free parameters as one, or both of them can
-// be zero.
-//
-// TODO(yy): move it into the above `Estimate`.
-func DetermineFeePerKw(feeEstimator chainfee.Estimator,
-	feePref FeePreference) (chainfee.SatPerKWeight, error) {
-
-	switch {
-	// If both values are set, then we'll return an error as we require a
-	// strict directive.
-	case feePref.FeeRate != 0 && feePref.ConfTarget != 0:
-		return 0, fmt.Errorf("only FeeRate or ConfTarget should " +
-			"be set for FeePreferences")
-
-	// If the target number of confirmations is set, then we'll use that to
-	// consult our fee estimator for an adequate fee.
-	case feePref.ConfTarget != 0:
-		feePerKw, err := feeEstimator.EstimateFeePerKW(
-			uint32(feePref.ConfTarget),
-		)
-		if err != nil {
-			return 0, fmt.Errorf("unable to query fee "+
-				"estimator: %v", err)
-		}
-
-		return feePerKw, nil
-
-	// If a manual sat/byte fee rate is set, then we'll use that directly.
-	// We'll need to convert it to sat/kw as this is what we use
-	// internally.
-	case feePref.FeeRate != 0:
-		feePerKW := feePref.FeeRate
-
-		// Because the user can specify 1 sat/vByte on the RPC
-		// interface, which corresponds to 250 sat/kw, we need to bump
-		// that to the minimum "safe" fee rate which is 253 sat/kw.
-		if feePerKW == chainfee.AbsoluteFeePerKwFloor {
-			log.Infof("Manual fee rate input of %d sat/kw is "+
-				"too low, using %d sat/kw instead", feePerKW,
-				chainfee.FeePerKwFloor)
-			feePerKW = chainfee.FeePerKwFloor
-		}
-
-		// If that bumped fee rate of at least 253 sat/kw is still lower
-		// than the relay fee rate, we return an error to let the user
-		// know. Note that "Relay fee rate" may mean slightly different
-		// things depending on the backend. For bitcoind, it is
-		// effectively max(relay fee, min mempool fee).
-		minFeePerKW := feeEstimator.RelayFeePerKW()
-		if feePerKW < minFeePerKW {
-			return 0, fmt.Errorf("manual fee rate input of %d "+
-				"sat/kw is too low to be accepted into the "+
-				"mempool or relayed to the network", feePerKW)
-		}
-
-		return feePerKW, nil
-
-	// Otherwise, we'll attempt a relaxed confirmation target for the
-	// transaction
-	default:
-		feePerKw, err := feeEstimator.EstimateFeePerKW(
-			defaultNumBlocksEstimate,
-		)
-		if err != nil {
-			return 0, fmt.Errorf("unable to query fee estimator: "+
-				"%v", err)
-		}
-
-		return feePerKw, nil
-	}
-}
-
 // UtxoSource is an interface that allows a caller to access a source of UTXOs
 // to use when crafting sweep transactions.
 type UtxoSource interface {

sweep/walletsweep_test.go

@@ -2,6 +2,7 @@ package sweep
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"testing"
 	"time"
@@ -17,106 +18,133 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// TestDetermineFeePerKw tests that given a fee preference, the
-// DetermineFeePerKw will properly map it to a concrete fee in sat/kw.
-func TestDetermineFeePerKw(t *testing.T) {
+// TestFeePreferenceEstimate checks `Estimate` method works as expected.
+func TestFeePreferenceEstimate(t *testing.T) {
 	t.Parallel()
 
-	defaultFee := chainfee.SatPerKWeight(999)
-	relayFee := chainfee.SatPerKWeight(300)
+	dummyErr := errors.New("dummy")
 
-	feeEstimator := newMockFeeEstimator(defaultFee, relayFee)
+	const (
+		// Set the relay fee rate to be 10 sat/kw.
+		relayFeeRate = 10
 
-	// We'll populate two items in the internal map which is used to query
-	// a fee based on a confirmation target: the default conf target, and
-	// an arbitrary conf target. We'll ensure below that both of these are
-	// properly
-	feeEstimator.blocksToFee[50] = 300
-	feeEstimator.blocksToFee[defaultNumBlocksEstimate] = 1000
+		// Set the max fee rate to be 1000 sat/vb.
+		maxFeeRate = 1000
+
+		// Create a valid fee rate to test the success case.
+		validFeeRate = (relayFeeRate + maxFeeRate) / 2
+
+		// Set the test conf target to be 1.
+		conf uint32 = 1
+	)
+
+	// Create a mock fee estimator.
+	estimator := &chainfee.MockEstimator{}
 
 	testCases := []struct {
-		// feePref is the target fee preference for this case.
+		name            string
+		setupMocker     func()
 		feePref         FeePreference
-
-		// fee is the value the DetermineFeePerKw should return given
-		// the FeePreference above
-		fee chainfee.SatPerKWeight
-
-		// fail determines if this test case should fail or not.
-		fail bool
+		expectedFeeRate chainfee.SatPerKWeight
+		expectedErr     error
 	}{
-		// A fee rate below the floor should error out.
-		{
-			feePref: FeePreference{
-				FeeRate: chainfee.SatPerKWeight(99),
-			},
-			fail: true,
-		},
-
-		// A fee rate below the relay fee should error out.
-		{
-			feePref: FeePreference{
-				FeeRate: chainfee.SatPerKWeight(299),
-			},
-			fail: true,
-		},
-
-		// A fee rate above the floor, should pass through and return
-		// the target fee rate.
-		{
-			feePref: FeePreference{
-				FeeRate: 900,
-			},
-			fee: 900,
-		},
-
-		// A specified confirmation target should cause the function to
-		// query the estimator which will return our value specified
-		// above.
-		{
-			feePref: FeePreference{
-				ConfTarget: 50,
-			},
-			fee: 300,
-		},
-
-		// If the caller doesn't specify any values at all, then we
-		// should query for the default conf target.
 		{
+			// When the fee preference is empty, we should see an
+			// error.
+			name:        "empty fee preference",
 			feePref:     FeePreference{},
-			fee:     1000,
+			expectedErr: ErrNoFeePreference,
 		},
-
-		// Both conf target and fee rate are set, we should return with
-		// an error.
 		{
+			// When the fee preference has conflicts, we should see
+			// an error.
+			name: "conflict fee preference",
 			feePref: FeePreference{
-				ConfTarget: 50,
-				FeeRate:    90000,
+				FeeRate:    validFeeRate,
+				ConfTarget: conf,
 			},
-			fee:  300,
-			fail: true,
+			expectedErr: ErrFeePreferenceConflict,
+		},
+		{
+			// When an error is returned from the fee estimator, we
+			// should return it.
+			name: "error from Estimator",
+			setupMocker: func() {
+				estimator.On("EstimateFeePerKW", conf).Return(
+					chainfee.SatPerKWeight(0), dummyErr,
+				).Once()
+			},
+			feePref:     FeePreference{ConfTarget: conf},
+			expectedErr: dummyErr,
+		},
+		{
+			// When FeePreference uses a too small value, we should
+			// return an error.
+			name: "fee rate below relay fee rate",
+			setupMocker: func() {
+				// Mock the relay fee rate.
+				estimator.On("RelayFeePerKW").Return(
+					chainfee.SatPerKWeight(relayFeeRate),
+				).Once()
+			},
+			feePref:     FeePreference{FeeRate: relayFeeRate - 1},
+			expectedErr: ErrFeePreferenceTooLow,
+		},
+		{
+			// When FeePreference gives a too large value, we
+			// should cap it at the max fee rate.
+			name: "fee rate above max fee rate",
+			setupMocker: func() {
+				// Mock the relay fee rate.
+				estimator.On("RelayFeePerKW").Return(
+					chainfee.SatPerKWeight(relayFeeRate),
+				).Once()
+			},
+			feePref:         FeePreference{FeeRate: maxFeeRate + 1},
+			expectedFeeRate: maxFeeRate,
+		},
+		{
+			// When Estimator gives a sane fee rate, we should
+			// return it without any error.
+			name: "success",
+			setupMocker: func() {
+				estimator.On("EstimateFeePerKW", conf).Return(
+					chainfee.SatPerKWeight(validFeeRate),
+					nil).Once()
+
+				// Mock the relay fee rate.
+				estimator.On("RelayFeePerKW").Return(
+					chainfee.SatPerKWeight(relayFeeRate),
+				).Once()
+			},
+			feePref:         FeePreference{ConfTarget: conf},
+			expectedFeeRate: validFeeRate,
 		},
 	}
-	for i, testCase := range testCases {
-		targetFee, err := DetermineFeePerKw(
-			feeEstimator, testCase.feePref,
+
+	for _, tc := range testCases {
+		tc := tc
+
+		t.Run(tc.name, func(t *testing.T) {
+			// Setup the mockers if specified.
+			if tc.setupMocker != nil {
+				tc.setupMocker()
+			}
+
+			// Call the function under test.
+			feerate, err := tc.feePref.Estimate(
+				estimator, maxFeeRate,
 			)
-		switch {
-		case testCase.fail && err != nil:
-			continue
 
-		case testCase.fail && err == nil:
-			t.Fatalf("expected failure for #%v", i)
+			// Assert the expected error.
+			require.ErrorIs(t, err, tc.expectedErr)
 
-		case !testCase.fail && err != nil:
-			t.Fatalf("unable to estimate fee; %v", err)
-		}
+			// Assert the expected feerate.
+			require.Equal(t, tc.expectedFeeRate, feerate)
 
-		if targetFee != testCase.fee {
-			t.Fatalf("#%v: wrong fee: expected %v got %v", i,
-				testCase.fee, targetFee)
-		}
+			// Assert the mockers.
+			estimator.AssertExpectations(t)
+		})
 	}
 }