Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add `-disablehot` mode: a sane mode for watchonly-wallets #9662
Conversation
jonasschnelli
added the
Wallet
label
Feb 1, 2017
|
It seems somewhat strange that this is set per-run and not per-wallet. Indeed, if it were set per-wallet, an easy way to get the intended behavior here is to simply encrypt your wallet with a garbage passphrase such that it can no longer be opened (not to say we shouldn't do this, better UX around that is good, but it might be easier to review/write if it re-used that infrastructure). |
|
Concept ACK. I agree that this would be better as a mode on the wallet instead of yet another startup option. |
jonasschnelli
referenced
this pull request
Feb 9, 2017
Open
Can create Watch Only HD wallet with -hdwatchonly #9728
|
I would like to rebase on top of that for #9728, can you rebase? |
|
Added two commit.
Disabling private keys on a wallet that already contains private keys is not possible (also the other way around). |
| + print("restarting node with disablehot=0 (try to re-enable private keys which must not be possible)") | ||
| + stop_nodes(self.nodes) | ||
| + try: | ||
| + self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, [[], ['-disablehot=0']]) |
jonasschnelli
Feb 17, 2017
Member
@MarcoFalke: any idea how I can make this pass? Right now, staring bitcoind in this case with this setup will cause bitcoind to write to stderr (and halt) which then result in pass=false in rpc-tests.py. But it's actually the correct behaviour.
MarcoFalke
Feb 18, 2017
Member
We might create a assert_start_raises_init_error() method. I will take a look at this tomorrow.
| @@ -1408,13 +1407,25 @@ bool CWallet::IsHDEnabled() | ||
| return !hdChain.masterKeyID.IsNull(); | ||
| } | ||
| +void CWallet::SetWalletFlag(uint64_t flag) | ||
| +{ | ||
| + walletFlags |= flag; |
NicolasDorier
Feb 20, 2017
•
Member
Kind of confusing....
I thought this would be equivalent to CWallet::SetWalletFlags(uint64_t flags, false).
However while CWallet::SetWalletFlags(uint64_t flags, bool memonly) set the flags to the indicated value, CWallet::SetWalletFlag(uint64_t flag) appends to it.
Maybe you should rename to AddWalletFlags.
saleemrashid
Feb 20, 2017
CWallet::AddWalletFlags(uint64_t flags, book memonly) seems most consistent.
jonasschnelli
Feb 21, 2017
Member
IMO set flags seems most appropriate here. Flags are not getting appended. Either you set or unset IMO. For consistency, we should have a UnsetWalletFlag().
But I agree its kind of confusing that we have a SetWalletFlags (plural) method that is capable or re-setting all 64 flags.
saleemrashid
Feb 21, 2017
I think this method should be plural, since there is no distinction between a single flag and multiple flags in its operation and it might be useful to be able to do the latter.
Then, of course, you have confusion. So, a verb change in the method name might be useful.
NicolasDorier
Feb 22, 2017
•
Member
@jonasschnelli the strange thing is that SetWalletFlag switch a bit, when SetWalletFlags set the walletFlags to a specific value. I was expecting SetWalletFlags to switch several bits.
ryanofsky
May 4, 2017
Contributor
Maybe renaming SetWalletFlags (plural) to OverwriteWalletFlags?
Since this is doing an | I'd call it AddFlags (which conceivably could be paired with a RemoveFlags doing & ~).
| + self.num_nodes = 2 | ||
| + | ||
| + def setup_network(self, split=False): | ||
| + self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, [[], ['-disablehot']]) |
NicolasDorier
Feb 20, 2017
Member
does the [] arguments is on purpose? because I think it makes -disablehot ignored
jonasschnelli
Feb 21, 2017
Member
We start two nodes here, the first node without startup parameters (thats why there is the []). Only second node has -disablehot.
NicolasDorier
referenced
this pull request
Feb 23, 2017
Merged
[qa] assert_start_raises_init_error #9832
Nice, so this guarantees that bitcoind process will contain no private key data at all? Travis should pass now that #9832 is merged. |
|
The
Yes. Event the default key is disabled (while still detecting the first start correctly). |
| + else if (IsArgSet("-disablehot")) { | ||
| + bool disableHot = GetBoolArg("-disablehot", DEFAULT_DISABLE_HOT_WALLET); | ||
| + LOCK(walletInstance->cs_wallet); | ||
| + if (walletInstance->vchDefaultKey.IsValid() && disableHot) { |
NicolasDorier
Mar 13, 2017
Member
I think you should use fFirstRun here instead of walletInstance->vchDefaultKey.IsValid()
jonasschnelli
Mar 13, 2017
Member
At this point fFirstRun is always false. And I think this also works in conjunction with salvagewallet.
NicolasDorier
Mar 13, 2017
Member
Indeed. Might be out of this PR matter, in the case of hdwatchonly, walletInstance->vchDefaultKey.IsValid() will be true. So this would mean disabledHot is incompatible with hdwatchonly mode ?
NicolasDorier
Mar 13, 2017
Member
OK forget what I said, there is no reason to have disabledHot with hdwatchonly at same time.
|
Rebased. |
|
Needs rebase again (sorry) |
|
Rebased. |
ryanofsky
reviewed
May 4, 2017
utACK 9b39bf9
The commit history of this PR is messy though. I would suggest moving the "Add facility to store wallet flags (64 bits)" commit to be first, then then squashing all the other changes down into single commit following that.
| @@ -2497,6 +2500,11 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT | ||
| // post-backup change. | ||
| // Reserve a new key pair from key pool | ||
| + if (fDisableHotKeys) { |
ryanofsky
May 4, 2017
Contributor
In commit "Add -disablehot option to ensure pure watchonly-wallets"
Do you think it might be better to fail earlier in CreateTransaction, regardless of the nChange > 0 condition? I'm thinking it might be, just so calls would fail consistently instead of sometimes randomly succeeding. But I don't have much understanding of the real-world use-cases for this, so this is just a thought.
Considering adding a code comment here explaining more.
jonasschnelli
May 5, 2017
Member
Falling earlier would disallow creating watch-only transactions (over fundrawtransaction).
| @@ -1408,13 +1407,25 @@ bool CWallet::IsHDEnabled() | ||
| return !hdChain.masterKeyID.IsNull(); | ||
| } | ||
| +void CWallet::SetWalletFlag(uint64_t flag) | ||
| +{ | ||
| + walletFlags |= flag; |
ryanofsky
May 4, 2017
Contributor
Maybe renaming SetWalletFlags (plural) to OverwriteWalletFlags?
Since this is doing an | I'd call it AddFlags (which conceivably could be paired with a RemoveFlags doing & ~).
| @@ -3915,6 +3948,7 @@ bool CWallet::ParameterInteraction() | ||
| nTxConfirmTarget = GetArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET); | ||
| bSpendZeroConfChange = GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE); | ||
| fWalletRbf = GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF); | ||
| + fDisableHotKeys = GetBoolArg("-disablehot", DEFAULT_DISABLE_HOT_WALLET); |
ryanofsky
May 4, 2017
Contributor
In commit "Add -disablehot option to ensure pure watchonly-wallets"
I don't understand why we want to call these "hot keys" and not "private keys," but I guess I'm out of the loop on terminology not knowing precisely what a "hot key" is.
| + } | ||
| + bool hotKeys = walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS); | ||
| + if (hotKeys && !disableHot) { | ||
| + InitError(strprintf(_("Error loading %s: You can't enable hot keys once you have initialized a wallet with disabled hot keys"), walletFile)); |
ryanofsky
May 4, 2017
Contributor
Add
-disablehotmode: a sane mode for watchonly-wallets
Maybe the error message could suggest passing -disablehot to avoid the problem. I think it would make the context of the error easier to understand for someone not expecting it.
| + self.nodes[1].getnewaddress() | ||
| + raise AssertionError("Wallet can derive private keys while -disablehot is enabled") | ||
| + except JSONRPCException as e: | ||
| + assert('Hot key' in e.error['message']) |
ryanofsky
May 4, 2017
Contributor
In commit "Add -disablehot option to ensure pure watchonly-wallets"
Consider using assert_raises_jsonrpc. (It's less verbose)
| @@ -705,7 +705,8 @@ bool WalletModel::hdEnabled() const | ||
| bool WalletModel::hotKeysDisabled() const | ||
| { | ||
| - return fDisableHotKeys; | ||
| + return (pwalletMain->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)); | ||
| + |
ryanofsky
May 4, 2017
Contributor
In commit "Make -disablehot per wallet (remove global state)"
Extra newline here
jonasschnelli
added some commits
Feb 16, 2017
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
2fde1cf
|
||
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
9935fbd
|
||
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
885e991
|
||
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
fcea55a
|
No strong opinion if this should be called |
|
utACK fcea55a Changes from previous review: pwalletMain removal, SetWalletFlag rename, error message tweaks, assert_raises_jsonrpc usage, and history cleanup. |
|
utACK fcea55a |
|
This has 3 utACKs so I'm building it now to provide a little testing. Maybe this PR should also update On "private keys" vs "hot keys" naming, "private keys" still sounds more comprehensible to me, but I wouldn't weigh my opinion very heavily if "hot key" is a term users will know. |
|
Tested ACK fcea55a. Confirmed disablehot option prevents generating keys, persists across reload even when disablehot option not specified in later runs, and that it prevents loading pre-existing or non-disablehot wallets. I don't think I encountered any bugs, and it seems to me this PR works well and is safe to merge. I was surprised by two things, and maybe these behaviors could be changed in the future if they aren't intentional:
|
jonasschnelli
added some commits
May 24, 2017
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
770db3a
|
||
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
9e94cc4
|
|
Added two commits. |
ryanofsky
reviewed
Jun 13, 2017
utACK 9e94cc4. Did not retest, but the two new commits are straightforward and look good. This needs rebase, and the last pwalletMain fixing commit should probably be squashed into the main "Add per wallet -disablehot" comment.
|
needs rebase, will review |
|
General comment while I wait for rebase: This is fundamentally in conflict with the hdwatchonly mode proposed in #9728 which was surprising to me. Perhaps the descriptions of the mode should stress that no (pub)key generation of any sort is allowed, or explicitly state that only imported watchonly keys are supported? Also, I think to make this mode less brittle if an assert in |
| @@ -2833,6 +2839,9 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet) | ||
| if (nLoadWalletRet != DB_LOAD_OK) | ||
| return nLoadWalletRet; | ||
| fFirstRunRet = !vchDefaultKey.IsValid(); | ||
| + if (IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) { |
instagibbs
Jun 19, 2017
Member
Maybe leave a comment here to make it clear there are two modes of detecting an initialized wallet here.
jnewbery
Jul 10, 2017
Member
Agree, and combine the two conditionals to make it clearer that that's what is happening.
fFirstRunRet = !vchDefaultKey.IsValid() && !IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS);
| @@ -2957,6 +2966,9 @@ bool CWallet::SetDefaultKey(const CPubKey &vchPubKey) | ||
| */ | ||
| bool CWallet::NewKeyPool() | ||
| { | ||
| + if (IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) { |
instagibbs
Jun 19, 2017
Member
Return condition for this function is never checked. Only used here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L639
For this mode do we even need the encrypt functionality?
jnewbery
reviewed
Jul 10, 2017
Generally looks good. A few comments inline. I haven't tested yet other than running the new functional test. I'll do some manual testing tomorrow.
I like the flag infrastructure you've put in place. I wonder if it can be used in #7729 as a way to upgrade from accounts to labels.
It'd be good to add the flags to the getwalletinfo output, but that can be done in a future PR.
I have a very slight preference for naming this -disableprivatekeys, but -disablehot is also fine.
| @@ -3579,6 +3617,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug) | ||
| { | ||
| std::string strUsage = HelpMessageGroup(_("Wallet options:")); | ||
| strUsage += HelpMessageOpt("-disablewallet", _("Do not load the wallet and disable wallet RPC calls")); | ||
| + strUsage += HelpMessageOpt("-disablehot", _("Disable the possibility of hot keys (only watchonlys are possible in this mode") + " " + strprintf(_("(default: %u)"), DEFAULT_DISABLE_HOT_WALLET)); |
| + LOCK(walletInstance->cs_wallet); | ||
| + /* make sure we don't have hot keys */ | ||
| + if (walletInstance->GetKeyPoolSize()) { | ||
| + InitError(strprintf(_("Error loading %s: You can't disable hot keys if your wallet already contains hot keys"), walletFile)); |
jnewbery
Jul 10, 2017
Member
This seems like the wrong error message: You can't disable hot keys... when the user has not specified the -disablehot parameter. Rather, this branch indicates some kind of wallet corruption: the DISABLE_HOT_KEYS flag is set, but the wallet contains private keys.
| @@ -0,0 +1,67 @@ | ||
| +#!/usr/bin/env python3 |
jnewbery
Jul 10, 2017
Member
Consider running a linter like flake8 over python modules to check for common nits.
| +# Copyright (c) 2017 The Bitcoin Core developers | ||
| +# Distributed under the MIT software license, see the accompanying | ||
| +# file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
| + |
jnewbery
Jul 10, 2017
Member
Can you add a short docstring explaining what the test is testing and how it is testing?
| + self.setup_clean_chain = True | ||
| + self.num_nodes = 2 | ||
| + | ||
| + def setup_network(self, split=False): |
jnewbery
Jul 10, 2017
Member
You can avoid having to override this method by setting:
self.extra_args = [[], ['-disablehot']]in the __init__() method
| + def run_test(self): | ||
| + assert_equal(self.nodes[1].getwalletinfo()['keypoolsize'], 0) | ||
| + | ||
| + print("Mining blocks...") |
| + n0addrC = self.nodes[0].getnewaddress() | ||
| + n0pubkC = self.nodes[0].validateaddress(n0addrC)['pubkey'] | ||
| + n0addr = self.nodes[0].getnewaddress() | ||
| + self.nodes[0].sendtoaddress(n0addrR, 10); |
jonasschnelli commentedFeb 1, 2017
This mode (
-disablehot) is intended for a sane pure watch-only mode, ideal for a use-case where one likes to use Bitcoin-Core in conjunction with a hardware-wallet or another solutions for cold-stogare.Since we have support for custom change addresses in
fundrawtransaction, pure watch-only wallets including coin-selection are possible and do make sense for some use cases.This new mode disables all forms of private key generation and ensure that no mix between hot and cold keys are possible.