Merge branch '0-19-3-branch-rc1-10117' into 0-19-3-branch-rc1

Author: guggero

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

@@ -41,6 +41,13 @@
   [increased](https://github.com/lightningnetwork/lnd/pull/10096) from 100KB to
   1MB, and `gossip.msg-burst-bytes` has been increased from 200KB to 2MB.
 
+- 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

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

@@ -1212,8 +1212,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.
@@ -1229,10 +1229,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
@@ -1255,6 +1260,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.
 	//
@@ -1415,10 +1425,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) /
@@ -1495,10 +1505,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
@@ -1565,6 +1575,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)