sweep: replace feeRateForPreference with Estimate

This commit refactors the sweeper so the method `feeRateForPreference`
is now moved to `FeePreference`, which makes our following refactor
easier to handle.

server.go

@@ -1065,7 +1065,6 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 
 	s.sweeper = sweep.New(&sweep.UtxoSweeperConfig{
 		FeeEstimator:         cc.FeeEstimator,
-		DetermineFeePerKw:    sweep.DetermineFeePerKw,
 		GenSweepScript:       newSweepPkScriptGen(cc.Wallet),
 		Signer:               cc.Wallet.Cfg.Signer,
 		Wallet:               newSweeperWallet(cc.Wallet),

sweep/sweeper.go

@@ -43,10 +43,6 @@ var (
 	// for the configured max number of attempts.
 	ErrTooManyAttempts = errors.New("sweep failed after max attempts")
 
-	// 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")
-
 	// ErrFeePreferenceTooLow is returned when the fee preference gives a
 	// fee rate that's below the relay fee rate.
 	ErrFeePreferenceTooLow = errors.New("fee preference too low")
@@ -242,21 +238,12 @@ type UtxoSweeper struct {
 	currentHeight int32
 }
 
-// feeDeterminer defines an alias to the function signature of
-// `DetermineFeePerKw`.
-type feeDeterminer func(chainfee.Estimator,
-	FeePreference) (chainfee.SatPerKWeight, error)
-
 // UtxoSweeperConfig contains dependencies of UtxoSweeper.
 type UtxoSweeperConfig struct {
 	// GenSweepScript generates a P2WKH script belonging to the wallet where
 	// funds can be swept.
 	GenSweepScript func() ([]byte, error)
 
-	// DetermineFeePerKw determines the fee in sat/kw based on the given
-	// estimator and fee preference.
-	DetermineFeePerKw feeDeterminer
-
 	// FeeEstimator is used when crafting sweep transactions to estimate
 	// the necessary fee relative to the expected size of the sweep
 	// transaction.
@@ -446,7 +433,10 @@ func (s *UtxoSweeper) SweepInput(input input.Input,
 	}
 
 	// Ensure the client provided a sane fee preference.
-	if _, err := s.feeRateForPreference(params.Fee); err != nil {
+	_, err := params.Fee.Estimate(
+		s.cfg.FeeEstimator, s.cfg.MaxFeeRate.FeePerKWeight(),
+	)
+	if err != nil {
 		return nil, err
 	}
 
@@ -474,42 +464,6 @@ func (s *UtxoSweeper) SweepInput(input input.Input,
 	return sweeperInput.resultChan, nil
 }
 
-// feeRateForPreference returns a fee rate for the given fee preference. It
-// ensures that the fee rate respects the bounds of the UtxoSweeper.
-func (s *UtxoSweeper) feeRateForPreference(
-	feePreference FeePreference) (chainfee.SatPerKWeight, error) {
-
-	// Ensure a type of fee preference is specified to prevent using a
-	// default below.
-	if feePreference.FeeRate == 0 && feePreference.ConfTarget == 0 {
-		return 0, ErrNoFeePreference
-	}
-
-	feeRate, err := s.cfg.DetermineFeePerKw(
-		s.cfg.FeeEstimator, feePreference,
-	)
-	if err != nil {
-		return 0, err
-	}
-
-	if feeRate < s.relayFeeRate {
-		return 0, fmt.Errorf("%w: got %v, minimum is %v",
-			ErrFeePreferenceTooLow, feeRate, s.relayFeeRate)
-	}
-
-	// If the estimated fee rate is above the maximum allowed fee rate,
-	// default to the max fee rate.
-	if feeRate > s.cfg.MaxFeeRate.FeePerKWeight() {
-		log.Warnf("Estimated fee rate %v exceeds max allowed fee "+
-			"rate %v, using max fee rate instead", feeRate,
-			s.cfg.MaxFeeRate.FeePerKWeight())
-
-		return s.cfg.MaxFeeRate.FeePerKWeight(), nil
-	}
-
-	return feeRate, nil
-}
-
 // removeConflictSweepDescendants removes any transactions from the wallet that
 // spend outputs included in the passed outpoint set. This needs to be done in
 // cases where we're not the only ones that can sweep an output, but there may
@@ -829,7 +783,9 @@ func (s *UtxoSweeper) clusterByLockTime(inputs pendingInputs) ([]inputCluster,
 		// returned, we'll skip sweeping this input for this round of
 		// cluster creation and retry it when we create the clusters
 		// from the pending inputs again.
-		feeRate, err := s.feeRateForPreference(input.params.Fee)
+		feeRate, err := input.params.Fee.Estimate(
+			s.cfg.FeeEstimator, s.cfg.MaxFeeRate.FeePerKWeight(),
+		)
 		if err != nil {
 			log.Warnf("Skipping input %v: %v", op, err)
 			continue
@@ -879,7 +835,9 @@ func (s *UtxoSweeper) clusterBySweepFeeRate(inputs pendingInputs) []inputCluster
 	// First, we'll group together all inputs with similar fee rates. This
 	// is done by determining the fee rate bucket they should belong in.
 	for op, input := range inputs {
-		feeRate, err := s.feeRateForPreference(input.params.Fee)
+		feeRate, err := input.params.Fee.Estimate(
+			s.cfg.FeeEstimator, s.cfg.MaxFeeRate.FeePerKWeight(),
+		)
 		if err != nil {
 			log.Warnf("Skipping input %v: %v", op, err)
 			continue
@@ -1373,7 +1331,10 @@ func (s *UtxoSweeper) UpdateParams(input wire.OutPoint,
 	params ParamsUpdate) (chan Result, error) {
 
 	// Ensure the client provided a sane fee preference.
-	if _, err := s.feeRateForPreference(params.Fee); err != nil {
+	_, err := params.Fee.Estimate(
+		s.cfg.FeeEstimator, s.cfg.MaxFeeRate.FeePerKWeight(),
+	)
+	if err != nil {
 		return nil, err
 	}
 
@@ -1467,7 +1428,7 @@ func (s *UtxoSweeper) handleUpdateReq(req *updateReq) (
 func (s *UtxoSweeper) CreateSweepTx(inputs []input.Input,
 	feePref FeePreference) (*wire.MsgTx, error) {
 
-	feePerKw, err := s.cfg.DetermineFeePerKw(s.cfg.FeeEstimator, feePref)
+	feePerKw, err := DetermineFeePerKw(s.cfg.FeeEstimator, feePref)
 	if err != nil {
 		return nil, err
 	}

sweep/sweeper_test.go

@@ -1,7 +1,6 @@
 package sweep
 
 import (
-	"errors"
 	"os"
 	"reflect"
 	"runtime/pprof"
@@ -151,7 +150,6 @@ func createSweeperTestContext(t *testing.T) *sweeperTestContext {
 		},
 		MaxFeeRate:        DefaultMaxFeeRate,
 		FeeRateBucketSize: DefaultFeeRateBucketSize,
-		DetermineFeePerKw: DetermineFeePerKw,
 	})
 
 	ctx.sweeper.Start()
@@ -2137,124 +2135,6 @@ func TestSweeperShutdownHandling(t *testing.T) {
 	require.Error(t, err)
 }
 
-// TestFeeRateForPreference checks `feeRateForPreference` works as expected.
-func TestFeeRateForPreference(t *testing.T) {
-	t.Parallel()
-
-	dummyErr := errors.New("dummy")
-
-	// Create a test sweeper.
-	s := New(&UtxoSweeperConfig{})
-
-	// errFeeFunc is a mock over DetermineFeePerKw that always return the
-	// above dummy error.
-	errFeeFunc := func(_ chainfee.Estimator, _ FeePreference) (
-		chainfee.SatPerKWeight, error) {
-
-		return 0, dummyErr
-	}
-
-	// Set the relay fee rate to be 1 sat/kw.
-	s.relayFeeRate = 1
-
-	// smallFeeFunc is a mock over DetermineFeePerKw that always return a
-	// fee rate that's below the relayFeeRate.
-	smallFeeFunc := func(_ chainfee.Estimator, _ FeePreference) (
-		chainfee.SatPerKWeight, error) {
-
-		return s.relayFeeRate - 1, nil
-	}
-
-	// Set the max fee rate to be 1000 sat/vb.
-	s.cfg.MaxFeeRate = 1000
-
-	// largeFeeFunc is a mock over DetermineFeePerKw that always return a
-	// fee rate that's larger than the MaxFeeRate.
-	largeFeeFunc := func(_ chainfee.Estimator, _ FeePreference) (
-		chainfee.SatPerKWeight, error) {
-
-		return s.cfg.MaxFeeRate.FeePerKWeight() + 1, nil
-	}
-
-	// validFeeRate is used to test the success case.
-	validFeeRate := (s.cfg.MaxFeeRate.FeePerKWeight() + s.relayFeeRate) / 2
-
-	// normalFeeFunc is a mock over DetermineFeePerKw that always return a
-	// fee rate that's within the range.
-	normalFeeFunc := func(_ chainfee.Estimator, _ FeePreference) (
-		chainfee.SatPerKWeight, error) {
-
-		return validFeeRate, nil
-	}
-
-	testCases := []struct {
-		name              string
-		feePref           FeePreference
-		determineFeePerKw feeDeterminer
-		expectedFeeRate   chainfee.SatPerKWeight
-		expectedErr       error
-	}{
-		{
-			// When the fee preference is empty, we should see an
-			// error.
-			name:        "empty fee preference",
-			feePref:     FeePreference{},
-			expectedErr: ErrNoFeePreference,
-		},
-		{
-			// When an error is returned from the fee determiner,
-			// we should return it.
-			name:              "error from DetermineFeePerKw",
-			feePref:           FeePreference{FeeRate: 1},
-			determineFeePerKw: errFeeFunc,
-			expectedErr:       dummyErr,
-		},
-		{
-			// When DetermineFeePerKw gives a too small value, we
-			// should return an error.
-			name:              "fee rate below relay fee rate",
-			feePref:           FeePreference{FeeRate: 1},
-			determineFeePerKw: smallFeeFunc,
-			expectedErr:       ErrFeePreferenceTooLow,
-		},
-		{
-			// When DetermineFeePerKw gives a too large value, we
-			// should cap it at the max fee rate.
-			name:              "fee rate above max fee rate",
-			feePref:           FeePreference{FeeRate: 1},
-			determineFeePerKw: largeFeeFunc,
-			expectedFeeRate:   s.cfg.MaxFeeRate.FeePerKWeight(),
-		},
-		{
-			// When DetermineFeePerKw gives a sane fee rate, we
-			// should return it without any error.
-			name:              "success",
-			feePref:           FeePreference{FeeRate: 1},
-			determineFeePerKw: normalFeeFunc,
-			expectedFeeRate:   validFeeRate,
-		},
-	}
-
-	//nolint:paralleltest
-	for _, tc := range testCases {
-		tc := tc
-
-		t.Run(tc.name, func(t *testing.T) {
-			// Attach the mocked method.
-			s.cfg.DetermineFeePerKw = tc.determineFeePerKw
-
-			// Call the function under test.
-			feerate, err := s.feeRateForPreference(tc.feePref)
-
-			// Assert the expected feerate.
-			require.Equal(t, tc.expectedFeeRate, feerate)
-
-			// Assert the expected error.
-			require.ErrorIs(t, err, tc.expectedErr)
-		})
-	}
-}
-
 // TestClusterByLockTime tests the method clusterByLockTime works as expected.
 func TestClusterByLockTime(t *testing.T) {
 	t.Parallel()
@@ -2344,13 +2224,7 @@ func TestClusterByLockTime(t *testing.T) {
 	// DetermineFeePerKw that always return the testing fee rate. This
 	// mocked method is then attached to the sweeper.
 	applyFeeRate := func(feeRate chainfee.SatPerKWeight) {
-		mockFeeFunc := func(_ chainfee.Estimator, _ FeePreference) (
+		// TODO(yy): fix the test here.
-			chainfee.SatPerKWeight, error) {
-
-			return feeRate, nil
-		}
-
-		s.cfg.DetermineFeePerKw = mockFeeFunc
 	}
 
 	testCases := []struct {

sweep/walletsweep.go

@@ -1,6 +1,7 @@
 package sweep
 
 import (
+	"errors"
 	"fmt"
 	"math"
 	"time"
@@ -22,6 +23,12 @@ const (
 	defaultNumBlocksEstimate = 6
 )
 
+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")
+)
+
 // FeePreference allows callers to express their time value for inclusion of a
 // transaction into a block via either a confirmation target, or a fee rate.
 type FeePreference struct {
@@ -42,10 +49,52 @@ func (p FeePreference) String() string {
 	return p.FeeRate.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.
+func (f FeePreference) Estimate(estimator chainfee.Estimator,
+	maxFeeRate chainfee.SatPerKWeight) (chainfee.SatPerKWeight, error) {
+
+	// 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 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 {
+		log.Warnf("Estimated fee rate %v exceeds max allowed fee "+
+			"rate %v, using max fee rate instead", feeRate,
+			maxFeeRate)
+
+		return maxFeeRate, nil
+	}
+
+	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) {