-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
sweep: remove conflicted sweep txns from the rebroadcaster #7599
Conversation
also related? #7104 |
There was a problem hiding this 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.
@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. |
095df55
to
24a9b99
Compare
24a9b99
to
8a274a6
Compare
Rebased! |
There was a problem hiding this 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🧐
There was a problem hiding this 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.
8a274a6
to
3d2daee
Compare
@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. 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 |
@Roasbeef I can open another issue on the above but would be great to get your insights on that beforehand. |
@roeierez correct, the very last sweep tx (but not all the others) is re-broadcasted on start up.
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.
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.
I think you're right. We should remove that tx, and also call this same routine to remove any conflicting transactions from the wallet: Lines 713 to 727 in 35917eb
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. |
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). |
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
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.