Declared wallet rpc function "sweepkey" in src/wallet/rpcwallet.cpp
Added "sweepkey" rpc function, can be used to sweep a wallet's private key's outputs to a new key.
In rpc wallet function "importprivkey", added 2 optional parameters bool sweep & destination bitcoin address for sweep transaction. (Optionally sweep funds on private key import)
Added SweepPrivKey function to src/wallet/rpcdump.cpp to help "sweepkey" and "importprivkey"
Declared wallet rpc function "sweepkey" in src/wallet/rpcwallet.cpp
Added "sweepkey" rpc function, can be used to sweep a wallet's private key's
outputs to a new key.
In rpc wallet function "importprivkey", added 2 optional parameters
bool sweep & destination bitcoin address for sweep transaction.
(Optionally sweep funds on private key import)
Added SweepPrivKey function to src/wallet/rpcdump.cpp to help "sweepkey"
and "importprivkey"
88 | + if (!dirtyKey.VerifyPubKey(pubkey)) 89 | + return false; 90 | + 91 | + LOCK2(cs_main, pwalletMain->cs_wallet); 92 | + 93 | + // Get outputs from wallet
But the wallet won't have tracked UTXOs for this private key...
This is intended for a private key that is already in the wallet, with outputs.
Why would you sweep it, if it's in the wallet (and thereby trusted to be exclusively yours)?
94 | + std::vector<COutput> vCoins; 95 | + pwalletMain->AvailableCoins(vCoins, false); 96 | + 97 | + // Find all outputs of the key 98 | + std::vector<COutput> vToSweep; 99 | + BOOST_FOREACH(COutput output, vCoins)
It's the full UTXO set you need to look through, not merely the wallet's coins.
Nit: Don't use BOOST_*
This is for after importing a private key.
I see BOOST_FOREACH particularly used in many places throughout the bitcoin source code, I will remove it but could you educate me as to me why I shouldn't use it?
We are moving towards using the cpp11 for loop, no need to go back in new code and use a framework.
126 | + if (!mtx.vin.size() || !(total > 0)) 127 | + return false; 128 | + 129 | + // Add sweep destination - fee 130 | + mtx.vout.push_back(CTxOut(total, GetScriptForDestination(destAddress.Get()))); 131 | + mtx.vout[0].nValue -= 3*CWallet::GetMinimumFee(GetVirtualTransactionSize(mtx), DEFAULT_TX_CONFIRM_TARGET, mempool);
3x the fee why? Probably should make this configurable...
Agree. Also, the new rpc should take a dictionary as input and not positional args to make this easier.
In testing I ended up with a sweep transaction where the fee estimation was not enough, and the tx was rejected from the memory pool. This is a bad solution though, I'm more than open to suggestions.
E.g. fundrawtransaction takes options to set the fee rate.
128 | + 129 | + // Add sweep destination - fee 130 | + mtx.vout.push_back(CTxOut(total, GetScriptForDestination(destAddress.Get()))); 131 | + mtx.vout[0].nValue -= 3*CWallet::GetMinimumFee(GetVirtualTransactionSize(mtx), DEFAULT_TX_CONFIRM_TARGET, mempool); 132 | + 133 | + mtx.nLockTime = chainActive.Height();
This should probably be shared with the wallet code doing the same...
Are you talking about setting nLockTime? I didn't see that... Will look
76 | @@ -74,19 +77,166 @@ std::string DecodeDumpString(const std::string &str) { 77 | return ret.str(); 78 | } 79 | 80 | +bool SweepPrivKey(CBitcoinSecret key, CBitcoinAddress destAddress) {
This should probably get moved into a non-RPC-specific location, and RPC-specific errors moved out.
Agreed
168 | + LogPrintf("SweepPrivKey(): Error: Transaction not valid\n"); 169 | + return false; 170 | + } 171 | + 172 | + if (pwalletMain->GetBroadcastTransactions() && !g_connman) 173 | + throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
Too late to error here - it's already been fed into AcceptToMemoryPool.
If I perform this check and throw the error before AcceptToMemoryPool, would that be okay?
n/a if create and broadcast are split up.
187 | + if (!EnsureWalletIsAvailable(fHelp)) 188 | + return NullUniValue; 189 | + 190 | + if (fHelp || params.size() != 2) 191 | + throw runtime_error( 192 | + "sweepkey \"bitcoinprivkey\" \"bitcoinaddress\"\n"
Suggest "sweepprivkey" to match "dumpprivkey" and "importprivkey"
191 | + throw runtime_error( 192 | + "sweepkey \"bitcoinprivkey\" \"bitcoinaddress\"\n" 193 | + "\nSweep the privkey's outputs to the specified address.\n" 194 | + "\nArguments:\n" 195 | + "1. \"bitcoinprivkey\" (string, required) The private key (see dumpprivkey)\n" 196 | + "2. \"bitcoinaddress\" (bitcoinaddress, required) The destination address\n"
bitcoinaddress isn't a JSON type. Just string.
Furthermore, so long as you're insisting on an address here, it makes no sense to include this as a wallet RPC...
201 | + + HelpExampleRpc("sweepkey", "\"mykey\" \"myaddress\"") 202 | + ); 203 | + 204 | + LOCK2(cs_main, pwalletMain->cs_wallet); 205 | + 206 | + EnsureWalletIsUnlocked();
Why? Shouldn't this work without unlocking the wallet?
It'll need to be unlocked at some point won't it? (in order to move the bitcoins from the swept address), although ideally there ought to be a way to do this without needing to unlock manually - i.e. store the private key for the swept address but require the private key of the wallet it is being swept to to access them.
No. The swept key (not address; addresses don't control bitcoins) is not part of the wallet, and the wallet doesn't need to be unlocked to receive. Furthermore, so long as the user is proving the sweep-to address, the wallet isn't even being used.
I was making this with the intention that the private key is actually in the wallet, or being imported, in which case it will be in the wallet before the sweep.
235 | "\nAdds a private key (as returned by dumpprivkey) to your wallet.\n"
236 | "\nArguments:\n"
237 | "1. \"bitcoinprivkey\" (string, required) The private key (see dumpprivkey)\n"
238 | "2. \"label\" (string, optional, default=\"\") An optional label\n"
239 | "3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n"
240 | + "4. sweepkey (boolean, optional, default=false) Sweep imported key for improved security\n"
Importing an untrusted key is never secure, so it doesn't make sense to import+sweep...
The first "use case" in #2751 I assume is poorly phrased. You never want to import such a key. What makes sense, and what @jgarzik probably meant, is that instead of importing the key, you sweep it, and keep a copy of the private key in an as-of-yet-non-existent part of the wallet to be continually swept in case someone sends funds to it in the future.
The second use case is not likely to occur often in practice, but would benefit from the old keys being moved into a separated sweep-only keystore as well.
I think a sweep command is desirable.
Just thinking: how would it be if we would have something like createrawsweeptransaction <pubkey> <outputaddress> as a first step.
The people can use signrawtransaction together with the privatekey.
We could still support a sweepkey RPC call that would use createrawsweeptransaction || signrawtransaction || sendrawtransaction.
Splitting it up into a createrawsweeptransaction would allow to have more control over the sweep (confirmation page, etc.).
Thanks everyone, I will take all of your input and come back with something improved when I have the time. Does everyone agree with @jonasschnelli 's suggestion to separate the creation of the sweep transaction from the signing / broadcast? It seems like a good idea to me.
As an aside, any new functionality which requires a rescan really must take a block range (allowing for a range that is open on the upper side.. e.g. a starting height) as an (optional) argument.
[hmm. so this could be done without a rescan by only evaluating the utxo set...]