Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sweep: remove conflicted sweep txns from the rebroadcaster #7599

Merged

Conversation

Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Apr 14, 2023

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:

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

📝 Please see our Contribution Guidelines for further guidance.

@ellemouton
Copy link
Collaborator

also related? #7104

Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Roasbeef
Copy link
Member Author

@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 Roasbeef force-pushed the sweeper-rebroadcaster-interaction branch from 095df55 to 24a9b99 Compare April 18, 2023 00:50
@saubyk saubyk requested a review from guggero April 20, 2023 15:17
@Roasbeef Roasbeef force-pushed the sweeper-rebroadcaster-interaction branch from 24a9b99 to 8a274a6 Compare April 20, 2023 18:36
@Roasbeef
Copy link
Member Author

Rebased!

Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to fix the interface🧐

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.
@Roasbeef Roasbeef force-pushed the sweeper-rebroadcaster-interaction branch from 8a274a6 to 3d2daee Compare April 21, 2023 18:54
@Roasbeef Roasbeef merged commit 588a7eb into lightningnetwork:master Apr 21, 2023
@roeierez
Copy link
Contributor

@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
Copy link
Contributor

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

@saubyk saubyk modified the milestones: v0.17.0, v0.16.1 Apr 28, 2023
@Roasbeef
Copy link
Member Author

Roasbeef commented Apr 28, 2023

@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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants