From 6ff6c8615525562f64b6ba6f0e66773d066e03af Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 3 Nov 2023 18:48:09 +0800 Subject: [PATCH] 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 | 1 + lnrpc/rpc_utils.go | 12 +-- lnwallet/chainfee/mocks.go | 51 ++++++++- rpcserver.go | 12 ++- sweep/fee_estimator_mock_test.go | 2 + sweep/sweeper.go | 4 +- sweep/walletsweep.go | 149 ++++++++++---------------- sweep/walletsweep_test.go | 176 ++++++++++++++++++------------- 8 files changed, 228 insertions(+), 179 deletions(-) diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index 2d4e88a74..a0f38c74f 100644 --- a/htlcswitch/mock.go +++ b/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 diff --git a/lnrpc/rpc_utils.go b/lnrpc/rpc_utils.go index 9d9dea320..1cc4b1754 100644 --- a/lnrpc/rpc_utils.go +++ b/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{ - ConfTarget: targetConf, - FeeRate: satPerKw, - }, - ) + 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 } diff --git a/lnwallet/chainfee/mocks.go b/lnwallet/chainfee/mocks.go index 03d40e11e..e14340d91 100644 --- a/lnwallet/chainfee/mocks.go +++ b/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) +} diff --git a/rpcserver.go b/rpcserver.go index 517bca41e..8203744a6 100644 --- a/rpcserver.go +++ b/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{ - ConfTarget: uint32(target), - }, - ) + 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 } diff --git a/sweep/fee_estimator_mock_test.go b/sweep/fee_estimator_mock_test.go index 4ca89f0c5..ab6dcdfd5 100644 --- a/sweep/fee_estimator_mock_test.go +++ b/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 diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 109324cbb..234ff17ef 100644 --- a/sweep/sweeper.go +++ b/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 } diff --git a/sweep/walletsweep.go b/sweep/walletsweep.go index e32696793..c6ee39d3d 100644 --- a/sweep/walletsweep.go +++ b/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 { diff --git a/sweep/walletsweep_test.go b/sweep/walletsweep_test.go index f36e472fa..44e236b15 100644 --- a/sweep/walletsweep_test.go +++ b/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. - 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 + name string + setupMocker func() + feePref FeePreference + expectedFeeRate chainfee.SatPerKWeight + expectedErr error }{ - // A fee rate below the floor should error out. { - feePref: FeePreference{ - FeeRate: chainfee.SatPerKWeight(99), - }, - fail: true, + // When the fee preference is empty, we should see an + // error. + name: "empty fee preference", + feePref: FeePreference{}, + expectedErr: ErrNoFeePreference, }, - - // A fee rate below the relay fee should error out. { + // When the fee preference has conflicts, we should see + // an error. + name: "conflict fee preference", feePref: FeePreference{ - FeeRate: chainfee.SatPerKWeight(299), + FeeRate: validFeeRate, + ConfTarget: conf, }, - fail: true, + expectedErr: ErrFeePreferenceConflict, }, - - // A fee rate above the floor, should pass through and return - // the target fee rate. { - feePref: FeePreference{ - FeeRate: 900, + // 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() }, - fee: 900, + feePref: FeePreference{ConfTarget: conf}, + expectedErr: dummyErr, }, - - // A specified confirmation target should cause the function to - // query the estimator which will return our value specified - // above. { - feePref: FeePreference{ - ConfTarget: 50, + // 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() }, - fee: 300, + feePref: FeePreference{FeeRate: relayFeeRate - 1}, + expectedErr: ErrFeePreferenceTooLow, }, - - // If the caller doesn't specify any values at all, then we - // should query for the default conf target. { - feePref: FeePreference{}, - fee: 1000, - }, - - // Both conf target and fee rate are set, we should return with - // an error. - { - feePref: FeePreference{ - ConfTarget: 50, - FeeRate: 90000, + // 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() }, - fee: 300, - fail: true, + 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, - ) - switch { - case testCase.fail && err != nil: - continue - case testCase.fail && err == nil: - t.Fatalf("expected failure for #%v", i) + for _, tc := range testCases { + tc := tc - case !testCase.fail && err != nil: - t.Fatalf("unable to estimate fee; %v", err) - } + t.Run(tc.name, func(t *testing.T) { + // Setup the mockers if specified. + if tc.setupMocker != nil { + tc.setupMocker() + } - if targetFee != testCase.fee { - t.Fatalf("#%v: wrong fee: expected %v got %v", i, - testCase.fee, targetFee) - } + // Call the function under test. + feerate, err := tc.feePref.Estimate( + estimator, maxFeeRate, + ) + + // Assert the expected error. + require.ErrorIs(t, err, tc.expectedErr) + + // Assert the expected feerate. + require.Equal(t, tc.expectedFeeRate, feerate) + + // Assert the mockers. + estimator.AssertExpectations(t) + }) } }