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

Wallet/RPC: sweepprivkeys method to scan UTXO set and send to local wallet #9152

Closed
wants to merge 6 commits into from

Conversation

@luke-jr
Copy link
Member

luke-jr commented Nov 13, 2016

Does this look like a good approach?

TODO:

  • rawtransaction sweep functionality
  • GUI sweep (Receive tab?)
  • abstract shared sweep logic
  • RPC tests

NOTE: Currently needs #14602 for tests to pass

@jonasschnelli
Copy link
Member

jonasschnelli commented Nov 14, 2016

Concept ACK (Haven't really looked at the code).
I think a sweep function would be a great feature. One could import "old" private keys into a new HD wallet for example.

Possible extension: sweepseed could be an extended version of that, moving all funds form a HD seed to a new one, generating large lookup-windows on different chainpathes. It could also be UTXO set only not requiring a -rescan.

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 15, 2016

From a raw flow perspective, the generation of the sweep transaction is something that works from public information and should be possible on an online node without access to the private keys... so that one should be a 'createrawsweeptransaction' which takes a list of adresses/pubkeys/redeemscripts (and maybe private keys ... maybe some kind of BIP32 chain spec) and returns a transaction that spends all coins assigned to matching keys, potentially with arguments to limit the set of inputs collected.

@paveljanik
Copy link
Contributor

paveljanik commented Nov 17, 2016

Hmm, what about extending RPC importprivkey with another optional argument sweep defaulting to false?

@luke-jr
Copy link
Member Author

luke-jr commented Nov 17, 2016

sweepprivkeys is intended for users, and not to import keys. Users should never use importprivkey.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 9, 2016
Change CCoinsViewCache::Cursor() method to return a cursor yielding the latest
CCoins entries, instead of just previous entries prior to the last cache flush.

The CCoinsViewCache::Cursor method is not currently used. This change just
enables new features that rely on scanning the UXTO set to work correctly (for
example bitcoin#9152, which adds a
sweepprivkeys RPC, and bitcoin#9137, which
improves handling of imported keys for nodes with pruning enabled.)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 15, 2016
Change CCoinsViewCache::Cursor() method to return a cursor yielding the latest
CCoins entries, instead of just previous entries prior to the last cache flush.

The CCoinsViewCache::Cursor method is not currently used. This change just
enables new features that rely on scanning the UXTO set to work correctly (for
example bitcoin#9152, which adds a
sweepprivkeys RPC, and bitcoin#9137, which
improves handling of imported keys for nodes with pruning enabled.)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 19, 2016
Make CCoinsViewCache::Cursor() return latest data

Change CCoinsViewCache::Cursor() method to return a cursor yielding the latest
CCoins entries, instead of just previous entries prior to the last cache flush.

The CCoinsViewCache::Cursor method is not currently used. This change just
enables new features that rely on scanning the UXTO set to work correctly (for
example bitcoin#9152, which adds a
sweepprivkeys RPC, and bitcoin#9137, which
improves handling of imported keys for nodes with pruning enabled.)
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
Github-Pull: bitcoin#9152
Rebased-From: 220750dc4aa298d806effd4c6d3685721dd53a88
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
…function

Github-Pull: bitcoin#9152
Rebased-From: 4437a5f875da68032f9539e35679219f316cc851
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
…allet

Github-Pull: bitcoin#9152
Rebased-From: fdc677846eb381427c2b6382c5e1b88568311f9f
Copy link
Contributor

ryanofsky left a comment

Re: "Refactor sweepprivkeys to deal with CCoinsView::Cursor limitations," I think I could extend #9306 to return a working cursor for CCoinsViewMemPool, if that would help.

src/wallet/rpcwallet.cpp Outdated
LOCK(cs_main);
mempool.FindScriptPubKey(setscriptSearch, mapcoins);
FlushStateToDisk();
pcoinsTip->FindScriptPubKey(setscriptSearch, mapcoins);

This comment has been minimized.

@ryanofsky

ryanofsky Dec 28, 2016 Contributor

It might be appropriate to loop over mapCoins calling CTxMemPool::pruneSpent, on each entry, to avoid trying to sweep from an output that is already in the process of being spent.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 31, 2016
Github-Pull: bitcoin#9152
Rebased-From: dd089adf87ea6078d1883ce1b4771760bfda1ddb
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 31, 2016
Github-Pull: bitcoin#9152
Rebased-From: ed604747289c12d605f219b3f859a9bef376174e
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 2, 2017
Make CCoinsViewCache::Cursor() return latest data

Change CCoinsViewCache::Cursor() method to return a cursor yielding the latest
CCoins entries, instead of just previous entries prior to the last cache flush.

The CCoinsViewCache::Cursor method is not currently used. This change just
enables new features that rely on scanning the UXTO set to work correctly (for
example bitcoin#9152, which adds a
sweepprivkeys RPC, and bitcoin#9137, which
improves handling of imported keys for nodes with pruning enabled.)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 2, 2017
Make CCoinsViewCache::Cursor() return latest data

Change CCoinsViewCache::Cursor() method to return a cursor yielding the latest
CCoins entries, instead of just previous entries prior to the last cache flush.

The CCoinsViewCache::Cursor method is not currently used. This change just
enables new features that rely on scanning the UXTO set to work correctly (for
example bitcoin#9152, which adds a
sweepprivkeys RPC, and bitcoin#9137, which
improves handling of imported keys for nodes with pruning enabled.)
@luke-jr luke-jr force-pushed the luke-jr:sweepprivkeys branch Jan 3, 2017
@luke-jr luke-jr force-pushed the luke-jr:sweepprivkeys branch 2 times, most recently to f6c9c12 Feb 4, 2017
@laanwj
Copy link
Member

laanwj commented Feb 27, 2017

Concept ACK.

Hmm, what about extending RPC importprivkey with another optional argument sweep defaulting to false?

Please don't do this. People confuse import and sweep all over the place. The least we can do is make them separate RPCs with separate documentation.

@laanwj laanwj added this to the 0.15.0 milestone Feb 27, 2017
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 2, 2017
Change CCoinsViewCache::Cursor() method to return a cursor yielding the latest
CCoins entries, instead of just previous entries prior to the last cache flush.

The CCoinsViewCache::Cursor method is not currently used. This change just
enables new features that rely on scanning the UXTO set to work correctly (for
example bitcoin#9152, which adds a
sweepprivkeys RPC, and bitcoin#9137, which
improves handling of imported keys for nodes with pruning enabled.)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 12, 2017
Change CCoinsViewCache::Cursor() method to return a cursor yielding the latest
CCoins entries, instead of just previous entries prior to the last cache flush.

The CCoinsViewCache::Cursor method is not currently used. This change just
enables new features that rely on scanning the UXTO set to work correctly (for
example bitcoin#9152, which adds a
sweepprivkeys RPC, and bitcoin#9137, which
improves handling of imported keys for nodes with pruning enabled.)
@sipa
Copy link
Member

sipa commented Jul 12, 2017

Seems this won't be making it for 0.15. Untagging.

@sipa sipa removed this from the 0.15.0 milestone Jul 12, 2017
@jonasschnelli
Copy link
Member

jonasschnelli commented Aug 15, 2017

Concept Re-ACK. Needs rebase. I guess this is also something we could want for 0.16.
I have no strong opinion about sweep versus createrawsweep. The approach we took for bumpfee was also to do the non raw one first, although I agree that the ramification of sweep is different then a bump.

@luke-jr luke-jr force-pushed the luke-jr:sweepprivkeys branch from f6c9c12 Aug 19, 2017
@luke-jr
Copy link
Member Author

luke-jr commented Aug 19, 2017

Rebased and added a simple functional test.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 29, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16129 (refactor: Remove unused includes by practicalswift)
  • #13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

const UniValue& privkeys_a = optval.get_array();
for (size_t privkey_i = 0; privkey_i < privkeys_a.size(); ++privkey_i) {
const UniValue& privkey_wif = privkeys_a[privkey_i];
std::string wif_secret = privkey_wif.get_str();

This comment has been minimized.

@practicalswift

practicalswift Oct 30, 2018 Member

Could be const reference?

@meshcollider
Copy link
Member

meshcollider commented Nov 15, 2018

Concept ACK. Still needs rebase. IMO this should be reworked to take a descriptor which would address the points above about accepting pubkeys/HD keys. Agree that returning the raw transaction is preferable (createrawsweeptransaction)

@luke-jr luke-jr force-pushed the luke-jr:sweepprivkeys branch from e341211 to fe16fc5 Feb 12, 2019
@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Feb 12, 2019
@luke-jr luke-jr force-pushed the luke-jr:sweepprivkeys branch from fe16fc5 to 0c90d54 Mar 29, 2019
@luke-jr luke-jr force-pushed the luke-jr:sweepprivkeys branch from 0c90d54 to e30d4b5 May 2, 2019
@DrahtBot DrahtBot removed the Needs rebase label May 2, 2019
@practicalswift
Copy link
Member

practicalswift commented May 7, 2019

I'm afraid this PR doesn't build when rebased on master.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 6, 2019

Needs rebase
@practicalswift
Copy link
Member

practicalswift commented Jun 6, 2019

Concept ACK

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 7, 2019

This should probably use "scantxoutset" internally (rebase issue).
I also think it should be a createrawsweep call (rather then directly signing and executing).
Maybe you can change this to be an argument for "scantxoutset" that would also spit out a rawtransaction (with configurable fee and eventually configurable target address) for the sweep transaction.

Copy link
Member

Sjors left a comment

Concept ACK. Using scantxoutset internally seems like a good idea. It needs a fee control option, could be as simple as feeRate with 1 sat/byte default. Ideally also RBF control, but that can wait for a followup.

As mentioned by others, it's good to be able to inspect the transaction. That could just be another option. In #16378 I introduce a add_to_wallet to option, and the RPC call returns a hex & PSBT if false.

Another potential followup could add descriptor support; those can have private keys now too. Others above suggest using those instead of WIF, but I think an array of WIFs is still common enough to be worth supporting directly.

This PR sweeps into the wallet itself. A followup could sweep into any arbitrary descriptor, which would make migrating wallets to some new scheme easier, e.g. moving from single-sig to multi-sig without joining UTXOs.

if (request.fHelp || request.params.size() != 1) {
throw std::runtime_error(
RPCHelpMan{"sweepprivkeys",
"\nSends bitcoins controlled by private key to specified destinations.\n",

This comment has been minimized.

@Sjors

Sjors Aug 15, 2019 Member

This says "specified destinations" but you can't specify the destinations. Can you also make it clear that coins are sweeped one by one to unique addresses (per priv key)?

@luke-jr
Copy link
Member Author

luke-jr commented Oct 14, 2019

I don't see any sane way to rebase this post-bitcoin-wallet, so going to have to just make it a Knots-only hack for now. :/

@luke-jr luke-jr closed this Oct 14, 2019
@ryanofsky
Copy link
Contributor

ryanofsky commented Oct 15, 2019

I don't see any sane way to rebase this post-bitcoin-wallet

I don't think it should be too bad if someone wants to pick it up. It should just require a new interfaces::Chain method to wrap FindScriptPubKey calls, since wallet code can no longer access mempool and pcoinsdbview globals directly.

@ryanofsky
Copy link
Contributor

ryanofsky commented Oct 24, 2019

Maintainers maybe remove "Waiting for author" and "Needs rebase" tags and mark "Up for grabs"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.