Merge pull request #10117 from yyforyongyu/keep-exclusive

contractcourt+sweep: make anchor inputs exclusive

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,
 		},
 	)
 

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
+
+* Olaoluwa Osuntokun
+* Yong Yu

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.

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

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) {
 

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)

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(),
+		)
 	}
 }