diff --git a/contractcourt/anchor_resolver.go b/contractcourt/anchor_resolver.go index c59e5d063..7e2676782 100644 --- a/contractcourt/anchor_resolver.go +++ b/contractcourt/anchor_resolver.go @@ -202,7 +202,10 @@ func (c *anchorResolver) Launch() error { // an output that we want to sweep only if it is economical to do so. // // An exclusive group is not necessary anymore, because we know that - // this is the only anchor that can be swept. + // this is the only anchor that can be swept. However, to avoid this + // anchor input being group with other inputs, we still keep the + // exclusive group here such that the anchor will be swept + // independently. // // We also clear the parent tx information for cpfp, because the // commitment tx is confirmed. @@ -222,6 +225,8 @@ func (c *anchorResolver) Launch() error { c.broadcastHeight, nil, ) + exclusiveGroup := c.ShortChanID.ToUint64() + resultChan, err := c.Sweeper.SweepInput( &anchorInput, sweep.Params{ @@ -233,6 +238,10 @@ func (c *anchorResolver) Launch() error { // There's no rush to sweep the anchor, so we use a nil // deadline here. DeadlineHeight: fn.None[int32](), + + // Use the chan id as the exclusive group. This prevents + // any of the anchors from being batched together. + ExclusiveGroup: &exclusiveGroup, }, ) diff --git a/docs/release-notes/release-notes-0.19.3.md b/docs/release-notes/release-notes-0.19.3.md index 24d60fec6..e282b2bbc 100644 --- a/docs/release-notes/release-notes-0.19.3.md +++ b/docs/release-notes/release-notes-0.19.3.md @@ -29,6 +29,13 @@ ## Functional Enhancements +- Previously, when sweeping non-time sensitive anchor outputs, they might be + grouped with other non-time sensitive outputs such as `to_local` outputs, + which potentially allow the sweeping tx to be pinned. This is now + [fixed](https://github.com/lightningnetwork/lnd/pull/10117) by moving sweeping + anchors into its own tx, which means the anchor outputs won't be swept in a + high fee environment. + ## RPC Additions ## lncli Additions @@ -62,4 +69,6 @@ ## Tooling and Documentation # Contributors (Alphabetical Order) -* Olaoluwa Osuntokun \ No newline at end of file + +* Olaoluwa Osuntokun +* Yong Yu diff --git a/itest/lnd_channel_force_close_test.go b/itest/lnd_channel_force_close_test.go index b05b838ff..3cb6d30e4 100644 --- a/itest/lnd_channel_force_close_test.go +++ b/itest/lnd_channel_force_close_test.go @@ -978,15 +978,6 @@ func runChannelForceClosureTestRestart(ht *lntest.HarnessTest, Outpoint: commitSweep.Outpoint, AmountSat: uint64(aliceBalance), } - op = fmt.Sprintf("%v:%v", anchorSweep.Outpoint.TxidStr, - anchorSweep.Outpoint.OutputIndex) - aliceReports[op] = &lnrpc.Resolution{ - ResolutionType: lnrpc.ResolutionType_ANCHOR, - Outcome: lnrpc.ResolutionOutcome_CLAIMED, - SweepTxid: sweepTxid.String(), - Outpoint: anchorSweep.Outpoint, - AmountSat: uint64(anchorSweep.AmountSat), - } // Check that we can find the commitment sweep in our set of known // sweeps, using the simple transaction id ListSweeps output. @@ -1101,8 +1092,9 @@ func runChannelForceClosureTestRestart(ht *lntest.HarnessTest, // Since Alice had numInvoices (6) htlcs extended to Carol before force // closing, we expect Alice to broadcast an htlc timeout txn for each - // one. - ht.AssertNumPendingSweeps(alice, numInvoices) + // one. In addition, the anchor input is still pending due to it's + // uneconomical to sweep. + ht.AssertNumPendingSweeps(alice, numInvoices+1) // Wait for them all to show up in the mempool htlcTxid := ht.AssertNumTxsInMempool(1)[0] @@ -1198,7 +1190,9 @@ func runChannelForceClosureTestRestart(ht *lntest.HarnessTest, numBlocks := int(htlcCsvMaturityHeight - uint32(curHeight) - 1) ht.MineEmptyBlocks(numBlocks) - ht.AssertNumPendingSweeps(alice, numInvoices) + // We should see numInvoices HTLC sweeps plus the uneconomical anchor + // sweep. + ht.AssertNumPendingSweeps(alice, numInvoices+1) // Fetch the htlc sweep transaction from the mempool. htlcSweepTx := ht.GetNumTxsFromMempool(1)[0] @@ -1220,7 +1214,7 @@ func runChannelForceClosureTestRestart(ht *lntest.HarnessTest, }, defaultTimeout) require.NoError(ht, err, "timeout while checking force closed channel") - ht.AssertNumPendingSweeps(alice, numInvoices) + ht.AssertNumPendingSweeps(alice, numInvoices+1) // Ensure the htlc sweep transaction only has one input for each htlc // Alice extended before force closing. diff --git a/itest/lnd_sweep_test.go b/itest/lnd_sweep_test.go index d0fe5f077..c5bcd3b15 100644 --- a/itest/lnd_sweep_test.go +++ b/itest/lnd_sweep_test.go @@ -1224,8 +1224,8 @@ func testSweepHTLCs(ht *lntest.HarnessTest) { // 4. Alice force closes the channel. // // Test: -// 1. Alice's anchor sweeping is not attempted, instead, it should be swept -// together with her to_local output using the no deadline path. +// 1. Alice's CPFP-anchor sweeping is not attempted, instead, it should be +// swept using the no deadline path and failed due it's not economical. // 2. Bob would also sweep his anchor and to_local outputs separately due to // they have different deadline heights, which means only the to_local // sweeping tx will succeed as the anchor sweeping is not economical. @@ -1241,10 +1241,15 @@ func testSweepCommitOutputAndAnchor(ht *lntest.HarnessTest) { // config. deadline := uint32(1000) - // deadlineA is the deadline used for Alice, since her commit output is - // offered to the sweeper at CSV-1. With a deadline of 1000, her actual - // width of her fee func is CSV+1000-1. Given we are using a CSV of 2 - // here, her fee func deadline then becomes 1001. + // deadlineA is the deadline used for Alice, given that, + // - the force close tx is broadcast at height 445, her inputs are + // registered at the same height, so her to_local and anchor outputs + // have a deadline height of 1445. + // - the force close tx is mined at 446, which means her anchor output + // now has a deadline delta of (1445-446) = 999 blocks. + // - for her to_local output, with a deadline of 1000, the width of the + // fee func is CSV+1000-1. Given we are using a CSV of 2 here, her fee + // func deadline then becomes 1001. deadlineA := deadline + 1 // deadlineB is the deadline used for Bob, the actual deadline used by @@ -1267,6 +1272,11 @@ func testSweepCommitOutputAndAnchor(ht *lntest.HarnessTest) { // conf target is the deadline. ht.SetFeeEstimateWithConf(startFeeRate, deadlineB) + // Set up the starting fee for Alice's anchor sweeping. With this low + // fee rate, her anchor sweeping should be attempted and failed due to + // dust output generated in the sweeping tx. + ht.SetFeeEstimateWithConf(startFeeRate, deadline-1) + // toLocalCSV is the CSV delay for Alice's to_local output. We use a // small value to save us from mining blocks. // @@ -1427,10 +1437,10 @@ func testSweepCommitOutputAndAnchor(ht *lntest.HarnessTest) { // With Alice's starting fee rate being validated, we now calculate her // ending fee rate and fee rate delta. // - // Alice sweeps two inputs - anchor and commit, so the starting budget - // should come from the sum of these two. However, due to the value - // being too large, the actual ending fee rate used should be the - // sweeper's max fee rate configured. + // Alice sweeps the to_local input, so the starting budget should come + // from the to_local balance. However, due to the value being too large, + // the actual ending fee rate used should be the sweeper's max fee rate + // configured. aliceTxWeight := uint64(ht.CalculateTxWeight(aliceSweepTx)) aliceEndingFeeRate := sweep.DefaultMaxFeeRate.FeePerKWeight() aliceFeeRateDelta := (aliceEndingFeeRate - aliceStartingFeeRate) / @@ -1507,10 +1517,10 @@ func testSweepCommitOutputAndAnchor(ht *lntest.HarnessTest) { } // We should see two txns in the mempool: - // - Alice's sweeping tx, which sweeps both her anchor and - // commit outputs, using the increased fee rate. - // - Bob's previous sweeping tx, which sweeps both his anchor - // and commit outputs, at the possible increased fee rate. + // - Alice's sweeping tx, which sweeps her commit output, using + // the increased fee rate. + // - Bob's previous sweeping tx, which sweeps his commit output, + // at the possible increased fee rate. txns := ht.GetNumTxsFromMempool(2) // Assume the first tx is Alice's sweeping tx, if the second tx @@ -1577,6 +1587,11 @@ func testSweepCommitOutputAndAnchor(ht *lntest.HarnessTest) { // Mine a block to confirm both sweeping txns, this is needed to clean // up the mempool. ht.MineBlocksAndAssertNumTxes(1, 2) + + // Finally, assert that both Alice and Bob still have the anchor + // outputs, which cannot be swept due to it being uneconomical. + ht.AssertNumPendingSweeps(alice, 1) + ht.AssertNumPendingSweeps(bob, 1) } // testBumpForceCloseFee tests that when a force close transaction, in diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go index 1602165ea..18d848b21 100644 --- a/sweep/fee_bumper.go +++ b/sweep/fee_bumper.go @@ -111,8 +111,8 @@ const ( // error, which means they cannot be retried with increased budget. TxFatal - // sentinalEvent is used to check if an event is unknown. - sentinalEvent + // sentinelEvent is used to check if an event is unknown. + sentinelEvent ) // String returns a human-readable string for the event. @@ -137,13 +137,13 @@ func (e BumpEvent) String() string { // Unknown returns true if the event is unknown. func (e BumpEvent) Unknown() bool { - return e >= sentinalEvent + return e >= sentinelEvent } // BumpRequest is used by the caller to give the Bumper the necessary info to // create and manage potential fee bumps for a set of inputs. type BumpRequest struct { - // Budget givens the total amount that can be used as fees by these + // Budget gives the total amount that can be used as fees by these // inputs. Budget btcutil.Amount @@ -589,7 +589,7 @@ func (t *TxPublisher) createRBFCompliantTx( // used up the budget, we will return an error // indicating this tx cannot be made. The // sweeper should handle this error and try to - // cluster these inputs differetly. + // cluster these inputs differently. increased, err = f.Increment() if err != nil { return nil, err @@ -1332,7 +1332,7 @@ func (t *TxPublisher) createAndPublishTx( // the fee bumper retry it at next block. // // NOTE: we may get this error if we've bypassed the mempool check, - // which means we are suing neutrino backend. + // which means we are using neutrino backend. if errors.Is(result.Err, chain.ErrInsufficientFee) || errors.Is(result.Err, lnwallet.ErrMempoolFee) { diff --git a/sweep/fee_bumper_test.go b/sweep/fee_bumper_test.go index aa5561b5a..fb939e732 100644 --- a/sweep/fee_bumper_test.go +++ b/sweep/fee_bumper_test.go @@ -73,7 +73,7 @@ func TestBumpResultValidate(t *testing.T) { // Unknown event type will give an error. b = BumpResult{ Tx: &wire.MsgTx{}, - Event: sentinalEvent, + Event: sentinelEvent, } require.ErrorIs(t, b.Validate(), ErrInvalidBumpResult) @@ -457,7 +457,7 @@ func TestCreateAndCheckTx(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -550,7 +550,7 @@ func TestCreateRBFCompliantTx(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1120,9 +1120,9 @@ func TestBroadcastImmediate(t *testing.T) { require.Empty(t, tp.subscriberChans.Len()) } -// TestCreateAnPublishFail checks all the error cases are handled properly in -// the method createAndPublish. -func TestCreateAnPublishFail(t *testing.T) { +// TestCreateAndPublishFail checks all the error cases are handled properly in +// the method createAndPublishTx. +func TestCreateAndPublishFail(t *testing.T) { t.Parallel() // Create a publisher using the mocks. @@ -1152,7 +1152,7 @@ func TestCreateAnPublishFail(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1190,9 +1190,9 @@ func TestCreateAnPublishFail(t *testing.T) { require.True(t, resultOpt.IsNone()) } -// TestCreateAnPublishSuccess checks the expected result is returned from the -// method createAndPublish. -func TestCreateAnPublishSuccess(t *testing.T) { +// TestCreateAndPublishSuccess checks the expected result is returned from the +// method createAndPublishTx. +func TestCreateAndPublishSuccess(t *testing.T) { t.Parallel() // Create a publisher using the mocks. @@ -1218,7 +1218,7 @@ func TestCreateAnPublishSuccess(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1445,7 +1445,7 @@ func TestHandleFeeBumpTx(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1830,7 +1830,7 @@ func TestHandleInitialBroadcastSuccess(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1916,7 +1916,7 @@ func TestHandleInitialBroadcastFail(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) diff --git a/sweep/sweeper.go b/sweep/sweeper.go index fdaecbefb..2b9bad082 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -45,8 +45,9 @@ var ( // Params contains the parameters that control the sweeping process. type Params struct { - // ExclusiveGroup is an identifier that, if set, prevents other inputs - // with the same identifier from being batched together. + // ExclusiveGroup is an identifier that, if set, ensures this input is + // swept in a transaction by itself, and not batched with any other + // inputs. ExclusiveGroup *uint64 // DeadlineHeight specifies an absolute block height that this input @@ -739,15 +740,23 @@ func (s *UtxoSweeper) collector() { } } -// removeExclusiveGroup removes all inputs in the given exclusive group. This -// function is called when one of the exclusive group inputs has been spent. The -// other inputs won't ever be spendable and can be removed. This also prevents -// them from being part of future sweep transactions that would fail. In -// addition sweep transactions of those inputs will be removed from the wallet. -func (s *UtxoSweeper) removeExclusiveGroup(group uint64) { +// removeExclusiveGroup removes all inputs in the given exclusive group except +// the input specified by the outpoint. This function is called when one of the +// exclusive group inputs has been spent or updated. The other inputs won't ever +// be spendable and can be removed. This also prevents them from being part of +// future sweep transactions that would fail. In addition sweep transactions of +// those inputs will be removed from the wallet. +func (s *UtxoSweeper) removeExclusiveGroup(group uint64, op wire.OutPoint) { for outpoint, input := range s.inputs { outpoint := outpoint + // Skip the input that caused the exclusive group to be removed. + if outpoint == op { + log.Debugf("Skipped removing exclusive input %v", input) + + continue + } + // Skip inputs that aren't exclusive. if input.params.ExclusiveGroup == nil { continue @@ -766,6 +775,8 @@ func (s *UtxoSweeper) removeExclusiveGroup(group uint64) { continue } + log.Debugf("Removing exclusive group for input %v", input) + // Signal result channels. s.signalResult(input, Result{ Err: ErrExclusiveGroupSpend, @@ -1366,22 +1377,19 @@ func (s *UtxoSweeper) decideRBFInfo( func (s *UtxoSweeper) handleExistingInput(input *sweepInputMessage, oldInput *SweeperInput) { - // Before updating the input details, check if an exclusive group was - // set. In case the same input is registered again without an exclusive - // group set, the previous input and its sweep parameters are outdated - // hence need to be replaced. This scenario currently only happens for - // anchor outputs. When a channel is force closed, in the worst case 3 - // different sweeps with the same exclusive group are registered with - // the sweeper to bump the closing transaction (cpfp) when its time - // critical. Receiving an input which was already registered with the - // sweeper but now without an exclusive group means non of the previous - // inputs were used as CPFP, so we need to make sure we update the - // sweep parameters but also remove all inputs with the same exclusive - // group because the are outdated too. + // Before updating the input details, check if a previous exclusive + // group was set. In case the same input is registered again, the + // previous input and its sweep parameters are outdated hence need to be + // replaced. This scenario currently only happens for anchor outputs. + // When a channel is force closed, in the worst case 3 different sweeps + // with the same exclusive group are registered with the sweeper to bump + // the closing transaction (cpfp) when its time critical. Receiving an + // input which was already registered with the sweeper means none of the + // previous inputs were used as CPFP, so we need to make sure we update + // the sweep parameters but also remove all inputs with the same + // exclusive group because they are outdated too. var prevExclGroup *uint64 - if oldInput.params.ExclusiveGroup != nil && - input.params.ExclusiveGroup == nil { - + if oldInput.params.ExclusiveGroup != nil { prevExclGroup = new(uint64) *prevExclGroup = *oldInput.params.ExclusiveGroup } @@ -1400,7 +1408,7 @@ func (s *UtxoSweeper) handleExistingInput(input *sweepInputMessage, oldInput.listeners = append(oldInput.listeners, input.resultChan) if prevExclGroup != nil { - s.removeExclusiveGroup(*prevExclGroup) + s.removeExclusiveGroup(*prevExclGroup, input.input.OutPoint()) } } @@ -1492,7 +1500,9 @@ func (s *UtxoSweeper) markInputsSwept(tx *wire.MsgTx, isOurTx bool) { // Remove all other inputs in this exclusive group. if input.params.ExclusiveGroup != nil { - s.removeExclusiveGroup(*input.params.ExclusiveGroup) + s.removeExclusiveGroup( + *input.params.ExclusiveGroup, outpoint, + ) } } } @@ -1907,7 +1917,9 @@ func (s *UtxoSweeper) markInputSwept(inp *SweeperInput, tx *wire.MsgTx) { // Remove all other inputs in this exclusive group. if inp.params.ExclusiveGroup != nil { - s.removeExclusiveGroup(*inp.params.ExclusiveGroup) + s.removeExclusiveGroup( + *inp.params.ExclusiveGroup, inp.OutPoint(), + ) } }