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

luke-jr commented at 10:07 PM on November 13, 2016: member

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

fanquake added the label RPC/REST/ZMQ on Nov 13, 2016

fanquake added the label Wallet on Nov 13, 2016

jonasschnelli commented at 7:41 AM on November 14, 2016: contributor

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 commented at 5:29 PM on November 15, 2016: contributor

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 commented at 11:20 AM on November 17, 2016: contributor

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

luke-jr commented at 11:32 AM on November 17, 2016: member

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

ryanofsky referenced this in commit 6494980802 on Dec 9, 2016

ryanofsky referenced this in commit 59a77a6ac5 on Dec 15, 2016

ryanofsky referenced this in commit ef97df2109 on Dec 19, 2016

luke-jr referenced this in commit 155e1cf9d6 on Dec 21, 2016

luke-jr referenced this in commit f73bc4fd62 on Dec 21, 2016

luke-jr referenced this in commit 29caf46ed8 on Dec 21, 2016

in src/wallet/rpcwallet.cpp:None in ed60474728 outdated

1070 | +        std::map<uint256, CCoins> mapcoins;
1071 | +        {
1072 | +            LOCK(cs_main);
1073 | +            mempool.FindScriptPubKey(setscriptSearch, mapcoins);
1074 | +            FlushStateToDisk();
1075 | +            pcoinsTip->FindScriptPubKey(setscriptSearch, mapcoins);

ryanofsky commented at 8:31 PM on December 28, 2016:

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.

ryanofsky commented at 8:34 PM on December 28, 2016: member

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.

luke-jr referenced this in commit 2d594e7154 on Dec 31, 2016

luke-jr referenced this in commit f3f5c79869 on Dec 31, 2016

ryanofsky referenced this in commit 96a1834f53 on Jan 2, 2017

ryanofsky referenced this in commit 2ca75bccf6 on Jan 2, 2017

luke-jr force-pushed on Jan 3, 2017

luke-jr force-pushed on Feb 4, 2017

luke-jr force-pushed on Feb 7, 2017

laanwj commented at 9:31 AM on February 27, 2017: member

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 added this to the milestone 0.15.0 on Feb 27, 2017

ryanofsky referenced this in commit d58b56d98d on Jun 2, 2017

ryanofsky referenced this in commit 095c2e7e78 on Jun 12, 2017

sipa commented at 10:50 PM on July 12, 2017: member

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

sipa removed this from the milestone 0.15.0 on Jul 12, 2017

jonasschnelli commented at 8:14 PM on August 15, 2017: contributor

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 force-pushed on Aug 19, 2017

luke-jr commented at 3:55 AM on August 19, 2017: member

Rebased and added a simple functional test.

in src/wallet/rpcwallet.cpp:1023 in c0bd1d3ff5 outdated

1105 | +
1106 | +                tempKeystore.AddKey(key);
1107 | +                CKeyID address = pubkey.GetID();
1108 | +                CScript script = GetScriptForDestination(address);
1109 | +                if (!script.empty()) {
1110 | +                    needles.insert(script);

promag commented at 3:17 PM on August 19, 2017:

Throw if duplicate? Add test.

luke-jr commented at 2:55 AM on August 21, 2017:

I don't see why a duplicate privkey should be an error.

in src/wallet/rpcwallet.cpp:1027 in c0bd1d3ff5 outdated

1109 | +                if (!script.empty()) {
1110 | +                    needles.insert(script);
1111 | +                }
1112 | +                script = GetScriptForRawPubKey(pubkey);
1113 | +                if (!script.empty()) {
1114 | +                    needles.insert(script);

promag commented at 3:17 PM on August 19, 2017:

Throw if duplicate? Add test.

in src/wallet/rpcwallet.cpp:1037 in c0bd1d3ff5 outdated

1119 | +        } else {
1120 | +            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Unrecognised option '%s'", optname));
1121 | +        }
1122 | +    }
1123 | +
1124 | +    // Ensure keypool is filled if possible

promag commented at 3:18 PM on August 19, 2017:

Throw if needles is empty? Add test.

luke-jr commented at 2:56 AM on August 21, 2017:

If it's empty, we'll throw "No value to sweep" later on. And so long as the user provides something, it will never be empty.

in src/wallet/rpcwallet.cpp:1050 in c0bd1d3ff5 outdated

1132 | +
1133 | +    // Reserve the key we will be using
1134 | +    CReserveKey reservekey(pwallet);
1135 | +    CPubKey pubkey;
1136 | +    if (!reservekey.GetReservedKey(pubkey)) {
1137 | +        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");

promag commented at 3:20 PM on August 19, 2017:

Add test for this error?

luke-jr commented at 3:07 AM on August 21, 2017:

I don't know how, and the two tests currently "testing" it don't make logical sense.

in src/wallet/rpcwallet.cpp:1055 in c0bd1d3ff5 outdated

1137 | +        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");
1138 | +    }
1139 | +
1140 | +    // Scan UTXO set for inputs
1141 | +    std::vector<CTxOut> input_txos;
1142 | +    {

promag commented at 3:21 PM on August 19, 2017:

Unnecessary block?

luke-jr commented at 3:08 AM on August 21, 2017:

Scope for coins

in src/wallet/rpcwallet.cpp:1098 in c0bd1d3ff5 outdated

1171 | +
1172 | +    while (true) {
1173 | +        if (IsDust(tx.vout[0], ::minRelayTxFee)) {
1174 | +            throw JSONRPCError(RPC_VERIFY_REJECTED, "Swept value would be dust");
1175 | +        }
1176 | +        for (size_t input_index = 0; input_index < tx.vin.size(); ++input_index) {

promag commented at 3:27 PM on August 19, 2017:

Take this out of fee loop?

luke-jr commented at 3:12 AM on August 21, 2017:

Fee changes require resigning. Perhaps it could be made to sign only before and after the loop, but the time here is trivial in comparison to the UTXO search.

promag commented at 3:31 PM on August 19, 2017: member

Is it me or this could use CWallet::FundTransaction with coin control configured and with subtract fee from amount?

luke-jr force-pushed on Aug 21, 2017

luke-jr commented at 3:18 AM on August 21, 2017: member

Fixed the test failure (missing cs_main lock on AcceptToMemoryPool)

luke-jr force-pushed on Aug 21, 2017

luke-jr force-pushed on Aug 26, 2017

jonasschnelli commented at 6:24 AM on November 30, 2017: contributor

@luke-jr: can you rebase this once again? Thanks

luke-jr force-pushed on Nov 30, 2017

luke-jr commented at 6:59 AM on November 30, 2017: member

Rebased

jonasschnelli commented at 7:18 AM on December 1, 2017: contributor

utACK a397deb247fc404064cf76ede8d64eb68f4fefe6 (will test soon). I personally dislike the "direct"-approach. I would like to "see" the transaction before submitting.

Possible extensions:

Remove the signing and broadcasting, accept a pubkey and rename it to createrawsweeptransaction (or similar)
Add support for HD sweep (xpub/xpriv)

jaromil commented at 6:09 PM on December 6, 2017: contributor

ACK using a397deb247fc40. This is a great feature many people need. I'm moving on to test it on mainnet using the v0.15.0.knots20170914 build. I encourage inclusion and later on apply extensions (esp what @jonasschnelli recommends above).

in test/functional/sweepprivkeys.py:8 in a397deb247 outdated

   0 | @@ -0,0 +1,66 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2014-2017 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +"""Test the sweepprivkeys RPC."""
   6 | +
   7 | +import decimal
   8 | +import math

MarcoFalke commented at 12:26 AM on December 11, 2017:

seems like unused imports?

jaromil commented at 4:39 PM on December 11, 2017: contributor

The check fail is unrelated to this PR and limited to the ARM build, lacking the "flake8" python library.

contrib/devtools/lint-python.sh: 10: contrib/devtools/lint-python.sh: flake8: not found

removing unused imports won't fix lint-python.sh either.

jaromil commented at 3:29 PM on January 8, 2018: contributor

ping and HNY everyone :^P

MarcoFalke commented at 12:08 AM on January 15, 2018: member

Since the keys are not added to the wallet, and thus bumpfee won't work, it makes sense to set nSequence to support rbf and return a list of signed raw transactions that each pay a slightly higher fee?

jaromil commented at 10:57 AM on January 15, 2018: contributor

@MarcoFalke I believe yours is a brilliant idea. Just to be clear, you mean a list of signed raw transactions that are ready to replace the one made and can be fired up from the lowest fee to the higher to bump if necessary, right? such transactions should be discarded once enough confirmations are present.

luke-jr force-pushed on Mar 3, 2018

MarcoFalke added the label Needs rebase on Jun 6, 2018

laanwj referenced this in commit 8fceae0d6f on Jul 17, 2018

laanwj commented at 8:53 AM on August 31, 2018: member

If we still want this, it needs rebase/implementation in terms of #12196. But I'm going to close it for now, it's been open since 2016 and there's no clear progress.

It might make sense to open a new PR if you start work on this again, or you can ask me to reopen it.

laanwj closed this on Aug 31, 2018

jaromil commented at 2:21 PM on August 31, 2018: contributor

Interesting revamp. I have read the whole thread in #12196, is there some documentation being worked about use examples for scantxoutset to be used in place of sweeping? I like to try to sort out my needs using that, but haven't kept myself up to date enough to understand it without handholding. There is the integration test in python, but...

luke-jr commented at 6:39 PM on October 29, 2018: member

Rebased, please reopen @laanwj

MarcoFalke commented at 7:11 PM on October 29, 2018: member

@luke-jr You can only (force) push after it has been reopened. If the branch is different in any way, it won't reopen.

jnewbery reopened this on Oct 29, 2018

luke-jr force-pushed on Oct 29, 2018

luke-jr commented at 8:56 PM on October 29, 2018: member

NOTE: Currently needs #14602 for tests to pass

luke-jr force-pushed on Oct 29, 2018

DrahtBot removed the label Needs rebase on Oct 29, 2018

DrahtBot commented at 11:04 PM on October 29, 2018: member

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.

in src/wallet/rpcwallet.cpp:1013 in e341211bf7 outdated

 942 | +        const UniValue& optval = request.params[0][optname];
 943 | +        if (optname == "privkeys") {
 944 | +            const UniValue& privkeys_a = optval.get_array();
 945 | +            for (size_t privkey_i = 0; privkey_i < privkeys_a.size(); ++privkey_i) {
 946 | +                const UniValue& privkey_wif = privkeys_a[privkey_i];
 947 | +                std::string wif_secret = privkey_wif.get_str();

practicalswift commented at 9:44 AM on October 30, 2018:

Could be const reference?

DrahtBot added the label Needs rebase on Nov 5, 2018

meshcollider commented at 7:34 PM on November 15, 2018: contributor

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 force-pushed on Feb 12, 2019

DrahtBot removed the label Needs rebase on Feb 12, 2019

DrahtBot added the label Needs rebase on Feb 14, 2019

luke-jr force-pushed on Mar 29, 2019

Document limitations of CCoinsView::Cursor implementations 8b8ba3f50d

CTxMemPool::FindScriptPubKey to search unconfirmed UTXOs 0d14432974

Wallet/RPC: sweepprivkeys method to scan UTXO set and send to local wallet 7c4e5e628c

GUI/RPCConsole: Include sweepprivkeys in history sensitive-command filter 799abbbb48

QA: Functional test for sweepprivkeys f8ff311404

RPC/Wallet: Use BroadcastTransaction for sweepprivkeys to ensure wallet is synced before we return e30d4b5562

luke-jr force-pushed on May 2, 2019

DrahtBot removed the label Needs rebase on May 2, 2019

practicalswift commented at 3:55 PM on May 7, 2019: contributor

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

DrahtBot added the label Needs rebase on Jun 6, 2019

DrahtBot commented at 3:24 PM on June 6, 2019: member

Needs rebase

practicalswift commented at 10:29 PM on June 6, 2019: contributor

Concept ACK

jonasschnelli commented at 12:27 PM on June 7, 2019: contributor

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.

meshcollider added the label Waiting for author on Jun 7, 2019

in src/wallet/rpcwallet.cpp:977 in e30d4b5562

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

Sjors commented at 12:04 PM on August 15, 2019:

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)?

Sjors commented at 12:16 PM on August 15, 2019: member

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.

luke-jr commented at 8:31 PM on October 14, 2019: member

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 closed this on Oct 14, 2019

ryanofsky commented at 4:47 PM on October 15, 2019: member

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 commented at 4:04 PM on October 24, 2019: member

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

laanwj removed the label Needs rebase on Oct 24, 2019

laanwj removed the label Waiting for author on Oct 24, 2019

laanwj added the label Up for grabs on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021