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: add wallet inputs to reach dust limit #3814

Merged
merged 9 commits into from Dec 18, 2019

Conversation

@joostjager
Copy link
Collaborator

joostjager commented Dec 10, 2019

This PR enables the sweeper to sweep inputs that are positively-yielding, but by itself wouldn't reach the dust limit for the sweep transaction output value at the chosen fee rate.

They way to deal with this is to attach additional wallet input(s) to bring up the output value of the transaction.

This is a preparation for sweeping anchor outputs that are too small to reach the dust limit of the sweep tx output.

There are also a few commits in this PR that aren't strictly necessary for the functionality that this PR adds, but are included anyway to prevent touching the code again in the follow-up.

Next step after this PR is to add forced sweeping of negatively yielding inputs.

@joostjager joostjager requested review from cfromknecht and Roasbeef as code owners Dec 10, 2019
@joostjager joostjager changed the title sweep: add wallet inputs to reach dust limit sweep: add wallet inputs to reach dust limit [do not review] Dec 10, 2019
We need it to be set in order to properly test the sweeper handling the
dust limit on regtest.
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch 3 times, most recently from bd3d6c8 to 215c322 Dec 12, 2019
@joostjager joostjager removed request for Roasbeef and cfromknecht Dec 12, 2019
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch 2 times, most recently from 26a6329 to 3dea473 Dec 12, 2019
joostjager added 2 commits Dec 9, 2019
Using a fee rate just as an identifier can be confusing.
Fixes a bug where bucket sizes were not the configured size, but the
configured size plus the min relay fee.
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch from 3dea473 to 86121bb Dec 13, 2019
@halseth halseth added the anchors label Dec 13, 2019
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch 4 times, most recently from c15f2ff to aa898b2 Dec 13, 2019
joostjager added 2 commits Dec 9, 2019
Prepares for adding more input-specific sweep parameters.
Get rid of needless referencing of the embedded object.
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch from aa898b2 to 3db8d0b Dec 13, 2019
Copy link
Collaborator

halseth left a comment

Good PR! Excellent commit structure, and a nice cleanup :)

sweep/sweeper.go Show resolved Hide resolved
sweep/tx_input_set.go Show resolved Hide resolved
sweep/sweeper.go Show resolved Hide resolved
sweep/tx_input_set.go Outdated Show resolved Hide resolved
sweep/txgenerator.go Outdated Show resolved Hide resolved
sweep/txgenerator.go Outdated Show resolved Hide resolved
sweep/txgenerator.go Outdated Show resolved Hide resolved
sweep/txgenerator.go Outdated Show resolved Hide resolved
sweep/tx_input_set.go Outdated Show resolved Hide resolved
sweep/txgenerator.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch 3 times, most recently from d98a1d1 to fb6377d Dec 13, 2019
@joostjager joostjager requested a review from halseth Dec 13, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Dec 13, 2019

Unit test added

@joostjager joostjager requested a review from wpaulino Dec 13, 2019
@joostjager joostjager added this to the 0.9.0 milestone Dec 13, 2019
@joostjager joostjager changed the title sweep: add wallet inputs to reach dust limit [do not review] sweep: add wallet inputs to reach dust limit Dec 13, 2019
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch from fb6377d to b142690 Dec 13, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Dec 13, 2019
sweep/sweeper.go Show resolved Hide resolved
sweep/tx_input_set.go Outdated Show resolved Hide resolved
sweep/tx_input_set.go Outdated Show resolved Hide resolved
sweep/tx_input_set.go Outdated Show resolved Hide resolved
sweep/tx_input_set.go Show resolved Hide resolved
sweep/sweeper.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch 2 times, most recently from 2187e18 to 61bdf32 Dec 16, 2019
@joostjager joostjager requested a review from wpaulino Dec 16, 2019
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch from 61bdf32 to 600e059 Dec 16, 2019
sweep/tx_input_set.go Show resolved Hide resolved
t.inputTotal = newInputTotal
t.outputValue = newOutputValue
t.inputs = append(t.inputs, input)
t.weightEstimate = newWeightEstimate

This comment has been minimized.

Copy link
@halseth

halseth Dec 17, 2019

Collaborator

looks like all these counters could be calculated on demand instead (just pointing it out, still up for discussion whether that is better than just keeping running counters).

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 17, 2019

Author Collaborator

In a later commit, i would also need the fromWallet state. I am not too convinced of it. When you add 20 inputs, 200 (virtual) add operations will be executed. (1+2+3+...+20).

@wpaulino what do you think?

This comment has been minimized.

Copy link
@wpaulino

wpaulino Dec 17, 2019

Collaborator

Adding a new input would make us recalculate everything, but the performance impact is likely negligible unless there are lots of inputs. I'm fine with leaving things as is given that the code is already there.

sweep/txgenerator.go Outdated Show resolved Hide resolved
sweep/sweeper.go Outdated Show resolved Hide resolved
sweep/tx_input_set.go Show resolved Hide resolved

// Retrieve wallet utxos. Only consider confirmed utxos to prevent
// problems around RBF rules for unconfirmed inputs.
utxos, err := t.wallet.ListUnspentWitness(1, math.MaxInt32)

This comment has been minimized.

Copy link
@halseth

halseth Dec 17, 2019

Collaborator

to avoid having the wallet as a field on the input set, the list of utxos can be passed into this method.

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 17, 2019

Author Collaborator

yes, but then we'd always get utxos, even if the dust limit is already reached. could also move that out then. but not sure if it is better

sweep/interface.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch from 600e059 to 997a67e Dec 17, 2019
@joostjager joostjager requested a review from halseth Dec 17, 2019
t.inputTotal = newInputTotal
t.outputValue = newOutputValue
t.inputs = append(t.inputs, input)
t.weightEstimate = newWeightEstimate

This comment has been minimized.

Copy link
@wpaulino

wpaulino Dec 17, 2019

Collaborator

Adding a new input would make us recalculate everything, but the performance impact is likely negligible unless there are lots of inputs. I'm fine with leaving things as is given that the code is already there.

sweep/tx_input_set.go Outdated Show resolved Hide resolved
sweep/tx_input_set.go Show resolved Hide resolved
sweep/sweeper.go Show resolved Hide resolved
sweep/tx_input_set_test.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch from 997a67e to e70fd44 Dec 17, 2019
joostjager added 4 commits Dec 10, 2019
A refactoring that introduces no functional changes. This prepares for
the addition of wallet utxos to push the sweep tx above the dust limit.

It also enabled access to input-specific sweep parameters during tx
generation. This will be used in later commits to control the sweep
process.
We need access to additional wallet functionality. This commit creates
an interface to prevent passing in multiple function pointers.
Prepares for adding another level of nesting.
This commit allows sweeper to sweep inputs that on its own are not able
to form a sweep transaction that meets the dust limit.

This functionality is useful for sweeping small outputs. In the future,
this will be particularly important to sweep anchors. Anchors will
typically be spent with a relatively large fee to pay for the parent tx.
It will then be necessary to attach an additional wallet utxo.
@joostjager joostjager requested a review from wpaulino Dec 17, 2019
@joostjager joostjager force-pushed the joostjager:sweeper-add-utxo branch from e70fd44 to e01600f Dec 17, 2019
Copy link
Collaborator

halseth left a comment

LGTM 👍

v0.9.0-beta automation moved this from Needs Review to Approved Dec 18, 2019
@joostjager joostjager merged commit 7ecbe22 into lightningnetwork:master Dec 18, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 62.165%
Details
v0.9.0-beta automation moved this from Approved to Done Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
3 participants
You can’t perform that action at this time.