lnrpc+sweep: properly remove any unconfirmed descendant chains a to-be-swept input is spent #6274

Roasbeef · 2022-02-18T02:32:30Z

In this PR, we address issue #6241 by ensuring that if someone sweeps an anchor output of ours after 16 blocks, then we recursively remove any transactions descendant from the set of outputs created by our sweep attempt. Along the way, we add a new LockedBalance output to the WalletBalance command that allows users to keep track of teh total sum of locked outputs more easily. Anytime we do a funding attempt, the outputs get locked, so if a user has a funding transaction that was invalidated by an input double spend, those funds would show as missing until the lease expired.

One quirk with the current solution is that end up cross checking every transaction the sweeper has ever published. This is due to the fact that right now the sweeper's store doesn't remove any prior sweeps even once they're confirmed. If we just looked at the last published sweep, then it's possible that we'd miss a set of relevant transactions.

The methods use in this PR can likely be used elsewhere to remove any weird transactions from the ListTransactions output.

TODO Items

Write integration test reproducing the issue to show solution resolves issue
Look into deleting past "well confirmed" sweeps to prune database state (just 32 bytes per element so not thaaat much data)

C-Otto · 2022-02-18T16:02:50Z

I'm a bit worried about reorgs. What if the foreign tx that steals my anchor is confirmed, your new code removes a bunch of outputs, but then there's a new block that un-confirms the foreign transaction?

Roasbeef · 2022-02-19T00:23:02Z

I'm a bit worried about reorgs. What if the foreign tx that steals my anchor is confirmed, your new code removes a bunch of outputs, but then there's a new block that un-confirms the foreign transaction?

Great question! I have a TODO there about only doing this after a certain "epoch", which would be an interval N blocks after the conflicting transaction confirms. Even w/o this, you'd have those issues when spending unconfirmed change for a funding input as anything can happen until confirmation.

So there's a bit of a tradeoff here: do we do this as soon as we see a conflicting confirmation, which can allow us to immediately free up those outputs, or do we wait for 12 blocks (or w/e) and then do this, which ends up leaving your transaction state (assuming unconfirmed outputs) in a weird state for a longer period of time? I think the change to start showing the leased/locked UTOXs in WalletBalance will make these scenarios easier to account for, as you'd be able to make sure all funds are safu even when weird stuff happens.

lightninglabs-deploy · 2022-02-28T01:00:00Z

@carlaKC: review reminder

Roasbeef · 2022-03-04T01:33:01Z

Ok I added ain integration test that does the following:

Open channel between Alice and Bob
Force close the channel
Use custom block generation to mine only the commitment transaction (Alice's anchor sweep is in the mempool at this point)
Have Alice spend the unconfirmed anchor sweep output into a random address (it's the miner's address)
Mine 16 empty blocks (so the anchor is up for grabs)
Make a custom spend to sweep the anchor output using the csv path
Restart Alice, wait for her to detect what happened
Assert that neither the original anchor sweep, nor the transaction that spent the sweep to another addr is found in Alice's wallet

With that, we've confirmed the scenario as well as the fix. The final thing I think we need to address is @C-Otto's point to either wait a few blocks until we remove the transactions, or do it immediately so the wallet UTXO state is correct sooner.

Roasbeef · 2022-03-04T01:34:28Z

It's worth noting that this fix won't remove the now invalid pending channel, but will ensure that those outputs are now free from the wallet's PoV. The pending channel fix is here: #5567.

yyforyongyu

Very nice itest(also cured the headache I had while writing the fee bumper itest, so thank you)! A major question is how efficient it is to find a double spend by querying all sweep transactions, and a few nits/typos.

lnwallet/btcwallet/btcwallet.go

sweep/sweeper.go

yyforyongyu · 2022-03-04T20:21:50Z

sweep/sweeper.go

+	//
+	// TODO(roasbeef): can be last sweep here if we remove anything confirmed
+	// from the store?
+	pastSweepHashes, err := s.cfg.Store.ListSweeps()


I wonder how large the sweeps can be? If a lot this may be expensive.

if we remove anything confirmed from the store

Do you think we can modify the sweeper-last-tx to store all unconfirmed txes? Atm it seems to only store the latest publish tx.

Yeah that's the thing, we never delete the set of past sweeps as is. For very old nodes, this could end up being hundreds of past weeps. I chose to do this rather than just use the last sweep tx as it seems possible that we back off the failed sweep and then try another combination, causing us to not actually remove this set of conflicting transactions...

Though the disk footprint isn't very large (32 bytes per sweep), I think we should start to prune these sweeps once either we remove them here, we find a conflict, or they confirm with a block of depth N.

So I guess for old sweeps we don't ever use them for anything...

So I guess for old sweeps we don't ever use them for anything...

Yeah nothing other than this new logic, but once the set of swept items are "well confirmed" then we no longer need them, I think it makes sense to roll out this garbage collection into a new issue, so we can expedite getting this into the next minor release.

yyforyongyu · 2022-03-04T20:22:30Z

sweep/sweeper.go

+	// in common with the inputs the spendingTx spent, then we'll remove
+	// those.
+	//
+	// TODO(roasbeef): need to start to remove all transaction hashes after


To save disk space?

Yeah exactly, also with bolt buckets that have a very large key space can bog things down over time.

The original design of the sweeper just intended to keep things simple and continue to try diff combinations of input sweeps untli something confirmed. We got the logic mostly right in that we signaled to the anchor resolver that it was "lost" (also added an assertion for that in the test), but didn't think of users spending unconfirmed change that was a descendent of a sweep output.

I think that we're going to want to keep around the sweeps that confirmed (either in a new bucket or filter our lookup to skip confirmed) because ListSweeps is pretty useful for accounting.

lntest/itest/lnd_onchain_test.go

lnwallet/btcwallet/btcwallet.go

sweep/sweeper.go

lntest/itest/assertions.go

lntest/itest/lnd_onchain_test.go

Roasbeef · 2022-03-17T01:05:09Z

PTAL! All review comments addressed. I'll pull out that sweeper garbage collection question to a new issue, as it isn't 100% critical we do so, but depending on how frequent operations like this happen, will make the conflict removal process more efficient (as there're be less transactions we need to scan).

yyforyongyu

Almost LGTM! A few nits and the only thing to address is that we are now using require instead of t.Fatalf to save some lines.

lntest/itest/assertions.go

carlaKC · 2022-03-17T12:48:44Z

sweep/sweeper.go

+	// in common with the inputs the spendingTx spent, then we'll remove
+	// those.
+	//
+	// TODO(roasbeef): need to start to remove all transaction hashes after


I think that we're going to want to keep around the sweeps that confirmed (either in a new bucket or filter our lookup to skip confirmed) because ListSweeps is pretty useful for accounting.

carlaKC · 2022-03-17T12:54:32Z

lntest/itest/lnd_onchain_test.go

+	// We'll fund her with *just* enough coins to open the channel.
+	const (
+		firstChanSize  = 1_000_000
+		secondChanSize = 500_000


secondUtxo size? We don't actually open a channel witth this.

Ah left over from a prior design where I was trying to open a channel which was juuust the right size, but then realized I could just do a sweep all instead. Will remove in the next revision.

Or actually the other portion was to ensure she had enough on chain to be able to add inputs for a sweep as well.

Updated to use a different variable name here.

In this commit, we add a new field to the WalletBalance call that permits users to account for the set of outputs that may be locked due to a pending transaction. Without this field any time users locked outputs for things like PSBT signing, then they disappear from the WalletBalance call, which may cause a panic.

…nflicts Before this commit, we we were trying to sweep an anchor output, and that output was spent by someone else (not the sweeper), then we would report this back to the original resolver (allowing it to be cleaned up), and also remove the set of inputs spent by that transaction from the set we need to sweep. However, it's possible that if a user is spending unconfirmed outputs, then the wallet is holding onto an invalid transaction, as the outputs that were used as inputs have been double spent elsewhere. In this commit, we fix this issue by recursively removing all descendant transactions of our past sweeps that have an intersecting input set as the spending transaction. In cases where a user spent an unconfirmed output to funding a channel, and that output was a descendant of the now swept anchor output, the funds will now properly be marked as available. Fixes lightningnetwork#6241

In this commit, we add a new integration tests to exercise the fix introduced in the prior commit. In this test, we reconstruct a scenario for a 3rd party to sweep an anchor spend after force closing, causing a prior spend we had to be invalidated. Without the prior commit, this test fails as the original anchor sweep is still found in the wallet.

Roasbeef · 2022-03-17T23:47:49Z

Github Actions is having sone issues, but I've run the itests+lint locally and confirmed they all pass. Given nothing major changd on this PR since the last run (just adding some newlines mainly), I think this can land.

Roasbeef added wallet The wallet (lnwallet) which LND uses utxo sweeping bug fix labels Feb 18, 2022

Roasbeef mentioned this pull request Feb 18, 2022

Lost anchor: stuck sweep transaction (unconfirmed balance) #6241

Closed

carlaKC self-requested a review February 18, 2022 05:59

Roasbeef requested a review from yyforyongyu March 3, 2022 01:09

Roasbeef force-pushed the anchor-utxos-unconf branch 2 times, most recently from 06fffd5 to 6b9d69d Compare March 4, 2022 01:27

Roasbeef added this to the v0.15.0 milestone Mar 4, 2022

Roasbeef force-pushed the anchor-utxos-unconf branch from 6b9d69d to 20c273b Compare March 4, 2022 01:42

yyforyongyu reviewed Mar 4, 2022

View reviewed changes

Roasbeef added the P1 MUST be fixed or reviewed label Mar 7, 2022

carlaKC reviewed Mar 16, 2022

View reviewed changes

Roasbeef modified the milestones: v0.15.0, v0.14.3 Mar 16, 2022

Roasbeef requested review from yyforyongyu and carlaKC March 17, 2022 01:03

Roasbeef force-pushed the anchor-utxos-unconf branch from 20c273b to f7ddacf Compare March 17, 2022 01:05

yyforyongyu reviewed Mar 17, 2022

View reviewed changes

lntest/itest/assertions.go Show resolved Hide resolved

lntest/itest/assertions.go Outdated Show resolved Hide resolved

lntest/itest/assertions.go Outdated Show resolved Hide resolved

lntest/itest/assertions.go Outdated Show resolved Hide resolved

carlaKC approved these changes Mar 17, 2022

View reviewed changes

Roasbeef force-pushed the anchor-utxos-unconf branch from f7ddacf to b714bba Compare March 17, 2022 23:07

Roasbeef added 3 commits March 17, 2022 16:36

docs/release-notes: add entry for 3rd party anchor sweep fix

d164307

Roasbeef force-pushed the anchor-utxos-unconf branch from b714bba to d164307 Compare March 17, 2022 23:37

Roasbeef merged commit 3a04017 into lightningnetwork:master Mar 17, 2022

Roasbeef mentioned this pull request Mar 24, 2022

build: create v0.14.3 rc1 release branch #6366

Merged

guggero mentioned this pull request Mar 28, 2022

UTXO Transaction not found #6367

Closed

guggero mentioned this pull request Apr 21, 2022

[bug]: listunspent shows old and confirmed TX #6434

Closed

Roasbeef mentioned this pull request May 3, 2022

How to remove sweeps which are never going to succeed! #6471

Closed

WesVleuten mentioned this pull request May 25, 2022

Sweep transaction stuck after switching backends #6572

Closed

ellemouton mentioned this pull request Nov 2, 2022

[bug]: Unknown Unconfirmed Balance #7104

Open

Roasbeef mentioned this pull request Nov 21, 2022

[bug]: sweep transaction not recognized by on-chain wallet #7181

Open

Horndev mentioned this pull request Feb 8, 2023

[bug]: sweeper keeps trying to sweep invalid anchor output until locking up all on-chain funds on node #7390

Closed

Roasbeef mentioned this pull request Apr 17, 2023

sweep: remove conflicted sweep txns from the rebroadcaster #7599

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lnrpc+sweep: properly remove any unconfirmed descendant chains a to-be-swept input is spent #6274

lnrpc+sweep: properly remove any unconfirmed descendant chains a to-be-swept input is spent #6274

Roasbeef commented Feb 18, 2022 •

edited

Loading

C-Otto commented Feb 18, 2022 •

edited

Loading

Roasbeef commented Feb 19, 2022

lightninglabs-deploy commented Feb 28, 2022

Roasbeef commented Mar 4, 2022

Roasbeef commented Mar 4, 2022

yyforyongyu left a comment

yyforyongyu Mar 4, 2022

Roasbeef Mar 5, 2022

yyforyongyu Mar 7, 2022

Roasbeef Mar 17, 2022

yyforyongyu Mar 4, 2022

Roasbeef Mar 5, 2022

carlaKC Mar 17, 2022

Roasbeef commented Mar 17, 2022

yyforyongyu left a comment

carlaKC Mar 17, 2022

carlaKC Mar 17, 2022

Roasbeef Mar 17, 2022

Roasbeef Mar 17, 2022

Roasbeef Mar 17, 2022

Roasbeef commented Mar 17, 2022

lnrpc+sweep: properly remove any unconfirmed descendant chains a to-be-swept input is spent #6274

lnrpc+sweep: properly remove any unconfirmed descendant chains a to-be-swept input is spent #6274

Conversation

Roasbeef commented Feb 18, 2022 • edited Loading

C-Otto commented Feb 18, 2022 • edited Loading

Roasbeef commented Feb 19, 2022

lightninglabs-deploy commented Feb 28, 2022

Roasbeef commented Mar 4, 2022

Roasbeef commented Mar 4, 2022

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Mar 17, 2022

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Mar 17, 2022

Roasbeef commented Feb 18, 2022 •

edited

Loading

C-Otto commented Feb 18, 2022 •

edited

Loading