sweep: remove conflicted sweep txns from the rebroadcaster

[Roasbeef]

In this commit, we an existing gap in our rebroadcast handling logic. As is, if we're trying to sweep a transaction and a conflicting transaction is mined (timeout lands on chain, anchor swept), then we'll continue to try to rebroadcast the tx in the background.

To resolve this, we give the sweeper a new closure function that it can use to mark conflicted transactions as no longer requiring rebroadcast.

Related to:

• LND does not recover limbo balance from Borked channels #7548
• [bug]: sweeper keeps trying to sweep invalid anchor output until locking up all on-chain funds on node #7390
• [bug]: sweep transaction not recognized by on-chain wallet #7181

Change Description

Description of change / link to associated issue.

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[ellemouton]

also related? #7104

[yyforyongyu]

Thanks for the PR🤙 I think this works, just need to fix the linter and tests tho.

[Roasbeef]

@ellemouton I think that's related to one of the linked issues. The question at large is still: why is the sweeper missing notifications for spends that conflict with its spend set?

The transaction is created, along with all the descendents that were created from those outputs should now be removed after this PR: #6274. I'll take a look at the itest, as at the time, it seemed fairly comprehensive.

[Roasbeef]

Rebased!

[yyforyongyu]

Needs to fix the interface🧐

[guggero]

Looks good, just needs a couple of fixes to pass the CI.

[roeierez]

@Roasbeef when running in mobile we see sweep tx is rebroadcasted on every restart using another mechanism here: https://github.com/lightningnetwork/lnd/blob/master/sweep/sweeper.go#L344

So there is a bucket that caches the last broadcasted tx and retries to broadcast it on every restart regardless if it double spends causes the wallet to stay in the same state where all balance is unconfirmed.
Will this PR fix this variant of the issue as well?

BTW I am not sure why we are not removing the tx from that bucket when we get the double spend here: https://github.com/lightningnetwork/lnd/blob/master/sweep/sweeper.go#L362

[roeierez]

@Roasbeef I can open another issue on the above but would be great to get your insights on that beforehand.

[Roasbeef]

@roeierez correct, the very last sweep tx (but not all the others) is re-broadcasted on start up.

So there is a bucket that caches the last broadcasted tx and retries to broadcast it on every restart regardless if it double spends causes the wallet to stay in the same state where all balance is unconfirmed.

Today the sweeper doesn't remove it's last transaction (just replaces it), and also doesn't currently garbage collect those old sweep hashes. This is the main storage interface used: https://github.com/lightningnetwork/lnd/blob/master/sweep/store.go#L46-L61 (has no deletion rn). The sweeper was intended to be mostly stateless, but I think there's no reason we shouldn't remove transactions after they've been double spent or confirmed.

Will this PR fix this variant of the issue as well?

This PR ensures that any older conflicting sweep transactions are removed from the wallet, but you're correct in that the sweeper will still hold onto the very last one. In 0.16.0, we started to rebroadcast more aggressively, so things that dropped out of the mempool would be sent back to the main node. However, the sweeper makes a lot of txns, and those aren't also being removed when a conflict was detected. This PR ensures those txns are removed fromthe underlying wallet, but as you note here, the sweeper will still hold onto the very last one.

BTW I am not sure why we are not removing the tx from that bucket when we get the double spend here: https://github.com/lightningnetwork/lnd/blob/master/sweep/sweeper.go#L362

I think you're right. We should remove that tx, and also call this same routine to remove any conflicting transactions from the wallet:

lnd/sweep/sweeper.go

Lines 713 to 727 in 35917eb

	err := s.removeLastSweepDescendants(
	spend.SpendingTx,
	)
	if err != nil {
	log.Warnf("unable to remove descendant "+
	"transactions due to tx %v: ",
	spendHash)
	}
	log.Debugf("Detected spend related to in flight inputs "+
	"(is_ours=%v): %v",
	newLogClosure(func() string {
	return spew.Sdump(spend.SpendingTx)
	}), isOurTx,
	)

.

Re the issue: I think it makes sense to make a tracking issue for sweeper tx deletion. This would cover the code area I linked above, and also generally removing well confirmed sweep hashes.

[Roasbeef]

it on every restart regardless if it double spends causes the wallet to stay in the same state where all balance is unconfirmed

We should be always removing the tx when we get a conflict: https://github.com/btcsuite/btcwallet/blob/68f7e23dca2742b98d13d6bd8e9340047bc6aa9e/wallet/wallet.go#L3799-L3824

We've had reports that this isn't always happening, but the last time I went to repro it, I found the txns were always being removed: #6583 (comment).