diff --git a/server.go b/server.go index 2c8b75af1..19d2c389c 100644 --- a/server.go +++ b/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), diff --git a/sweep/sweeper.go b/sweep/sweeper.go index e071bba25..109324cbb 100644 --- a/sweep/sweeper.go +++ b/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 } diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index de899f96f..94dba8558 100644 --- a/sweep/sweeper_test.go +++ b/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) ( - chainfee.SatPerKWeight, error) { - - return feeRate, nil - } - - s.cfg.DetermineFeePerKw = mockFeeFunc + // TODO(yy): fix the test here. } testCases := []struct { diff --git a/sweep/walletsweep.go b/sweep/walletsweep.go index 0ec301ba0..e32696793 100644 --- a/sweep/walletsweep.go +++ b/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) {