sweep: create sweeper by joostjager · Pull Request #1960 · lightningnetwork/lnd

joostjager · 2018-09-21T18:19:23Z

This PR moves the sweeper logic from utxonursery into a separate sweeper struct.

A follow up PR is #2000, which will let resolvers use the sweeper directly instead of going through nursery. The end goal is to remove nursery completely.

In this PR, there is no db migration to clean up now unused nursery store data. This allows us to keep the downgrade path open for a while.

halseth · 2018-12-12T08:49:55Z

What happens when the publish returns an error? It could be a non-recoverable error that has something to do with one of the inputs (wrong script, sign, ..). Or it could be a double spend because one or more of the inputs have been spent by the remote party or by our own sweep tx (that was published before restart). I think there are not many options then retry the next block?

We can be smarter about handling these errors. If the error is non-recoverable, then publishing in the next block won't really help (also can potentially get us banned by peers if we keep sending them invalid transactions). In that case we should return the error and exit IMO to surface the bug.

For double spends we can detect which input was already spent, remove it from the sweep, and retry. I think this is what is implicitly done already (since an input will be removed from the set after spend is detected), but I think it would help to make it more clear. We do something similar in the breacharbiter.

What happens when the tx never confirms (for example fee too low)? If we keep waiting, it will block sweeping any other input forever. At some point we need to give up and go back to 1. But will we forget about the lost inputs then and loose the money? Or retry them? In that case, we'll get a double spend with our own stuck tx and sweeping will again be blocked forever.

It should be handled, but my suggestion is to reduce the scope of this PR to not include it. This is an existing problem.

It will fail, but after that sweep tx confirms, and allsets doesn't contain those inputs anymore, it will succeed.

Yes, I think this is actually my suggestion. Maybe we are actually talking about the same solution here, the behavior is just not obvious. See comment above.

I don't see how the PR can be reduced in size significantly by simplifying the retry mechanism. I only see about 20 lines dealing with the retry logic. And a form of retry is needed in any case (items above).

I don't think retry logic is strictly needed, and can be added in a follow-up PR.

joostjager · 2018-12-12T09:10:42Z

@Roasbeef your comments have been processed.

joostjager · 2018-12-12T10:45:25Z

Discussed simplification with @halseth. We could take out the reschedule logic and just try to sweep once every block. Start the batch timer at the beginning of every block and if it expires, try to sweep.

The downside of this is that a transaction that is stuck because of low fees will keep blocking all subsequent sweep txes if the backend doesn't support RBF. (This is something that would be solved in a probabilistic way by the randomized exponential back off.) It is also a regression from nursery, because nursery does skip over stuck txes because they are organized per height.

If we'd decide to go this path, the follow up pr with exponential backoff should probably go in before release.

@Roasbeef your input please

joostjager · 2018-12-12T11:03:52Z

Added one commit that handles errors in a strict way, not to camouflage potential bugs.

halseth

This is starting to look pretty good to me :)

halseth · 2018-12-14T12:47:53Z

Can be moved to immediately before the loop, I guess. Rationale being that we might encounter an error before ever using the result of this calculation :)

halseth · 2018-12-14T13:03:54Z

since this is only used for calculating the dust limit(?), can we instead store the dust limit directly here?

I wanted to have the dust limit calculation inside txgenerator to logically group the functionality together. generateInputPartitionings takes two fees and uses that as the basis to calculate the partitionings.

halseth · 2018-12-14T13:06:12Z

If this happens, feels like the whole sweeper should exit.

What do you mean with exit? The only thing that is running is the sweeper main event loop and that one is exited.

halseth · 2018-12-14T13:26:12Z

Do you have an estimate how many blocks from sending the input to the sweeper until this condition is met, on average? (if it never successfully confirms)

I think ((1 << 10) - 1) / 2 😄 so about 500 blocks.

halseth · 2018-12-14T13:29:01Z

is this not critical at this point?

It is a pre-existing bug that I wanted to mark with a TODO.

halseth · 2018-12-14T13:31:49Z

I think this is still happy flow, therefore Info.

Roasbeef · 2018-12-16T23:48:36Z

Why not just read the buckets directly? The migration logic would be more precise that way and less risk of accidentally reaching into the wrong bucket, if a future key/bucket in the codebase starts with the prefix.

Roasbeef · 2018-12-17T00:55:13Z

Made a commit with some additional logs that should likely be cherry picked in:

c2e4fb2

e8e9b52

Here's the branch itself as I'm modifying commits as I test: https://github.com/Roasbeef/lnd/pull/new/sweeper-debug

Roasbeef · 2018-12-17T01:21:53Z

With the patch above, I ran on a very old node with an ancient nursery and saw no transactions migrated:

./lnd.log.18:2018-12-16 20:06:20.012 [INF] SWPR: Migrating UTXO nursery finalized TXIDs

joostjager · 2018-12-17T09:49:53Z

Commits cherry-picked except for wraps at 79 (instead of 80) chars.
Sweeper store bucket scan replaced by exact bucket retrieval.
Commit added to soften error handling again. Both error handling commits to be squashed when we are really certain that we want it like this.

joostjager · 2018-12-17T09:50:45Z

Tested nursery tx migration again. Seems to work on my machine.

Roasbeef

🎉 LGTM 🎉

Ready to merge on my end once the final two commits squashed.

joostjager · 2018-12-18T09:44:20Z

Added one more commit: lnwallet: prevent static fee estimator fees from being modified
Squashed error handling commits
Fixed unit test flake

Modifying the static fees is not thread safe. In this commit the fees are made immutable.

We need to distinguish an lnd build for the purpose of integration testing from a regular dev build. This makes it possible to adapt parameters to let integration tests run faster (for example: sweeper batch window).

This commit is a preparation for the implementation of remote spend detection. Remote spends may happen before we broadcast our own sweep tx. This calls for accurate height hints.

This commit adds a function that takes a set of inputs and splits them in sensible sets to be used for generating transactions.

This commit adds a store for the sweeper. The sweeper needs minimal persistent data to be able to recognize its own sweeps.

In this commit, the sweep package is extended from just tx generation to an active sweeper that collects sweep inputs and autonomously proceeds to publish the sweep tx after the batch window time interval has passed without new inputs being added.

Previously, nursery generated and published its own sweep txes. It stored the sweep tx in nursery_store to prevent a new tx with a new sweep address from being generated on restart. In this commit, sweep generation and publication is removed from nursery and delegated to the sweeper. Also the confirmation notification is received from the sweeper.

cfromknecht

Awesome work @joostjager, excited to finally have this in and help people sweep their stuck outputs! Very happy with the final iteration and the huge reduction in complexity over the nursery, LGTM 🎉

Roasbeef

LGTM 🔮

Roasbeef added enhancement Improvements to existing features / behaviour channel management The management of the nodes channels contracts P3 might get fixed, nice to have utxo sweeping labels Sep 22, 2018

joostjager force-pushed the sweeper branch 9 times, most recently from 5ce7feb to 0e9fbeb Compare September 27, 2018 02:41

This was referenced Oct 1, 2018

sweep: prepare for sweeper #1978

Merged

utxonursery: remove nursery [DO NOT REVIEW] #2000

Closed

joostjager force-pushed the sweeper branch 14 times, most recently from 369d13b to 58740fd Compare October 23, 2018 15:02

halseth reviewed Dec 14, 2018

View reviewed changes

Roasbeef reviewed Dec 17, 2018

View reviewed changes

Roasbeef previously approved these changes Dec 18, 2018

View reviewed changes

joostjager and others added 13 commits December 18, 2018 10:50

lnwallet: expose relay fee on fee estimators

423dd8a

lnwallet: prevent static fee estimator fees from being modified

91f3df0

Modifying the static fees is not thread safe. In this commit the fees are made immutable.

build: apply rpctest tag in itest

3b1357c

We need to distinguish an lnd build for the purpose of integration testing from a regular dev build. This makes it possible to adapt parameters to let integration tests run faster (for example: sweeper batch window).

lnwallet: add a String() method to WitnessType

3fd03b2

sweep: refactor weight estimation

26cfc50

sweep+utxonursery+cnct+breacharbiter: add height hint to input

e43e895

This commit is a preparation for the implementation of remote spend detection. Remote spends may happen before we broadcast our own sweep tx. This calls for accurate height hints.

sweep: move tx generation to separate file

067817f

sweep: add input partitionings generator

a2dcca2

This commit adds a function that takes a set of inputs and splits them in sensible sets to be used for generating transactions.

sweep: add sweeper store

1f06565

This commit adds a store for the sweeper. The sweeper needs minimal persistent data to be able to recognize its own sweeps.

sweep: add time-based sweeper

01e64af

In this commit, the sweep package is extended from just tx generation to an active sweeper that collects sweep inputs and autonomously proceeds to publish the sweep tx after the batch window time interval has passed without new inputs being added.

sweep: add sweeper test

eec2efd

cnct: add todo comment for sweeper

687d4e7

cfromknecht approved these changes Dec 19, 2018

View reviewed changes

Roasbeef approved these changes Dec 19, 2018

View reviewed changes

Roasbeef mentioned this pull request Dec 19, 2018

Pending channel with negative blocks_til_maturity #1610

Closed

joostjager mentioned this pull request Nov 7, 2019

utxonursery: delete #3688

Closed

yyforyongyu mentioned this pull request Sep 28, 2023

[epic] sweeper: improve sweeper with defined responsibility and functionalities #8042

Closed

Conversation

joostjager commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halseth commented Dec 12, 2018

Uh oh!

joostjager commented Dec 12, 2018

Uh oh!

joostjager commented Dec 12, 2018

Uh oh!

joostjager commented Dec 12, 2018

Uh oh!

halseth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Roasbeef commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Roasbeef commented Dec 17, 2018

Uh oh!

joostjager commented Dec 17, 2018

Uh oh!

joostjager commented Dec 17, 2018

Uh oh!

Roasbeef left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager commented Dec 18, 2018

Uh oh!

cfromknecht left a comment

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joostjager commented Sep 21, 2018 •

edited

Loading

joostjager Dec 14, 2018 •

edited

Loading

Roasbeef commented Dec 17, 2018 •

edited

Loading

Roasbeef left a comment •

edited

Loading