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.

Sign up for GitHub

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

Jump to bottom

sweep: allow force sweeping of negatively yielding inputs #3809

Merged
merged 4 commits into from
Jan 23, 2020
Merged

sweep: allow force sweeping of negatively yielding inputs #3809

merged 4 commits into from
Jan 23, 2020

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Dec 9, 2019 •
edited
Loading

This PR adds force sweep functionality to the sweeper. Force sweep signals that an input needs to be swept even if its yield is negative. This is useful in CPFP scenarios where the parent tx output is only small. This will be common in the upcoming new anchor outputs commitment format.

The force flag is exposed on the BumpFee rpc call and through lncli. In many cases it will be required to have a wallet utxo available to bring the tx output above the dust limit. PR #3814 extended the sweeper with the capability to attach wallet utxos if needed.

@joostjager joostjager added incomplete WIP PR, not fully finalized, but light review possible utxo sweeping labels Dec 9, 2019
@joostjager joostjager changed the title sweep: prepare for anchor sweeping sweep: prepare for anchor sweeping [don't review] Dec 9, 2019
@joostjager joostjager force-pushed the anchor-sweeper branch 3 times, most recently from b82c3f7 to 3964624 Compare December 9, 2019 11:24
@halseth halseth added the anchors label Dec 9, 2019
@joostjager joostjager force-pushed the anchor-sweeper branch 2 times, most recently from 3136500 to 1319eff Compare December 9, 2019 14:40
@joostjager joostjager force-pushed the anchor-sweeper branch 7 times, most recently from 2bfce68 to d7a2265 Compare December 13, 2019 14:09
@roeierez
Copy link
Contributor

@joostjager Will this PR solve the use case where channel was closed with small local balance?
Right now I have ~2000 sat that can't be swept due to fee rate.
in #3814 there is an attempt to use additional input but I don't have any in my wallet.

@joostjager joostjager force-pushed the anchor-sweeper branch 2 times, most recently from 339d292 to 0178cea Compare January 9, 2020 11:13
@joostjager
Copy link
Contributor Author

With an input of that size, the idea is to modify the sweep fee rate to a lower value using the bumpfee rpc, so that it becomes economical to sweep. Without a wallet utxo, it still needs to produce a tx that has an output above the dust limit.

Note that the sweeper uses fee rate buckets to batch inputs together. The first bucket is 1-10 sat/byte. The actual sweep fee rate is the average of all inputs in a bucket. If your input is the only input in the 1-10 bucket and you've lowered the input fee rate preference to 1 sat/b, it should be swept at that rate.

This PR is not adding anything to that. It mainly allows negatively yielding inputs to be swept using the force flag, but for that you also need a wallet utxo.

@joostjager joostjager force-pushed the anchor-sweeper branch 3 times, most recently from ffed1e5 to 0a31880 Compare January 9, 2020 12:57
@joostjager
sweep+walletrpc+lncli: report requested fee preference for pending sw…
280611a 
…eeps

Previously only the fee rate used for the last sweep (the sweep bucket
average) was reported. This commit adds the request fee preference to
the report, which is used to select a bucket and the sweep tx fee rate.
@joostjager joostjager force-pushed the anchor-sweeper branch 2 times, most recently from 7cb17d1 to b114491 Compare January 9, 2020 19:54
@joostjager joostjager changed the title sweep: prepare for anchor sweeping [don't review] sweep: allow force sweeping of negatively yielding inputs Jan 9, 2020
@joostjager joostjager removed the incomplete WIP PR, not fully finalized, but light review possible label Jan 9, 2020
@joostjager
Copy link
Contributor Author

Note to self: @cfromknecht suggested to require regular inputs to not just be positively yielding, but yield at least 2/3 (for example) of their value

@joostjager
Copy link
Contributor Author

Checked what is needed for that minimum yield requirement. The sweeper change is small, but there are quite a few changes in the test (setup) required. Leaving that change for a separate pr.

wpaulino
wpaulino reviewed Jan 13, 2020
View reviewed changes
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Nice small change! Will give it another pass, but LGTM so far.

lnrpc/walletrpc/walletkit.proto Show resolved Hide resolved
cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
sweep/sweeper.go Outdated Show resolved Hide resolved
sweep/sweeper.go Show resolved Hide resolved
@joostjager
Copy link
Contributor Author

Comments processed, ready for another pass

@Roasbeef Roasbeef added this to the 0.10.0 milestone Jan 14, 2020
@Roasbeef Roasbeef added the v0.10 label Jan 14, 2020
wpaulino
wpaulino approved these changes Jan 14, 2020
View reviewed changes
sweep/sweeper.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef modified the milestones: 0.10.0, 0.9.0 Jan 14, 2020
halseth
halseth approved these changes Jan 15, 2020
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Nice PR :)

sweep/sweeper.go Outdated Show resolved Hide resolved
sweep/tx_input_set.go
}
// Initialize new wallet total with the current wallet total. This is
// updated below if this input is a wallet input.
newWalletTotal := t.walletInputTotal
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like the handling of this field can stay inside the switch case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it outside, because otherwise you have to be very careful not to update t.walletInputTotal before you do any additional checks. In that case, the total would be updated without the add happening.

cmd/lncli/walletrpc_active.go
@@ -142,24 +151,14 @@ func bumpFee(ctx *cli.Context) error {
return err
}

var confTarget, satPerByte uint32
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove this validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The server already validates this, so this is just unnecessary maintenance. Change was suggested by @wpaulino and I liked it.

sweep/tx_input_set.go Show resolved Hide resolved
@joostjager
sweep: allow updating all sweep parameters
16832ce 
This is a preparation for adding additional parameters besides the fee
preference.
@joostjager
sweep: create add constraints
14237f5 
This refactor prepares for the addition of specific constraints for
force sweep inputs.
@joostjager
sweep: allow force sweeps
b0aae13
@joostjager
Copy link
Contributor Author

Holding off merge until 0.9 final is cut.

@joostjager joostjager merged commit ae9c6fa into lightningnetwork:master Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halseth halseth halseth approved these changes

@wpaulino wpaulino wpaulino approved these changes

Assignees
No one assigned
Labels
anchors utxo sweeping
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@joostjager @roeierez @halseth @wpaulino @Roasbeef