From 6b366083ed8c92e132ecd57db51df4c57bf19113 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 26 Oct 2023 16:00:13 +0800 Subject: [PATCH 01/13] sweep: make sweeper block-driven instead of time-driven This commit changes the source that drives the state changes in the sweeper. Previously we used a ticker with default interval of 30s to trigger sweepings periodically. The assumption is, within this 30s we'd batch multiple inputs into one transaction to maximize profits. However, the efficacy of this batch is questionable. At a high level, we can put our inputs into two categories - one that's forced, and one that's not. For forced inputs, we should sweep them immediately as the force flag indicates they are very urgent, eg, CPFPing the force closing tx. For non-forced inputs, such as anchors or HTLCs with CLTV that's far away, we can wait to sweep them till a new block comes in and triggers the sweeping process. Eventually, all inputs will be deadline-aware, and the sweeper will consult our fee bumper about the most economical fee rate to be used for a given deadline. Since the deadlines here are blockstamp, it's also easier to manage them if the sweeper is also using blockstamp instead of timestamp. --- sweep/sweeper.go | 34 ++++++++++++++++++---------------- sweep/sweeper_test.go | 25 ++++++++++++++++++------- sweep/test_utils.go | 21 +++++++++++++++++++++ 3 files changed, 57 insertions(+), 23 deletions(-) diff --git a/sweep/sweeper.go b/sweep/sweeper.go index c3ce504ec0a..549095868fe 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -631,12 +631,6 @@ func (s *UtxoSweeper) collector(blockEpochs <-chan *chainntnfs.BlockEpoch) { return } - // Create a ticker based on the config duration. - ticker := time.NewTicker(s.cfg.TickerDuration) - defer ticker.Stop() - - log.Debugf("Sweep ticker started") - for { // Clean inputs, which will remove inputs that are swept, // failed, or excluded from the sweeper and return inputs that @@ -651,6 +645,13 @@ func (s *UtxoSweeper) collector(blockEpochs <-chan *chainntnfs.BlockEpoch) { case input := <-s.newInputs: s.handleNewInput(input) + // If this input is forced, we perform an sweep + // immediately. + if input.params.Force { + inputs = s.updateSweeperInputs() + s.sweepPendingInputs(inputs) + } + // A spend of one of our inputs is detected. Signal sweep // results to the caller(s). case spend := <-s.spendChan: @@ -670,14 +671,6 @@ func (s *UtxoSweeper) collector(blockEpochs <-chan *chainntnfs.BlockEpoch) { err: err, } - // The timer expires and we are going to (re)sweep. - case <-ticker.C: - log.Debugf("Sweep ticker ticks, attempt sweeping %d "+ - "inputs", len(inputs)) - - // Sweep the remaining pending inputs. - s.sweepPendingInputs(inputs) - // A new block comes in, update the bestHeight. // // TODO(yy): this is where we check our published transactions @@ -685,13 +678,22 @@ func (s *UtxoSweeper) collector(blockEpochs <-chan *chainntnfs.BlockEpoch) { // bumper to get an updated fee rate. case epoch, ok := <-blockEpochs: if !ok { + // We should stop the sweeper before stopping + // the chain service. Otherwise it indicates an + // error. + log.Error("Block epoch channel closed") + return } + // Update the sweeper to the best height. s.currentHeight = epoch.Height - log.Debugf("New block: height=%v, sha=%v", - epoch.Height, epoch.Hash) + log.Debugf("Received new block: height=%v, attempt "+ + "sweeping %d inputs", epoch.Height, len(inputs)) + + // Attempt to sweep any pending inputs. + s.sweepPendingInputs(inputs) case <-s.quit: return diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index 0168d9f08f8..0a914c4c6e7 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -43,7 +43,8 @@ type sweeperTestContext struct { backend *mockBackend store SweeperStore - publishChan chan wire.MsgTx + publishChan chan wire.MsgTx + currentHeight int32 } var ( @@ -125,12 +126,13 @@ func createSweeperTestContext(t *testing.T) *sweeperTestContext { ) ctx := &sweeperTestContext{ - notifier: notifier, - publishChan: backend.publishChan, - t: t, - estimator: estimator, - backend: backend, - store: store, + notifier: notifier, + publishChan: backend.publishChan, + t: t, + estimator: estimator, + backend: backend, + store: store, + currentHeight: mockChainHeight, } ctx.sweeper = New(&UtxoSweeperConfig{ @@ -214,6 +216,11 @@ func (ctx *sweeperTestContext) assertNoTx() { func (ctx *sweeperTestContext) receiveTx() wire.MsgTx { ctx.t.Helper() + + // Every time we want to receive a tx, we send a new block epoch to the + // sweeper to trigger a sweeping action. + ctx.notifier.NotifyEpochNonBlocking(ctx.currentHeight + 1) + var tx wire.MsgTx select { case tx = <-ctx.publishChan: @@ -1775,6 +1782,10 @@ func TestRequiredTxOuts(t *testing.T) { inputs[*op] = inp } + // Send a new block epoch to trigger the sweeper to + // sweep the inputs. + ctx.notifier.NotifyEpoch(ctx.sweeper.currentHeight + 1) + // Check the sweeps transactions, ensuring all inputs // are there, and all the locktimes are satisfied. var sweeps []*wire.MsgTx diff --git a/sweep/test_utils.go b/sweep/test_utils.go index e36b56a6b89..bd4b91bee05 100644 --- a/sweep/test_utils.go +++ b/sweep/test_utils.go @@ -40,6 +40,27 @@ func NewMockNotifier(t *testing.T) *MockNotifier { } } +// NotifyEpochNonBlocking simulates a new epoch arriving without blocking when +// the epochChan is not read. +func (m *MockNotifier) NotifyEpochNonBlocking(height int32) { + m.t.Helper() + + for epochChan, chanHeight := range m.epochChan { + // Only send notifications if the height is greater than the + // height the caller passed into the register call. + if chanHeight >= height { + continue + } + + log.Debugf("Notifying height %v to listener", height) + + select { + case epochChan <- &chainntnfs.BlockEpoch{Height: height}: + default: + } + } +} + // NotifyEpoch simulates a new epoch arriving. func (m *MockNotifier) NotifyEpoch(height int32) { m.t.Helper() From e9ca09308fb4280e17f97a0c25f3ade300ad60c2 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Mon, 30 Oct 2023 18:22:29 +0800 Subject: [PATCH 02/13] sweep: add new interface `Cluster` to manage grouping inputs This commit adds a new interface `Cluster` to manage cluster-level inputs grouping. This new interface replaces the `inputCluster` and will be futher refactored so the sweeper can use a much smaller coin selection lock. --- sweep/aggregator.go | 164 ++++++++++++++++++++++++++++++++++++++++-- sweep/mocks.go | 4 +- sweep/sweeper.go | 139 +++-------------------------------- sweep/sweeper_test.go | 109 ---------------------------- sweep/tx_input_set.go | 2 +- sweep/txgenerator.go | 106 --------------------------- 6 files changed, 171 insertions(+), 353 deletions(-) diff --git a/sweep/aggregator.go b/sweep/aggregator.go index 6797e3573df..dc75500e3e4 100644 --- a/sweep/aggregator.go +++ b/sweep/aggregator.go @@ -4,6 +4,8 @@ import ( "sort" "github.com/btcsuite/btcd/wire" + "github.com/lightningnetwork/lnd/input" + "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/lnwallet/chainfee" ) @@ -21,12 +23,152 @@ const ( DefaultFeeRateBucketSize = 10 ) +// inputSet is a set of inputs that can be used as the basis to generate a tx +// on. +type inputSet []input.Input + +// Cluster defines an interface that prepares inputs of a cluster to be grouped +// into a list of sets that can be used to create sweep transactions. +type Cluster interface { + // CreateInputSets goes through the cluster's inputs and constructs + // sets of inputs that can be used to generate a sensible transaction. + CreateInputSets(wallet Wallet, maxFeeRate chainfee.SatPerKWeight, + maxInputs int) ([]inputSet, error) + + // FeeRate returns the fee rate of the cluster. + FeeRate() chainfee.SatPerKWeight +} + +// Compile-time constraint to ensure inputCluster implements Cluster. +var _ Cluster = (*inputCluster)(nil) + +// inputCluster is a helper struct to gather a set of pending inputs that +// should be swept with the specified fee rate. +type inputCluster struct { + lockTime *uint32 + sweepFeeRate chainfee.SatPerKWeight + inputs pendingInputs +} + +// FeeRate returns the fee rate of the cluster. +func (c *inputCluster) FeeRate() chainfee.SatPerKWeight { + return c.sweepFeeRate +} + +// GroupInputs goes through the cluster's inputs and constructs sets of inputs +// that can be used to generate a sensible transaction. Each set contains up to +// the configured maximum number of inputs. Negative yield inputs are skipped. +// No input sets with a total value after fees below the dust limit are +// returned. +func (c *inputCluster) CreateInputSets( + wallet Wallet, maxFeeRate chainfee.SatPerKWeight, + maxInputs int) ([]inputSet, error) { + + // Turn the inputs into a slice so we can sort them. + inputList := make([]*pendingInput, 0, len(c.inputs)) + for _, input := range c.inputs { + inputList = append(inputList, input) + } + + // Yield is calculated as the difference between value and added fee + // for this input. The fee calculation excludes fee components that are + // common to all inputs, as those wouldn't influence the order. The + // single component that is differentiating is witness size. + // + // For witness size, the upper limit is taken. The actual size depends + // on the signature length, which is not known yet at this point. + calcYield := func(input *pendingInput) int64 { + size, _, err := input.WitnessType().SizeUpperBound() + if err != nil { + log.Errorf("Failed to get input weight: %v", err) + + return 0 + } + + yield := input.SignDesc().Output.Value - + int64(c.sweepFeeRate.FeeForWeight(int64(size))) + + return yield + } + + // Sort input by yield. We will start constructing input sets starting + // with the highest yield inputs. This is to prevent the construction + // of a set with an output below the dust limit, causing the sweep + // process to stop, while there are still higher value inputs + // available. It also allows us to stop evaluating more inputs when the + // first input in this ordering is encountered with a negative yield. + sort.Slice(inputList, func(i, j int) bool { + // Because of the specific ordering and termination condition + // that is described above, we place force sweeps at the start + // of the list. Otherwise we can't be sure that they will be + // included in an input set. + if inputList[i].parameters().Force { + return true + } + + return calcYield(inputList[i]) > calcYield(inputList[j]) + }) + + // Select blocks of inputs up to the configured maximum number. + var sets []inputSet + for len(inputList) > 0 { + // Start building a set of positive-yield tx inputs under the + // condition that the tx will be published with the specified + // fee rate. + txInputs := newTxInputSet( + wallet, c.sweepFeeRate, maxFeeRate, maxInputs, + ) + + // From the set of sweepable inputs, keep adding inputs to the + // input set until the tx output value no longer goes up or the + // maximum number of inputs is reached. + txInputs.addPositiveYieldInputs(inputList) + + // If there are no positive yield inputs, we can stop here. + inputCount := len(txInputs.inputs) + if inputCount == 0 { + return sets, nil + } + + // Check the current output value and add wallet utxos if + // needed to push the output value to the lower limit. + if err := txInputs.tryAddWalletInputsIfNeeded(); err != nil { + return nil, err + } + + // If the output value of this block of inputs does not reach + // the dust limit, stop sweeping. Because of the sorting, + // continuing with the remaining inputs will only lead to sets + // with an even lower output value. + if !txInputs.enoughInput() { + // The change output is always a p2tr here. + dl := lnwallet.DustLimitForSize(input.P2TRSize) + log.Debugf("Input set value %v (required=%v, "+ + "change=%v) below dust limit of %v", + txInputs.totalOutput(), txInputs.requiredOutput, + txInputs.changeOutput, dl) + return sets, nil + } + + log.Infof("Candidate sweep set of size=%v (+%v wallet inputs),"+ + " has yield=%v, weight=%v", + inputCount, len(txInputs.inputs)-inputCount, + txInputs.totalOutput()-txInputs.walletInputTotal, + txInputs.weightEstimate(true).weight()) + + sets = append(sets, txInputs.inputs) + inputList = inputList[inputCount:] + } + + return sets, nil +} + // UtxoAggregator defines an interface that takes a list of inputs and // aggregate them into groups. Each group is used as the inputs to create a // sweeping transaction. type UtxoAggregator interface { // ClusterInputs takes a list of inputs and groups them into clusters. - ClusterInputs(pendingInputs) []inputCluster + ClusterInputs(pendingInputs) []Cluster } // SimpleAggregator aggregates inputs known by the Sweeper based on each @@ -72,11 +214,7 @@ func NewSimpleUtxoAggregator(estimator chainfee.Estimator, // inputs known by the UtxoSweeper. It clusters inputs by // 1) Required tx locktime // 2) Similar fee rates. -// -// TODO(yy): remove this nolint once done refactoring. -// -//nolint:revive -func (s *SimpleAggregator) ClusterInputs(inputs pendingInputs) []inputCluster { +func (s *SimpleAggregator) ClusterInputs(inputs pendingInputs) []Cluster { // We start by getting the inputs clusters by locktime. Since the // inputs commit to the locktime, they can only be clustered together // if the locktime is equal. @@ -88,7 +226,19 @@ func (s *SimpleAggregator) ClusterInputs(inputs pendingInputs) []inputCluster { // Since the inputs that we clustered by fee rate don't commit to a // specific locktime, we can try to merge a locktime cluster with a fee // cluster. - return zipClusters(lockTimeClusters, feeClusters) + clusters := zipClusters(lockTimeClusters, feeClusters) + + sort.Slice(clusters, func(i, j int) bool { + return clusters[i].sweepFeeRate > + clusters[j].sweepFeeRate + }) + + result := make([]Cluster, 0, len(clusters)) + for _, c := range clusters { + result = append(result, &c) + } + + return result } // clusterByLockTime takes the given set of pending inputs and clusters those diff --git a/sweep/mocks.go b/sweep/mocks.go index 3c88823087f..0e144ab4fc8 100644 --- a/sweep/mocks.go +++ b/sweep/mocks.go @@ -37,8 +37,8 @@ type mockUtxoAggregator struct { var _ UtxoAggregator = (*mockUtxoAggregator)(nil) // ClusterInputs takes a list of inputs and groups them into clusters. -func (m *mockUtxoAggregator) ClusterInputs(pendingInputs) []inputCluster { +func (m *mockUtxoAggregator) ClusterInputs(pendingInputs) []Cluster { args := m.Called(pendingInputs{}) - return args.Get(0).([]inputCluster) + return args.Get(0).([]Cluster) } diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 549095868fe..c3ba0a7e637 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "math/rand" - "sort" "sync" "sync/atomic" "time" @@ -224,14 +223,6 @@ func (p *pendingInput) terminated() bool { // pendingInputs is a type alias for a set of pending inputs. type pendingInputs = map[wire.OutPoint]*pendingInput -// inputCluster is a helper struct to gather a set of pending inputs that should -// be swept with the specified fee rate. -type inputCluster struct { - lockTime *uint32 - sweepFeeRate chainfee.SatPerKWeight - inputs pendingInputs -} - // pendingSweepsReq is an internal message we'll use to represent an external // caller's intent to retrieve all of the pending inputs the UtxoSweeper is // attempting to sweep. @@ -750,60 +741,26 @@ func (s *UtxoSweeper) removeExclusiveGroup(group uint64) { } // sweepCluster tries to sweep the given input cluster. -func (s *UtxoSweeper) sweepCluster(cluster inputCluster) error { +func (s *UtxoSweeper) sweepCluster(cluster Cluster) error { // Execute the sweep within a coin select lock. Otherwise the coins // that we are going to spend may be selected for other transactions // like funding of a channel. + // + // TODO(yy): decrease the lock scope. return s.cfg.Wallet.WithCoinSelectLock(func() error { // Examine pending inputs and try to construct lists of inputs. - allSets, newSets, err := s.getInputLists(cluster) + sets, err := cluster.CreateInputSets( + s.cfg.Wallet, + s.cfg.MaxFeeRate.FeePerKWeight(), + s.cfg.MaxInputsPerTx, + ) if err != nil { return fmt.Errorf("examine pending inputs: %w", err) } - // errAllSets records the error from broadcasting the sweeping - // transactions for all input sets. - var errAllSets error - - // allSets contains retried inputs and new inputs. To avoid - // creating an RBF for the new inputs, we'd sweep this set - // first. - for _, inputs := range allSets { - errAllSets = s.sweep(inputs, cluster.sweepFeeRate) - // TODO(yy): we should also find out which set created - // this error. If there are new inputs in this set, we - // should give it a second chance by sweeping them - // below. To enable this, we need to provide richer - // state for each input other than just recording the - // publishAttempts. We'd also need to refactor how we - // create the input sets. Atm, the steps are, - // 1. create a list of input sets. - // 2. sweep each set by creating and publishing the tx. - // We should change the flow as, - // 1. create a list of input sets, and for each set, - // 2. when created, we create and publish the tx. - // 3. if the publish fails, find out which input is - // causing the failure and retry the rest of the - // inputs. - if errAllSets != nil { - log.Errorf("Sweep all inputs got error: %v", - errAllSets) - break - } - } - - // If we have successfully swept all inputs, there's no need to - // sweep the new inputs as it'd create an RBF case. - if allSets != nil && errAllSets == nil { - return nil - } - - // We'd end up there if there's no retried inputs or the above - // sweeping tx failed. In this case, we'd sweep the new input - // sets. If there's an error when sweeping a given set, we'd - // log the error and sweep the next set. - for _, inputs := range newSets { - err := s.sweep(inputs, cluster.sweepFeeRate) + // Create sweeping transaction for each set. + for _, inputs := range sets { + err := s.sweep(inputs, cluster.FeeRate()) if err != nil { log.Errorf("sweep new inputs: %w", err) } @@ -844,76 +801,6 @@ func (s *UtxoSweeper) signalResult(pi *pendingInput, result Result) { } } -// getInputLists goes through the given inputs and constructs multiple distinct -// sweep lists with the given fee rate, each up to the configured maximum -// number of inputs. Negative yield inputs are skipped. Transactions with an -// output below the dust limit are not published. Those inputs remain pending -// and will be bundled with future inputs if possible. It returns two list - -// one containing all inputs and the other containing only the new inputs. If -// there's no retried inputs, the first set returned will be empty. -func (s *UtxoSweeper) getInputLists( - cluster inputCluster) ([]inputSet, []inputSet, error) { - - // Filter for inputs that need to be swept. Create two lists: all - // sweepable inputs and a list containing only the new, never tried - // inputs. - // - // We want to create as large a tx as possible, so we return a final - // set list that starts with sets created from all inputs. However, - // there is a chance that those txes will not publish, because they - // already contain inputs that failed before. Therefore we also add - // sets consisting of only new inputs to the list, to make sure that - // new inputs are given a good, isolated chance of being published. - // - // TODO(yy): this would lead to conflict transactions as the same input - // can be used in two sweeping transactions, and our rebroadcaster will - // retry the failed one. We should instead understand why the input is - // failed in the first place, and start tracking input states in - // sweeper to avoid this. - var newInputs, retryInputs []txInput - for _, input := range cluster.inputs { - // Add input to the either one of the lists. - if input.publishAttempts == 0 { - newInputs = append(newInputs, input) - } else { - retryInputs = append(retryInputs, input) - } - } - - // Convert the max fee rate's unit from sat/vb to sat/kw. - maxFeeRate := s.cfg.MaxFeeRate.FeePerKWeight() - - // If there is anything to retry, combine it with the new inputs and - // form input sets. - var allSets []inputSet - if len(retryInputs) > 0 { - var err error - allSets, err = generateInputPartitionings( - append(retryInputs, newInputs...), - cluster.sweepFeeRate, maxFeeRate, - s.cfg.MaxInputsPerTx, s.cfg.Wallet, - ) - if err != nil { - return nil, nil, fmt.Errorf("input partitionings: %w", - err) - } - } - - // Create sets for just the new inputs. - newSets, err := generateInputPartitionings( - newInputs, cluster.sweepFeeRate, maxFeeRate, - s.cfg.MaxInputsPerTx, s.cfg.Wallet, - ) - if err != nil { - return nil, nil, fmt.Errorf("input partitionings: %w", err) - } - - log.Debugf("Sweep candidates at height=%v: total_num_pending=%v, "+ - "total_num_new=%v", s.currentHeight, len(allSets), len(newSets)) - - return allSets, newSets, nil -} - // sweep takes a set of preselected inputs, creates a sweep tx and publishes the // tx. The output address is only marked as used if the publish succeeds. func (s *UtxoSweeper) sweep(inputs inputSet, @@ -1678,10 +1565,6 @@ func (s *UtxoSweeper) sweepPendingInputs(inputs pendingInputs) { // rate order. We do this to ensure any inputs which have had their fee // rate bumped are broadcast first in order enforce the RBF policy. inputClusters := s.cfg.Aggregator.ClusterInputs(inputs) - sort.Slice(inputClusters, func(i, j int) bool { - return inputClusters[i].sweepFeeRate > - inputClusters[j].sweepFeeRate - }) for _, cluster := range inputClusters { err := s.sweepCluster(cluster) diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index 0a914c4c6e7..a13b41fc8b2 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -1882,115 +1882,6 @@ func TestSweeperShutdownHandling(t *testing.T) { require.Error(t, err) } -// TestGetInputLists checks that the expected input sets are returned based on -// whether there are retried inputs or not. -func TestGetInputLists(t *testing.T) { - t.Parallel() - - // Create a test param with a dummy fee preference. This is needed so - // `feeRateForPreference` won't throw an error. - param := Params{Fee: FeeEstimateInfo{ConfTarget: 1}} - - // Create a mock input and mock all the methods used in this test. - testInput := &input.MockInput{} - testInput.On("RequiredLockTime").Return(0, false) - testInput.On("WitnessType").Return(input.CommitmentAnchor) - testInput.On("OutPoint").Return(&wire.OutPoint{Index: 1}) - testInput.On("RequiredTxOut").Return(nil) - testInput.On("UnconfParent").Return(nil) - testInput.On("SignDesc").Return(&input.SignDescriptor{ - Output: &wire.TxOut{Value: 100_000}, - }) - - // Create a new and a retried input. - // - // NOTE: we use the same input.Input for both pending inputs as we only - // test the logic of returning the correct non-nil input sets, and not - // the content the of sets. To validate the content of the sets, we - // should test `generateInputPartitionings` instead. - newInput := &pendingInput{ - Input: testInput, - params: param, - } - oldInput := &pendingInput{ - Input: testInput, - params: param, - publishAttempts: 1, - } - - // clusterNew contains only new inputs. - clusterNew := pendingInputs{ - wire.OutPoint{Index: 1}: newInput, - } - - // clusterMixed contains a mixed of new and retried inputs. - clusterMixed := pendingInputs{ - wire.OutPoint{Index: 1}: newInput, - wire.OutPoint{Index: 2}: oldInput, - } - - // clusterOld contains only retried inputs. - clusterOld := pendingInputs{ - wire.OutPoint{Index: 2}: oldInput, - } - - // Create a test sweeper. - s := New(&UtxoSweeperConfig{ - MaxInputsPerTx: DefaultMaxInputsPerTx, - }) - - testCases := []struct { - name string - cluster inputCluster - expectedNilAllSet bool - expectNilNewSet bool - }{ - { - // When there are only new inputs, we'd expect the - // first returned set(allSets) to be empty. - name: "new inputs only", - cluster: inputCluster{inputs: clusterNew}, - expectedNilAllSet: true, - expectNilNewSet: false, - }, - { - // When there are only retried inputs, we'd expect the - // second returned set(newSet) to be empty. - name: "retried inputs only", - cluster: inputCluster{inputs: clusterOld}, - expectedNilAllSet: false, - expectNilNewSet: true, - }, - { - // When there are mixed inputs, we'd expect two sets - // are returned. - name: "mixed inputs", - cluster: inputCluster{inputs: clusterMixed}, - expectedNilAllSet: false, - expectNilNewSet: false, - }, - } - - for _, tc := range testCases { - tc := tc - - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - allSets, newSets, err := s.getInputLists(tc.cluster) - require.NoError(t, err) - - if tc.expectNilNewSet { - require.Nil(t, newSets) - } - - if tc.expectedNilAllSet { - require.Nil(t, allSets) - } - }) - } -} - // TestMarkInputsPendingPublish checks that given a list of inputs with // different states, only the non-terminal state will be marked as `Published`. func TestMarkInputsPendingPublish(t *testing.T) { diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index ecec52eb981..8be408c89ce 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -331,7 +331,7 @@ func (t *txInputSet) add(input input.Input, constraints addConstraints) bool { // up the utxo set even if it costs us some fees up front. In the spirit of // minimizing any negative externalities we cause for the Bitcoin system as a // whole. -func (t *txInputSet) addPositiveYieldInputs(sweepableInputs []txInput) { +func (t *txInputSet) addPositiveYieldInputs(sweepableInputs []*pendingInput) { for i, inp := range sweepableInputs { // Apply relaxed constraints for force sweeps. constraints := constraintsRegular diff --git a/sweep/txgenerator.go b/sweep/txgenerator.go index 2fae5b88674..3c6c6d3789e 100644 --- a/sweep/txgenerator.go +++ b/sweep/txgenerator.go @@ -37,112 +37,6 @@ type txInput interface { parameters() Params } -// inputSet is a set of inputs that can be used as the basis to generate a tx -// on. -type inputSet []input.Input - -// generateInputPartitionings goes through all given inputs and constructs sets -// of inputs that can be used to generate a sensible transaction. Each set -// contains up to the configured maximum number of inputs. Negative yield -// inputs are skipped. No input sets with a total value after fees below the -// dust limit are returned. -func generateInputPartitionings(sweepableInputs []txInput, - feePerKW, maxFeeRate chainfee.SatPerKWeight, maxInputsPerTx int, - wallet Wallet) ([]inputSet, error) { - - // Sort input by yield. We will start constructing input sets starting - // with the highest yield inputs. This is to prevent the construction - // of a set with an output below the dust limit, causing the sweep - // process to stop, while there are still higher value inputs - // available. It also allows us to stop evaluating more inputs when the - // first input in this ordering is encountered with a negative yield. - // - // Yield is calculated as the difference between value and added fee - // for this input. The fee calculation excludes fee components that are - // common to all inputs, as those wouldn't influence the order. The - // single component that is differentiating is witness size. - // - // For witness size, the upper limit is taken. The actual size depends - // on the signature length, which is not known yet at this point. - yields := make(map[wire.OutPoint]int64) - for _, input := range sweepableInputs { - size, _, err := input.WitnessType().SizeUpperBound() - if err != nil { - return nil, fmt.Errorf( - "failed adding input weight: %v", err) - } - - yields[*input.OutPoint()] = input.SignDesc().Output.Value - - int64(feePerKW.FeeForWeight(int64(size))) - } - - sort.Slice(sweepableInputs, func(i, j int) bool { - // Because of the specific ordering and termination condition - // that is described above, we place force sweeps at the start - // of the list. Otherwise we can't be sure that they will be - // included in an input set. - if sweepableInputs[i].parameters().Force { - return true - } - - return yields[*sweepableInputs[i].OutPoint()] > - yields[*sweepableInputs[j].OutPoint()] - }) - - // Select blocks of inputs up to the configured maximum number. - var sets []inputSet - for len(sweepableInputs) > 0 { - // Start building a set of positive-yield tx inputs under the - // condition that the tx will be published with the specified - // fee rate. - txInputs := newTxInputSet( - wallet, feePerKW, maxFeeRate, maxInputsPerTx, - ) - - // From the set of sweepable inputs, keep adding inputs to the - // input set until the tx output value no longer goes up or the - // maximum number of inputs is reached. - txInputs.addPositiveYieldInputs(sweepableInputs) - - // If there are no positive yield inputs, we can stop here. - inputCount := len(txInputs.inputs) - if inputCount == 0 { - return sets, nil - } - - // Check the current output value and add wallet utxos if - // needed to push the output value to the lower limit. - if err := txInputs.tryAddWalletInputsIfNeeded(); err != nil { - return nil, err - } - - // If the output value of this block of inputs does not reach - // the dust limit, stop sweeping. Because of the sorting, - // continuing with the remaining inputs will only lead to sets - // with an even lower output value. - if !txInputs.enoughInput() { - // The change output is always a p2tr here. - dl := lnwallet.DustLimitForSize(input.P2TRSize) - log.Debugf("Input set value %v (required=%v, "+ - "change=%v) below dust limit of %v", - txInputs.totalOutput(), txInputs.requiredOutput, - txInputs.changeOutput, dl) - return sets, nil - } - - log.Infof("Candidate sweep set of size=%v (+%v wallet inputs), "+ - "has yield=%v, weight=%v", - inputCount, len(txInputs.inputs)-inputCount, - txInputs.totalOutput()-txInputs.walletInputTotal, - txInputs.weightEstimate(true).weight()) - - sets = append(sets, txInputs.inputs) - sweepableInputs = sweepableInputs[inputCount:] - } - - return sets, nil -} - // createSweepTx builds a signed tx spending the inputs to the given outputs, // sending any leftover change to the change script. func createSweepTx(inputs []input.Input, outputs []*wire.TxOut, From 5146b0e72c11d5899e4a23d6b3eaf0f7eb61908e Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Mon, 30 Oct 2023 20:43:33 +0800 Subject: [PATCH 03/13] sweep: add new inteface `InputSet` to manage inputs in a tx Previously the fee rate is tracked at cluster level, which may not be accurate as each cluster is then divided into input sets. And these sets are what's actually included in the final sweeping tx. To properly reflect the final fee rate used in the sweeping tx, `InputSet` is added so more customized clustering logic can be implemented in the future. For intance, atm it's clustered by fee rates, while we may also cluster by deadlines, urgencies, etc. --- sweep/aggregator.go | 16 ++++------------ sweep/sweeper.go | 15 ++++++++------- sweep/tx_input_set.go | 23 +++++++++++++++++++++++ sweep/txgenerator.go | 16 ++++------------ 4 files changed, 39 insertions(+), 31 deletions(-) diff --git a/sweep/aggregator.go b/sweep/aggregator.go index dc75500e3e4..33199367964 100644 --- a/sweep/aggregator.go +++ b/sweep/aggregator.go @@ -33,10 +33,7 @@ type Cluster interface { // CreateInputSets goes through the cluster's inputs and constructs // sets of inputs that can be used to generate a sensible transaction. CreateInputSets(wallet Wallet, maxFeeRate chainfee.SatPerKWeight, - maxInputs int) ([]inputSet, error) - - // FeeRate returns the fee rate of the cluster. - FeeRate() chainfee.SatPerKWeight + maxInputs int) ([]InputSet, error) } // Compile-time constraint to ensure inputCluster implements Cluster. @@ -50,11 +47,6 @@ type inputCluster struct { inputs pendingInputs } -// FeeRate returns the fee rate of the cluster. -func (c *inputCluster) FeeRate() chainfee.SatPerKWeight { - return c.sweepFeeRate -} - // GroupInputs goes through the cluster's inputs and constructs sets of inputs // that can be used to generate a sensible transaction. Each set contains up to // the configured maximum number of inputs. Negative yield inputs are skipped. @@ -62,7 +54,7 @@ func (c *inputCluster) FeeRate() chainfee.SatPerKWeight { // returned. func (c *inputCluster) CreateInputSets( wallet Wallet, maxFeeRate chainfee.SatPerKWeight, - maxInputs int) ([]inputSet, error) { + maxInputs int) ([]InputSet, error) { // Turn the inputs into a slice so we can sort them. inputList := make([]*pendingInput, 0, len(c.inputs)) @@ -110,7 +102,7 @@ func (c *inputCluster) CreateInputSets( }) // Select blocks of inputs up to the configured maximum number. - var sets []inputSet + var sets []InputSet for len(inputList) > 0 { // Start building a set of positive-yield tx inputs under the // condition that the tx will be published with the specified @@ -156,7 +148,7 @@ func (c *inputCluster) CreateInputSets( txInputs.totalOutput()-txInputs.walletInputTotal, txInputs.weightEstimate(true).weight()) - sets = append(sets, txInputs.inputs) + sets = append(sets, txInputs) inputList = inputList[inputCount:] } diff --git a/sweep/sweeper.go b/sweep/sweeper.go index c3ba0a7e637..8bfcd7075f7 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -760,7 +760,7 @@ func (s *UtxoSweeper) sweepCluster(cluster Cluster) error { // Create sweeping transaction for each set. for _, inputs := range sets { - err := s.sweep(inputs, cluster.FeeRate()) + err := s.sweep(inputs) if err != nil { log.Errorf("sweep new inputs: %w", err) } @@ -803,9 +803,7 @@ func (s *UtxoSweeper) signalResult(pi *pendingInput, result Result) { // sweep takes a set of preselected inputs, creates a sweep tx and publishes the // tx. The output address is only marked as used if the publish succeeds. -func (s *UtxoSweeper) sweep(inputs inputSet, - feeRate chainfee.SatPerKWeight) error { - +func (s *UtxoSweeper) sweep(set InputSet) error { // Generate an output script if there isn't an unused script available. if s.currentOutputScript == nil { pkScript, err := s.cfg.GenSweepScript() @@ -817,8 +815,9 @@ func (s *UtxoSweeper) sweep(inputs inputSet, // Create sweep tx. tx, fee, err := createSweepTx( - inputs, nil, s.currentOutputScript, uint32(s.currentHeight), - feeRate, s.cfg.MaxFeeRate.FeePerKWeight(), s.cfg.Signer, + set.Inputs(), nil, s.currentOutputScript, + uint32(s.currentHeight), set.FeeRate(), + s.cfg.MaxFeeRate.FeePerKWeight(), s.cfg.Signer, ) if err != nil { return fmt.Errorf("create sweep tx: %w", err) @@ -826,7 +825,7 @@ func (s *UtxoSweeper) sweep(inputs inputSet, tr := &TxRecord{ Txid: tx.TxHash(), - FeeRate: uint64(feeRate), + FeeRate: uint64(set.FeeRate()), Fee: uint64(fee), } @@ -1195,6 +1194,8 @@ func (s *UtxoSweeper) handleUpdateReq(req *updateReq) ( // - Make handling re-orgs easier. // - Thwart future possible fee sniping attempts. // - Make us blend in with the bitcoind wallet. +// +// TODO(yy): remove this method and only allow sweeping via requests. func (s *UtxoSweeper) CreateSweepTx(inputs []input.Input, feePref FeeEstimateInfo) (*wire.MsgTx, error) { diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index 8be408c89ce..6beba7a742a 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -30,6 +30,16 @@ const ( constraintsForce ) +// InputSet defines an interface that's responsible for filtering a set of +// inputs that can be swept economically. +type InputSet interface { + // Inputs returns the set of inputs that should be used to create a tx. + Inputs() []input.Input + + // FeeRate returns the fee rate that should be used for the tx. + FeeRate() chainfee.SatPerKWeight +} + type txInputSetState struct { // feeRate is the fee rate to use for the sweep transaction. feeRate chainfee.SatPerKWeight @@ -121,6 +131,9 @@ type txInputSet struct { wallet Wallet } +// Compile-time constraint to ensure txInputSet implements InputSet. +var _ InputSet = (*txInputSet)(nil) + // newTxInputSet constructs a new, empty input set. func newTxInputSet(wallet Wallet, feePerKW, maxFeeRate chainfee.SatPerKWeight, maxInputs int) *txInputSet { @@ -139,6 +152,16 @@ func newTxInputSet(wallet Wallet, feePerKW, maxFeeRate chainfee.SatPerKWeight, return &b } +// Inputs returns the inputs that should be used to create a tx. +func (t *txInputSet) Inputs() []input.Input { + return t.inputs +} + +// FeeRate returns the fee rate that should be used for the tx. +func (t *txInputSet) FeeRate() chainfee.SatPerKWeight { + return t.feeRate +} + // enoughInput returns true if we've accumulated enough inputs to pay the fees // and have at least one output that meets the dust limit. func (t *txInputSet) enoughInput() bool { diff --git a/sweep/txgenerator.go b/sweep/txgenerator.go index 3c6c6d3789e..353513cb64e 100644 --- a/sweep/txgenerator.go +++ b/sweep/txgenerator.go @@ -30,22 +30,15 @@ var ( ErrLocktimeConflict = errors.New("incompatible locktime") ) -// txInput is an interface that provides the input data required for tx -// generation. -type txInput interface { - input.Input - parameters() Params -} - // createSweepTx builds a signed tx spending the inputs to the given outputs, // sending any leftover change to the change script. func createSweepTx(inputs []input.Input, outputs []*wire.TxOut, changePkScript []byte, currentBlockHeight uint32, - feePerKw, maxFeeRate chainfee.SatPerKWeight, + feeRate, maxFeeRate chainfee.SatPerKWeight, signer input.Signer) (*wire.MsgTx, btcutil.Amount, error) { inputs, estimator, err := getWeightEstimate( - inputs, outputs, feePerKw, maxFeeRate, changePkScript, + inputs, outputs, feeRate, maxFeeRate, changePkScript, ) if err != nil { return nil, 0, err @@ -216,7 +209,7 @@ func createSweepTx(inputs []input.Input, outputs []*wire.TxOut, "using %v sat/kw, tx_weight=%v, tx_fee=%v, parents_count=%v, "+ "parents_fee=%v, parents_weight=%v", sweepTx.TxHash(), len(inputs), - inputTypeSummary(inputs), int64(feePerKw), + inputTypeSummary(inputs), feeRate, estimator.weight(), txFee, len(estimator.parents), estimator.parentsFee, estimator.parentsWeight, @@ -229,8 +222,7 @@ func createSweepTx(inputs []input.Input, outputs []*wire.TxOut, // Additionally, it returns counts for the number of csv and cltv inputs. func getWeightEstimate(inputs []input.Input, outputs []*wire.TxOut, feeRate, maxFeeRate chainfee.SatPerKWeight, - outputPkScript []byte) ([]input.Input, - *weightEstimator, error) { + outputPkScript []byte) ([]input.Input, *weightEstimator, error) { // We initialize a weight estimator so we can accurately asses the // amount of fees we need to pay for this sweep transaction. From 14df7619055cc1bd7687bcfb16a1677d642c7c18 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Mon, 30 Oct 2023 21:32:46 +0800 Subject: [PATCH 04/13] sweep: deepen the interface `Aggregator` This commit makes the `ClusterInputs` directly returning the `InputSet` so the sweeper doesn't know about the existence of `Cluster` interface. This way we can have a deeper interface as the sweeper only needs to interact with `Aggregator` only to get the final input sets, leaving the implementation details being managed by `SimpleAggregator` and future aggregators. --- server.go | 1 + sweep/aggregator.go | 49 ++++++++++++++--------------- sweep/aggregator_test.go | 2 +- sweep/mocks.go | 8 +++-- sweep/sweeper.go | 66 ++++++++++++++++------------------------ sweep/sweeper_test.go | 16 +++++++--- 6 files changed, 70 insertions(+), 72 deletions(-) diff --git a/server.go b/server.go index 10ad4a0e2fa..d966018e7e5 100644 --- a/server.go +++ b/server.go @@ -1065,6 +1065,7 @@ func newServer(cfg *Config, listenAddrs []net.Addr, aggregator := sweep.NewSimpleUtxoAggregator( cc.FeeEstimator, cfg.Sweeper.MaxFeeRate.FeePerKWeight(), + sweep.DefaultMaxInputsPerTx, ) s.sweeper = sweep.New(&sweep.UtxoSweeperConfig{ diff --git a/sweep/aggregator.go b/sweep/aggregator.go index 33199367964..60d835abe09 100644 --- a/sweep/aggregator.go +++ b/sweep/aggregator.go @@ -23,22 +23,6 @@ const ( DefaultFeeRateBucketSize = 10 ) -// inputSet is a set of inputs that can be used as the basis to generate a tx -// on. -type inputSet []input.Input - -// Cluster defines an interface that prepares inputs of a cluster to be grouped -// into a list of sets that can be used to create sweep transactions. -type Cluster interface { - // CreateInputSets goes through the cluster's inputs and constructs - // sets of inputs that can be used to generate a sensible transaction. - CreateInputSets(wallet Wallet, maxFeeRate chainfee.SatPerKWeight, - maxInputs int) ([]InputSet, error) -} - -// Compile-time constraint to ensure inputCluster implements Cluster. -var _ Cluster = (*inputCluster)(nil) - // inputCluster is a helper struct to gather a set of pending inputs that // should be swept with the specified fee rate. type inputCluster struct { @@ -52,7 +36,7 @@ type inputCluster struct { // the configured maximum number of inputs. Negative yield inputs are skipped. // No input sets with a total value after fees below the dust limit are // returned. -func (c *inputCluster) CreateInputSets( +func (c *inputCluster) createInputSets( wallet Wallet, maxFeeRate chainfee.SatPerKWeight, maxInputs int) ([]InputSet, error) { @@ -160,7 +144,7 @@ func (c *inputCluster) CreateInputSets( // sweeping transaction. type UtxoAggregator interface { // ClusterInputs takes a list of inputs and groups them into clusters. - ClusterInputs(pendingInputs) []Cluster + ClusterInputs(Wallet, pendingInputs) []InputSet } // SimpleAggregator aggregates inputs known by the Sweeper based on each @@ -175,6 +159,11 @@ type SimpleAggregator struct { // SimpleAggregator. MaxFeeRate chainfee.SatPerKWeight + // MaxInputsPerTx specifies the default maximum number of inputs allowed + // in a single sweep tx. If more need to be swept, multiple txes are + // created and published. + MaxInputsPerTx int + // FeeRateBucketSize is the default size of fee rate buckets we'll use // when clustering inputs into buckets with similar fee rates within // the SimpleAggregator. @@ -193,11 +182,12 @@ var _ UtxoAggregator = (*SimpleAggregator)(nil) // NewSimpleUtxoAggregator creates a new instance of a SimpleAggregator. func NewSimpleUtxoAggregator(estimator chainfee.Estimator, - max chainfee.SatPerKWeight) *SimpleAggregator { + max chainfee.SatPerKWeight, maxTx int) *SimpleAggregator { return &SimpleAggregator{ FeeEstimator: estimator, MaxFeeRate: max, + MaxInputsPerTx: maxTx, FeeRateBucketSize: DefaultFeeRateBucketSize, } } @@ -206,7 +196,9 @@ func NewSimpleUtxoAggregator(estimator chainfee.Estimator, // inputs known by the UtxoSweeper. It clusters inputs by // 1) Required tx locktime // 2) Similar fee rates. -func (s *SimpleAggregator) ClusterInputs(inputs pendingInputs) []Cluster { +func (s *SimpleAggregator) ClusterInputs( + wallet Wallet, inputs pendingInputs) []InputSet { + // We start by getting the inputs clusters by locktime. Since the // inputs commit to the locktime, they can only be clustered together // if the locktime is equal. @@ -225,12 +217,21 @@ func (s *SimpleAggregator) ClusterInputs(inputs pendingInputs) []Cluster { clusters[j].sweepFeeRate }) - result := make([]Cluster, 0, len(clusters)) - for _, c := range clusters { - result = append(result, &c) + // Now that we have the clusters, we can create the input sets. + var inputSets []InputSet + for _, cluster := range clusters { + sets, err := cluster.createInputSets( + wallet, s.MaxFeeRate, s.MaxInputsPerTx, + ) + if err != nil { + log.Errorf("Unable to create input sets: %v", err) + continue + } + + inputSets = append(inputSets, sets...) } - return result + return inputSets } // clusterByLockTime takes the given set of pending inputs and clusters those diff --git a/sweep/aggregator_test.go b/sweep/aggregator_test.go index f3bf2cd288e..2058464ad28 100644 --- a/sweep/aggregator_test.go +++ b/sweep/aggregator_test.go @@ -320,7 +320,7 @@ func TestClusterByLockTime(t *testing.T) { ) // Create a test aggregator. - s := NewSimpleUtxoAggregator(nil, maxFeeRate) + s := NewSimpleUtxoAggregator(nil, maxFeeRate, 100) testCases := []struct { name string diff --git a/sweep/mocks.go b/sweep/mocks.go index 0e144ab4fc8..665a16d93e5 100644 --- a/sweep/mocks.go +++ b/sweep/mocks.go @@ -37,8 +37,10 @@ type mockUtxoAggregator struct { var _ UtxoAggregator = (*mockUtxoAggregator)(nil) // ClusterInputs takes a list of inputs and groups them into clusters. -func (m *mockUtxoAggregator) ClusterInputs(pendingInputs) []Cluster { - args := m.Called(pendingInputs{}) +func (m *mockUtxoAggregator) ClusterInputs(wallet Wallet, + inputs pendingInputs) []InputSet { - return args.Get(0).([]Cluster) + args := m.Called(wallet, inputs) + + return args.Get(0).([]InputSet) } diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 8bfcd7075f7..328d0b1c09e 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -740,36 +740,6 @@ func (s *UtxoSweeper) removeExclusiveGroup(group uint64) { } } -// sweepCluster tries to sweep the given input cluster. -func (s *UtxoSweeper) sweepCluster(cluster Cluster) error { - // Execute the sweep within a coin select lock. Otherwise the coins - // that we are going to spend may be selected for other transactions - // like funding of a channel. - // - // TODO(yy): decrease the lock scope. - return s.cfg.Wallet.WithCoinSelectLock(func() error { - // Examine pending inputs and try to construct lists of inputs. - sets, err := cluster.CreateInputSets( - s.cfg.Wallet, - s.cfg.MaxFeeRate.FeePerKWeight(), - s.cfg.MaxInputsPerTx, - ) - if err != nil { - return fmt.Errorf("examine pending inputs: %w", err) - } - - // Create sweeping transaction for each set. - for _, inputs := range sets { - err := s.sweep(inputs) - if err != nil { - log.Errorf("sweep new inputs: %w", err) - } - } - - return nil - }) -} - // signalResult notifies the listeners of the final result of the input sweep. // It also cancels any pending spend notification. func (s *UtxoSweeper) signalResult(pi *pendingInput, result Result) { @@ -1561,17 +1531,33 @@ func (s *UtxoSweeper) updateSweeperInputs() pendingInputs { // sweepPendingInputs is called when the ticker fires. It will create clusters // and attempt to create and publish the sweeping transactions. func (s *UtxoSweeper) sweepPendingInputs(inputs pendingInputs) { - // We'll attempt to cluster all of our inputs with similar fee rates. - // Before attempting to sweep them, we'll sort them in descending fee - // rate order. We do this to ensure any inputs which have had their fee - // rate bumped are broadcast first in order enforce the RBF policy. - inputClusters := s.cfg.Aggregator.ClusterInputs(inputs) - - for _, cluster := range inputClusters { - err := s.sweepCluster(cluster) - if err != nil { - log.Errorf("input cluster sweep: %v", err) + // Execute the sweep within a coin select lock. Otherwise the coins + // that we are going to spend may be selected for other transactions + // like funding of a channel. + // + // TODO(yy): decrease the lock scope - we need to remove the wallet + // used here, which means we need to ask the aggregator to return input + // sets and specifying whether wallet utoxs are needed or not. Then, by + // calling `TxInput.NeedWalletInput`, we can then lock and add the + // wallet input, creating a much smaller lock scope. + err := s.cfg.Wallet.WithCoinSelectLock(func() error { + // Cluster all of our inputs based on the specific Aggregator. + inputSets := s.cfg.Aggregator.ClusterInputs( + s.cfg.Wallet, inputs, + ) + + // Create sweeping transaction for each set. + for _, inputs := range inputSets { + err := s.sweep(inputs) + if err != nil { + log.Errorf("sweep new inputs: %v", err) + } } + + return nil + }) + if err != nil { + log.Errorf("input cluster sweep: %v", err) } } diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index a13b41fc8b2..e7cd27827e2 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -123,6 +123,7 @@ func createSweeperTestContext(t *testing.T) *sweeperTestContext { aggregator := NewSimpleUtxoAggregator( estimator, DefaultMaxFeeRate.FeePerKWeight(), + testMaxInputsPerTx, ) ctx := &sweeperTestContext{ @@ -1287,6 +1288,11 @@ func TestLockTimes(t *testing.T) { // impact our test. ctx.sweeper.cfg.MaxInputsPerTx = 100 + // We also need to update the aggregator about this new config. + ctx.sweeper.cfg.Aggregator = NewSimpleUtxoAggregator( + ctx.estimator, DefaultMaxFeeRate.FeePerKWeight(), 100, + ) + // We will set up the lock times in such a way that we expect the // sweeper to divide the inputs into 4 diffeerent transactions. const numSweeps = 4 @@ -1369,7 +1375,7 @@ func TestLockTimes(t *testing.T) { // The should be no inputs not foud in any of the sweeps. if len(inputs) != 0 { - t.Fatalf("had unsweeped inputs") + t.Fatalf("had unsweeped inputs: %v", inputs) } // Mine the first sweeps @@ -1377,9 +1383,11 @@ func TestLockTimes(t *testing.T) { // Results should all come back. for i := range results { - result := <-results[i] - if result.Err != nil { - t.Fatal("expected input to be swept") + select { + case result := <-results[i]: + require.NoError(t, result.Err) + case <-time.After(1 * time.Second): + t.Fatalf("result %v did not come back", i) } } } From 429a53e2cf1af2318c28ec1a73c994c613a155ec Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 1 Nov 2023 17:52:53 +0800 Subject: [PATCH 05/13] sweep: decrease coin selection lock scope This commit changes how `WithCoinSelectLock` is used - previously the lock is held when creating the input sets, now it's only be held after the input sets have been created and explicitly signal they need wallet inputs. --- sweep/aggregator.go | 53 ++++++++-------------------------- sweep/mocks.go | 6 ++-- sweep/sweeper.go | 55 +++++++++++++++++++++--------------- sweep/tx_input_set.go | 58 ++++++++++++++++++++++++++++++++------ sweep/tx_input_set_test.go | 8 +++--- 5 files changed, 100 insertions(+), 80 deletions(-) diff --git a/sweep/aggregator.go b/sweep/aggregator.go index 60d835abe09..260f9f6592f 100644 --- a/sweep/aggregator.go +++ b/sweep/aggregator.go @@ -4,8 +4,6 @@ import ( "sort" "github.com/btcsuite/btcd/wire" - "github.com/lightningnetwork/lnd/input" - "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/lnwallet/chainfee" ) @@ -36,9 +34,8 @@ type inputCluster struct { // the configured maximum number of inputs. Negative yield inputs are skipped. // No input sets with a total value after fees below the dust limit are // returned. -func (c *inputCluster) createInputSets( - wallet Wallet, maxFeeRate chainfee.SatPerKWeight, - maxInputs int) ([]InputSet, error) { +func (c *inputCluster) createInputSets(maxFeeRate chainfee.SatPerKWeight, + maxInputs int) []InputSet { // Turn the inputs into a slice so we can sort them. inputList := make([]*pendingInput, 0, len(c.inputs)) @@ -91,9 +88,7 @@ func (c *inputCluster) createInputSets( // Start building a set of positive-yield tx inputs under the // condition that the tx will be published with the specified // fee rate. - txInputs := newTxInputSet( - wallet, c.sweepFeeRate, maxFeeRate, maxInputs, - ) + txInputs := newTxInputSet(c.sweepFeeRate, maxFeeRate, maxInputs) // From the set of sweepable inputs, keep adding inputs to the // input set until the tx output value no longer goes up or the @@ -103,27 +98,7 @@ func (c *inputCluster) createInputSets( // If there are no positive yield inputs, we can stop here. inputCount := len(txInputs.inputs) if inputCount == 0 { - return sets, nil - } - - // Check the current output value and add wallet utxos if - // needed to push the output value to the lower limit. - if err := txInputs.tryAddWalletInputsIfNeeded(); err != nil { - return nil, err - } - - // If the output value of this block of inputs does not reach - // the dust limit, stop sweeping. Because of the sorting, - // continuing with the remaining inputs will only lead to sets - // with an even lower output value. - if !txInputs.enoughInput() { - // The change output is always a p2tr here. - dl := lnwallet.DustLimitForSize(input.P2TRSize) - log.Debugf("Input set value %v (required=%v, "+ - "change=%v) below dust limit of %v", - txInputs.totalOutput(), txInputs.requiredOutput, - txInputs.changeOutput, dl) - return sets, nil + return sets } log.Infof("Candidate sweep set of size=%v (+%v wallet inputs),"+ @@ -136,15 +111,16 @@ func (c *inputCluster) createInputSets( inputList = inputList[inputCount:] } - return sets, nil + return sets } // UtxoAggregator defines an interface that takes a list of inputs and // aggregate them into groups. Each group is used as the inputs to create a // sweeping transaction. type UtxoAggregator interface { - // ClusterInputs takes a list of inputs and groups them into clusters. - ClusterInputs(Wallet, pendingInputs) []InputSet + // ClusterInputs takes a list of inputs and groups them into input + // sets. Each input set will be used to create a sweeping transaction. + ClusterInputs(pendingInputs) []InputSet } // SimpleAggregator aggregates inputs known by the Sweeper based on each @@ -196,9 +172,7 @@ func NewSimpleUtxoAggregator(estimator chainfee.Estimator, // inputs known by the UtxoSweeper. It clusters inputs by // 1) Required tx locktime // 2) Similar fee rates. -func (s *SimpleAggregator) ClusterInputs( - wallet Wallet, inputs pendingInputs) []InputSet { - +func (s *SimpleAggregator) ClusterInputs(inputs pendingInputs) []InputSet { // We start by getting the inputs clusters by locktime. Since the // inputs commit to the locktime, they can only be clustered together // if the locktime is equal. @@ -220,14 +194,9 @@ func (s *SimpleAggregator) ClusterInputs( // Now that we have the clusters, we can create the input sets. var inputSets []InputSet for _, cluster := range clusters { - sets, err := cluster.createInputSets( - wallet, s.MaxFeeRate, s.MaxInputsPerTx, + sets := cluster.createInputSets( + s.MaxFeeRate, s.MaxInputsPerTx, ) - if err != nil { - log.Errorf("Unable to create input sets: %v", err) - continue - } - inputSets = append(inputSets, sets...) } diff --git a/sweep/mocks.go b/sweep/mocks.go index 665a16d93e5..154fc0f9402 100644 --- a/sweep/mocks.go +++ b/sweep/mocks.go @@ -37,10 +37,8 @@ type mockUtxoAggregator struct { var _ UtxoAggregator = (*mockUtxoAggregator)(nil) // ClusterInputs takes a list of inputs and groups them into clusters. -func (m *mockUtxoAggregator) ClusterInputs(wallet Wallet, - inputs pendingInputs) []InputSet { - - args := m.Called(wallet, inputs) +func (m *mockUtxoAggregator) ClusterInputs(inputs pendingInputs) []InputSet { + args := m.Called(inputs) return args.Get(0).([]InputSet) } diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 328d0b1c09e..a16694b2a21 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1531,33 +1531,44 @@ func (s *UtxoSweeper) updateSweeperInputs() pendingInputs { // sweepPendingInputs is called when the ticker fires. It will create clusters // and attempt to create and publish the sweeping transactions. func (s *UtxoSweeper) sweepPendingInputs(inputs pendingInputs) { - // Execute the sweep within a coin select lock. Otherwise the coins - // that we are going to spend may be selected for other transactions - // like funding of a channel. - // - // TODO(yy): decrease the lock scope - we need to remove the wallet - // used here, which means we need to ask the aggregator to return input - // sets and specifying whether wallet utoxs are needed or not. Then, by - // calling `TxInput.NeedWalletInput`, we can then lock and add the - // wallet input, creating a much smaller lock scope. - err := s.cfg.Wallet.WithCoinSelectLock(func() error { - // Cluster all of our inputs based on the specific Aggregator. - inputSets := s.cfg.Aggregator.ClusterInputs( - s.cfg.Wallet, inputs, - ) + // Cluster all of our inputs based on the specific Aggregator. + sets := s.cfg.Aggregator.ClusterInputs(inputs) + + // sweepWithLock is a helper closure that executes the sweep within a + // coin select lock to prevent the coins being selected for other + // transactions like funding of a channel. + sweepWithLock := func(set InputSet) error { + return s.cfg.Wallet.WithCoinSelectLock(func() error { + // Try to add inputs from our wallet. + err := set.AddWalletInputs(s.cfg.Wallet) + if err != nil { + return err + } - // Create sweeping transaction for each set. - for _, inputs := range inputSets { - err := s.sweep(inputs) + // Create sweeping transaction for each set. + err = s.sweep(set) if err != nil { - log.Errorf("sweep new inputs: %v", err) + return err } + + return nil + }) + } + + for _, set := range sets { + var err error + if set.NeedWalletInput() { + // Sweep the set of inputs that need the wallet inputs. + err = sweepWithLock(set) + } else { + // Sweep the set of inputs that don't need the wallet + // inputs. + err = s.sweep(set) } - return nil - }) - if err != nil { - log.Errorf("input cluster sweep: %v", err) + if err != nil { + log.Errorf("Sweep new inputs: %v", err) + } } } diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index 6beba7a742a..09367624f57 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -30,6 +30,12 @@ const ( constraintsForce ) +var ( + // ErrNotEnoughInputs is returned when there are not enough wallet + // inputs to construct a non-dust change output for an input set. + ErrNotEnoughInputs = fmt.Errorf("not enough inputs") +) + // InputSet defines an interface that's responsible for filtering a set of // inputs that can be swept economically. type InputSet interface { @@ -38,6 +44,15 @@ type InputSet interface { // FeeRate returns the fee rate that should be used for the tx. FeeRate() chainfee.SatPerKWeight + + // AddWalletInputs adds wallet inputs to the set until a non-dust + // change output can be made. Return an error if there are not enough + // wallet inputs. + AddWalletInputs(wallet Wallet) error + + // NeedWalletInput returns true if the input set needs more wallet + // inputs. + NeedWalletInput() bool } type txInputSetState struct { @@ -125,17 +140,13 @@ type txInputSet struct { // maxInputs is the maximum number of inputs that will be accepted in // the set. maxInputs int - - // wallet contains wallet functionality required by the input set to - // retrieve utxos. - wallet Wallet } // Compile-time constraint to ensure txInputSet implements InputSet. var _ InputSet = (*txInputSet)(nil) // newTxInputSet constructs a new, empty input set. -func newTxInputSet(wallet Wallet, feePerKW, maxFeeRate chainfee.SatPerKWeight, +func newTxInputSet(feePerKW, maxFeeRate chainfee.SatPerKWeight, maxInputs int) *txInputSet { state := txInputSetState{ @@ -145,7 +156,6 @@ func newTxInputSet(wallet Wallet, feePerKW, maxFeeRate chainfee.SatPerKWeight, b := txInputSet{ maxInputs: maxInputs, - wallet: wallet, txInputSetState: state, } @@ -162,6 +172,11 @@ func (t *txInputSet) FeeRate() chainfee.SatPerKWeight { return t.feeRate } +// NeedWalletInput returns true if the input set needs more wallet inputs. +func (t *txInputSet) NeedWalletInput() bool { + return !t.enoughInput() +} + // enoughInput returns true if we've accumulated enough inputs to pay the fees // and have at least one output that meets the dust limit. func (t *txInputSet) enoughInput() bool { @@ -384,9 +399,36 @@ func (t *txInputSet) addPositiveYieldInputs(sweepableInputs []*pendingInput) { // We managed to add all inputs to the set. } +// AddWalletInputs adds wallet inputs to the set until a non-dust output can be +// made. This non-dust output is either a change output or a required output. +// Return an error if there are not enough wallet inputs. +func (t *txInputSet) AddWalletInputs(wallet Wallet) error { + // Check the current output value and add wallet utxos if needed to + // push the output value to the lower limit. + if err := t.tryAddWalletInputsIfNeeded(wallet); err != nil { + return err + } + + // If the output value of this block of inputs does not reach the dust + // limit, stop sweeping. Because of the sorting, continuing with the + // remaining inputs will only lead to sets with an even lower output + // value. + if !t.enoughInput() { + // The change output is always a p2tr here. + dl := lnwallet.DustLimitForSize(input.P2TRSize) + log.Debugf("Input set value %v (required=%v, change=%v) "+ + "below dust limit of %v", t.totalOutput(), + t.requiredOutput, t.changeOutput, dl) + + return ErrNotEnoughInputs + } + + return nil +} + // tryAddWalletInputsIfNeeded retrieves utxos from the wallet and tries adding // as many as required to bring the tx output value above the given minimum. -func (t *txInputSet) tryAddWalletInputsIfNeeded() error { +func (t *txInputSet) tryAddWalletInputsIfNeeded(wallet Wallet) error { // If we've already have enough to pay the transaction fees and have at // least one output materialize, no action is needed. if t.enoughInput() { @@ -396,7 +438,7 @@ func (t *txInputSet) tryAddWalletInputsIfNeeded() error { // Retrieve wallet utxos. Only consider confirmed utxos to prevent // problems around RBF rules for unconfirmed inputs. This currently // ignores the configured coin selection strategy. - utxos, err := t.wallet.ListUnspentWitnessFromDefaultAccount( + utxos, err := wallet.ListUnspentWitnessFromDefaultAccount( 1, math.MaxInt32, ) if err != nil { diff --git a/sweep/tx_input_set_test.go b/sweep/tx_input_set_test.go index 110db1af132..51afff7b77a 100644 --- a/sweep/tx_input_set_test.go +++ b/sweep/tx_input_set_test.go @@ -16,7 +16,7 @@ func TestTxInputSet(t *testing.T) { feeRate = 1000 maxInputs = 10 ) - set := newTxInputSet(nil, feeRate, 0, maxInputs) + set := newTxInputSet(feeRate, 0, maxInputs) // Create a 300 sat input. The fee to sweep this input to a P2WKH output // is 439 sats. That means that this input yields -139 sats and we @@ -65,7 +65,7 @@ func TestTxInputSetFromWallet(t *testing.T) { ) wallet := &mockWallet{} - set := newTxInputSet(wallet, feeRate, 0, maxInputs) + set := newTxInputSet(feeRate, 0, maxInputs) // Add a 500 sat input to the set. It yields positively, but doesn't // reach the output dust limit. @@ -86,7 +86,7 @@ func TestTxInputSetFromWallet(t *testing.T) { t.Fatal("expected forced add to succeed") } - err := set.tryAddWalletInputsIfNeeded() + err := set.AddWalletInputs(wallet) if err != nil { t.Fatal(err) } @@ -134,7 +134,7 @@ func TestTxInputSetRequiredOutput(t *testing.T) { feeRate = 1000 maxInputs = 10 ) - set := newTxInputSet(nil, feeRate, 0, maxInputs) + set := newTxInputSet(feeRate, 0, maxInputs) // Attempt to add an input with a required txout below the dust limit. // This should fail since we cannot trim such outputs. From e46c99a7a0b53455ed98619c19a33d9220d4a416 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 25 Jan 2024 02:38:00 +0800 Subject: [PATCH 06/13] sweep: remove deprecated `rand.Seed` --- sweep/sweeper.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/sweep/sweeper.go b/sweep/sweeper.go index a16694b2a21..a785bdacc39 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -3,7 +3,6 @@ package sweep import ( "errors" "fmt" - "math/rand" "sync" "sync/atomic" "time" @@ -1571,8 +1570,3 @@ func (s *UtxoSweeper) sweepPendingInputs(inputs pendingInputs) { } } } - -// init initializes the random generator for random input rescheduling. -func init() { - rand.Seed(time.Now().Unix()) -} From 419013732778cf4265aebaf867eebc12f461e165 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 30 Jan 2024 17:51:20 +0800 Subject: [PATCH 07/13] sweep: move mocks into one file --- sweep/backend_mock_test.go | 164 --------------- sweep/fee_estimator_mock_test.go | 80 -------- sweep/mock_test.go | 334 +++++++++++++++++++++++++++++++ sweep/mocks.go | 44 ---- sweep/store_mock.go | 60 ------ 5 files changed, 334 insertions(+), 348 deletions(-) delete mode 100644 sweep/backend_mock_test.go delete mode 100644 sweep/fee_estimator_mock_test.go create mode 100644 sweep/mock_test.go delete mode 100644 sweep/mocks.go delete mode 100644 sweep/store_mock.go diff --git a/sweep/backend_mock_test.go b/sweep/backend_mock_test.go deleted file mode 100644 index 9fd79fa0355..00000000000 --- a/sweep/backend_mock_test.go +++ /dev/null @@ -1,164 +0,0 @@ -package sweep - -import ( - "sync" - "testing" - "time" - - "github.com/btcsuite/btcd/chaincfg/chainhash" - "github.com/btcsuite/btcd/wire" - "github.com/lightningnetwork/lnd/lnwallet" -) - -// mockBackend simulates a chain backend for realistic behaviour in unit tests -// around double spends. -type mockBackend struct { - t *testing.T - - lock sync.Mutex - - notifier *MockNotifier - - confirmedSpendInputs map[wire.OutPoint]struct{} - - unconfirmedTxes map[chainhash.Hash]*wire.MsgTx - unconfirmedSpendInputs map[wire.OutPoint]struct{} - - publishChan chan wire.MsgTx - - walletUtxos []*lnwallet.Utxo - utxoCnt int -} - -func newMockBackend(t *testing.T, notifier *MockNotifier) *mockBackend { - return &mockBackend{ - t: t, - notifier: notifier, - unconfirmedTxes: make(map[chainhash.Hash]*wire.MsgTx), - confirmedSpendInputs: make(map[wire.OutPoint]struct{}), - unconfirmedSpendInputs: make(map[wire.OutPoint]struct{}), - publishChan: make(chan wire.MsgTx, 2), - } -} - -func (b *mockBackend) publishTransaction(tx *wire.MsgTx) error { - b.lock.Lock() - defer b.lock.Unlock() - - txHash := tx.TxHash() - if _, ok := b.unconfirmedTxes[txHash]; ok { - // Tx already exists - testLog.Tracef("mockBackend duplicate tx %v", tx.TxHash()) - return lnwallet.ErrDoubleSpend - } - - for _, in := range tx.TxIn { - if _, ok := b.unconfirmedSpendInputs[in.PreviousOutPoint]; ok { - // Double spend - testLog.Tracef("mockBackend double spend tx %v", tx.TxHash()) - return lnwallet.ErrDoubleSpend - } - - if _, ok := b.confirmedSpendInputs[in.PreviousOutPoint]; ok { - // Already included in block - testLog.Tracef("mockBackend already in block tx %v", tx.TxHash()) - return lnwallet.ErrDoubleSpend - } - } - - b.unconfirmedTxes[txHash] = tx - for _, in := range tx.TxIn { - b.unconfirmedSpendInputs[in.PreviousOutPoint] = struct{}{} - } - - testLog.Tracef("mockBackend publish tx %v", tx.TxHash()) - - return nil -} - -func (b *mockBackend) PublishTransaction(tx *wire.MsgTx, _ string) error { - log.Tracef("Publishing tx %v", tx.TxHash()) - err := b.publishTransaction(tx) - select { - case b.publishChan <- *tx: - case <-time.After(defaultTestTimeout): - b.t.Fatalf("unexpected tx published") - } - return err -} - -func (b *mockBackend) ListUnspentWitnessFromDefaultAccount(minConfs, maxConfs int32) ( - []*lnwallet.Utxo, error) { - - b.lock.Lock() - defer b.lock.Unlock() - - // Each time we list output, we increment the utxo counter, to - // ensure we don't return the same outpoint every time. - b.utxoCnt++ - - for i := range b.walletUtxos { - b.walletUtxos[i].OutPoint.Hash[0] = byte(b.utxoCnt) - } - - return b.walletUtxos, nil -} - -func (b *mockBackend) WithCoinSelectLock(f func() error) error { - return f() -} - -func (b *mockBackend) deleteUnconfirmed(txHash chainhash.Hash) { - b.lock.Lock() - defer b.lock.Unlock() - - tx, ok := b.unconfirmedTxes[txHash] - if !ok { - // Tx already exists - testLog.Errorf("mockBackend delete tx not existing %v", txHash) - return - } - - testLog.Tracef("mockBackend delete tx %v", tx.TxHash()) - delete(b.unconfirmedTxes, txHash) - for _, in := range tx.TxIn { - delete(b.unconfirmedSpendInputs, in.PreviousOutPoint) - } -} - -func (b *mockBackend) mine() { - b.lock.Lock() - defer b.lock.Unlock() - - notifications := make(map[wire.OutPoint]*wire.MsgTx) - for _, tx := range b.unconfirmedTxes { - testLog.Tracef("mockBackend mining tx %v", tx.TxHash()) - for _, in := range tx.TxIn { - b.confirmedSpendInputs[in.PreviousOutPoint] = struct{}{} - notifications[in.PreviousOutPoint] = tx - } - } - b.unconfirmedSpendInputs = make(map[wire.OutPoint]struct{}) - b.unconfirmedTxes = make(map[chainhash.Hash]*wire.MsgTx) - - for outpoint, tx := range notifications { - testLog.Tracef("mockBackend delivering spend ntfn for %v", - outpoint) - b.notifier.SpendOutpoint(outpoint, *tx) - } -} - -func (b *mockBackend) isDone() bool { - return len(b.unconfirmedTxes) == 0 -} - -func (b *mockBackend) RemoveDescendants(*wire.MsgTx) error { - return nil -} - -func (b *mockBackend) FetchTx(chainhash.Hash) (*wire.MsgTx, error) { - return nil, nil -} - -func (b *mockBackend) CancelRebroadcast(tx chainhash.Hash) { -} diff --git a/sweep/fee_estimator_mock_test.go b/sweep/fee_estimator_mock_test.go deleted file mode 100644 index ab6dcdfd50a..00000000000 --- a/sweep/fee_estimator_mock_test.go +++ /dev/null @@ -1,80 +0,0 @@ -package sweep - -import ( - "sync" - - "github.com/lightningnetwork/lnd/lnwallet/chainfee" -) - -// 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 - - relayFee chainfee.SatPerKWeight - - blocksToFee map[uint32]chainfee.SatPerKWeight - - // A closure that when set is used instead of the - // mockFeeEstimator.EstimateFeePerKW method. - estimateFeePerKW func(numBlocks uint32) (chainfee.SatPerKWeight, error) - - lock sync.Mutex -} - -func newMockFeeEstimator(feePerKW, - relayFee chainfee.SatPerKWeight) *mockFeeEstimator { - - return &mockFeeEstimator{ - feePerKW: feePerKW, - relayFee: relayFee, - blocksToFee: make(map[uint32]chainfee.SatPerKWeight), - } -} - -func (e *mockFeeEstimator) updateFees(feePerKW, - relayFee chainfee.SatPerKWeight) { - - e.lock.Lock() - defer e.lock.Unlock() - - e.feePerKW = feePerKW - e.relayFee = relayFee -} - -func (e *mockFeeEstimator) EstimateFeePerKW(numBlocks uint32) ( - chainfee.SatPerKWeight, error) { - - e.lock.Lock() - defer e.lock.Unlock() - - if e.estimateFeePerKW != nil { - return e.estimateFeePerKW(numBlocks) - } - - if fee, ok := e.blocksToFee[numBlocks]; ok { - return fee, nil - } - - return e.feePerKW, nil -} - -func (e *mockFeeEstimator) RelayFeePerKW() chainfee.SatPerKWeight { - e.lock.Lock() - defer e.lock.Unlock() - - return e.relayFee -} - -func (e *mockFeeEstimator) Start() error { - return nil -} - -func (e *mockFeeEstimator) Stop() error { - return nil -} - -var _ chainfee.Estimator = (*mockFeeEstimator)(nil) diff --git a/sweep/mock_test.go b/sweep/mock_test.go new file mode 100644 index 00000000000..5ef6b78a226 --- /dev/null +++ b/sweep/mock_test.go @@ -0,0 +1,334 @@ +package sweep + +import ( + "sync" + "testing" + "time" + + "github.com/btcsuite/btcd/chaincfg/chainhash" + "github.com/btcsuite/btcd/wire" + "github.com/lightningnetwork/lnd/lnwallet" + "github.com/lightningnetwork/lnd/lnwallet/chainfee" + "github.com/stretchr/testify/mock" +) + +// mockBackend simulates a chain backend for realistic behaviour in unit tests +// around double spends. +type mockBackend struct { + t *testing.T + + lock sync.Mutex + + notifier *MockNotifier + + confirmedSpendInputs map[wire.OutPoint]struct{} + + unconfirmedTxes map[chainhash.Hash]*wire.MsgTx + unconfirmedSpendInputs map[wire.OutPoint]struct{} + + publishChan chan wire.MsgTx + + walletUtxos []*lnwallet.Utxo + utxoCnt int +} + +func newMockBackend(t *testing.T, notifier *MockNotifier) *mockBackend { + return &mockBackend{ + t: t, + notifier: notifier, + unconfirmedTxes: make(map[chainhash.Hash]*wire.MsgTx), + confirmedSpendInputs: make(map[wire.OutPoint]struct{}), + unconfirmedSpendInputs: make(map[wire.OutPoint]struct{}), + publishChan: make(chan wire.MsgTx, 2), + } +} + +func (b *mockBackend) publishTransaction(tx *wire.MsgTx) error { + b.lock.Lock() + defer b.lock.Unlock() + + txHash := tx.TxHash() + if _, ok := b.unconfirmedTxes[txHash]; ok { + // Tx already exists + testLog.Tracef("mockBackend duplicate tx %v", tx.TxHash()) + return lnwallet.ErrDoubleSpend + } + + for _, in := range tx.TxIn { + if _, ok := b.unconfirmedSpendInputs[in.PreviousOutPoint]; ok { + // Double spend + testLog.Tracef("mockBackend double spend tx %v", + tx.TxHash()) + return lnwallet.ErrDoubleSpend + } + + if _, ok := b.confirmedSpendInputs[in.PreviousOutPoint]; ok { + // Already included in block + testLog.Tracef("mockBackend already in block tx %v", + tx.TxHash()) + return lnwallet.ErrDoubleSpend + } + } + + b.unconfirmedTxes[txHash] = tx + for _, in := range tx.TxIn { + b.unconfirmedSpendInputs[in.PreviousOutPoint] = struct{}{} + } + + testLog.Tracef("mockBackend publish tx %v", tx.TxHash()) + + return nil +} + +func (b *mockBackend) PublishTransaction(tx *wire.MsgTx, _ string) error { + log.Tracef("Publishing tx %v", tx.TxHash()) + err := b.publishTransaction(tx) + select { + case b.publishChan <- *tx: + case <-time.After(defaultTestTimeout): + b.t.Fatalf("unexpected tx published") + } + + return err +} + +func (b *mockBackend) ListUnspentWitnessFromDefaultAccount(minConfs, + maxConfs int32) ([]*lnwallet.Utxo, error) { + + b.lock.Lock() + defer b.lock.Unlock() + + // Each time we list output, we increment the utxo counter, to + // ensure we don't return the same outpoint every time. + b.utxoCnt++ + + for i := range b.walletUtxos { + b.walletUtxos[i].OutPoint.Hash[0] = byte(b.utxoCnt) + } + + return b.walletUtxos, nil +} + +func (b *mockBackend) WithCoinSelectLock(f func() error) error { + return f() +} + +func (b *mockBackend) deleteUnconfirmed(txHash chainhash.Hash) { + b.lock.Lock() + defer b.lock.Unlock() + + tx, ok := b.unconfirmedTxes[txHash] + if !ok { + // Tx already exists + testLog.Errorf("mockBackend delete tx not existing %v", txHash) + return + } + + testLog.Tracef("mockBackend delete tx %v", tx.TxHash()) + delete(b.unconfirmedTxes, txHash) + for _, in := range tx.TxIn { + delete(b.unconfirmedSpendInputs, in.PreviousOutPoint) + } +} + +func (b *mockBackend) mine() { + b.lock.Lock() + defer b.lock.Unlock() + + notifications := make(map[wire.OutPoint]*wire.MsgTx) + for _, tx := range b.unconfirmedTxes { + testLog.Tracef("mockBackend mining tx %v", tx.TxHash()) + for _, in := range tx.TxIn { + b.confirmedSpendInputs[in.PreviousOutPoint] = struct{}{} + notifications[in.PreviousOutPoint] = tx + } + } + b.unconfirmedSpendInputs = make(map[wire.OutPoint]struct{}) + b.unconfirmedTxes = make(map[chainhash.Hash]*wire.MsgTx) + + for outpoint, tx := range notifications { + testLog.Tracef("mockBackend delivering spend ntfn for %v", + outpoint) + b.notifier.SpendOutpoint(outpoint, *tx) + } +} + +func (b *mockBackend) isDone() bool { + return len(b.unconfirmedTxes) == 0 +} + +func (b *mockBackend) RemoveDescendants(*wire.MsgTx) error { + return nil +} + +func (b *mockBackend) FetchTx(chainhash.Hash) (*wire.MsgTx, error) { + return nil, nil +} + +func (b *mockBackend) CancelRebroadcast(tx chainhash.Hash) { +} + +// 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 + + relayFee chainfee.SatPerKWeight + + blocksToFee map[uint32]chainfee.SatPerKWeight + + // A closure that when set is used instead of the + // mockFeeEstimator.EstimateFeePerKW method. + estimateFeePerKW func(numBlocks uint32) (chainfee.SatPerKWeight, error) + + lock sync.Mutex +} + +func newMockFeeEstimator(feePerKW, + relayFee chainfee.SatPerKWeight) *mockFeeEstimator { + + return &mockFeeEstimator{ + feePerKW: feePerKW, + relayFee: relayFee, + blocksToFee: make(map[uint32]chainfee.SatPerKWeight), + } +} + +func (e *mockFeeEstimator) updateFees(feePerKW, + relayFee chainfee.SatPerKWeight) { + + e.lock.Lock() + defer e.lock.Unlock() + + e.feePerKW = feePerKW + e.relayFee = relayFee +} + +func (e *mockFeeEstimator) EstimateFeePerKW(numBlocks uint32) ( + chainfee.SatPerKWeight, error) { + + e.lock.Lock() + defer e.lock.Unlock() + + if e.estimateFeePerKW != nil { + return e.estimateFeePerKW(numBlocks) + } + + if fee, ok := e.blocksToFee[numBlocks]; ok { + return fee, nil + } + + return e.feePerKW, nil +} + +func (e *mockFeeEstimator) RelayFeePerKW() chainfee.SatPerKWeight { + e.lock.Lock() + defer e.lock.Unlock() + + return e.relayFee +} + +func (e *mockFeeEstimator) Start() error { + return nil +} + +func (e *mockFeeEstimator) Stop() error { + return nil +} + +var _ chainfee.Estimator = (*mockFeeEstimator)(nil) + +// MockSweeperStore is a mock implementation of sweeper store. This type is +// exported, because it is currently used in nursery tests too. +type MockSweeperStore struct { + mock.Mock +} + +// NewMockSweeperStore returns a new instance. +func NewMockSweeperStore() *MockSweeperStore { + return &MockSweeperStore{} +} + +// IsOurTx determines whether a tx is published by us, based on its hash. +func (s *MockSweeperStore) IsOurTx(hash chainhash.Hash) (bool, error) { + args := s.Called(hash) + + return args.Bool(0), args.Error(1) +} + +// StoreTx stores a tx we are about to publish. +func (s *MockSweeperStore) StoreTx(tr *TxRecord) error { + args := s.Called(tr) + return args.Error(0) +} + +// ListSweeps lists all the sweeps we have successfully published. +func (s *MockSweeperStore) ListSweeps() ([]chainhash.Hash, error) { + args := s.Called() + + return args.Get(0).([]chainhash.Hash), args.Error(1) +} + +// GetTx queries the database to find the tx that matches the given txid. +// Returns ErrTxNotFound if it cannot be found. +func (s *MockSweeperStore) GetTx(hash chainhash.Hash) (*TxRecord, error) { + args := s.Called(hash) + + tr := args.Get(0) + if tr != nil { + return args.Get(0).(*TxRecord), args.Error(1) + } + + return nil, args.Error(1) +} + +// DeleteTx removes the given tx from db. +func (s *MockSweeperStore) DeleteTx(txid chainhash.Hash) error { + args := s.Called(txid) + + return args.Error(0) +} + +// Compile-time constraint to ensure MockSweeperStore implements SweeperStore. +var _ SweeperStore = (*MockSweeperStore)(nil) + +type MockFeePreference struct { + mock.Mock +} + +// Compile-time constraint to ensure MockFeePreference implements FeePreference. +var _ FeePreference = (*MockFeePreference)(nil) + +func (m *MockFeePreference) String() string { + return "mock fee preference" +} + +func (m *MockFeePreference) Estimate(estimator chainfee.Estimator, + maxFeeRate chainfee.SatPerKWeight) (chainfee.SatPerKWeight, error) { + + args := m.Called(estimator, maxFeeRate) + + if args.Get(0) == nil { + return 0, args.Error(1) + } + + return args.Get(0).(chainfee.SatPerKWeight), args.Error(1) +} + +type mockUtxoAggregator struct { + mock.Mock +} + +// Compile-time constraint to ensure mockUtxoAggregator implements +// UtxoAggregator. +var _ UtxoAggregator = (*mockUtxoAggregator)(nil) + +// ClusterInputs takes a list of inputs and groups them into clusters. +func (m *mockUtxoAggregator) ClusterInputs(inputs pendingInputs) []InputSet { + args := m.Called(inputs) + + return args.Get(0).([]InputSet) +} diff --git a/sweep/mocks.go b/sweep/mocks.go deleted file mode 100644 index 154fc0f9402..00000000000 --- a/sweep/mocks.go +++ /dev/null @@ -1,44 +0,0 @@ -package sweep - -import ( - "github.com/lightningnetwork/lnd/lnwallet/chainfee" - "github.com/stretchr/testify/mock" -) - -type MockFeePreference struct { - mock.Mock -} - -// Compile-time constraint to ensure MockFeePreference implements FeePreference. -var _ FeePreference = (*MockFeePreference)(nil) - -func (m *MockFeePreference) String() string { - return "mock fee preference" -} - -func (m *MockFeePreference) Estimate(estimator chainfee.Estimator, - maxFeeRate chainfee.SatPerKWeight) (chainfee.SatPerKWeight, error) { - - args := m.Called(estimator, maxFeeRate) - - if args.Get(0) == nil { - return 0, args.Error(1) - } - - return args.Get(0).(chainfee.SatPerKWeight), args.Error(1) -} - -type mockUtxoAggregator struct { - mock.Mock -} - -// Compile-time constraint to ensure mockUtxoAggregator implements -// UtxoAggregator. -var _ UtxoAggregator = (*mockUtxoAggregator)(nil) - -// ClusterInputs takes a list of inputs and groups them into clusters. -func (m *mockUtxoAggregator) ClusterInputs(inputs pendingInputs) []InputSet { - args := m.Called(inputs) - - return args.Get(0).([]InputSet) -} diff --git a/sweep/store_mock.go b/sweep/store_mock.go deleted file mode 100644 index 73b797963dc..00000000000 --- a/sweep/store_mock.go +++ /dev/null @@ -1,60 +0,0 @@ -package sweep - -import ( - "github.com/btcsuite/btcd/chaincfg/chainhash" - "github.com/stretchr/testify/mock" -) - -// MockSweeperStore is a mock implementation of sweeper store. This type is -// exported, because it is currently used in nursery tests too. -type MockSweeperStore struct { - mock.Mock -} - -// NewMockSweeperStore returns a new instance. -func NewMockSweeperStore() *MockSweeperStore { - return &MockSweeperStore{} -} - -// IsOurTx determines whether a tx is published by us, based on its hash. -func (s *MockSweeperStore) IsOurTx(hash chainhash.Hash) (bool, error) { - args := s.Called(hash) - - return args.Bool(0), args.Error(1) -} - -// StoreTx stores a tx we are about to publish. -func (s *MockSweeperStore) StoreTx(tr *TxRecord) error { - args := s.Called(tr) - return args.Error(0) -} - -// ListSweeps lists all the sweeps we have successfully published. -func (s *MockSweeperStore) ListSweeps() ([]chainhash.Hash, error) { - args := s.Called() - - return args.Get(0).([]chainhash.Hash), args.Error(1) -} - -// GetTx queries the database to find the tx that matches the given txid. -// Returns ErrTxNotFound if it cannot be found. -func (s *MockSweeperStore) GetTx(hash chainhash.Hash) (*TxRecord, error) { - args := s.Called(hash) - - tr := args.Get(0) - if tr != nil { - return args.Get(0).(*TxRecord), args.Error(1) - } - - return nil, args.Error(1) -} - -// DeleteTx removes the given tx from db. -func (s *MockSweeperStore) DeleteTx(txid chainhash.Hash) error { - args := s.Called(txid) - - return args.Error(0) -} - -// Compile-time constraint to ensure MockSweeperStore implements SweeperStore. -var _ SweeperStore = (*MockSweeperStore)(nil) From 47be8d259b5b2740cb7684db45009a71fdaf794e Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 31 Jan 2024 03:25:58 +0800 Subject: [PATCH 08/13] sweep: add mocks and patch unit test for `sweepPendingInputs` --- sweep/mock_test.go | 114 ++++++++++++++++++++++++++++++++++++++++++ sweep/sweeper_test.go | 64 ++++++++++++++++++++++++ 2 files changed, 178 insertions(+) diff --git a/sweep/mock_test.go b/sweep/mock_test.go index 5ef6b78a226..fc7ff9c34d7 100644 --- a/sweep/mock_test.go +++ b/sweep/mock_test.go @@ -7,6 +7,7 @@ import ( "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/wire" + "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/lnwallet/chainfee" "github.com/stretchr/testify/mock" @@ -332,3 +333,116 @@ func (m *mockUtxoAggregator) ClusterInputs(inputs pendingInputs) []InputSet { return args.Get(0).([]InputSet) } + +// MockWallet is a mock implementation of the Wallet interface. +type MockWallet struct { + mock.Mock +} + +// Compile-time constraint to ensure MockWallet implements Wallet. +var _ Wallet = (*MockWallet)(nil) + +// PublishTransaction performs cursory validation (dust checks, etc) and +// broadcasts the passed transaction to the Bitcoin network. +func (m *MockWallet) PublishTransaction(tx *wire.MsgTx, label string) error { + args := m.Called(tx, label) + + return args.Error(0) +} + +// ListUnspentWitnessFromDefaultAccount returns all unspent outputs which are +// version 0 witness programs from the default wallet account. The 'minConfs' +// and 'maxConfs' parameters indicate the minimum and maximum number of +// confirmations an output needs in order to be returned by this method. +func (m *MockWallet) ListUnspentWitnessFromDefaultAccount( + minConfs, maxConfs int32) ([]*lnwallet.Utxo, error) { + + args := m.Called(minConfs, maxConfs) + if args.Get(0) == nil { + return nil, args.Error(1) + } + + return args.Get(0).([]*lnwallet.Utxo), args.Error(1) +} + +// WithCoinSelectLock will execute the passed function closure in a +// synchronized manner preventing any coin selection operations from proceeding +// while the closure is executing. This can be seen as the ability to execute a +// function closure under an exclusive coin selection lock. +func (m *MockWallet) WithCoinSelectLock(f func() error) error { + m.Called(f) + + return f() +} + +// RemoveDescendants removes any wallet transactions that spends +// outputs created by the specified transaction. +func (m *MockWallet) RemoveDescendants(tx *wire.MsgTx) error { + args := m.Called(tx) + + return args.Error(0) +} + +// FetchTx returns the transaction that corresponds to the transaction +// hash passed in. If the transaction can't be found then a nil +// transaction pointer is returned. +func (m *MockWallet) FetchTx(txid chainhash.Hash) (*wire.MsgTx, error) { + args := m.Called(txid) + + if args.Get(0) == nil { + return nil, args.Error(1) + } + + return args.Get(0).(*wire.MsgTx), args.Error(1) +} + +// CancelRebroadcast is used to inform the rebroadcaster sub-system +// that it no longer needs to try to rebroadcast a transaction. This is +// used to ensure that invalid transactions (inputs spent) aren't +// retried in the background. +func (m *MockWallet) CancelRebroadcast(tx chainhash.Hash) { + m.Called(tx) +} + +// MockInputSet is a mock implementation of the InputSet interface. +type MockInputSet struct { + mock.Mock +} + +// Compile-time constraint to ensure MockInputSet implements InputSet. +var _ InputSet = (*MockInputSet)(nil) + +// Inputs returns the set of inputs that should be used to create a tx. +func (m *MockInputSet) Inputs() []input.Input { + args := m.Called() + + if args.Get(0) == nil { + return nil + } + + return args.Get(0).([]input.Input) +} + +// FeeRate returns the fee rate that should be used for the tx. +func (m *MockInputSet) FeeRate() chainfee.SatPerKWeight { + args := m.Called() + + return args.Get(0).(chainfee.SatPerKWeight) +} + +// AddWalletInputs adds wallet inputs to the set until a non-dust +// change output can be made. Return an error if there are not enough +// wallet inputs. +func (m *MockInputSet) AddWalletInputs(wallet Wallet) error { + args := m.Called(wallet) + + return args.Error(0) +} + +// NeedWalletInput returns true if the input set needs more wallet +// inputs. +func (m *MockInputSet) NeedWalletInput() bool { + args := m.Called() + + return args.Bool(0) +} diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index e7cd27827e2..7396407b796 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -21,6 +21,7 @@ import ( lnmock "github.com/lightningnetwork/lnd/lntest/mock" "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/lnwallet/chainfee" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -2413,3 +2414,66 @@ func TestMarkInputFailed(t *testing.T) { // Assert the state is updated. require.Equal(t, StateFailed, pi.state) } + +// TestSweepPendingInputs checks that `sweepPendingInputs` correctly executes +// its workflow based on the returned values from the interfaces. +func TestSweepPendingInputs(t *testing.T) { + t.Parallel() + + // Create a mock wallet and aggregator. + wallet := &MockWallet{} + aggregator := &mockUtxoAggregator{} + + // Create a test sweeper. + s := New(&UtxoSweeperConfig{ + Wallet: wallet, + Aggregator: aggregator, + }) + + // Create an input set that needs wallet inputs. + setNeedWallet := &MockInputSet{} + + // Mock this set to ask for wallet input. + setNeedWallet.On("NeedWalletInput").Return(true).Once() + setNeedWallet.On("AddWalletInputs", wallet).Return(nil).Once() + + // Mock the wallet to require the lock once. + wallet.On("WithCoinSelectLock", mock.Anything).Return(nil).Once() + + // Create an input set that doesn't need wallet inputs. + normalSet := &MockInputSet{} + normalSet.On("NeedWalletInput").Return(false).Once() + + // Mock the methods used in `sweep`. This is not important for this + // unit test. + feeRate := chainfee.SatPerKWeight(1000) + setNeedWallet.On("Inputs").Return(nil).Once() + setNeedWallet.On("FeeRate").Return(feeRate).Once() + normalSet.On("Inputs").Return(nil).Once() + normalSet.On("FeeRate").Return(feeRate).Once() + + // Make pending inputs for testing. We don't need real values here as + // the returned clusters are mocked. + pis := make(pendingInputs) + + // Mock the aggregator to return the mocked input sets. + aggregator.On("ClusterInputs", pis).Return([]InputSet{ + setNeedWallet, normalSet, + }) + + // Set change output script to an invalid value. This should cause the + // `createSweepTx` inside `sweep` to fail. This is done so we can + // terminate the method early as we are only interested in testing the + // workflow in `sweepPendingInputs`. We don't need to test `sweep` here + // as it should be tested in its own unit test. + s.currentOutputScript = []byte{1} + + // Call the method under test. + s.sweepPendingInputs(pis) + + // Assert mocked methods are called as expected. + wallet.AssertExpectations(t) + aggregator.AssertExpectations(t) + setNeedWallet.AssertExpectations(t) + normalSet.AssertExpectations(t) +} From 1d3d2c7b82f7eb57b780909cdfa1091c7d2d3d69 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 31 Jan 2024 14:06:47 +0800 Subject: [PATCH 09/13] multi: deprecate `batchwindowduration` config option --- cmd/lncli/walletrpc_active.go | 4 ---- config.go | 3 +-- lncfg/sweeper.go | 2 +- sample-lnd.conf | 5 +++-- server.go | 4 ---- sweep/defaults.go | 7 ------- sweep/sweeper.go | 7 ------- sweep/sweeper_test.go | 9 ++++----- 8 files changed, 9 insertions(+), 32 deletions(-) diff --git a/cmd/lncli/walletrpc_active.go b/cmd/lncli/walletrpc_active.go index 3e01ecf1420..2f892bc465f 100644 --- a/cmd/lncli/walletrpc_active.go +++ b/cmd/lncli/walletrpc_active.go @@ -286,8 +286,6 @@ var bumpCloseFeeCommand = cli.Command{ to sweep the anchor outputs of the closing transaction at the requested fee rate or confirmation target. The specified fee rate will be the effective fee rate taking the parent fee into account. - Depending on the sweeper configuration (batchwindowduration) the sweeptx - will not be published immediately. NOTE: This cmd is DEPRECATED please use bumpforceclosefee instead. `, Flags: []cli.Flag{ @@ -321,8 +319,6 @@ var bumpForceCloseFeeCommand = cli.Command{ to sweep the anchor outputs of the closing transaction at the requested fee rate or confirmation target. The specified fee rate will be the effective fee rate taking the parent fee into account. - Depending on the sweeper configuration (batchwindowduration) the sweeptx - will not be published immediately. `, Flags: []cli.Flag{ cli.Uint64Flag{ diff --git a/config.go b/config.go index f9aec0284da..5a65d8fbb76 100644 --- a/config.go +++ b/config.go @@ -689,8 +689,7 @@ func DefaultConfig() Config { Timeout: lncfg.DefaultRemoteSignerRPCTimeout, }, Sweeper: &lncfg.Sweeper{ - BatchWindowDuration: sweep.DefaultBatchWindowDuration, - MaxFeeRate: sweep.DefaultMaxFeeRate, + MaxFeeRate: sweep.DefaultMaxFeeRate, }, Htlcswitch: &lncfg.Htlcswitch{ MailboxDeliveryTimeout: htlcswitch.DefaultMailboxDeliveryTimeout, diff --git a/lncfg/sweeper.go b/lncfg/sweeper.go index 08b12f6dabf..5bd3b196474 100644 --- a/lncfg/sweeper.go +++ b/lncfg/sweeper.go @@ -19,7 +19,7 @@ const ( //nolint:lll type Sweeper struct { - BatchWindowDuration time.Duration `long:"batchwindowduration" description:"Duration of the sweep batch window. The sweep is held back during the batch window to allow more inputs to be added and thereby lower the fee per input."` + BatchWindowDuration time.Duration `long:"batchwindowduration" description:"Duration of the sweep batch window. The sweep is held back during the batch window to allow more inputs to be added and thereby lower the fee per input." hidden:"true"` MaxFeeRate chainfee.SatPerVByte `long:"maxfeerate" description:"Maximum fee rate in sat/vb that the sweeper is allowed to use when sweeping funds. Setting this value too low can result in transactions not being confirmed in time, causing HTLCs to expire hence potentially losing funds."` } diff --git a/sample-lnd.conf b/sample-lnd.conf index dd538b07a29..e4e3f552b57 100644 --- a/sample-lnd.conf +++ b/sample-lnd.conf @@ -1609,8 +1609,9 @@ [sweeper] -; Duration of the sweep batch window. The sweep is held back during the batch -; window to allow more inputs to be added and thereby lower the fee per input. +; DEPRECATED: Duration of the sweep batch window. The sweep is held back during +; the batch window to allow more inputs to be added and thereby lower the fee +; per input. ; sweeper.batchwindowduration=30s ; The max fee rate in sat/vb which can be used when sweeping funds. Setting diff --git a/server.go b/server.go index d966018e7e5..25db38b664f 100644 --- a/server.go +++ b/server.go @@ -1052,9 +1052,6 @@ func newServer(cfg *Config, listenAddrs []net.Addr, return nil, err } - srvrLog.Debugf("Sweeper batch window duration: %v", - cfg.Sweeper.BatchWindowDuration) - sweeperStore, err := sweep.NewSweeperStore( dbs.ChanStateDB, s.cfg.ActiveNetParams.GenesisHash, ) @@ -1073,7 +1070,6 @@ func newServer(cfg *Config, listenAddrs []net.Addr, GenSweepScript: newSweepPkScriptGen(cc.Wallet), Signer: cc.Wallet.Cfg.Signer, Wallet: newSweeperWallet(cc.Wallet), - TickerDuration: cfg.Sweeper.BatchWindowDuration, Mempool: cc.MempoolNotifier, Notifier: cc.ChainNotifier, Store: sweeperStore, diff --git a/sweep/defaults.go b/sweep/defaults.go index 3ea49219001..fc5d12fafff 100644 --- a/sweep/defaults.go +++ b/sweep/defaults.go @@ -1,17 +1,10 @@ package sweep import ( - "time" - "github.com/lightningnetwork/lnd/lnwallet/chainfee" ) var ( - // DefaultBatchWindowDuration specifies duration of the sweep batch - // window. The sweep is held back during the batch window to allow more - // inputs to be added and thereby lower the fee per input. - DefaultBatchWindowDuration = 30 * time.Second - // DefaultMaxFeeRate is the default maximum fee rate allowed within the // UtxoSweeper. The current value is equivalent to a fee rate of 1,000 // sat/vbyte. diff --git a/sweep/sweeper.go b/sweep/sweeper.go index a785bdacc39..8fb2c84c112 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -5,7 +5,6 @@ import ( "fmt" "sync" "sync/atomic" - "time" "github.com/btcsuite/btcd/btcutil" "github.com/btcsuite/btcd/chaincfg/chainhash" @@ -318,12 +317,6 @@ type UtxoSweeperConfig struct { // Wallet contains the wallet functions that sweeper requires. Wallet Wallet - // TickerDuration is used to create a channel that will be sent on when - // a certain time window has passed. During this time window, new - // inputs can still be added to the sweep tx that is about to be - // generated. - TickerDuration time.Duration - // Notifier is an instance of a chain notifier we'll use to watch for // certain on-chain events. Notifier chainntnfs.ChainNotifier diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index 7396407b796..f3e4fb16fdc 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -138,11 +138,10 @@ func createSweeperTestContext(t *testing.T) *sweeperTestContext { } ctx.sweeper = New(&UtxoSweeperConfig{ - Notifier: notifier, - Wallet: backend, - TickerDuration: 100 * time.Millisecond, - Store: store, - Signer: &lnmock.DummySigner{}, + Notifier: notifier, + Wallet: backend, + Store: store, + Signer: &lnmock.DummySigner{}, GenSweepScript: func() ([]byte, error) { script := make([]byte, input.P2WPKHSize) script[0] = 0 From fd517882d6f226bbea849d5c72b8714d1aeafe60 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Sat, 2 Mar 2024 19:24:43 +0800 Subject: [PATCH 10/13] trivial: fix comments and docs --- sweep/aggregator.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sweep/aggregator.go b/sweep/aggregator.go index 260f9f6592f..9c6d7f5cbf1 100644 --- a/sweep/aggregator.go +++ b/sweep/aggregator.go @@ -29,11 +29,11 @@ type inputCluster struct { inputs pendingInputs } -// GroupInputs goes through the cluster's inputs and constructs sets of inputs -// that can be used to generate a sensible transaction. Each set contains up to -// the configured maximum number of inputs. Negative yield inputs are skipped. -// No input sets with a total value after fees below the dust limit are -// returned. +// createInputSets goes through the cluster's inputs and constructs sets of +// inputs that can be used to generate a sweeping transaction. Each set +// contains up to the configured maximum number of inputs. Negative yield +// inputs are skipped. No input sets with a total value after fees below the +// dust limit are returned. func (c *inputCluster) createInputSets(maxFeeRate chainfee.SatPerKWeight, maxInputs int) []InputSet { From 85aa572b0f1b0cbd62dcbf469f1305c46b84e9e4 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 15 Mar 2024 04:24:06 +0800 Subject: [PATCH 11/13] walletrpc: pass `Force` flag when sweeping via `BumpFee` --- lnrpc/walletrpc/walletkit_server.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lnrpc/walletrpc/walletkit_server.go b/lnrpc/walletrpc/walletkit_server.go index abd19cd4993..ffd956e195d 100644 --- a/lnrpc/walletrpc/walletkit_server.go +++ b/lnrpc/walletrpc/walletkit_server.go @@ -1043,7 +1043,10 @@ func (w *WalletKit) BumpFee(ctx context.Context, op, witnessType, signDesc, uint32(currentHeight), ) - sweepParams := sweep.Params{Fee: feePreference} + sweepParams := sweep.Params{ + Fee: feePreference, + Force: in.Force, + } if _, err = w.cfg.Sweeper.SweepInput(inp, sweepParams); err != nil { return nil, err } From d3d21bb8aed14b8f1c25e14d483fa5c6907144e3 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 14 Mar 2024 20:39:22 +0800 Subject: [PATCH 12/13] itest+lntest: fix itest re the new block driven behavior --- itest/lnd_channel_backup_test.go | 58 ++++- itest/lnd_channel_force_close_test.go | 68 ++++-- itest/lnd_multi-hop_test.go | 319 +++++++++++++++++++++----- itest/lnd_onchain_test.go | 10 +- itest/lnd_revocation_test.go | 49 ++-- itest/lnd_watchtower_test.go | 6 + itest/lnd_wipe_fwdpkgs_test.go | 6 + lntest/harness.go | 12 +- lntest/harness_assertion.go | 17 ++ lntest/node/config.go | 6 +- 10 files changed, 444 insertions(+), 107 deletions(-) diff --git a/itest/lnd_channel_backup_test.go b/itest/lnd_channel_backup_test.go index ea04a139a66..8d0c0071202 100644 --- a/itest/lnd_channel_backup_test.go +++ b/itest/lnd_channel_backup_test.go @@ -1266,6 +1266,12 @@ func testDataLossProtection(ht *lntest.HarnessTest) { // information Dave needs to sweep his funds. require.NoError(ht, restartDave(), "unable to restart Eve") + // Dave should have a pending sweep. + ht.AssertNumPendingSweeps(dave, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) + // Dave should sweep his funds. ht.Miner.AssertNumTxsInMempool(1) @@ -1417,6 +1423,11 @@ func assertTimeLockSwept(ht *lntest.HarnessTest, carol, dave *node.HarnessNode, // Carol should sweep her funds immediately, as they are not // timelocked. + ht.AssertNumPendingSweeps(carol, 2) + ht.AssertNumPendingSweeps(dave, 1) + + // Mine a block to trigger the sweeps. + ht.MineBlocks(1) ht.Miner.AssertNumTxsInMempool(expectedTxes) // Carol should consider the channel pending force close (since she is @@ -1444,9 +1455,13 @@ func assertTimeLockSwept(ht *lntest.HarnessTest, carol, dave *node.HarnessNode, // After the Dave's output matures, he should reclaim his funds. // // The commit sweep resolver publishes the sweep tx at defaultCSV-1 and - // we already mined one block after the commitment was published, so - // take that into account. - ht.MineBlocks(defaultCSV - 1 - 1) + // we already mined one block after the commitment was published, and + // one block to trigger Carol's sweeps, so take that into account. + ht.MineBlocks(1) + ht.AssertNumPendingSweeps(dave, 1) + + // Mine a block to trigger the sweeps. + ht.MineBlocks(1) daveSweep := ht.Miner.AssertNumTxsInMempool(1)[0] block := ht.MineBlocksAndAssertNumTxes(1, 1)[0] ht.Miner.AssertTxInBlock(block, daveSweep) @@ -1526,6 +1541,12 @@ func assertDLPExecuted(ht *lntest.HarnessTest, // Dave should sweep his anchor only, since he still has the // lease CLTV constraint on his commitment output. We'd also // see Carol's anchor sweep here. + ht.AssertNumPendingSweeps(dave, 1) + ht.AssertNumPendingSweeps(carol, 1) + + // Mine a block to trigger the sweeps. + ht.MineBlocks(1) + blocksMined++ ht.Miner.AssertNumTxsInMempool(2) // Mine anchor sweep txes for Carol and Dave. @@ -1539,6 +1560,10 @@ func assertDLPExecuted(ht *lntest.HarnessTest, // defaultCSV-1 and we already mined one block after the // commitmment was published, so take that into account. ht.MineBlocks(defaultCSV - blocksMined) + ht.AssertNumPendingSweeps(carol, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) ht.MineBlocksAndAssertNumTxes(1, 1) // Now the channel should be fully closed also from Carol's POV. @@ -1552,6 +1577,10 @@ func assertDLPExecuted(ht *lntest.HarnessTest, require.Positive(ht, blocksTilMaturity) ht.MineBlocks(uint32(blocksTilMaturity)) + ht.AssertNumPendingSweeps(dave, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) ht.MineBlocksAndAssertNumTxes(1, 1) // Now Dave should consider the channel fully closed. @@ -1559,7 +1588,22 @@ func assertDLPExecuted(ht *lntest.HarnessTest, } else { // Dave should sweep his funds immediately, as they are not // timelocked. We also expect Carol and Dave sweep their - // anchors. + // anchors if it's an anchor channel. + if lntest.CommitTypeHasAnchors(commitType) { + ht.AssertNumPendingSweeps(carol, 1) + ht.AssertNumPendingSweeps(dave, 2) + } else { + ht.AssertNumPendingSweeps(dave, 1) + } + + // Mine one block to trigger the sweeper to sweep. + ht.MineBlocks(1) + blocksMined++ + + // For anchor channels, we expect three txns, + // 1. the anchor sweeping tx from Dave. + // 2. the anchor sweeping tx from Carol. + // 3. the commitment sweep from Dave. if lntest.CommitTypeHasAnchors(commitType) { ht.MineBlocksAndAssertNumTxes(1, 3) } else { @@ -1578,6 +1622,12 @@ func assertDLPExecuted(ht *lntest.HarnessTest, // defaultCSV-1 and we already have blocks mined after the // commitmment was published, so take that into account. ht.MineBlocks(defaultCSV - blocksMined) + + // Mine one block to trigger the sweeper to sweep. + ht.MineBlocks(1) + ht.AssertNumPendingSweeps(carol, 1) + + // Assert the sweeping tx is mined. ht.MineBlocksAndAssertNumTxes(1, 1) // Now the channel should be fully closed also from Carol's diff --git a/itest/lnd_channel_force_close_test.go b/itest/lnd_channel_force_close_test.go index b0aba4c074e..3f73c17a873 100644 --- a/itest/lnd_channel_force_close_test.go +++ b/itest/lnd_channel_force_close_test.go @@ -179,12 +179,17 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) { // Bob should now sweep his to_local output and anchor output. expectedNumTxes = 2 + ht.AssertNumPendingSweeps(bob, 2) // If Alice's anchor is not swept above, we should see it here. if !expectAnchor { expectedNumTxes = 3 + ht.AssertNumPendingSweeps(alice, 1) } + // Mine one block to trigger the sweeps. + ht.MineBlocks(1) + // Mine one more block to assert the sweep transactions. ht.MineBlocksAndAssertNumTxes(1, expectedNumTxes) @@ -386,16 +391,6 @@ func channelForceClosureTest(ht *lntest.HarnessTest, ) ) - // If we are dealing with an anchor channel type, the sweeper will - // sweep the HTLC second level output one block earlier (than the - // nursery that waits an additional block, and handles non-anchor - // channels). So we set a maturity height that is one less. - if lntest.CommitTypeHasAnchors(channelType) { - htlcCsvMaturityHeight = padCLTV( - startHeight + defaultCLTV + defaultCSV, - ) - } - aliceChan := ht.QueryChannelByChanPoint(alice, chanPoint) require.NotZero(ht, aliceChan.NumUpdates, "alice should see at least one update to her channel") @@ -523,6 +518,13 @@ func channelForceClosureTest(ht *lntest.HarnessTest, // (the "kindergarten" bucket.) ht.RestartNode(alice) + // Carol should have pending sweeps now. + ht.AssertNumPendingSweeps(carol, expectedTxes) + + // Mine a block to trigger the sweep transactions. + blocksMined := int32(1) + ht.MineBlocks(1) + // Carol's sweep tx should be in the mempool already, as her output is // not timelocked. If there are anchors, we also expect Carol's anchor // sweep now. @@ -560,7 +562,8 @@ func channelForceClosureTest(ht *lntest.HarnessTest, // For the persistence test, we generate two blocks, then trigger // a restart and then generate the final block that should trigger // the creation of the sweep transaction. - ht.MineBlocks(defaultCSV - 2) + ht.MineBlocks(1) + blocksMined++ // The following restart checks to ensure that outputs in the // kindergarten bucket are persisted while waiting for the required @@ -592,7 +595,8 @@ func channelForceClosureTest(ht *lntest.HarnessTest, // outputs should also reflect that this many blocks have // passed. err = checkCommitmentMaturity( - forceClose, commCsvMaturityHeight, 2, + forceClose, commCsvMaturityHeight, + defaultCSV-blocksMined, ) if err != nil { return err @@ -621,8 +625,13 @@ func channelForceClosureTest(ht *lntest.HarnessTest, ht.MineBlocks(1) // At this point, the CSV will expire in the next block, meaning that - // the sweeping transaction should now be broadcast. So we fetch the - // node's mempool to ensure it has been properly broadcast. + // the output should be offered to the sweeper. + ht.AssertNumPendingSweeps(alice, 1) + + // Mine one block and the sweeping transaction should now be broadcast. + // So we fetch the node's mempool to ensure it has been properly + // broadcast. + ht.MineBlocks(1) sweepingTXID := ht.Miner.AssertNumTxsInMempool(1)[0] // Fetch the sweep transaction, all input it's spending should be from @@ -729,7 +738,16 @@ func channelForceClosureTest(ht *lntest.HarnessTest, // number of blocks we have generated since adding it to the nursery, // and take an additional block off so that we end up one block shy of // the expiry height, and add the block padding. - cltvHeightDelta := padCLTV(defaultCLTV - defaultCSV - 1 - 1) + cltvHeightDelta := padCLTV(defaultCLTV - defaultCSV - 1 - 1 - 1) + + // NOTE: this rest of the test would only pass if we remove the `Force` + // flag used in sweeping HTLCs, otherwise an immediate sweep will be + // attempted due to being forced. This flag will be removed once we can + // conditionally cancel back upstream htlcs to avoid cascading FCs. + ht.Shutdown(alice) + ht.Shutdown(carol) + ht.MineBlocksAndAssertNumTxes(1, 0) + ht.Skip("Skipping due until force flags are removed") // Advance the blockchain until just before the CLTV expires, nothing // exciting should have happened during this time. @@ -773,20 +791,24 @@ func channelForceClosureTest(ht *lntest.HarnessTest, }, defaultTimeout) require.NoError(ht, err, "timeout while checking force closed channel") - // Now, generate the block which will cause Alice to broadcast the - // presigned htlc timeout txns. + // Now, generate the block which will cause Alice to offer the + // presigned htlc timeout txns to the sweeper. ht.MineBlocks(1) // Since Alice had numInvoices (6) htlcs extended to Carol before force // closing, we expect Alice to broadcast an htlc timeout txn for each // one. expectedTxes = numInvoices + ht.AssertNumPendingSweeps(alice, numInvoices) // In case of anchors, the timeout txs will be aggregated into one. if lntest.CommitTypeHasAnchors(channelType) { expectedTxes = 1 } + // Mine a block to trigger the sweeps. + ht.MineBlocks(1) + // Wait for them all to show up in the mempool. htlcTxIDs := ht.Miner.AssertNumTxsInMempool(expectedTxes) @@ -905,7 +927,7 @@ func channelForceClosureTest(ht *lntest.HarnessTest, ht.RestartNode(alice) // Advance the chain until just before the 2nd-layer CSV delays expire. - // For anchor channels thhis is one block earlier. + // For anchor channels this is one block earlier. numBlocks := uint32(defaultCSV - 1) if lntest.CommitTypeHasAnchors(channelType) { numBlocks = defaultCSV - 2 @@ -935,6 +957,10 @@ func channelForceClosureTest(ht *lntest.HarnessTest, // Generate a block that causes Alice to sweep the htlc outputs in the // kindergarten bucket. ht.MineBlocks(1) + ht.AssertNumPendingSweeps(alice, 6) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) // Wait for the single sweep txn to appear in the mempool. htlcSweepTxID := ht.Miner.AssertNumTxsInMempool(1)[0] @@ -1009,7 +1035,7 @@ func channelForceClosureTest(ht *lntest.HarnessTest, } err = checkPendingHtlcStageAndMaturity( - forceClose, 2, htlcCsvMaturityHeight, 0, + forceClose, 2, htlcCsvMaturityHeight, -1, ) if err != nil { return err @@ -1133,6 +1159,10 @@ func testFailingChannel(ht *lntest.HarnessTest) { // Mine enough blocks for Alice to sweep her funds from the force // closed channel. ht.MineBlocks(defaultCSV - 1) + ht.AssertNumPendingSweeps(alice, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) // Wait for the sweeping tx to be broadcast. ht.Miner.AssertNumTxsInMempool(1) diff --git a/itest/lnd_multi-hop_test.go b/itest/lnd_multi-hop_test.go index 1c0cd10072e..b3abc79b963 100644 --- a/itest/lnd_multi-hop_test.go +++ b/itest/lnd_multi-hop_test.go @@ -236,7 +236,8 @@ func runMultiHopHtlcLocalTimeout(ht *lntest.HarnessTest, ht.MineBlocks(numBlocks) // Bob's force close transaction should now be found in the mempool. If - // there are anchors, we also expect Bob's anchor sweep. + // there are anchors, we also expect Bob's anchor sweep as it's a + // forced sweep. expectedTxes := 1 hasAnchors := lntest.CommitTypeHasAnchors(c) if hasAnchors { @@ -256,10 +257,18 @@ func runMultiHopHtlcLocalTimeout(ht *lntest.HarnessTest, ht.AssertActiveHtlcs(alice, payHash) // With the closing transaction confirmed, we should expect Bob's HTLC - // timeout transaction to be broadcast due to the expiry being reached. - // If there are anchors, we also expect Carol's anchor sweep now. - ht.Miner.AssertNumTxsInMempool(expectedTxes) + // timeout transaction to be offered to the sweeper due to the expiry + // being reached. If there are anchors, we also expect Carol's anchor + // sweep now. + ht.AssertNumPendingSweeps(bob, 1) + if hasAnchors { + ht.AssertNumPendingSweeps(carol, 1) + } + // Bob's HTLC timeout transaction should now be found in the mempool as + // it's a forced sweep, which means we don't need to mine a block to + // trigger it. + // // We'll also obtain the expected HTLC timeout transaction hash. htlcOutpoint := wire.OutPoint{Hash: closeTx.TxHash(), Index: 0} commitOutpoint := wire.OutPoint{Hash: closeTx.TxHash(), Index: 1} @@ -271,8 +280,15 @@ func runMultiHopHtlcLocalTimeout(ht *lntest.HarnessTest, htlcOutpoint, ).TxHash() - // Mine a block to confirm the expected transactions. - ht.MineBlocksAndAssertNumTxes(1, expectedTxes) + // Mine a block to confirm Bob's sweep. + ht.MineBlocksAndAssertNumTxes(1, 1) + + // The above block will trigger Carol's sweeper to broadcast her anchor + // sweep. + if hasAnchors { + // Carol's anchor sweep should now be found in the mempool. + ht.Miner.AssertNumTxsInMempool(1) + } // With Bob's HTLC timeout transaction confirmed, there should be no // active HTLC's on the commitment transaction from Alice -> Bob. @@ -297,6 +313,12 @@ func runMultiHopHtlcLocalTimeout(ht *lntest.HarnessTest, blocksTilMaturity := uint32(forceCloseChan.BlocksTilMaturity) ht.MineBlocks(blocksTilMaturity) + // Check that Bob has two pending sweeping txns. + ht.AssertNumPendingSweeps(bob, 2) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) + // Check that the sweep spends the expected inputs. ht.Miner.AssertOutpointInMempool(commitOutpoint) ht.Miner.AssertOutpointInMempool(htlcTimeoutOutpoint) @@ -308,6 +330,12 @@ func runMultiHopHtlcLocalTimeout(ht *lntest.HarnessTest, numBlocks := uint32(forceCloseChan.BlocksTilMaturity - 1) ht.MineBlocks(numBlocks) + // Check that Bob has a pending sweeping tx. + ht.AssertNumPendingSweeps(bob, 1) + + // Mine a block the trigger the sweeping behavior. + ht.MineBlocks(1) + // Check that the sweep spends from the mined commitment. ht.Miner.AssertOutpointInMempool(commitOutpoint) @@ -427,7 +455,7 @@ func runMultiHopReceiverChainClaim(ht *lntest.HarnessTest, // At this point, Carol should broadcast her active commitment // transaction in order to go to the chain and sweep her HTLC. If there - // are anchors, Carol also sweeps hers. + // are anchors, Carol also sweeps hers as it's a forced sweep. expectedTxes := 1 hasAnchors := lntest.CommitTypeHasAnchors(c) if hasAnchors { @@ -443,6 +471,10 @@ func runMultiHopReceiverChainClaim(ht *lntest.HarnessTest, // Confirm the commitment. ht.MineBlocksAndAssertNumTxes(1, expectedTxes) + // After the force close transaction is mined, Carol should offer her + // second level HTLC tx to the sweeper. + ht.AssertNumPendingSweeps(carol, 1) + // Restart bob again. require.NoError(ht, restartBob()) @@ -456,12 +488,14 @@ func runMultiHopReceiverChainClaim(ht *lntest.HarnessTest, // Carol. case lnrpc.CommitmentType_LEGACY: expectedTxes = 2 + ht.AssertNumPendingSweeps(bob, 1) // Carol should broadcast her second level HTLC transaction and Bob // should broadcast a sweep tx to sweep his output in the channel with // Carol, and another sweep tx to sweep his anchor output. case lnrpc.CommitmentType_ANCHORS, lnrpc.CommitmentType_SIMPLE_TAPROOT: expectedTxes = 3 + ht.AssertNumPendingSweeps(bob, 2) // Carol should broadcast her second level HTLC transaction and Bob // should broadcast a sweep tx to sweep his anchor output. Bob's commit @@ -469,11 +503,15 @@ func runMultiHopReceiverChainClaim(ht *lntest.HarnessTest, // being the channel initiator of a script-enforced leased channel. case lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE: expectedTxes = 2 + ht.AssertNumPendingSweeps(bob, 1) default: ht.Fatalf("unhandled commitment type %v", c) } + // Mine one block to trigger the sweeper to sweep. + ht.MineEmptyBlocks(1) + // All transactions should be spending from the commitment transaction. txes := ht.Miner.GetNumTxsFromMempool(expectedTxes) ht.AssertAllTxesSpendFrom(txes, closingTxid) @@ -494,7 +532,12 @@ func runMultiHopReceiverChainClaim(ht *lntest.HarnessTest, // If we mine 4 additional blocks, then Carol can sweep the second // level HTLC output once the CSV expires. - ht.MineEmptyBlocks(defaultCSV) + ht.MineEmptyBlocks(defaultCSV - 1) + + ht.AssertNumPendingSweeps(carol, 1) + + // Mine one block to trigger the sweeper to sweep. + ht.MineBlocks(1) // We should have a new transaction in the mempool. ht.Miner.AssertNumTxsInMempool(1) @@ -534,9 +577,13 @@ func runMultiHopReceiverChainClaim(ht *lntest.HarnessTest, // and sweep it. numBlocks := uint32(forceCloseChan.BlocksTilMaturity) ht.MineBlocks(numBlocks) + + ht.AssertNumPendingSweeps(bob, 1) + ht.MineBlocks(1) + commitOutpoint := wire.OutPoint{Hash: closingTxid, Index: 3} ht.Miner.AssertOutpointInMempool(commitOutpoint) - ht.MineBlocks(1) + ht.MineBlocksAndAssertNumTxes(1, 1) } ht.AssertNumPendingForceClose(bob, 0) @@ -617,12 +664,20 @@ func runMultiHopLocalForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest, bob, bobChanPoint, hasAnchors, stream, ) - // Increase the blocks mined. At this step + // Increase the blocks mined. At the step // AssertStreamChannelForceClosed mines one block. blocksMined++ - // If the channel closed has anchors, we should expect to see a sweep - // transaction for Carol's anchor. + // If the channel closed has anchors, we should expect to see a pending + // sweep request for Carol's anchor. + if hasAnchors { + ht.AssertNumPendingSweeps(carol, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) + blocksMined++ + } + htlcOutpoint := wire.OutPoint{Hash: *closeTx, Index: 0} bobCommitOutpoint := wire.OutPoint{Hash: *closeTx, Index: 1} if hasAnchors { @@ -640,7 +695,13 @@ func runMultiHopLocalForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest, // CSV expires and the commitment was already mined inside // AssertStreamChannelForceClosed(), so mine one block less // than defaultCSV in order to perform mempool assertions. - ht.MineBlocks(defaultCSV - 1) + ht.MineBlocks(defaultCSV - blocksMined) + blocksMined = defaultCSV + + // Assert Bob has the sweep and trigger it.. + ht.AssertNumPendingSweeps(bob, 1) + ht.MineBlocks(1) + blocksMined++ commitSweepTx := ht.Miner.AssertOutpointInMempool( bobCommitOutpoint, @@ -649,7 +710,7 @@ func runMultiHopLocalForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest, block := ht.MineBlocksAndAssertNumTxes(1, 1)[0] ht.Miner.AssertTxInBlock(block, &txid) - blocksMined += defaultCSV + blocksMined++ } // We'll now mine enough blocks for the HTLC to expire. After this, Bob @@ -697,16 +758,22 @@ func runMultiHopLocalForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest, // either only sweep the HTLC timeout transaction, or sweep both the // HTLC timeout transaction and Bob's commit output depending on the // commitment type. - htlcTimeoutOutpoint := wire.OutPoint{Hash: timeoutTx, Index: 0} - sweepTx := ht.Miner.AssertOutpointInMempool( - htlcTimeoutOutpoint, - ).TxHash() if c == lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE { - ht.Miner.AssertOutpointInMempool(bobCommitOutpoint) + // Assert the expected number of pending sweeps are found. + ht.AssertNumPendingSweeps(bob, 2) + } else { + ht.AssertNumPendingSweeps(bob, 1) } - block = ht.MineBlocksAndAssertNumTxes(1, 1)[0] - ht.Miner.AssertTxInBlock(block, &sweepTx) + // Mine a block to trigger the sweep. + ht.MineBlocks(1) + + // Assert the sweeping tx is found in the mempool. + htlcTimeoutOutpoint := wire.OutPoint{Hash: timeoutTx, Index: 0} + ht.Miner.AssertOutpointInMempool(htlcTimeoutOutpoint) + + // Mine a block to confirm the sweep. + ht.MineBlocksAndAssertNumTxes(1, 1) // At this point, Bob should no longer show any channels as pending // close. @@ -809,25 +876,38 @@ func runMultiHopRemoteForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest, var expectedTxes int switch c { - // Bob can sweep his commit output immediately. + // Bob can sweep his commit output immediately, so we should see it + // being offer to the sweeper. case lnrpc.CommitmentType_LEGACY: + ht.AssertNumPendingSweeps(bob, 1) + expectedTxes = 1 // Bob can sweep his commit and anchor outputs immediately. Carol will // also sweep her anchor. case lnrpc.CommitmentType_ANCHORS, lnrpc.CommitmentType_SIMPLE_TAPROOT: + ht.AssertNumPendingSweeps(bob, 2) + ht.AssertNumPendingSweeps(carol, 1) + expectedTxes = 3 // Bob can't sweep his commit output yet as he was the initiator of a // script-enforced leased channel, so he'll always incur the additional // CLTV. He can still sweep his anchor output however. case lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE: + ht.AssertNumPendingSweeps(bob, 1) + ht.AssertNumPendingSweeps(carol, 1) + expectedTxes = 2 default: ht.Fatalf("unhandled commitment type %v", c) } + // Mine one block to trigger the sweeps. + ht.MineBlocks(1) + blocksMined++ + // We now mine a block to clear up the mempool. ht.MineBlocksAndAssertNumTxes(1, expectedTxes) blocksMined++ @@ -875,6 +955,12 @@ func runMultiHopRemoteForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest, numBlocks := uint32(forceCloseChan.BlocksTilMaturity) ht.MineBlocks(numBlocks) + // Assert the commit output has been offered to the sweeper. + ht.AssertNumPendingSweeps(bob, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) + bobCommitOutpoint := wire.OutPoint{Hash: *closeTx, Index: 3} bobCommitSweep := ht.Miner.AssertOutpointInMempool( bobCommitOutpoint, @@ -988,23 +1074,35 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, switch c { // Alice will sweep her commitment output immediately. case lnrpc.CommitmentType_LEGACY: + ht.AssertNumPendingSweeps(alice, 1) + expectedTxes = 1 // Alice will sweep her commitment and anchor output immediately. Bob // will also sweep his anchor. case lnrpc.CommitmentType_ANCHORS, lnrpc.CommitmentType_SIMPLE_TAPROOT: + ht.AssertNumPendingSweeps(alice, 2) + ht.AssertNumPendingSweeps(bob, 1) + expectedTxes = 3 // Alice will sweep her anchor output immediately. Her commitment // output cannot be swept yet as it has incurred an additional CLTV due // to being the initiator of a script-enforced leased channel. case lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE: + ht.AssertNumPendingSweeps(alice, 1) + ht.AssertNumPendingSweeps(bob, 1) + expectedTxes = 2 default: ht.Fatalf("unhandled commitment type %v", c) } + // Mine a block to trigger the sweeps. + ht.MineBlocks(1) + blocksMined++ + ht.Miner.AssertNumTxsInMempool(expectedTxes) // Suspend Bob to force Carol to go to chain. @@ -1025,7 +1123,8 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, ht.MineBlocks(numBlocks - blocksMined) // Carol's commitment transaction should now be in the mempool. If - // there is an anchor, Carol will sweep that too. + // there is an anchor, Carol will sweep that too as it's forced sweep. + expectedTxes = 1 if lntest.CommitTypeHasAnchors(c) { expectedTxes = 2 } @@ -1043,15 +1142,20 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, block := ht.MineBlocksAndAssertNumTxes(1, expectedTxes)[0] ht.Miner.AssertTxInBlock(block, &closingTxid) + // After the force close transaction is mined, Carol should offer her + // second-level success HTLC tx to the sweeper. + ht.AssertNumPendingSweeps(carol, 1) + // Restart bob again. require.NoError(ht, restartBob()) // After the force close transaction is mined, transactions will be // broadcast by both Bob and Carol. switch c { - // Carol will broadcast her second level HTLC transaction and Bob will + // Carol will sweep her second level HTLC transaction and Bob will // sweep his commitment output. case lnrpc.CommitmentType_LEGACY: + ht.AssertNumPendingSweeps(bob, 1) expectedTxes = 2 // Carol will broadcast her second level HTLC transaction and Bob will @@ -1062,6 +1166,7 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, // - Bob's sweep tx spending two anchor outputs, one from channel Alice // to Bob and the other from channel Bob to Carol. case lnrpc.CommitmentType_ANCHORS, lnrpc.CommitmentType_SIMPLE_TAPROOT: + ht.AssertNumPendingSweeps(bob, 2) expectedTxes = 3 // Carol will broadcast her second level HTLC transaction, and Bob will @@ -1069,12 +1174,16 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, // as it has incurred an additional CLTV due to being the initiator of // a script-enforced leased channel. case lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE: + ht.AssertNumPendingSweeps(bob, 1) expectedTxes = 2 default: ht.Fatalf("unhandled commitment type %v", c) } + // Mine a block to trigger the sweeps. + ht.MineEmptyBlocks(1) + // Assert transactions can be found in the mempool. ht.Miner.AssertNumTxsInMempool(expectedTxes) @@ -1103,6 +1212,13 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, // When Bob notices Carol's second level transaction in the block, he // will extract the preimage and broadcast a second level tx to claim // the HTLC in his (already closed) channel with Alice. + ht.AssertNumPendingSweeps(bob, 1) + + // Mine a block to trigger the sweep of the second level tx. + ht.MineBlocks(1) + carolSecondLevelCSV-- + + // Check Bob's second level tx. bobSecondLvlTx := ht.Miner.GetNumTxsFromMempool(1)[0] bobSecondLvlTxid := bobSecondLvlTx.TxHash() @@ -1140,6 +1256,10 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, // If we then mine 3 additional blocks, Carol's second level tx should // mature, and she can pull the funds from it with a sweep tx. ht.MineBlocks(carolSecondLevelCSV) + ht.AssertNumPendingSweeps(carol, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) carolSweep := ht.Miner.AssertNumTxsInMempool(1)[0] // Mining one additional block, Bob's second level tx is mature, and he @@ -1181,6 +1301,14 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, numBlocks := uint32(forceCloseChan.BlocksTilMaturity) ht.MineBlocks(numBlocks) + // Both Alice and Bob should now offer their commit outputs to + // the sweeper. + ht.AssertNumPendingSweeps(alice, 1) + ht.AssertNumPendingSweeps(bob, 1) + + // Mine a block to trigger the sweeps. + ht.MineBlocks(1) + // Both Alice and Bob show broadcast their commit sweeps. aliceCommitOutpoint := wire.OutPoint{ Hash: *bobForceClose, Index: 3, @@ -1306,6 +1434,14 @@ func runMultiHopHtlcRemoteChainClaim(ht *lntest.HarnessTest, // so now bob will attempt to redeem his anchor commitment (if the // channel type is of that type). if hasAnchors { + // Check the anchor is offered to the sweeper. + ht.AssertNumPendingSweeps(bob, 1) + + // Mine a block to trigger the anchor sweep. + ht.MineBlocks(1) + blocksMined++ + + // Check that the anchor sweep is in the mempool. ht.Miner.AssertNumTxsInMempool(1) } @@ -1316,10 +1452,17 @@ func runMultiHopHtlcRemoteChainClaim(ht *lntest.HarnessTest, // commit sweep tx will be broadcast immediately before it can // be included in a block, so mine one less than defaultCSV in // order to perform mempool assertions. - ht.MineBlocks(defaultCSV - 1) - blocksMined += (defaultCSV - 1) + ht.MineBlocks(defaultCSV - blocksMined) + blocksMined += (defaultCSV - blocksMined) // Alice should now sweep her funds. + ht.AssertNumPendingSweeps(alice, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) + blocksMined++ + + // Assert the commitment sweep tx is in the mempool. ht.Miner.AssertNumTxsInMempool(1) } @@ -1346,7 +1489,8 @@ func runMultiHopHtlcRemoteChainClaim(ht *lntest.HarnessTest, } // Carol's commitment transaction should now be in the mempool. If - // there are anchors, Carol also sweeps her anchor. + // there are anchors, Carol also sweeps her anchor as it's a forced + // sweep. ht.Miner.AssertNumTxsInMempool(expectedTxes) // The closing transaction should be spending from the funding @@ -1361,6 +1505,10 @@ func runMultiHopHtlcRemoteChainClaim(ht *lntest.HarnessTest, block := ht.MineBlocksAndAssertNumTxes(1, expectedTxes)[0] ht.Miner.AssertTxInBlock(block, &closingTxid) + // After the force close transaction is mined, Carol should offer her + // second level HTLC tx to the sweeper. + ht.AssertNumPendingSweeps(carol, 1) + // Restart bob again. require.NoError(ht, restartBob()) @@ -1371,12 +1519,14 @@ func runMultiHopHtlcRemoteChainClaim(ht *lntest.HarnessTest, // Carol should broadcast her second level HTLC transaction and Bob // should broadcast a transaction to sweep his commitment output. case lnrpc.CommitmentType_LEGACY: + ht.AssertNumPendingSweeps(bob, 1) expectedTxes = 2 // Carol should broadcast her second level HTLC transaction and Bob // should broadcast a transaction to sweep his commitment output and // another to sweep his anchor output. case lnrpc.CommitmentType_ANCHORS, lnrpc.CommitmentType_SIMPLE_TAPROOT: + ht.AssertNumPendingSweeps(bob, 2) expectedTxes = 3 // Carol should broadcast her second level HTLC transaction and Bob @@ -1385,22 +1535,27 @@ func runMultiHopHtlcRemoteChainClaim(ht *lntest.HarnessTest, // due to being the channel initiator of a force closed script-enforced // leased channel. case lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE: + ht.AssertNumPendingSweeps(bob, 1) expectedTxes = 2 default: ht.Fatalf("unhandled commitment type %v", c) } + + // Keep track of the second level tx maturity. + carolSecondLevelCSV := uint32(defaultCSV) + + // Mine a block to trigger the sweeps. + ht.MineBlocks(1) + carolSecondLevelCSV-- txes := ht.Miner.GetNumTxsFromMempool(expectedTxes) // All transactions should be pending from the commitment transaction. ht.AssertAllTxesSpendFrom(txes, closingTxid) - // Mine a block to confirm the two transactions (+ coinbase). + // Mine a block to confirm the expected transactions. ht.MineBlocksAndAssertNumTxes(1, expectedTxes) - // Keep track of the second level tx maturity. - carolSecondLevelCSV := uint32(defaultCSV) - // When Bob notices Carol's second level transaction in the block, he // will extract the preimage and broadcast a sweep tx to directly claim // the HTLC in his (already closed) channel with Alice. @@ -1432,6 +1587,10 @@ func runMultiHopHtlcRemoteChainClaim(ht *lntest.HarnessTest, // If we then mine 3 additional blocks, Carol's second level tx will // mature, and she should pull the funds. ht.MineEmptyBlocks(int(carolSecondLevelCSV)) + ht.AssertNumPendingSweeps(carol, 1) + + // Mine a block to trigger the sweep of the second level tx. + ht.MineBlocks(1) carolSweep := ht.Miner.AssertNumTxsInMempool(1)[0] // When Carol's sweep gets confirmed, she should have no more pending @@ -1457,7 +1616,14 @@ func runMultiHopHtlcRemoteChainClaim(ht *lntest.HarnessTest, numBlocks := uint32(forceCloseChan.BlocksTilMaturity) ht.MineBlocks(numBlocks) - // Both Alice and Bob show broadcast their commit sweeps. + // Both Alice and Bob should offer their commit sweeps. + ht.AssertNumPendingSweeps(alice, 1) + ht.AssertNumPendingSweeps(bob, 1) + + // Mine a block to trigger the sweeps. + ht.MineBlocks(1) + + // Both Alice and Bob should broadcast their commit sweeps. aliceCommitOutpoint := wire.OutPoint{ Hash: *aliceForceClose, Index: 3, } @@ -1499,6 +1665,11 @@ func runMultiHopHtlcRemoteChainClaim(ht *lntest.HarnessTest, // case of anchor channels, the second-level spends can also be aggregated and // properly feebumped, so we'll check that as well. func testMultiHopHtlcAggregation(ht *lntest.HarnessTest) { + // NOTE: this test would only pass if we remove the `Force` flag used + // in sweeping HTLCs, otherwise an immediate sweep will be attempted + // due to being forced. This flag will be removed once we can + // conditionally cancel back upstream htlcs to avoid cascading FCs. + ht.Skip("Skipping due until force flags are removed") runMultiHopHtlcClaimTest(ht, runMultiHopHtlcAggregation) } @@ -1659,7 +1830,8 @@ func runMultiHopHtlcAggregation(ht *lntest.HarnessTest, ht.MineBlocks(numBlocks) // Bob's force close transaction should now be found in the mempool. If - // there are anchors, we also expect Bob's anchor sweep. + // there are anchors, we also expect Bob's anchor sweep as it's a + // forced sweep. hasAnchors := lntest.CommitTypeHasAnchors(c) expectedTxes := 1 if hasAnchors { @@ -1672,12 +1844,6 @@ func runMultiHopHtlcAggregation(ht *lntest.HarnessTest, ) closeTxid := closeTx.TxHash() - // Restart Bob to increase the batch window duration so the sweeper - // will aggregate all the pending inputs. - ht.RestartNodeWithExtraArgs( - bob, []string{"--sweeper.batchwindowduration=15s"}, - ) - // Go through the closing transaction outputs, and make an index for // the HTLC outputs. successOuts := make(map[wire.OutPoint]struct{}) @@ -1724,6 +1890,9 @@ func runMultiHopHtlcAggregation(ht *lntest.HarnessTest, // preimages from Alice. We also expect Carol to sweep her commitment // output. case lnrpc.CommitmentType_LEGACY: + ht.AssertNumPendingSweeps(bob, numInvoices*2) + ht.AssertNumPendingSweeps(carol, 1) + expectedTxes = 2*numInvoices + 1 // In case of anchors, all success transactions will be aggregated into @@ -1734,11 +1903,18 @@ func runMultiHopHtlcAggregation(ht *lntest.HarnessTest, lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE, lnrpc.CommitmentType_SIMPLE_TAPROOT: + ht.AssertNumPendingSweeps(bob, numInvoices*2) + ht.AssertNumPendingSweeps(carol, 2) + expectedTxes = 4 default: ht.Fatalf("unhandled commitment type %v", c) } + + // Mine a block to trigger the sweeps. + ht.MineBlocks(1) + txes := ht.Miner.GetNumTxsFromMempool(expectedTxes) // Since Bob can aggregate the transactions, we expect a single @@ -1798,7 +1974,13 @@ func runMultiHopHtlcAggregation(ht *lntest.HarnessTest, if c != lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE { // If we then mine additional blocks, Bob can sweep his // commitment output. - ht.MineBlocks(defaultCSV - 2) + ht.MineBlocks(1) + + // Assert the tx has been offered to the sweeper. + ht.AssertNumPendingSweeps(bob, 1) + + // Mine one block to trigger the sweep. + ht.MineBlocks(1) // Find the commitment sweep. bobCommitSweep := ht.Miner.GetNumTxsFromMempool(1)[0] @@ -1820,12 +2002,6 @@ func runMultiHopHtlcAggregation(ht *lntest.HarnessTest, } } - // We now restart Bob with a much larger batch window duration since it - // takes some time to aggregate all the 10 inputs below. - ht.RestartNodeWithExtraArgs( - bob, []string{"--sweeper.batchwindowduration=45s"}, - ) - switch c { // In case this is a non-anchor channel type, we must mine 2 blocks, as // the nursery waits an extra block before sweeping. Before the blocks @@ -1860,6 +2036,9 @@ func runMultiHopHtlcAggregation(ht *lntest.HarnessTest, ht.Fatalf("unhandled commitment type %v", c) } + // Mine one block to trigger the sweeps. + ht.MineBlocks(1) + // Make sure it spends from the second level tx. secondLevelSweep := ht.Miner.GetNumTxsFromMempool(1)[0] bobSweep := secondLevelSweep.TxHash() @@ -2021,12 +2200,6 @@ func createThreeHopNetwork(ht *lntest.HarnessTest, aliceChanPoint := resp[0] bobChanPoint := resp[1] - // Remove the ChannelAcceptor for Bob and Carol. - if zeroConf { - cancelBob() - cancelCarol() - } - // Make sure alice and carol know each other's channels. // // We'll only do this though if it wasn't a private channel we opened @@ -2041,6 +2214,12 @@ func createThreeHopNetwork(ht *lntest.HarnessTest, ht.AssertChannelExists(carol, bobChanPoint) } + // Remove the ChannelAcceptor for Bob and Carol. + if zeroConf { + cancelBob() + cancelCarol() + } + return aliceChanPoint, bobChanPoint, carol } @@ -2138,7 +2317,11 @@ func runExtraPreimageFromRemoteCommit(ht *lntest.HarnessTest, ht.MineClosingTx(bobChanPoint, c) // With the closing transaction confirmed, we should expect Carol's - // HTLC success transaction to be broadcast. + // HTLC success transaction to be offered to the sweeper. + ht.AssertNumPendingSweeps(carol, 1) + + // Mine a block to trigger the sweep. + ht.MineEmptyBlocks(1) ht.Miner.AssertNumTxsInMempool(1) // Restart Bob. Once he finishes syncing the channel state, he should @@ -2240,7 +2423,7 @@ func runExtraPreimageFromLocalCommit(ht *lntest.HarnessTest, Hash: payHash[:], RouteHints: routeHints, } - eveInvoice := carol.RPC.AddHoldInvoice(invoiceReq) + carolInvoice := carol.RPC.AddHoldInvoice(invoiceReq) // Subscribe the invoice. stream := carol.RPC.SubscribeSingleInvoice(payHash[:]) @@ -2249,7 +2432,7 @@ func runExtraPreimageFromLocalCommit(ht *lntest.HarnessTest, // Alice to Carol. We won't wait for the response however, as Carol // will not immediately settle the payment. req := &routerrpc.SendPaymentRequest{ - PaymentRequest: eveInvoice.PaymentRequest, + PaymentRequest: carolInvoice.PaymentRequest, TimeoutSeconds: 60, FeeLimitMsat: noFeeLimitMsat, } @@ -2302,8 +2485,32 @@ func runExtraPreimageFromLocalCommit(ht *lntest.HarnessTest, invoiceReq.CltvExpiry - lncfg.DefaultIncomingBroadcastDelta - 1, )) + blocksMined := 0 + + // If this is a nont script-enforced channel, Bob will be able to sweep + // his commit output after 4 blocks. + if c != lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE { + // Mine 3 blocks so the output will be offered to the sweeper. + ht.MineBlocks(defaultCSV - 1) + + // Assert the commit output has been offered to the sweeper. + ht.AssertNumPendingSweeps(bob, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) + blocksMined = defaultCSV + } + // Mine empty blocks so it's easier to check Bob's sweeping txes below. - ht.MineEmptyBlocks(int(numBlocks)) + ht.MineEmptyBlocks(int(numBlocks) - blocksMined) + + // With the above blocks mined, we should expect Carol's to offer the + // htlc output on Bob's commitment to the sweeper. + // + // TODO(yy): it's not offered to the sweeper yet, instead, the utxo + // nursery is creating and broadcasting the sweep tx - we should unify + // this behavior and offer it to the sweeper. + // ht.AssertNumPendingSweeps(carol, 1) // Increase the fee rate used by the sweeper so Carol's direct spend tx // won't be replaced by Bob's timeout tx. diff --git a/itest/lnd_onchain_test.go b/itest/lnd_onchain_test.go index 9eac66cc32e..dff7f996b3b 100644 --- a/itest/lnd_onchain_test.go +++ b/itest/lnd_onchain_test.go @@ -280,6 +280,8 @@ func runCPFP(ht *lntest.HarnessTest, alice, bob *node.HarnessNode) { // We use a higher fee rate than the default max and expect the // sweeper to cap the fee rate at the max value. SatPerVbyte: maxFeeRate * 2, + // We use a force param to create the sweeping tx immediately. + Force: true, } bob.RPC.BumpFee(bumpFeeReq) @@ -900,10 +902,14 @@ func testListSweeps(ht *lntest.HarnessTest) { ) // Mine enough blocks for the node to sweep its funds from the force - // closed channel. The commit sweep resolver is able to broadcast the - // sweep tx up to one block before the CSV elapses, so wait until + // closed channel. The commit sweep resolver offers the outputs to the + // sweeper up to one block before the CSV elapses, so wait until // defaulCSV-1. ht.MineEmptyBlocks(node.DefaultCSV - 1) + ht.AssertNumPendingSweeps(alice, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) // Now we can expect that the sweep has been broadcast. pendingTxHash := ht.Miner.AssertNumTxsInMempool(1) diff --git a/itest/lnd_revocation_test.go b/itest/lnd_revocation_test.go index d94fa7c4350..2c4f5fd253f 100644 --- a/itest/lnd_revocation_test.go +++ b/itest/lnd_revocation_test.go @@ -163,19 +163,23 @@ func breachRetributionTestCase(ht *lntest.HarnessTest, // again. ht.RestartNode(carol) - // Now mine a block, this transaction should include Carol's justice - // transaction which was just accepted into the mempool. - expectedNumTxes := 1 - - // For anchor channels, we'd also create the sweeping transaction. + // For anchor channels, we'd offer the anchor output to the sweeper. if lntest.CommitTypeHasAnchors(commitType) { - expectedNumTxes = 2 + ht.AssertNumPendingSweeps(carol, 1) } - block = ht.MineBlocksAndAssertNumTxes(1, expectedNumTxes)[0] + // Now mine a block, this transaction should include Carol's justice + // transaction which was just accepted into the mempool. + block = ht.MineBlocksAndAssertNumTxes(1, 1)[0] justiceTxid := justiceTx.TxHash() ht.Miner.AssertTxInBlock(block, &justiceTxid) + // The above mined block should trigger the sweeper to sweep the + // anchor. + if lntest.CommitTypeHasAnchors(commitType) { + ht.MineBlocksAndAssertNumTxes(1, 1) + } + ht.AssertNodeNumChannels(carol, 0) // Mine enough blocks for Bob's channel arbitrator to wrap up the @@ -354,19 +358,24 @@ func revokedCloseRetributionZeroValueRemoteOutputCase(ht *lntest.HarnessTest, // the justice transaction to confirm again. ht.RestartNode(dave) - // Now mine a block, this transaction should include Dave's justice - // transaction which was just accepted into the mempool. - expectedNumTxes := 1 - // For anchor channels, we'd also create the sweeping transaction. if lntest.CommitTypeHasAnchors(commitType) { - expectedNumTxes = 2 + ht.AssertNumPendingSweeps(dave, 1) } - block := ht.MineBlocksAndAssertNumTxes(1, expectedNumTxes)[0] + // Now mine a block, this transaction should include Dave's justice + // transaction which was just accepted into the mempool. + block := ht.MineBlocksAndAssertNumTxes(1, 1)[0] justiceTxid := justiceTx.TxHash() ht.Miner.AssertTxInBlock(block, &justiceTxid) + // The above mined block should trigger the sweeper to sweep the + // anchor. + if lntest.CommitTypeHasAnchors(commitType) { + ht.MineBlocksAndAssertNumTxes(1, 1) + } + + // At this point, Dave should have no pending channels. ht.AssertNodeNumChannels(dave, 0) } @@ -676,17 +685,21 @@ func revokedCloseRetributionRemoteHodlCase(ht *lntest.HarnessTest, // waiting for the justice transaction to confirm again. ht.RestartNode(dave) + // For anchor channels, we'd also create the sweeping transaction. + if lntest.CommitTypeHasAnchors(commitType) { + ht.AssertNumPendingSweeps(dave, 1) + } + // Now mine a block, this transaction should include Dave's justice // transaction which was just accepted into the mempool. - expectedNumTxes := 1 + ht.MineBlocksAndAssertNumTxes(1, 1) - // For anchor channels, we'd also create the sweeping transaction. + // The above mined block should trigger the sweeper to sweep the + // anchor. if lntest.CommitTypeHasAnchors(commitType) { - expectedNumTxes = 2 + ht.MineBlocksAndAssertNumTxes(1, 1) } - ht.MineBlocksAndAssertNumTxes(1, expectedNumTxes) - // Dave should have no open channels. ht.AssertNodeNumChannels(dave, 0) } diff --git a/itest/lnd_watchtower_test.go b/itest/lnd_watchtower_test.go index e97e604f435..af44f7b0fbb 100644 --- a/itest/lnd_watchtower_test.go +++ b/itest/lnd_watchtower_test.go @@ -581,6 +581,12 @@ func testRevokedCloseRetributionAltruistWatchtowerCase(ht *lntest.HarnessTest, // If this is an anchor channel, Dave would sweep the anchor. if lntest.CommitTypeHasAnchors(commitType) { + ht.AssertNumPendingSweeps(dave, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) + + // Mine a block to confirm the sweep. ht.MineBlocksAndAssertNumTxes(1, 1) } diff --git a/itest/lnd_wipe_fwdpkgs_test.go b/itest/lnd_wipe_fwdpkgs_test.go index a3632914bc8..cee1d8e760b 100644 --- a/itest/lnd_wipe_fwdpkgs_test.go +++ b/itest/lnd_wipe_fwdpkgs_test.go @@ -114,6 +114,12 @@ func testWipeForwardingPackages(ht *lntest.HarnessTest) { pendingAB = pending.Channel require.Zero(ht, pendingAB.NumForwardingPackages) + // Alice should one pending sweep. + ht.AssertNumPendingSweeps(alice, 1) + + // Mine a block to trigger the sweep. + ht.MineBlocks(1) + // Mine 1 block to get Alice's sweeping tx confirmed. ht.MineBlocksAndAssertNumTxes(1, 1) diff --git a/lntest/harness.go b/lntest/harness.go index 6e0da417d5d..6960d06e176 100644 --- a/lntest/harness.go +++ b/lntest/harness.go @@ -1579,14 +1579,20 @@ func (h *HarnessTest) CleanupForceClose(hn *node.HarnessNode) { h.AssertNumPendingForceClose(hn, 1) // Mine enough blocks for the node to sweep its funds from the force - // closed channel. The commit sweep resolver is able to broadcast the - // sweep tx up to one block before the CSV elapses, so wait until - // defaulCSV-1. + // closed channel. The commit sweep resolver is able to offer the input + // to the sweeper at defaulCSV-1, and broadcast the sweep tx once one + // more block is mined. // // NOTE: we might empty blocks here as we don't know the exact number // of blocks to mine. This may end up mining more blocks than needed. h.MineEmptyBlocks(node.DefaultCSV - 1) + // Assert there is one pending sweep. + h.AssertNumPendingSweeps(hn, 1) + + // Mine a block to trigger the sweep. + h.MineEmptyBlocks(1) + // The node should now sweep the funds, clean up by mining the sweeping // tx. h.MineBlocksAndAssertNumTxes(1, 1) diff --git a/lntest/harness_assertion.go b/lntest/harness_assertion.go index 2f4ebeb7f85..042cf324004 100644 --- a/lntest/harness_assertion.go +++ b/lntest/harness_assertion.go @@ -2591,3 +2591,20 @@ func (h *HarnessTest) AssertWalletLockedBalance(hn *node.HarnessNode, require.NoError(h, err, "%s: timeout checking locked balance", hn.Name()) } + +// AssertNumPendingSweeps asserts the number of pending sweeps for the given +// node. +func (h *HarnessTest) AssertNumPendingSweeps(hn *node.HarnessNode, n int) { + err := wait.NoError(func() error { + resp := hn.RPC.PendingSweeps() + num := len(resp.PendingSweeps) + + if num == n { + return nil + } + + return fmt.Errorf("want %d , got %d", n, num) + }, DefaultTimeout) + + require.NoErrorf(h, err, "%s: check pending sweeps timeout", hn.Name()) +} diff --git a/lntest/node/config.go b/lntest/node/config.go index 5a7013a2154..d0de2fdd4f5 100644 --- a/lntest/node/config.go +++ b/lntest/node/config.go @@ -199,7 +199,7 @@ func (cfg *BaseNodeConfig) GenArgs() []string { nodeArgs := []string{ "--nobootstrap", - "--debuglevel=debug,DISC=trace", + "--debuglevel=debug", "--bitcoin.defaultchanconfs=1", "--accept-keysend", "--keep-failed-payment-attempts", @@ -217,10 +217,6 @@ func (cfg *BaseNodeConfig) GenArgs() []string { fmt.Sprintf("--trickledelay=%v", trickleDelay), fmt.Sprintf("--profile=%d", cfg.ProfilePort), - // Use a small batch window so we can broadcast our sweep - // transactions faster. - "--sweeper.batchwindowduration=5s", - // Use a small batch delay so we can broadcast the // announcements quickly in the tests. "--gossip.sub-batch-delay=5ms", From 7bb12c568b3f2413a5e42ffdc636a18bf7ecd5fc Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 20 Mar 2024 05:56:56 +0800 Subject: [PATCH 13/13] sweep: change `MaxInputsPerTx` from `int` to `uint32` --- sweep/aggregator.go | 6 +++--- sweep/sweeper.go | 2 +- sweep/sweeper_test.go | 2 +- sweep/tx_input_set.go | 6 +++--- sweep/txgenerator.go | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/sweep/aggregator.go b/sweep/aggregator.go index 9c6d7f5cbf1..379ff982964 100644 --- a/sweep/aggregator.go +++ b/sweep/aggregator.go @@ -35,7 +35,7 @@ type inputCluster struct { // inputs are skipped. No input sets with a total value after fees below the // dust limit are returned. func (c *inputCluster) createInputSets(maxFeeRate chainfee.SatPerKWeight, - maxInputs int) []InputSet { + maxInputs uint32) []InputSet { // Turn the inputs into a slice so we can sort them. inputList := make([]*pendingInput, 0, len(c.inputs)) @@ -138,7 +138,7 @@ type SimpleAggregator struct { // MaxInputsPerTx specifies the default maximum number of inputs allowed // in a single sweep tx. If more need to be swept, multiple txes are // created and published. - MaxInputsPerTx int + MaxInputsPerTx uint32 // FeeRateBucketSize is the default size of fee rate buckets we'll use // when clustering inputs into buckets with similar fee rates within @@ -158,7 +158,7 @@ var _ UtxoAggregator = (*SimpleAggregator)(nil) // NewSimpleUtxoAggregator creates a new instance of a SimpleAggregator. func NewSimpleUtxoAggregator(estimator chainfee.Estimator, - max chainfee.SatPerKWeight, maxTx int) *SimpleAggregator { + max chainfee.SatPerKWeight, maxTx uint32) *SimpleAggregator { return &SimpleAggregator{ FeeEstimator: estimator, diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 8fb2c84c112..bd266aaa0b6 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -335,7 +335,7 @@ type UtxoSweeperConfig struct { // MaxInputsPerTx specifies the default maximum number of inputs allowed // in a single sweep tx. If more need to be swept, multiple txes are // created and published. - MaxInputsPerTx int + MaxInputsPerTx uint32 // MaxSweepAttempts specifies the maximum number of times an input is // included in a publish attempt before giving up and returning an error diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index f3e4fb16fdc..519bbdbb2a0 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -30,7 +30,7 @@ var ( testMaxSweepAttempts = 3 - testMaxInputsPerTx = 3 + testMaxInputsPerTx = uint32(3) defaultFeePref = Params{Fee: FeeEstimateInfo{ConfTarget: 1}} ) diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index 09367624f57..b80ea2db098 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -139,7 +139,7 @@ type txInputSet struct { // maxInputs is the maximum number of inputs that will be accepted in // the set. - maxInputs int + maxInputs uint32 } // Compile-time constraint to ensure txInputSet implements InputSet. @@ -147,7 +147,7 @@ var _ InputSet = (*txInputSet)(nil) // newTxInputSet constructs a new, empty input set. func newTxInputSet(feePerKW, maxFeeRate chainfee.SatPerKWeight, - maxInputs int) *txInputSet { + maxInputs uint32) *txInputSet { state := txInputSetState{ feeRate: feePerKW, @@ -215,7 +215,7 @@ func (t *txInputSet) addToState(inp input.Input, // Stop if max inputs is reached. Do not count additional wallet inputs, // because we don't know in advance how many we may need. if constraints != constraintsWallet && - len(t.inputs) >= t.maxInputs { + uint32(len(t.inputs)) >= t.maxInputs { return nil } diff --git a/sweep/txgenerator.go b/sweep/txgenerator.go index 353513cb64e..0cab9a6e220 100644 --- a/sweep/txgenerator.go +++ b/sweep/txgenerator.go @@ -19,7 +19,7 @@ var ( // DefaultMaxInputsPerTx specifies the default maximum number of inputs // allowed in a single sweep tx. If more need to be swept, multiple txes // are created and published. - DefaultMaxInputsPerTx = 100 + DefaultMaxInputsPerTx = uint32(100) // ErrLocktimeConflict is returned when inputs with different // transaction nLockTime values are included in the same transaction.