Merge pull request #8091 from yyforyongyu/remove-rbf-sweeper

sweep: remove possible RBF when sweeping new inputs

docs/release-notes/release-notes-0.18.0.md

@@ -28,6 +28,10 @@
 * LND will now [enforce pong responses
   ](https://github.com/lightningnetwork/lnd/pull/7828) from its peers
 
+* [Fixed a possible unintended RBF
+  attempt](https://github.com/lightningnetwork/lnd/pull/8091) when sweeping new
+  inputs with retried ones.
+
 # New Features
 ## Functional Enhancements
 

itest/lnd_channel_backup_test.go

@@ -1524,18 +1524,12 @@ func assertDLPExecuted(ht *lntest.HarnessTest,
 
 	if commitType == lnrpc.CommitmentType_SCRIPT_ENFORCED_LEASE {
 		// Dave should sweep his anchor only, since he still has the
-		// lease CLTV constraint on his commitment output.
-		ht.Miner.AssertNumTxsInMempool(1)
+		// lease CLTV constraint on his commitment output. We'd also
+		// see Carol's anchor sweep here.
+		ht.Miner.AssertNumTxsInMempool(2)
 
-		// Mine Dave's anchor sweep tx.
-		ht.MineBlocksAndAssertNumTxes(1, 1)
-		blocksMined++
-
-		// The above block will trigger Carol's sweeper to reconsider
-		// the anchor sweeping. Because we are now sweeping at the fee
-		// rate floor, the sweeper will consider this input has
-		// positive yield thus attempts the sweeping.
-		ht.MineBlocksAndAssertNumTxes(1, 1)
+		// Mine anchor sweep txes for Carol and Dave.
+		ht.MineBlocksAndAssertNumTxes(1, 2)
 		blocksMined++
 
 		// After Carol's output matures, she should also reclaim her
@@ -1564,10 +1558,10 @@ func assertDLPExecuted(ht *lntest.HarnessTest,
 		ht.AssertNumPendingForceClose(dave, 0)
 	} else {
 		// Dave should sweep his funds immediately, as they are not
-		// timelocked. We also expect Dave to sweep his anchor, if
-		// present.
+		// timelocked. We also expect Carol and Dave sweep their
+		// anchors.
 		if lntest.CommitTypeHasAnchors(commitType) {
-			ht.MineBlocksAndAssertNumTxes(1, 2)
+			ht.MineBlocksAndAssertNumTxes(1, 3)
 		} else {
 			ht.MineBlocksAndAssertNumTxes(1, 1)
 		}
@@ -1577,15 +1571,6 @@ func assertDLPExecuted(ht *lntest.HarnessTest,
 		// Now Dave should consider the channel fully closed.
 		ht.AssertNumPendingForceClose(dave, 0)
 
-		// The above block will trigger Carol's sweeper to reconsider
-		// the anchor sweeping. Because we are now sweeping at the fee
-		// rate floor, the sweeper will consider this input has
-		// positive yield thus attempts the sweeping.
-		if lntest.CommitTypeHasAnchors(commitType) {
-			ht.MineBlocksAndAssertNumTxes(1, 1)
-			blocksMined++
-		}
-
 		// After Carol's output matures, she should also reclaim her
 		// funds.
 		//

itest/lnd_channel_force_close_test.go

@@ -94,7 +94,7 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) {
 
 	// calculateSweepFeeRate runs multiple steps to calculate the fee rate
 	// used in sweeping the transactions.
-	calculateSweepFeeRate := func(expectedSweepTxNum, deadline int) int64 {
+	calculateSweepFeeRate := func(expectAnchor bool, deadline int) int64 {
 		// Create two nodes, Alice and Bob.
 		alice := setupNode("Alice")
 		defer ht.Shutdown(alice)
@@ -143,12 +143,32 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) {
 		// section.
 		ht.AssertChannelWaitingClose(alice, chanPoint)
 
+		// We should see Alice's force closing tx in the mempool.
+		expectedNumTxes := 1
+
+		// If anchor is expected, we should see the anchor sweep tx in
+		// the mempool too.
+		if expectAnchor {
+			expectedNumTxes = 2
+		}
+
 		// Check our sweep transactions can be found in mempool.
-		sweepTxns := ht.Miner.GetNumTxsFromMempool(expectedSweepTxNum)
+		sweepTxns := ht.Miner.GetNumTxsFromMempool(expectedNumTxes)
 
 		// Mine a block to confirm these transactions such that they
 		// don't remain in the mempool for any subsequent tests.
-		ht.MineBlocks(1)
+		ht.MineBlocksAndAssertNumTxes(1, expectedNumTxes)
+
+		// Bob should now sweep his to_local output and anchor output.
+		expectedNumTxes = 2
+
+		// If Alice's anchor is not swept above, we should see it here.
+		if !expectAnchor {
+			expectedNumTxes = 3
+		}
+
+		// Mine one more block to assert the sweep transactions.
+		ht.MineBlocksAndAssertNumTxes(1, expectedNumTxes)
 
 		// Calculate the fee rate used.
 		feeRate := ht.CalculateTxesFeeRate(sweepTxns)
@@ -163,7 +183,7 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) {
 
 	// Calculate fee rate used and assert only the force close tx is
 	// broadcast.
-	feeRate := calculateSweepFeeRate(1, deadline)
+	feeRate := calculateSweepFeeRate(false, deadline)
 
 	// We expect the default max fee rate is used. Allow some deviation
 	// because weight estimates during tx generation are estimates.
@@ -181,7 +201,7 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) {
 
 	// Calculate fee rate used and assert only the force close tx is
 	// broadcast.
-	feeRate = calculateSweepFeeRate(1, defaultDeadline)
+	feeRate = calculateSweepFeeRate(false, defaultDeadline)
 
 	// We expect the default max fee rate is used. Allow some deviation
 	// because weight estimates during tx generation are estimates.
@@ -198,7 +218,7 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) {
 
 	// Calculate fee rate used and assert both the force close tx and the
 	// anchor sweeping tx are broadcast.
-	feeRate = calculateSweepFeeRate(2, deadline)
+	feeRate = calculateSweepFeeRate(true, deadline)
 
 	// We expect the anchor to be swept with the deadline, which has the
 	// fee rate of feeRateLarge.

itest/lnd_multi-hop_test.go

@@ -319,15 +319,17 @@ func runMultiHopHtlcLocalTimeout(ht *lntest.HarnessTest,
 		// was in fact mined.
 		ht.MineBlocksAndAssertNumTxes(1, 1)
 
-		// Mine an additional block to prompt Bob to broadcast their
-		// second layer sweep due to the CSV on the HTLC timeout output.
-		ht.MineBlocksAndAssertNumTxes(1, 0)
+		// Mine one more block to trigger the timeout path.
+		ht.MineEmptyBlocks(1)
+
+		// Bob's sweeper should now broadcast his second layer sweep
+		// due to the CSV on the HTLC timeout output.
 		ht.Miner.AssertOutpointInMempool(htlcTimeoutOutpoint)
 	}
 
 	// Next, we'll mine a final block that should confirm the sweeping
 	// transactions left.
-	ht.MineBlocks(1)
+	ht.MineBlocksAndAssertNumTxes(1, 1)
 
 	// Once this transaction has been confirmed, Bob should detect that he
 	// no longer has any pending channels.
@@ -482,7 +484,7 @@ func runMultiHopReceiverChainClaim(ht *lntest.HarnessTest,
 
 	// We'll now mine an additional block which should confirm both the
 	// second layer transactions.
-	ht.MineBlocks(1)
+	ht.MineBlocksAndAssertNumTxes(1, expectedTxes)
 
 	// Carol's pending channel report should now show two outputs under
 	// limbo: her commitment output, as well as the second-layer claim
@@ -494,16 +496,16 @@ func runMultiHopReceiverChainClaim(ht *lntest.HarnessTest,
 	// clearing the HTLC off-chain.
 	ht.AssertNumActiveHtlcs(alice, 0)
 
-	// If we mine 4 additional blocks, then Carol can sweep the second level
-	// HTLC output.
-	ht.MineBlocks(defaultCSV)
+	// If we mine 4 additional blocks, then Carol can sweep the second
+	// level HTLC output once the CSV expires.
+	ht.MineEmptyBlocks(defaultCSV)
 
 	// We should have a new transaction in the mempool.
 	ht.Miner.AssertNumTxsInMempool(1)
 
-	// Finally, if we mine an additional block to confirm these two sweep
-	// transactions, Carol should not show a pending channel in her report
-	// afterwards.
+	// Finally, if we mine an additional block to confirm Carol's second
+	// level success transaction. Carol should not show a pending channel
+	// in her report afterwards.
 	ht.MineBlocks(1)
 	ht.AssertNumPendingForceClose(carol, 0)
 
@@ -815,15 +817,16 @@ func runMultiHopRemoteForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest,
 	case lnrpc.CommitmentType_LEGACY:
 		expectedTxes = 1
 
-	// Bob can sweep his commit and anchor outputs immediately.
+	// Bob can sweep his commit and anchor outputs immediately. Carol will
+	// also sweep her anchor.
 	case lnrpc.CommitmentType_ANCHORS, lnrpc.CommitmentType_SIMPLE_TAPROOT:
-		expectedTxes = 2
+		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:
-		expectedTxes = 1
+		expectedTxes = 2
 
 	default:
 		ht.Fatalf("unhandled commitment type %v", c)
@@ -833,15 +836,6 @@ func runMultiHopRemoteForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest,
 	ht.MineBlocksAndAssertNumTxes(1, expectedTxes)
 	blocksMined++
 
-	// The above block will trigger Carol's sweeper to reconsider the
-	// anchor sweeping. Because we are now sweeping at the fee rate floor,
-	// the sweeper will consider this input has positive yield thus
-	// attempts the sweeping.
-	if lntest.CommitTypeHasAnchors(c) {
-		ht.MineBlocksAndAssertNumTxes(1, 1)
-		blocksMined++
-	}
-
 	// Next, we'll mine enough blocks for the HTLC to expire. At this
 	// point, Bob should hand off the output to his internal utxo nursery,
 	// which will broadcast a sweep transaction.
@@ -1000,15 +994,16 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest,
 	case lnrpc.CommitmentType_LEGACY:
 		expectedTxes = 1
 
-	// Alice will sweep her commitment and anchor output immediately.
+	// Alice will sweep her commitment and anchor output immediately. Bob
+	// will also sweep his anchor.
 	case lnrpc.CommitmentType_ANCHORS, lnrpc.CommitmentType_SIMPLE_TAPROOT:
-		expectedTxes = 2
+		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:
-		expectedTxes = 1
+		expectedTxes = 2
 
 	default:
 		ht.Fatalf("unhandled commitment type %v", c)
@@ -2162,16 +2157,19 @@ func runExtraPreimageFromRemoteCommit(ht *lntest.HarnessTest,
 	numBlocks = htlc.ExpirationHeight - uint32(height) -
 		lncfg.DefaultOutgoingBroadcastDelta
 
-	// We should now have Carol's htlc suucess tx in the mempool.
+	// We should now have Carol's htlc success tx in the mempool.
 	numTxesMempool := 1
+	ht.Miner.AssertNumTxsInMempool(numTxesMempool)
 
 	// For neutrino backend, the timeout resolver needs to extract the
 	// preimage from the blocks.
 	if ht.IsNeutrinoBackend() {
 		// Mine a block to confirm Carol's 2nd level success tx.
 		ht.MineBlocksAndAssertNumTxes(1, 1)
 		numTxesMempool--
+		numBlocks--
 	}
+
 	// Mine empty blocks so Carol's htlc success tx stays in mempool. Once
 	// the height is reached, Bob's timeout resolver will resolve the htlc
 	// by extracing the preimage from the mempool.

itest/lnd_routing_test.go

@@ -528,7 +528,6 @@ func testPrivateChannels(ht *lntest.HarnessTest) {
 			Private: true,
 		},
 	)
-	defer ht.CloseChannel(carol, chanPointPrivate)
 
 	// The channel should be available for payments between Carol and Alice.
 	// We check this by sending payments from Carol to Bob, that
@@ -602,6 +601,7 @@ func testPrivateChannels(ht *lntest.HarnessTest) {
 	ht.CloseChannel(alice, chanPointAlice)
 	ht.CloseChannel(dave, chanPointDave)
 	ht.CloseChannel(carol, chanPointCarol)
+	ht.CloseChannel(carol, chanPointPrivate)
 }
 
 // testInvoiceRoutingHints tests that the routing hints for an invoice are

server.go

@@ -1059,14 +1059,12 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 	}
 
 	s.sweeper = sweep.New(&sweep.UtxoSweeperConfig{
-		FeeEstimator:      cc.FeeEstimator,
-		DetermineFeePerKw: sweep.DetermineFeePerKw,
-		GenSweepScript:    newSweepPkScriptGen(cc.Wallet),
-		Signer:            cc.Wallet.Cfg.Signer,
-		Wallet:            newSweeperWallet(cc.Wallet),
-		NewBatchTimer: func() <-chan time.Time {
-			return time.NewTimer(cfg.Sweeper.BatchWindowDuration).C
-		},
+		FeeEstimator:         cc.FeeEstimator,
+		DetermineFeePerKw:    sweep.DetermineFeePerKw,
+		GenSweepScript:       newSweepPkScriptGen(cc.Wallet),
+		Signer:               cc.Wallet.Cfg.Signer,
+		Wallet:               newSweeperWallet(cc.Wallet),
+		TickerDuration:       cfg.Sweeper.BatchWindowDuration,
 		Notifier:             cc.ChainNotifier,
 		Store:                sweeperStore,
 		MaxInputsPerTx:       sweep.DefaultMaxInputsPerTx,