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

multi: add ability to sweep all coins in the the wallet to an addr to sendcoins #2198

Merged
merged 13 commits into from Jan 15, 2019

Conversation

5 participants
@Roasbeef
Copy link
Member

Roasbeef commented Nov 18, 2018

In this PR, we add a new --sweepall flag to sendcoins which allows users to sweep all funds to a target address. Along the way, we fix a possible double-spend race condition bug, refactor transaction generation in the rpcserver, and introduce a new set of abstractions in the sweep package we use to implement this functionality.

Migration to lnwallet.InputScript for witness generation

In this PR, we extend the WitnessGenerator type to now return an InputScript. This allows it to be more encompassing, as now callers can expect a sigScript to be populated if the input being swept requires a sigScript field. Along the way, we add the ability to generate witnesses for p2wkh and np2wkh types to the lnwallet.GenWitnessFunc method.

We then take a step towards allowing the sweeper to also craft sweeping transaction for n2pwkh outputs. We do so by modifying the input.BuildWitness method to instead return an InputScript. Additionally, when populating inputs if a sigScript is present, it will now be populated by the sweeper.

Expansion of CreateSweepTx for other input types

In this PR, we add two additional cases to getWeightEstimate which will allow it to properly sweep non CSV and CLTV outputs. Before this commit, any p2wkh and np2wkh outputs would be filtered out by this method and not applied to the final sweeping transaction.

Sweeping all inputs: sweep.CraftSweepAllTx

In this commit, we add a new function, sweep.CraftSweepAllTx. This function allows callers to craft a transaction which sweeps ALL outputs from the wallet to a single target address. It can either be used for UTXO consolidation (at the cost of privacy by co-mingling inputs), or simply to sweep all funds out of a wallet for various reasons.

In an attempt to ensure this method is loosely coupled and testable, for all behavior structs, we create brand new interface to accept. This ensures that we only rely on the minimal number of methods needed to perform our duty.

Fixing Lingering Double-Spend Race Conditions

In this PR, we add a new method WithCoinSelectLock. This method will allow us to fix bugs in the project atm that can arise if a channel funding is attempted (either manually or by autopilot) while a users is attempting to send an on-chain transaction. If this happens concurrently, then both contexts will grab the set of UTXOs and attempt to lock them one by one. However, since they didn't obtain an exclusive snapshot of the UTXO set of the wallet, they may both attempt to lock the same input.

We also ensure that calls to SendMany cannot run into this issue by using the WithCoinSelectLock synchronization when attempting to instruct the internal wallet to send payments.


Example run:

⛰   lncli --network=simnet sendcoins --sweepall --addr=sb1qsy8772pkfucsvmuyw82gexyd4u69pvve9w98v3
{
    "txid": "1931f6653ecb2add24e00ce03d6a66ce705ebf633bfffb2b67674000d5f3d5d4"
}
2018-11-17 20:33:36.392 [INF] RPCS: [sendcoins] addr=sb1qsy8772pkfucsvmuyw82gexyd4u69pvve9w98v3, amt=0 BTC, sat/kw=12500, sweep_all=true
2018-11-17 20:33:36.397 [INF] SWPR: Creating sweep transaction for 107 inputs (0 CSV, 0 CLTV, 107 other) using 3125000 sat/kw
2018-11-17 20:33:36.416 [DBG] RPCS: Sweeping all coins from wallet to addr=sb1qsy8772pkfucsvmuyw82gexyd4u69pvve9w98v3, with tx=(*wire.MsgTx)(0xc0006de000)({
 Version: (int32) 2,
 TxIn: ([]*wire.TxIn) (len=107 cap=128) {

@Roasbeef Roasbeef changed the title multi: add ability to sweep all coins in the the wallet to an addr to sendcoins [WIP] multi: add ability to sweep all coins in the the wallet to an addr to sendcoins Nov 24, 2018

@molxyz

This comment has been minimized.

Copy link

molxyz commented Dec 12, 2018

Hi @Roasbeef ,

I've compiled this PR and testing it on Windows (I guess the results should be the same on linux or whatever).. But it seems the tx fee is miscalculated at a very high rate, and the PR won't let me set a tx fee.

According to my bitcoind estimatesmartfee, the average fee is 1sat/byte:

{
  "feerate": 0.00001000,
  "blocks": 6
}

But here's the result from lnd:

c:\>tlncli walletbalance

{
    "total_balance": "51000000",
    "confirmed_balance": "51000000",
    "unconfirmed_balance": "0"
}

c:\>tlncli sendcoins mumpvbYmFkkRVtGiC8spLqSYixza653J3x --sat_per_byte=5 --sweepall

[lncli] rpc error: code = Unknown desc = transaction is not sane: transaction output has negative value of -90484375

c:\>tlncli sendcoins mumpvbYmFkkRVtGiC8spLqSYixza653J3x --sweepall

{
    "txid": "fc413d9d14f70922cc6669755705ed778063bcea59775bc382f423b82c84e81d"
}

The Fee Size for this tx is 266.38 sat/B

@Roasbeef Roasbeef added this to In progress in High Priority via automation Dec 18, 2018

@Roasbeef Roasbeef force-pushed the Roasbeef:sendall-rpc branch from 76a9082 to 9db5fe9 Dec 21, 2018

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Dec 21, 2018

Just pushed up a new version that's rebased on top of the new sweeper changes, and fixes the fee related bug above. Removing the WIP tag now, only final thing is additional unit tests for the new file and also an integration test for p2wkh and np2wkh.

@Roasbeef Roasbeef changed the title [WIP] multi: add ability to sweep all coins in the the wallet to an addr to sendcoins multi: add ability to sweep all coins in the the wallet to an addr to sendcoins Dec 21, 2018

@Roasbeef Roasbeef force-pushed the Roasbeef:sendall-rpc branch from 9db5fe9 to 54df1ef Dec 21, 2018

Show resolved Hide resolved sweep/walletsweep.go
Show resolved Hide resolved lnwallet/witnessgen.go
weightEstimate.AddNestedP2WSHInput(size)
} else {
weightEstimate.AddWitnessInput(size)
}

This comment has been minimized.

@joostjager

joostjager Dec 21, 2018

Collaborator

It looks like the logic to estimate the weight gets spread out. How about creating a func on the weightestimator that takes an Input?

This comment has been minimized.

@joostjager

joostjager Dec 21, 2018

Collaborator

Might also prevent code duplication below.

This comment has been minimized.

@Roasbeef

Roasbeef Dec 24, 2018

Author Member

Arguably, we don't even need to check if it's a nested witness or not below since we're only seeking a relative ordering. Below we get a more precise estimate since it factors in things like the var int signal the number of inputs, the witness marker bytes, etc.

Morphing the estimator to take a sweep.Input would cause an import cycle atm as the sweep package imports lnwallet. We could move sweep.Input into lnwallet but then that exacerbates the "catch all" trait of the lnwallet package as is. IMO, the package should be split up to 4+ (at the very least) distinct packages.

An alternative approach would be to have the method in question take the input and the estimator, mutating the estimator, and returning no value. What do you think of that?

This comment has been minimized.

@joostjager

joostjager Dec 25, 2018

Collaborator

With only segwit inputs, the relative ordering was correct, because all the common elements of (the serialization) of an input cancelled out. Now with nested p2wsh I think it is necessary to make this change.

Creating that helper method doesn't make it much better imo.

Splitting up in 4+ packages: how about going one step in that direction? Determine in which package you'd like to ultimately have TxWeightEstimator and only create that package in this PR. TxWeightEstimator has no dependencies on lnwallet is seems, so this should be an isolated change.

This comment has been minimized.

@Roasbeef

Roasbeef Jan 3, 2019

Author Member

I'd prefer to defer making the new package all together to avoid having to re-create/destroy the packages in the future. IMO in order to do it properly we need to take a step back, analyze dependancies like the one brought up here, and group the interfaces/structs/etc into logical components based on their responsibilities, context, etc.

This comment has been minimized.

@joostjager

joostjager Jan 3, 2019

Collaborator

My worry in general with these kind of things, is that if we don't think about it now, people will start using the weight estimator in this way in other places too, thereby increasing the technical debt. I don't yet see the problem of making a small step now. Yes, there is a risk that code needs to be moved again later, but at least the interface is clean then.

I'd be interested to hear a 2nd reviewers opinion.

This comment has been minimized.

@halseth

halseth Jan 3, 2019

Collaborator

I generally prefer making the change in a new PR that this PR builds on. However, sometimes it is hard to determine exactly how to refactor before the code that uses the refactored parts are ready.

This comment has been minimized.

@halseth

halseth Jan 8, 2019

Collaborator

If the change to create a new package is small, then I think it's best to just do it right away. As @joostjager mentions, deferring it might make the change more significant at a later point.

Not sure what the change would be here though? Create a new package sweep/TxWeightEstimator, and move the TxWeightEstimator from lnwallet there. But this cannot take a sweep.Input (which would be quite nice!) since it imports lnwallet. Might be a bigger change to make this work..

This comment has been minimized.

@joostjager

joostjager Jan 8, 2019

Collaborator

I couldn't resist giving it a try instead of writing another comment.
master...joostjager:move-input

I created the package lnwallet/input (name not accurate) and moved in what needed to move. I think the end result is a logical group, but I have to admit that it involved many more renames than I expected.

I got to see only max 10 errors, so didn't now when it was finally over. But it builds.

Show resolved Hide resolved sweep/walletsweep.go
Show resolved Hide resolved rpcserver.go
Show resolved Hide resolved rpcserver.go Outdated
Show resolved Hide resolved rpcserver.go Outdated
Show resolved Hide resolved sweep/walletsweep.go Outdated

@Roasbeef Roasbeef force-pushed the Roasbeef:sendall-rpc branch from 54df1ef to 14f3fa8 Dec 24, 2018

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Dec 24, 2018

Added unit and integration tests in the latest diff.

@Roasbeef Roasbeef force-pushed the Roasbeef:sendall-rpc branch 2 times, most recently from 71eec85 to 19a68c5 Dec 24, 2018

@Roasbeef Roasbeef force-pushed the Roasbeef:sendall-rpc branch from 19a68c5 to 5473add Jan 3, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 3, 2019

Pushed up fresh version with the latest comments addressed. Major diff is one less commit now (removed commit that added check for non-zero amount for the sweep call, it's now just in the main RPC commit), and modified the new function in the sweep package to no longer use an ephemeral sweeper.

PTAL!

Show resolved Hide resolved sweep/c.out Outdated
Show resolved Hide resolved lnwallet/interface.go
Show resolved Hide resolved sweep/input.go
weightEstimate.AddNestedP2WSHInput(size)
} else {
weightEstimate.AddWitnessInput(size)
}

This comment has been minimized.

@joostjager

joostjager Jan 3, 2019

Collaborator

My worry in general with these kind of things, is that if we don't think about it now, people will start using the weight estimator in this way in other places too, thereby increasing the technical debt. I don't yet see the problem of making a small step now. Yes, there is a risk that code needs to be moved again later, but at least the interface is clean then.

I'd be interested to hear a 2nd reviewers opinion.

Show resolved Hide resolved sweep/walletsweep.go
@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Jan 3, 2019

General question: how does the functionality that is added by this PR differ from doing lncli walletbalance followed by lncli sendcoins ?

@halseth
Copy link
Collaborator

halseth left a comment

Did a quick pass, and looks mostly good.

Again, I think this PR would be quite natural to split up into smaller parts, especially looking at the PR description highlighting the different behavior changes. That being said, this is mostly new functionality, so it should be safe to add.

Show resolved Hide resolved sweep/c.out Outdated
// number of desired blocks between first broadcast, and confirmation.
ConfTarget uint32

// FeeRate if non-zero, signals a fee pre fence expressed in the fee

This comment has been minimized.

@halseth

halseth Jan 3, 2019

Collaborator

typo

This comment has been minimized.

@Roasbeef

Roasbeef Jan 4, 2019

Author Member

Fixed!

This comment has been minimized.

@halseth

halseth Jan 8, 2019

Collaborator

Not yet fixed: s/pre fence/preference

Show resolved Hide resolved lnwallet/reservation.go
Show resolved Hide resolved sweep/txgenerator.go Outdated
weightEstimate.AddNestedP2WSHInput(size)
} else {
weightEstimate.AddWitnessInput(size)
}

This comment has been minimized.

@halseth

halseth Jan 3, 2019

Collaborator

I generally prefer making the change in a new PR that this PR builds on. However, sometimes it is hard to determine exactly how to refactor before the code that uses the refactored parts are ready.

Show resolved Hide resolved sweep/walletsweep.go Outdated
Show resolved Hide resolved sweep/walletsweep.go
Show resolved Hide resolved sweep/walletsweep.go
Show resolved Hide resolved sweep/walletsweep.go Outdated
Show resolved Hide resolved sweep/walletsweep.go
@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Jan 3, 2019

General question: how does the functionality that is added by this PR differ from doing lncli walletbalance followed by lncli sendcoins ?

@joostjager sendcoins doesn't factor in fees, so you must "guess" how much you are able to send and still have enough left to cover fees.

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Jan 3, 2019

@joostjager sendcoins doesn't factor in fees, so you must "guess" how much you are able to send and still have enough left to cover fees.

Okay, so the workaround could be to sendcoins with the balance minus something. Maybe try first with a small something and increase if it doesn't work. To me this sounds reasonable for the special case of wanting to move all funds out of the node. Does it really necessitate this +2000/-700 lines PR?

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 3, 2019

Does it really necessitate this +2000/-700 lines PR?

The PR does prep to ensure things like the sweeper understand nested p2sh which we'll need in the future, and also fixes a race condition between lnd and the internal wallet that could lead to a de-sync of coins. (also keep in mind that the gRPC diff is most of that, as with the new version even if you change a single field the blow up multiplies much more)

Maybe try first with a small something and increase if it doesn't work.

People routinely try this and get very confused as they have no idea how much they should subtract, and at the end of the day, they still have funds left in the wallet as it's a guessing game. This PR takes away all that pain and gives them a simple command. This feature is probably one of the most request lncli related features amongst users.

@Roasbeef Roasbeef force-pushed the Roasbeef:sendall-rpc branch from 5473add to b11a204 Jan 4, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 4, 2019

Latest set of comments addressed, PTAL!

@Roasbeef Roasbeef force-pushed the Roasbeef:sendall-rpc branch from b11a204 to 0254f7c Jan 8, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 8, 2019

Pushed up rebased version after another PR that touched lnrpc landed today.

case lnwallet.CommitmentNoDelay:
return lnwallet.P2WKHWitnessSize, nil
return lnwallet.P2WKHWitnessSize, false, nil

This comment has been minimized.

@halseth

halseth Jan 8, 2019

Collaborator

Yeah, would be nice to avoid to have to do this if possible. Instead pass the Input directly to the weight estimator.

This comment has been minimized.

@Roasbeef

Roasbeef Jan 9, 2019

Author Member

That requires a new package and several updates throughout lnd. I think that can stand alone as its own PR instead of expanding this PR further. Once this is in, I can pick up on @joostjager's partial branch to make the move over.

Show resolved Hide resolved sweep/walletsweep.go Outdated
Show resolved Hide resolved sweep/walletsweep.go
Show resolved Hide resolved sweep/walletsweep.go Outdated
Show resolved Hide resolved sweep/walletsweep_test.go Outdated
Show resolved Hide resolved sweep/walletsweep_test.go Outdated

High Priority automation moved this from In progress to Needs review Jan 8, 2019

@Roasbeef Roasbeef force-pushed the Roasbeef:sendall-rpc branch from 0254f7c to def2429 Jan 9, 2019

Roasbeef added some commits Nov 18, 2018

sweep+cnct+nursery+rpc: extract DetermineFeePerKw to func, add FeePre…
…ference

In this commit, we extract the existing determineFeePerKw method on the
RPC server into a new file in the sweep package. Along the way, we
consolidate code by introducing a new FeePreference struct, which allows
the caller to express their fee preference either in blocks to
confirmation, or a direct fee rate. This move takes a small step to
father decoupling calls in the main RPC server.
lnwallet+sweep: extend the WitnessGenerator type to return an InputSc…
…ript

In this commit, we extend the WitnessGenerator type to now return an
InputScript. This allows it to be more encompassing, as now callers can
expect a sigScript to be populated if the input being swept requires a
sigScript field.

Along the way, we've also renamed input.BuildWitness to
input.CraftInputScript.  We also take a step towards allowing the
sweeper to sweep transactions for n2pwkh outputs. We do so by modifying
the BuiltWitness method to instead return an InputScript. Additionally,
when populating inputs if a sigScript is present, it will now be
populated.
sweep: update getInputWitnessSizeUpperBound to be aware of nested p2sh
In this commit, we update the `getInputWitnessSizeUpperBound` and all
its callers to be aware of nested p2sh witness inputs. We do so by
adding another bool which is true if the output is a nested p2sh output.
If so, then in order to properly estimate the total weight, the caller
needs to factor in the non-witness data of the additional sigScript data
push.
sweep: add new CraftSweepAllTx method to fully withdrawal from a wallet
In this commit, we add a new function, CraftSweepAllTx. This function
allows callers to craft a transaction which sweeps ALL outputs from the
wallet to a single target address. It can either be used for UTXO
consolidation (at the cost of privacy by co-mingling inputs), or simply
to sweep all funds out of a wallet for various reasons.

In an attempt to ensure this method is loosely coupled and testable, for
all behavior structs, we create brand new interface to accept. This
ensures that we only rely on the minimal number of methods needed to
perform our duty.
lnwallet: add new WithCoinSelectLock method to fix coin select race c…
…onditions

In this commit, we add a new method WithCoinSelectLock. This method will
allow us to fix bugs in the project atm that can arise if a channel
funding is attempted (either manually or by autopilot) while a users is
attempting to send an on-chain transaction. If this happens
concurrently, then both contexts will grab the set of UTXOs and attempt
to lock them one by one. However, since they didn't obtain an exclusive
snapshot of the UTXO set of the wallet, they may both attempt to lock
the same input.

We also ensure that calls to SendMany cannot run into this issue by
using the WithCoinSelectLock synchronization when attempting to instruct
the internal wallet to send payments.
rpc: implement new feature of sendcoins to sweep all wallet UTXOs
In this commit, we implement the new feature which allows sendcoins to
sweep all the coins in the wallet. We use the new sweep.CraftSweepAllTx
method, and also use WithCoinSelectLock to ensure that we don't trigger
any double-spend errors triggered by coin selection race conditions.
cmd/lncli: add new --sweepall flag to sendcoins
In this commit, we add a new flag to the sendcoins command that allows
callers to sweep all funds out of the daemon's wallet. This CANNOT be
set at the same time that an amount is specified.
sweep: add unit tests for TestDetermineFeePerKw and CraftSweepAllTx
Along the way we also extend the mockFeeEstimator to be able to return a
fee rate for a specified confirmation target.

@Roasbeef Roasbeef force-pushed the Roasbeef:sendall-rpc branch from def2429 to e140cbb Jan 9, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 15, 2019

Any final comments? Have this queued up to land.

return fmt.Errorf("amount cannot be set if attempting to " +
"sweep all coins out of the wallet")
}

This comment has been minimized.

@cfromknecht

cfromknecht Jan 15, 2019

Collaborator

should we add a user confirmation for sweep all? i would hate for someone to accidentally do this and send all their coins to an address they don't control. that could partially be addressed by using a different rpc method altogether, though i think the confirmation is a happy medium

High Priority automation moved this from Needs review to Final Testing -- Ready For Merge Jan 15, 2019

@halseth
Copy link
Collaborator

halseth left a comment

LGTM 💯

A user confirmation would be nice to have, but can be made a "good first issue" for someone to pick up.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 15, 2019

Agree the user confirmation would be a nice addition, and even generally to the entire sendcoins command as well.

@Roasbeef Roasbeef merged commit 509bed6 into lightningnetwork:master Jan 15, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.04%) to 56.168%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

High Priority automation moved this from Final Testing -- Ready For Merge to Done Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment