Add createwallet "disableprivatekeys" option: a sane mode for watchonly-wallets

jonasschnelli commented at 1:16 pm on February 1, 2017: contributor

This mode (‘createwallet {“disableprivatekeys”: true}’) 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-storage.

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.

jonasschnelli added the label Wallet on Feb 1, 2017

jonasschnelli force-pushed on Feb 1, 2017

TheBlueMatt commented at 9:23 pm on February 1, 2017: member

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

laanwj commented at 8:03 am on February 2, 2017: member

Concept ACK. I agree that this would be better as a mode on the wallet instead of yet another startup option.

NicolasDorier commented at 1:53 am on February 16, 2017: contributor

I would like to rebase on top of that for #9728, can you rebase? I would switch that on if hdwatchonly is used.

jonasschnelli force-pushed on Feb 17, 2017

jonasschnelli commented at 9:11 am on February 17, 2017: contributor

Added two commit.

First commit adds a facility to store and check wallet flags (64bit). Made DISABLE_PRIVATE_KEYS the first flag.
Second commit makes the disablehot function per wallet (no longer a global state)

Disabling private keys on a wallet that already contains private keys is not possible (also the other way around).

in qa/rpc-tests/disablehot.py: in cd93c2da62 outdated

59+        # check if we can re-enable private keys
60+        assert_equal(self.nodes[1].signrawtransaction(frawtx['hex'])['complete'], False)
61+        print("restarting node with disablehot=0 (try to re-enable private keys which must not be possible)")
62+        stop_nodes(self.nodes)
63+        try:
64+            self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, [[], ['-disablehot=0']])

jonasschnelli commented at 9:37 am on February 17, 2017:

@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 commented at 2:10 am on February 18, 2017:

We might create a assert_start_raises_init_error() method. I will take a look at this tomorrow.

jonasschnelli force-pushed on Feb 17, 2017

in src/wallet/wallet.cpp: in e581d843f0 outdated

1406@@ -1408,13 +1407,25 @@ bool CWallet::IsHDEnabled()
1407     return !hdChain.masterKeyID.IsNull();
1408 }
1409 
1410+void CWallet::SetWalletFlag(uint64_t flag)
1411+{
1412+    walletFlags |= flag;

NicolasDorier commented at 5:11 am on February 20, 2017:

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 commented at 7:03 am on February 20, 2017:

CWallet::AddWalletFlags(uint64_t flags, book memonly) seems most consistent.

jonasschnelli commented at 1:51 pm on February 21, 2017:

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 commented at 5:04 pm on February 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 commented at 2:30 am on February 22, 2017:

@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.

jonasschnelli commented at 8:12 am on February 22, 2017:

Maybe renaming SetWalletFlags (plural) to OverwriteWalletFlags?

ryanofsky commented at 6:53 pm on May 4, 2017:

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 & ~).

in qa/rpc-tests/disablehot.py: in e581d843f0 outdated

12+        super().__init__()
13+        self.setup_clean_chain = True
14+        self.num_nodes = 2
15+
16+    def setup_network(self, split=False):
17+        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, [[], ['-disablehot']])

NicolasDorier commented at 6:33 am on February 20, 2017:

does the [] arguments is on purpose? because I think it makes -disablehot ignored

jonasschnelli commented at 1:52 pm on February 21, 2017:

We start two nodes here, the first node without startup parameters (thats why there is the []). Only second node has -disablehot.

jonasschnelli force-pushed on Mar 3, 2017

jonasschnelli commented at 3:36 pm on March 3, 2017: contributor

Needed rebase (#8775). Travis will fail because of the missing assert_start_raises_init_error (see #9832).

laanwj commented at 9:13 am on March 6, 2017: member

ideal for a use-case where one likes to use Bitcoin-Core in conjunction with a hardware-wallet or another solutions for cold-stogare

Nice, so this guarantees that bitcoind process will contain no private key data at all? Seems like s a good step towards supporting hardware wallets too. Concept ACK.

Travis should pass now that #9832 is merged.

jonasschnelli force-pushed on Mar 6, 2017

jonasschnelli commented at 9:31 am on March 6, 2017: contributor

The disablehot.py now makes use of the new (#9832) assert_start_raises_init_error call.

Nice, so this guarantees that bitcoind process will contain no private key data at all?

Yes. Event the default key is disabled (while still detecting the first start correctly).

in src/wallet/wallet.cpp: in 9549e2c909 outdated

3714@@ -3670,6 +3715,27 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3715 
3716         walletInstance->SetBestChain(chainActive.GetLocator());
3717     }
3718+    else if (IsArgSet("-disablehot")) {
3719+        bool disableHot = GetBoolArg("-disablehot", DEFAULT_DISABLE_HOT_WALLET);
3720+        LOCK(walletInstance->cs_wallet);
3721+        if (walletInstance->vchDefaultKey.IsValid() && disableHot) {

NicolasDorier commented at 7:59 am on March 13, 2017:

I think you should use fFirstRun here instead of walletInstance->vchDefaultKey.IsValid()

jonasschnelli commented at 8:07 am on March 13, 2017:

At this point fFirstRun is always false. And I think this also works in conjunction with salvagewallet.

NicolasDorier commented at 8:22 am on March 13, 2017:

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 commented at 8:24 am on March 13, 2017:

OK forget what I said, there is no reason to have disabledHot with hdwatchonly at same time.

luke-jr commented at 2:27 pm on October 30, 2017:

This should at least be abstracted somehow. vchDefaultKey’s existence is not exactly identical to “does this wallet have keys?”

jonasschnelli force-pushed on Mar 29, 2017

jonasschnelli commented at 2:44 pm on March 29, 2017: contributor

Rebased.

jonasschnelli force-pushed on Apr 2, 2017

laanwj commented at 1:32 pm on May 2, 2017: member

Needs rebase again (sorry) utACK otherwise.

jonasschnelli force-pushed on May 4, 2017

jonasschnelli commented at 1:11 pm on May 4, 2017: contributor

Rebased.

in src/wallet/wallet.cpp:3951 in 69d64075fc outdated

3947@@ -3915,6 +3948,7 @@ bool CWallet::ParameterInteraction()
3948     nTxConfirmTarget = GetArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET);
3949     bSpendZeroConfChange = GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE);
3950     fWalletRbf = GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF);
3951+    fDisableHotKeys = GetBoolArg("-disablehot", DEFAULT_DISABLE_HOT_WALLET);

ryanofsky commented at 4:00 pm on May 4, 2017:

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.

in qa/rpc-tests/disablehot.py:45 in 69d64075fc outdated

40+
41+        try:
42+            self.nodes[1].getnewaddress()
43+            raise AssertionError("Wallet can derive private keys while -disablehot is enabled")
44+        except JSONRPCException as e:
45+            assert('Hot key' in e.error['message'])

ryanofsky commented at 4:29 pm on May 4, 2017:

In commit “Add -disablehot option to ensure pure watchonly-wallets”

Consider using assert_raises_jsonrpc. (It’s less verbose)

in src/wallet/wallet.cpp:2503 in 69d64075fc outdated

2499@@ -2497,6 +2500,11 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
2500                         //  post-backup change.
2501 
2502                         // Reserve a new key pair from key pool
2503+                        if (fDisableHotKeys) {

ryanofsky commented at 4:54 pm on May 4, 2017:

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 commented at 7:19 am on May 5, 2017:

Falling earlier would disallow creating watch-only transactions (over fundrawtransaction).

in src/qt/walletmodel.cpp:742 in 4f3acb5d8c outdated

704@@ -705,7 +705,8 @@ bool WalletModel::hdEnabled() const
705 
706 bool WalletModel::hotKeysDisabled() const
707 {
708-    return fDisableHotKeys;
709+    return (pwalletMain->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS));
710+

ryanofsky commented at 5:03 pm on May 4, 2017:

In commit “Make -disablehot per wallet (remove global state)”

Extra newline here

in src/wallet/wallet.cpp:3763 in 4f3acb5d8c outdated

3762+            InitError(strprintf(_("Error loading %s: You can't disable hot keys if your wallet already contains hot keys"), walletFile));
3763+            return NULL;
3764+        }
3765+        bool hotKeys = walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS);
3766+        if (hotKeys && !disableHot) {
3767+            InitError(strprintf(_("Error loading %s: You can't enable hot keys once you have initialized a wallet with disabled hot keys"), walletFile));

ryanofsky commented at 7:03 pm on May 4, 2017:

Add -disablehot mode: 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.

ryanofsky commented at 7:12 pm on May 4, 2017: member

utACK 9b39bf94f168ab82707df91f21cffea0b286f919

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.

jonasschnelli force-pushed on May 5, 2017

jonasschnelli commented at 7:27 am on May 5, 2017: contributor

Completely rewrote the commit history (as reported by @ryanofsky it was messy).
Fixed @ryanofsky points

No strong opinion if this should be called -disableprivatekeys (which somehow nails it but may confuse people who just want cold key storage). Other opinions?

ryanofsky commented at 2:40 pm on May 5, 2017: member

utACK fcea55a6ec6be57ee96458bde6fb24656269d3ee

Changes from previous review: pwalletMain removal, SetWalletFlag rename, error message tweaks, assert_raises_jsonrpc usage, and history cleanup.

NicolasDorier commented at 8:46 am on May 6, 2017: contributor

utACK fcea55a6ec6be57ee96458bde6fb24656269d3ee

ryanofsky commented at 5:01 pm on May 23, 2017: member

This has 3 utACKs so I’m building it now to provide a little testing. Maybe this PR should also update doc/release-notes.md since it’s adding a new option.

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.

ryanofsky commented at 6:30 pm on May 23, 2017: member

Tested ACK fcea55a6ec6be57ee96458bde6fb24656269d3ee. 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:

I was surprised that -disablehot could only be used on a new wallet and would refuse to load a preexisting wallet that did not have any private keys.
I was surprised that -disablehot did seem to allow importing private keys via importprivkey and importwallet rpcs.

jonasschnelli commented at 9:20 am on May 24, 2017: contributor

Added two commits. The first addresses the “importprivkey” / “Importwallet” issues reported by @ryanofsky (disable both commands in -disablehot mode). The second fixes a rebase pwallet/pwalletmain issue.

ryanofsky commented at 6:59 pm on June 13, 2017: member

utACK 9e94cc41b3180949011d2299da14c13471b6aa2b. 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.

instagibbs commented at 1:34 pm on June 19, 2017: member

needs rebase, will review

in src/wallet/wallet.cpp:2842 in 9935fbd829 outdated

2838@@ -2833,6 +2839,9 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
2839     if (nLoadWalletRet != DB_LOAD_OK)
2840         return nLoadWalletRet;
2841     fFirstRunRet = !vchDefaultKey.IsValid();
2842+    if (IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {

instagibbs commented at 1:35 pm on June 19, 2017:

Maybe leave a comment here to make it clear there are two modes of detecting an initialized wallet here.

jnewbery commented at 8:44 pm on July 10, 2017:

Agree, and combine the two conditionals to make it clearer that that’s what is happening.

0fFirstRunRet = !vchDefaultKey.IsValid() && !IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS);

luke-jr commented at 2:24 pm on October 30, 2017:

Why are we touching this logic at all? Seems we should just set a default key for watchonly-only wallets, until default keys are removed normally…

in src/wallet/wallet.cpp:2969 in 9935fbd829 outdated

2965@@ -2957,6 +2966,9 @@ bool CWallet::SetDefaultKey(const CPubKey &vchPubKey)
2966  */
2967 bool CWallet::NewKeyPool()
2968 {
2969+    if (IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {

instagibbs commented at 1:39 pm on June 19, 2017:

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?

instagibbs commented at 2:20 pm on June 19, 2017: member

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 AddKeyPubKey is added, since this should never be reachable.

in src/wallet/wallet.cpp:3620 in 9e94cc41b3 outdated

3616@@ -3579,6 +3617,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
3617 {
3618     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
3619     strUsage += HelpMessageOpt("-disablewallet", _("Do not load the wallet and disable wallet RPC calls"));
3620+    strUsage += HelpMessageOpt("-disablehot", _("Disable the possibility of hot keys (only watchonlys are possible in this mode") + " " + strprintf(_("(default: %u)"), DEFAULT_DISABLE_HOT_WALLET));

jnewbery commented at 8:46 pm on July 10, 2017:

nit: place alphabetically (above -disablewallet)

in src/wallet/wallet.cpp:3771 in 9e94cc41b3 outdated

3766+    }
3767+    else if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
3768+        LOCK(walletInstance->cs_wallet);
3769+        /* make sure we don't have hot keys */
3770+        if (walletInstance->GetKeyPoolSize()) {
3771+            InitError(strprintf(_("Error loading %s: You can't disable hot keys if your wallet already contains hot keys"), walletFile));

jnewbery commented at 8:49 pm on July 10, 2017:

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.

in test/functional/disablehot.py:16 in 9e94cc41b3 outdated

11+    def __init__(self):
12+        super().__init__()
13+        self.setup_clean_chain = True
14+        self.num_nodes = 2
15+
16+    def setup_network(self, split=False):

jnewbery commented at 8:58 pm on July 10, 2017:

You can avoid having to override this method by setting:

0self.extra_args = [[], ['-disablehot']]

in the __init__() method

in test/functional/disablehot.py:27 in 9e94cc41b3 outdated

22+        self.sync_all()
23+
24+    def run_test(self):
25+        assert_equal(self.nodes[1].getwalletinfo()['keypoolsize'], 0)
26+
27+        print("Mining blocks...")

jnewbery commented at 8:59 pm on July 10, 2017:

please replace print() calls with self.log.info()

in test/functional/disablehot.py:37 in 9e94cc41b3 outdated

32+        n0addrR = self.nodes[0].getnewaddress()
33+        n0pubkR = self.nodes[0].validateaddress(n0addrR)['pubkey']
34+        n0addrC = self.nodes[0].getnewaddress()
35+        n0pubkC = self.nodes[0].validateaddress(n0addrC)['pubkey']
36+        n0addr  = self.nodes[0].getnewaddress()
37+        self.nodes[0].sendtoaddress(n0addrR, 10);

jnewbery commented at 8:59 pm on July 10, 2017:

no ; please :slightly_smiling_face:

in test/functional/disablehot.py:5 in 9e94cc41b3 outdated

0@@ -0,0 +1,67 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 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+

jnewbery commented at 9:00 pm on July 10, 2017:

Can you add a short docstring explaining what the test is testing and how it is testing?

in test/functional/disablehot.py:1 in 9e94cc41b3 outdated

0@@ -0,0 +1,67 @@
1+#!/usr/bin/env python3

jnewbery commented at 9:03 pm on July 10, 2017:

Consider running a linter like flake8 over python modules to check for common nits.

jnewbery commented at 9:09 pm on July 10, 2017: member

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.

in src/wallet/walletdb.cpp:512 in 9e94cc41b3 outdated

536@@ -537,6 +537,12 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
537                 return false;
538             }
539         }
540+        else if (strType == "flags")
541+        {
542+            uint64_t flags;
543+            ssValue >> flags;
544+            pwallet->SetWalletFlags(flags, true);

luke-jr commented at 2:18 pm on October 30, 2017:

I would think we should fail somehow, if an unrecognised flag is set?

in src/wallet/wallet.cpp:3620 in 9935fbd829 outdated

3616@@ -3598,6 +3617,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
3617 {
3618     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
3619     strUsage += HelpMessageOpt("-disablewallet", _("Do not load the wallet and disable wallet RPC calls"));
3620+    strUsage += HelpMessageOpt("-disablehot", _("Disable the possibility of hot keys (only watchonlys are possible in this mode") + " " + strprintf(_("(default: %u)"), DEFAULT_DISABLE_HOT_WALLET));

luke-jr commented at 2:24 pm on October 30, 2017:

Prefer:

0strprintf(_("Disable the possibility of hot keys (only watchonlys are possible in this mode; default: %u)"), DEFAULT_DISABLE_HOT_WALLET)

in src/wallet/wallet.cpp:3763 in 9935fbd829 outdated

3758+            InitError(strprintf(_("Error loading %s: You can't disable hot keys if your wallet already contains hot keys"), walletFile));
3759+            return NULL;
3760+        }
3761+        bool hotKeys = walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS);
3762+        if (hotKeys && !disableHot) {
3763+            InitError(strprintf(_("Error loading %s: You can't enable hot keys once you have initialized a wallet with disabled hot keys (use -disablehot instead)"), walletFile));

luke-jr commented at 2:28 pm on October 30, 2017:

Drop the “(use -disablehot instead)”, since that will also fail in a multiwallet scenario.

in src/wallet/wallet.cpp:3741 in 9935fbd829 outdated

3736@@ -3717,6 +3737,9 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3737             if (!walletInstance->SetHDMasterKey(masterPubKey))
3738                 throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
3739         }
3740+        if (GetBoolArg("-disablehot", DEFAULT_DISABLE_HOT_WALLET)) {
3741+            walletInstance->AddWalletFlag(WALLET_FLAG_DISABLE_HOT_KEYS);

luke-jr commented at 2:30 pm on October 30, 2017:

This should also probably fake an encrypted wallet, so it can be read by older versions sanely.

in src/wallet/rpcdump.cpp:106 in 770db3a530 outdated

101@@ -102,6 +102,10 @@ UniValue importprivkey(const JSONRPCRequest& request)
102         );
103 
104 
105+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
106+        throw JSONRPCError(RPC_WALLET_ERROR, "Error: Hot keys are disabled (-disablehot)");

luke-jr commented at 2:33 pm on October 30, 2017:

It doesn’t make sense to mention the option here, since it’s tied to the wallet and can’t just be toggled…

in src/wallet/rpcdump.cpp:478 in 770db3a530 outdated

473@@ -470,6 +474,10 @@ UniValue importwallet(const JSONRPCRequest& request)
474     if (fPruneMode)
475         throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled in pruned mode");
476 
477+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
478+        throw JSONRPCError(RPC_WALLET_ERROR, "Error: Hot keys are disabled (-disablehot)");

luke-jr commented at 2:33 pm on October 30, 2017:

It doesn’t make sense to mention the option here, since it’s tied to the wallet and can’t just be toggled…

luke-jr changes_requested

in src/qt/receivecoinsdialog.cpp:97 in 9e94cc41b3 outdated

92@@ -93,6 +93,10 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)
93             SLOT(recentRequestsView_selectionChanged(QItemSelection, QItemSelection)));
94         // Last 2 columns are set by the columnResizingFixer, when the table geometry is ready.
95         columnResizingFixer = new GUIUtil::TableViewLastColumnResizingFixer(tableView, AMOUNT_MINIMUM_COLUMN_WIDTH, DATE_COLUMN_WIDTH, this);
96+
97+        if (_model->hotKeysDisabled()) {

promag commented at 3:16 pm on November 3, 2017:

0ui->receiveButton->setEnabled(!_model->hotKeysDisabled());

in src/qt/walletmodel.cpp:708 in 9e94cc41b3 outdated

702@@ -703,6 +703,11 @@ bool WalletModel::hdEnabled() const
703     return wallet->IsHDEnabled();
704 }
705 
706+bool WalletModel::hotKeysDisabled() const
707+{
708+    return (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS));

promag commented at 3:16 pm on November 3, 2017:

Drop outer ()?

in src/wallet/walletdb.cpp:848 in 9e94cc41b3 outdated

876@@ -871,6 +877,12 @@ bool CWalletDB::WriteHDChain(const CHDChain& chain)
877     return batch.Write(std::string("hdchain"), chain);
878 }
879 
880+bool CWalletDB::WriteWalletFlags(const uint64_t flags)

promag commented at 3:26 pm on November 3, 2017:

Remove const.

jonasschnelli commented at 1:18 pm on February 28, 2018:

Why?

in src/wallet/walletdb.h:237 in 9e94cc41b3 outdated

211@@ -212,6 +212,7 @@ class CWalletDB
212     //! write the hdchain model (external chain child index counter)
213     bool WriteHDChain(const CHDChain& chain);
214 
215+    bool WriteWalletFlags(const uint64_t flags);

promag commented at 3:26 pm on November 3, 2017:

Remove const.

kallewoof commented at 8:30 am on April 11, 2018:

Why?

in test/functional/disablehot.py:11 in 9e94cc41b3 outdated

 6+from test_framework.test_framework import BitcoinTestFramework
 7+from test_framework.util import *
 8+
 9+class DisableHotKeysTest(BitcoinTestFramework):
10+
11+    def __init__(self):

promag commented at 3:27 pm on November 3, 2017:

set_test_params instead.

promag commented at 3:36 pm on November 3, 2017: member

Concept ACK. Partial review. Needs rebase.

This option affects all loaded wallets? How will this interact with #10740?

Sjors commented at 5:41 pm on February 8, 2018: member

Needs rebase.

Tend to agree that this should be a per wallet thing and that this should be set upon wallet creation, rather than application launch.

If I understand correctly this is purely a safety feature. But I wonder if it doesn’t make too many assumptions.

The term watch-only wallet is ambiguous. It could mean that the user doesn’t have access to private keys at all, or that they do, but managed by an external device. They could also be part of a multisig arrangement, in which case it does have some keys and needs to generate (multisig) change addresses, but shouldn’t generate “solo” keys.

If I understand correctly, the scope of this PR is wallets where keys are managed externally, e.g. a hardware wallet. Would such a wallet have it’s own UI that uses the RPC, or would the user interact directly with the RPC somehow? In the first case, it would seem that the hardware wallet developer should just be careful about which RPC commands to call. Or am I missing something? Not that I’m against making it safer.

jonasschnelli force-pushed on Feb 28, 2018

jonasschnelli commented at 1:25 pm on February 28, 2018: contributor

Rebased and fixed. Renamed -disablehot to -disableprivatekeys

Since we have no dynamic wallet creation (something like #10740), disabling private keys is only possible during wallet creation and with the -disableprivatekeys option.

Use case:

Splitting the private keys from the wallet (hardware wallets, cold storage, use wallet as pure index), one wants to make sure, fundrawtransaction, etc. won’t accidentally use an internal private key as change address or something similar

jonasschnelli renamed this:
~~Add `-disablehot` mode: a sane mode for watchonly-wallets~~
Add `-disableprivatekeys` mode: a sane mode for watchonly-wallets
on Feb 28, 2018

jonasschnelli force-pushed on Feb 28, 2018

jonasschnelli force-pushed on Mar 3, 2018

jonasschnelli commented at 5:45 am on March 3, 2018: contributor

Rebased (and fixed travis issue)

jonasschnelli force-pushed on Mar 18, 2018

jonasschnelli commented at 9:30 am on March 18, 2018: contributor

Rebased.

jonasschnelli force-pushed on Apr 5, 2018

jonasschnelli force-pushed on Apr 11, 2018

jonasschnelli commented at 7:29 am on April 11, 2018: contributor

Had to rebase after #10244 (non trivial). Glad if utACKers can review again.

kallewoof commented at 8:28 am on April 11, 2018: member

Late so can be safely ignored, but we say importprivkey so maybe disableprivkeys for consistency? Will review.

in src/wallet/wallet.h:1160 in c572c52f19 outdated

1155+    void AddWalletFlag(uint64_t flags);
1156+
1157+    /** check if a certain wallet flag is set */
1158+    bool IsWalletFlagSet(uint64_t flag);
1159+
1160+    /** overwrite all flags by the given unit64_t */

kallewoof commented at 8:30 am on April 11, 2018:

Typo: uint64_t

in src/wallet/wallet.cpp:1539 in c572c52f19 outdated

1488+    m_wallet_flags |= flags;
1489+    if (!WalletBatch(*database).WriteWalletFlags(m_wallet_flags))
1490+        throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");
1491+}
1492+
1493+bool CWallet::IsWalletFlagSet(uint64_t flag)

kallewoof commented at 8:32 am on April 11, 2018:

μNit: Why not just GetWalletFlag?

in src/wallet/wallet.cpp:1550 in 56c0798d32 outdated

1501+bool CWallet::SetWalletFlags(uint64_t overwriteFlags, bool memonly)
1502 {
1503     m_wallet_flags = overwriteFlags;
1504+    if ( (overwriteFlags >> 32 | g_known_wallet_flags >> 32) != (g_known_wallet_flags >> 32) ) {
1505+        // contains unkown non-tolerable wallet flags
1506+        return false;

kallewoof commented at 8:33 am on April 11, 2018:

I wonder if this should throw here instead of being a can-fail method.

jonasschnelli commented at 2:02 pm on June 21, 2018:

I think a specific error handling makes more sense (see error handling in walletdb)

in src/wallet/wallet.cpp:4212 in 56c0798d32 outdated

4041-        if (!walletInstance->SetHDMasterKey(masterPubKey))
4042-            throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
4043+        if (gArgs.GetBoolArg("-disableprivatekeys", DEFAULT_DISABLE_PRIVKEY_WALLET)) {
4044+            walletInstance->AddWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
4045+        }
4046+        else {

kallewoof commented at 8:34 am on April 11, 2018:

Nit: } else {

in src/wallet/wallet.cpp:4058 in 56c0798d32 outdated

4058             return nullptr;
4059         }
4060 
4061         walletInstance->SetBestChain(chainActive.GetLocator());
4062+    }
4063+    else if (gArgs.IsArgSet("-disableprivatekeys")) {

kallewoof commented at 8:35 am on April 11, 2018:

Nit: } else if (...) {

in src/wallet/wallet.cpp:4063 in 56c0798d32 outdated

4063+    else if (gArgs.IsArgSet("-disableprivatekeys")) {
4064+        bool disablePrivKeys = gArgs.GetBoolArg("-disableprivatekeys", DEFAULT_DISABLE_PRIVKEY_WALLET);
4065+        LOCK(walletInstance->cs_wallet);
4066+        if ((!walletInstance->mapKeys.empty() || !walletInstance->mapCryptedKeys.empty()) && disablePrivKeys) {
4067+            InitError(strprintf(_("Error loading %s: You can't disable private keys if your wallet already contains private keys"), walletFile));
4068+            return NULL;

kallewoof commented at 8:35 am on April 11, 2018:

I think we prefer nullptr over NULL these days.

in src/wallet/wallet.cpp:4067 in 56c0798d32 outdated

4067+            InitError(strprintf(_("Error loading %s: You can't disable private keys if your wallet already contains private keys"), walletFile));
4068+            return NULL;
4069+        }
4070+        if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !disablePrivKeys) {
4071+            InitError(strprintf(_("Error loading %s: You can't enable private keys once you have initialized a wallet with disabled private keys (use -disableprivatekeys instead)"), walletFile));
4072+            return NULL;

kallewoof commented at 8:36 am on April 11, 2018:

I think we prefer nullptr over NULL these days.

in src/wallet/wallet.cpp:4234 in 56c0798d32 outdated

4075+    else if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
4076+        LOCK(walletInstance->cs_wallet);
4077+        /* make sure we don't have private keys */
4078+        if (walletInstance->GetKeyPoolSize()) {
4079+            InitError(strprintf(_("Error loading %s: Private keys present in a wallet where private keys are disabled"), walletFile));
4080+            return NULL;

kallewoof commented at 8:36 am on April 11, 2018:

I think we prefer nullptr over NULL these days.

in src/wallet/wallet.cpp:4070 in 56c0798d32 outdated

4070+        if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !disablePrivKeys) {
4071+            InitError(strprintf(_("Error loading %s: You can't enable private keys once you have initialized a wallet with disabled private keys (use -disableprivatekeys instead)"), walletFile));
4072+            return NULL;
4073+        }
4074+    }
4075+    else if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {

kallewoof commented at 8:36 am on April 11, 2018:

Nit: } else if ...

in src/wallet/wallet.h:1219 in 56c0798d32 outdated

1166@@ -1157,8 +1167,9 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
1167     /** check if a certain wallet flag is set */
1168     bool IsWalletFlagSet(uint64_t flag);
1169 
1170-    /** overwrite all flags by the given unit64_t */
1171-    void SetWalletFlags(uint64_t overwriteFlags, bool memOnly);
1172+    /** overwrite all flags by the given unit64_t

kallewoof commented at 8:37 am on April 11, 2018:

Typo: uint64_t

in src/wallet/walletdb.cpp:584 in 56c0798d32 outdated

575+                if (IsKeyType(strType) || strType == "defaultkey") {
576                     result = DBErrors::CORRUPT;
577-                else
578-                {
579+                }
580+                else if(strType == "flags") {

kallewoof commented at 8:38 am on April 11, 2018:

Nit: } else

jnewbery commented at 7:35 pm on June 20, 2018:

not addressed

in src/wallet/walletdb.cpp:586 in 56c0798d32 outdated

579+                }
580+                else if(strType == "flags") {
581+                    // reading the wallet flags can only fail if unknown flags are present
582+                    result = DBErrors::TOO_NEW;
583+                }
584+                else {

kallewoof commented at 8:38 am on April 11, 2018:

Nit: } else

in test/functional/wallet_disableprivkeys.py:40 in 74d25a289e outdated

35+        assert_raises_rpc_error(-4,"Error: Private keys are disabled (-disableprivatekeys)", self.nodes[1].getnewaddress)
36+        assert_raises_rpc_error(-4,"Error: Private keys are disabled (-disableprivatekeys)", self.nodes[1].getrawchangeaddress)
37+
38+        self.nodes[1].importpubkey(n0pubkR) #TODO switch to importmulti
39+        self.nodes[1].importpubkey(n0pubkC)
40+        assert_equal(self.nodes[1].getbalance("*", 1, True), 10)

kallewoof commented at 8:41 am on April 11, 2018:

Unsure why you’re explicitly giving the "*", 1, True and not just doing getbalance() here.

in test/functional/wallet_disableprivkeys.py:43 in 74d25a289e outdated

38+        self.nodes[1].importpubkey(n0pubkR) #TODO switch to importmulti
39+        self.nodes[1].importpubkey(n0pubkC)
40+        assert_equal(self.nodes[1].getbalance("*", 1, True), 10)
41+        rawtx = self.nodes[1].createrawtransaction([], {n0addr: 1.0})
42+        frawtx= self.nodes[1].fundrawtransaction(rawtx, {"changeAddress": n0addrC, "includeWatching": True})
43+        assert_equal(self.nodes[1].signrawtransactionwithwallet(frawtx['hex'])['complete'], False)

kallewoof commented at 8:42 am on April 11, 2018:

Maybe add a test that ensures it can signrawtransactionwithkeys (disabling privkeys should still let you sign, if you provide the keys, I think?).

kallewoof commented at 8:42 am on April 11, 2018: member

utACK 74d25a289ea040ed33e0aaed120e77f3be7e5311

Note 3rd commit message needs to be updated (still says disablehot).

Edit: I know I’m late to the party, so take my nits with a grain of salt.

Sjors commented at 11:05 am on April 11, 2018: member

Concept ACK on the use of flags that are stored in the wallet (I think I overlooked that the first time I reviewed). This prevents accidentally disabling the feature. I also think it’s a safe choice that you can’t enable this feature on a wallet that already has private keys.

There are some problems in a multi wallet environment though.

It correctly remembers which wallets have disabled private keys, disabling the payment request button in QT where appropriate. So that’s good.

But if you try to load an existing normal wallet and create a new -disableprivatekeys wallet at the same time, you’ll get an error upon launch “You can’t disable private keys if your wallet already contains private keys”.

One solution would be to explain in documentation that in order to create such a wallet you can only have one -wallet argument. But I think a better approach is to create a new -createwallet RPC which takes disableprivatekeys as an argument. Such an RPC could also be useful for dynamic loading #10740 .

MarcoFalke added the label Needs rebase on Jun 6, 2018

jonasschnelli force-pushed on Jun 13, 2018

jonasschnelli renamed this:
~~Add `-disableprivatekeys` mode: a sane mode for watchonly-wallets~~
Add createwallet "disableprivatekeys" option: a sane mode for watchonly-wallets
on Jun 13, 2018

jonasschnelli force-pushed on Jun 13, 2018

jonasschnelli commented at 8:46 pm on June 13, 2018: contributor

Overhauled this PR.

Removed the global -disableprivatkeys argument.
Added an options object to createwallet with currently a single boolean disableprivatekeys.

DrahtBot removed the label Needs rebase on Jun 13, 2018

Sjors commented at 12:18 pm on June 14, 2018: member

Much better!

Some stray files made it into the PR though (Bitcoin-Core.*).

jonasschnelli force-pushed on Jun 14, 2018

jonasschnelli commented at 12:21 pm on June 14, 2018: contributor

Thanks @Sjors. Removed the accidentally added IDE files.

in src/wallet/rpcwallet.cpp:3125 in 8cb2e81eaa outdated

3122             "\nCreates and loads a new wallet.\n"
3123             "\nArguments:\n"
3124             "1. \"wallet_name\"    (string, required) The name for the new wallet. If this is a path, the wallet will be created at the path location.\n"
3125+            "2. options          (json, optional)\n"
3126+            "  {\n"
3127+            "     \"disableprivatekeys\": <state>,         (boolean, optional, default: false) Disable the possibility of private keys (only watchonlys are possible in this mode).\n"

promag commented at 12:30 pm on June 14, 2018:

I was under the impression that named arguments is preferable to options object.

jnewbery commented at 2:14 pm on June 14, 2018:

I agree. Named arguments are much preferable. Using named arguments provides type- and error- checking for free, and simplify/flatten the interface

For example, if I typo the an option disableprivtaekeys here, this implementation will silently ignore the option. That’s not possible with named arguments.

in src/wallet/wallet.cpp:1535 in 8cb2e81eaa outdated

1528+}
1529+
1530+bool CWallet::SetWalletFlags(uint64_t overwriteFlags, bool memonly)
1531+{
1532+    m_wallet_flags = overwriteFlags;
1533+    if ( (overwriteFlags >> 32 | g_known_wallet_flags >> 32) != (g_known_wallet_flags >> 32) ) {

jnewbery commented at 2:23 pm on June 14, 2018:

if ((overwriteFlags & ^g_known_wallet_flags) >> 32) { seems slightly more readable to me.

in src/wallet/wallet.cpp:4233 in 8cb2e81eaa outdated

4233+        InitError(strprintf(_("Error loading %s: Private keys can only be disabled during creation"), walletFile));
4234+        return NULL;
4235+    } else if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
4236+        LOCK(walletInstance->cs_wallet);
4237+        if (!walletInstance->mapKeys.empty() || !walletInstance->mapCryptedKeys.empty()) {
4238+            InitError(strprintf(_("Error loading %s: Private keys detected in wallet with disabled private keys"), walletFile));

jnewbery commented at 2:27 pm on June 14, 2018:

This seems a little dangerous. If we get into this condition for any reason, it’ll be impossible for the user to load the wallet and retrieve their private keys.

Sjors commented at 6:35 pm on June 14, 2018:

Sounds like that should be a warning, not an error.

in src/qt/receivecoinsdialog.cpp:103 in 8cb2e81eaa outdated

 98@@ -99,6 +99,9 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)
 99         } else {
100             ui->useBech32->setCheckState(Qt::Unchecked);
101         }
102+
103+        // eventually disable the main receive button if private key operations are disabled

jnewbery commented at 2:29 pm on June 14, 2018:

How does this interact with multiwallet? Loading/unloading wallets at run time?

Sjors commented at 6:38 pm on June 14, 2018:

I still think multiwallet should use one window per wallet rather than a dropdown inside a single window. Pending that, receive screen should at least be visible if any of the wallets aren’t read-only. The receive button itself would have to be updated when the user switches between wallets.

Another question is: provided there’s enough warnings (including about scams), why shouldn’t a user be able to receive money on a watch-only address (assuming there’s an unused one)?

jonasschnelli commented at 12:44 pm on June 18, 2018:

@Sjors comment is orthogonal. @jnewbery: this seems to work perfectly fine with mtulitwallet. Not sure about unloading… but I guess unloading does also call setModel()… or can you be more specific why this should be a problem?

jnewbery commented at 8:05 pm on June 20, 2018:

Yes, you’re right that this works fine with multiwallet.

It would be nice if there was help text saying that the ‘receive’ button is disabled because the wallet has private keys disabled (and so can’t generate new receiving addresses). However, that could be added in a separate PR.

in test/functional/wallet_multiwallet.py:154 in 8cb2e81eaa outdated

150@@ -151,7 +151,9 @@ def run_test(self):
151         assert_equal(w3.getbalance(), 0)
152         assert_equal(w4.getbalance(), 0)
153 
154-        w1.sendtoaddress(w2.getnewaddress(), 1)
155+        w2addrR = w2.getnewaddress()

jnewbery commented at 2:31 pm on June 14, 2018:

nit: I’m not sure what the R is for here. In general, prefer to use lower_case_with_underscores for variable names in Python.

in test/functional/wallet_multiwallet.py:238 in 8cb2e81eaa outdated

232@@ -231,8 +233,14 @@ def run_test(self):
233         assert_equal(loadwallet_name['name'], new_wallet_name)
234         w10 = node.get_wallet_rpc(new_wallet_name)
235         assert_equal(w10.getwalletinfo()['walletname'], new_wallet_name)
236-
237         assert new_wallet_name in self.nodes[0].listwallets()
238 
239+        self.log.info("Test disableprivatekeys creation.")

jnewbery commented at 2:32 pm on June 14, 2018:

It’s not obvious to me why this test is in wallet_multiwallet.py. What does it have to do with multiwallet?

jonasschnelli commented at 8:01 pm on June 14, 2018:

I guess I placed it there because this was the only test that used createwallet. But right, I shall move it to a new test.

jnewbery commented at 2:34 pm on June 14, 2018: member

Concept ACK. I can run the new functional test, but I wasn’t able to get the receive tab to disappear in qt.

A couple of high-level things that I think could improve this PR:

some unit test of the new wallet/walletdb functionality.
update getwalletinfo to display whether the wallet has private keys disabled.

in src/wallet/wallet.cpp:1534 in ec2fb56a39 outdated

1512@@ -1513,6 +1513,25 @@ bool CWallet::IsHDEnabled() const
1513     return !hdChain.seed_id.IsNull();
1514 }
1515 
1516+void CWallet::AddWalletFlag(uint64_t flags)
1517+{
1518+    m_wallet_flags |= flags;

promag commented at 4:52 pm on June 14, 2018:

Commit “Add facility to store wallet flags (64 bits)”

Missing LOCK(cs_wallet);?

jonasschnelli commented at 1:57 pm on June 21, 2018:

Fixed.

in src/wallet/wallet.cpp:4211 in bf85f0b60d outdated

4210-        if (!walletInstance->SetHDSeed(seed))
4211-            throw std::runtime_error(std::string(__func__) + ": Storing HD seed failed");
4212+        if ((wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
4213+            //selective allow to set flags
4214+            walletInstance->AddWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
4215+        }

promag commented at 5:00 pm on June 14, 2018:

Commit “Add per option to disable private keys during internal wallet creation”

nit } else {.

jonasschnelli force-pushed on Jun 14, 2018

jonasschnelli force-pushed on Jun 18, 2018

jonasschnelli commented at 12:45 pm on June 18, 2018: contributor

Fixed @jnewbery s points.

Added privatekeys_enabled to getwalletinfo
Added unit test and own rpc test

in src/wallet/wallet.h:1205 in 4c2b3fc3bf outdated

1208@@ -1198,6 +1209,16 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
1209 
1210     /** Whether a given output is spendable by this wallet */
1211     bool OutputEligibleForSpending(const COutput& output, const CoinEligibilityFilter& eligibility_filter) const;
1212+
1213+    /** set a single wallet flag */

jnewbery commented at 7:34 pm on June 20, 2018:

Any reason for not naming these SetWalletFlag/GetWalletFlag/SetWalletFlags?

jonasschnelli commented at 2:10 pm on June 21, 2018:

Agree… changed from AddWalletFlag to SetWalletFlag

in src/wallet/wallet.cpp:1536 in 4c2b3fc3bf outdated

1531+bool CWallet::SetWalletFlags(uint64_t overwriteFlags, bool memonly)
1532+{
1533+    LOCK(cs_wallet);
1534+    m_wallet_flags = overwriteFlags;
1535+    if ( (overwriteFlags >> 32 | g_known_wallet_flags >> 32) != (g_known_wallet_flags >> 32) ) {
1536+        // contains unkown non-tolerable wallet flags

jnewbery commented at 7:36 pm on June 20, 2018:

nit: s/unkown/unknown

in src/wallet/rpcwallet.cpp:3125 in 4c2b3fc3bf outdated

3123+            "createwallet \"wallet_name\" ( disableprivatekeys )\n"
3124             "\nCreates and loads a new wallet.\n"
3125             "\nArguments:\n"
3126-            "1. \"wallet_name\"    (string, required) The name for the new wallet. If this is a path, the wallet will be created at the path location.\n"
3127+            "1. \"wallet_name\"        (string, required) The name for the new wallet. If this is a path, the wallet will be created at the path location.\n"
3128+            "2. disableprivatekeys   (boolean, optional, default: false) Disable the possibility of private keys (only watchonlys are possible in this mode).\n"

jnewbery commented at 7:48 pm on June 20, 2018:

nit: rpc argument names should be in lower_case_with_underscores (https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#rpc-interface-guidelines). I think this should be disable_private_keys.

jnewbery commented at 8:06 pm on June 20, 2018: member

I tested 4c2b3fc3b and it looks great. A few nits inline.

jonasschnelli force-pushed on Jun 21, 2018

jonasschnelli commented at 2:11 pm on June 21, 2018: contributor

Fixed remaining points reported by @jnewbery and @kallewoof

in src/wallet/walletdb.cpp:585 in 81d45b501e outdated

584-                else
585-                {
586+                } else if(strType == "flags") {
587+                    // reading the wallet flags can only fail if unknown flags are present
588+                    result = DBErrors::TOO_NEW;
589+                }

jnewbery commented at 2:17 pm on June 22, 2018:

nit: } else {

in src/wallet/walletdb.cpp:520 in 81d45b501e outdated

516+            ssValue >> flags;
517+            if (!pwallet->SetWalletFlags(flags, true)) {
518+                strErr = "Error reading wallet database: Unknown non-tolerable wallet flags found";
519+                return false;
520+            }
521+        }

jnewbery commented at 2:18 pm on June 22, 2018:

nit: } else if (...) {

in src/wallet/rpcwallet.cpp:3155 in 81d45b501e outdated

3151@@ -3132,7 +3152,7 @@ UniValue createwallet(const JSONRPCRequest& request)
3152         throw JSONRPCError(RPC_WALLET_ERROR, "Wallet file verification failed: " + error);
3153     }
3154 
3155-    std::shared_ptr<CWallet> const wallet = CWallet::CreateWalletFromFile(wallet_name, fs::absolute(wallet_name, GetWalletDir()));
3156+    std::shared_ptr<CWallet> const wallet = CWallet::CreateWalletFromFile(wallet_name, fs::absolute(wallet_name, GetWalletDir()), (disable_privatekeys ? WALLET_FLAG_DISABLE_PRIVATE_KEYS : 0));

jnewbery commented at 2:23 pm on June 22, 2018:

This introduces a new build warning for me:

0wallet/rpcwallet.cpp: In function ‘UniValue createwallet(const JSONRPCRequest&)’:
1wallet/rpcwallet.cpp:3155:152: warning: enumeral and non-enumeral type in conditional expression [-Wextra]
2     std::shared_ptr<CWallet> const wallet = CWallet::CreateWalletFromFile(wallet_name, fs::absolute(wallet_name, GetWalletDir()), (disable_privatekeys ? WALLET_FLAG_DISABLE_PRIVATE_KEYS : 0));
3                                                                                                                                                        ^

in src/wallet/wallet.h:124 in 81d45b501e outdated

119+
120+    // will enforce the rule that the wallet can't contain any private keys (only watch-only/pubkeys)
121+    WALLET_FLAG_DISABLE_PRIVATE_KEYS = (1ULL << 32),
122+};
123+
124+static const uint64_t g_known_wallet_flags = WALLET_FLAG_DISABLE_PRIVATE_KEYS;

jnewbery commented at 2:24 pm on June 22, 2018:

nit: use constexpr

in src/wallet/wallet.cpp:1553 in 81d45b501e outdated

1535+    if ( (overwriteFlags >> 32 | g_known_wallet_flags >> 32) != (g_known_wallet_flags >> 32) ) {
1536+        // contains unknown non-tolerable wallet flags
1537+        return false;
1538+    }
1539+    if (!memonly && !WalletBatch(*database).WriteWalletFlags(m_wallet_flags))
1540+        throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");

jnewbery commented at 2:27 pm on June 22, 2018:

nit: if block should have braces.

in src/wallet/wallet.cpp:4195 in 81d45b501e outdated

4216+            walletInstance->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
4217+        } else {
4218+            // generate a new seed
4219+            CPubKey seed = walletInstance->GenerateNewSeed();
4220+            if (!walletInstance->SetHDSeed(seed))
4221+                throw std::runtime_error(std::string(__func__) + ": Storing HD seed failed");

jnewbery commented at 2:29 pm on June 22, 2018:

nit: if block should use braces

in src/wallet/wallet.cpp:4227 in 81d45b501e outdated

4227             InitError(_("Unable to generate initial keys") += "\n");
4228             return nullptr;
4229         }
4230 
4231         walletInstance->ChainStateFlushed(chainActive.GetLocator());
4232+    } else if ((wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {

jnewbery commented at 2:31 pm on June 22, 2018:

nit: no need for double parens

in src/wallet/wallet.cpp:4234 in 81d45b501e outdated

4234+        InitError(strprintf(_("Error loading %s: Private keys can only be disabled during creation"), walletFile));
4235+        return NULL;
4236+    } else if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
4237+        LOCK(walletInstance->cs_wallet);
4238+        if (!walletInstance->mapKeys.empty() || !walletInstance->mapCryptedKeys.empty()) {
4239+            InitWarning(strprintf(_("Error loading %s: Private keys detected in wallet with disabled private keys"), walletFile));

jnewbery commented at 2:33 pm on June 22, 2018:

nit: change log to:

"Warning: Private keys detected in wallet {%s} with disabled private keys"), walletFile));

(since this is now a warning instead of an error)

in src/wallet/rpcwallet.cpp:2998 in 81d45b501e outdated

3006+            "  \"keypoolsize_hd_internal\": xxxx,   (numeric) how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)\n"
3007+            "  \"unlocked_until\": ttt,             (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n"
3008+            "  \"paytxfee\": x.xxxx,                (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n"
3009+            "  \"hdseedid\": \"<hash160>\"            (string, optional) the Hash160 of the HD seed (only present when HD is enabled)\n"
3010+            "  \"hdmasterkeyid\": \"<hash160>\"       (string, optional) alias for hdseedid retained for backwards-compatibility. Will be removed in V0.18.\n"
3011+            "  \"privatekeys_enabled\": true|false  (boolean) false if privatekeys are disabled for this wallet (enforced watch-only wallet)\n"

jnewbery commented at 2:38 pm on June 22, 2018:

nit: private_keys_enabled

jnewbery commented at 2:38 pm on June 22, 2018: member

A few more nits. Sorry!

jonasschnelli force-pushed on Jun 28, 2018

jonasschnelli commented at 7:15 pm on June 28, 2018: contributor

Fixed @jnewbery’s nits.

jnewbery commented at 10:28 am on July 3, 2018: member

utACK 5caf38f42f34b2aec7440dec752c10856e5d85a7

laanwj commented at 1:13 pm on July 4, 2018: member

utACK 5caf38f42f34b2aec7440dec752c10856e5d85a7

in src/wallet/wallet.cpp:1535 in 5caf38f42f outdated

1530+
1531+bool CWallet::SetWalletFlags(uint64_t overwriteFlags, bool memonly)
1532+{
1533+    LOCK(cs_wallet);
1534+    m_wallet_flags = overwriteFlags;
1535+    if ( (overwriteFlags >> 32 | g_known_wallet_flags >> 32) != (g_known_wallet_flags >> 32) ) {

laanwj commented at 1:21 pm on July 4, 2018:

nit: Code style is a bit funky here.

kallewoof commented at 2:52 pm on July 4, 2018:

Personally think this definitely needs parens to clarify priority, but maybe I’m just easily confused.

jonasschnelli commented at 3:01 pm on July 4, 2018:

Fixed

jonasschnelli force-pushed on Jul 4, 2018

laanwj commented at 2:13 pm on July 5, 2018: member

@jonasschnelli didn’t you accidentally remove the logic so that only changes to the upper 32 bits are checked?

On Wed, Jul 4, 2018 at 5:01 PM, Jonas Schnelli notifications@github.com wrote:

@jonasschnelli commented on this pull request.

In src/wallet/wallet.cpp https://github.com/bitcoin/bitcoin/pull/9662#discussion_r200152951:

LOCK(cs_wallet);
m_wallet_flags |= flags;

if (!WalletBatch(*database).WriteWalletFlags(m_wallet_flags))
   throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");
+} + +bool CWallet::IsWalletFlagSet(uint64_t flag) +{

return (m_wallet_flags & flag); +}

+bool CWallet::SetWalletFlags(uint64_t overwriteFlags, bool memonly) +{

LOCK(cs_wallet);

m_wallet_flags = overwriteFlags;

if ( (overwriteFlags » 32 | g_known_wallet_flags » 32) != (g_known_wallet_flags » 32) ) {

Fixed

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9662#discussion_r200152951, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutoKKghkVC66tkbFR3O1hWX2N0O9Vks5uDNjCgaJpZM4Lz2fK .

Sjors commented at 2:59 pm on July 10, 2018: member

Lightly tested by creating a wallet via RPC, seeing it appear in the GUI, importing a watch-only address and sending some coins to it from a normal wallet. That money doesn’t show up, even after confirmation, until I restart.

After restart, importing a watch-only address and sending money to it, that transaction shows up immediately. @jnewbery said somewhere above:

Named arguments are much preferable. Using named arguments provides type- and error- checking for free, and simplify/flatten the interface

For example, if I typo the an option disableprivtaekeys here, this implementation will silently ignore the option. That’s not possible with named arguments.

disable_private_keys is still positional, which would be painful to change later…

This GUI behavior is confusing - because it suggests the wallet isn’t HD, even though it may or may not be - but I’m fine with addressing it later:

DrahtBot added the label Needs rebase on Jul 12, 2018

Add facility to store wallet flags (64 bits) 9995a602a6

jonasschnelli force-pushed on Jul 12, 2018

jonasschnelli commented at 9:38 am on July 12, 2018: contributor

Rebased and fixed the upper-bits restriction (for the wallet flags) found by @laanwj (https://github.com/bitcoin/bitcoin/pull/9662#issuecomment-402735925).

Sjors commented at 11:21 am on July 12, 2018: member

Unhappy Travis for the Darwin machine:

I these warnings too when locally on macOS:

DrahtBot removed the label Needs rebase on Jul 12, 2018

Add option to disable private keys during internal wallet creation cebefba085

Add disable privatekeys option to createwallet 2f15c2bc20

[Qt] Disable creating receive addresses when private keys are disabled c7b8f343e9

[QA] add createwallet disableprivatekey test 4704e5f074

QA: Fix bug in -usecli logic that converts booleans to non-lowercase strings a3fa4d6a6a

jonasschnelli force-pushed on Jul 12, 2018

jonasschnelli commented at 7:39 pm on July 12, 2018: contributor

Fixed the rebase issue (the warning; requires cs_KeyStore instead of cs_wallet now).

laanwj added this to the milestone 0.17.0 on Jul 19, 2018

laanwj merged this on Jul 20, 2018

laanwj closed this on Jul 20, 2018

laanwj referenced this in commit 6b6e854362 on Jul 20, 2018

jasonbcox referenced this in commit d36def6dea on Nov 22, 2019

jonspock referenced this in commit 545cab0772 on Feb 26, 2020

jonspock referenced this in commit d5478a137d on Feb 27, 2020

jonspock referenced this in commit b9a85a15e2 on Mar 2, 2020

jonspock referenced this in commit 1aac5e469f on Mar 7, 2020

jonspock referenced this in commit 1cc55bd1fe on Mar 14, 2020

jonspock referenced this in commit 9970edc786 on Mar 21, 2020

jonspock referenced this in commit 2371935f06 on Mar 24, 2020

jonspock referenced this in commit 5698563c2e on Apr 6, 2020

jonspock referenced this in commit 34d88f2afe on Apr 8, 2020

jonspock referenced this in commit f648500de0 on Apr 8, 2020

jonspock referenced this in commit 462b1297a6 on Apr 8, 2020

jonspock referenced this in commit eebf39dcb5 on Apr 8, 2020

jonspock referenced this in commit b6894ce94d on Apr 9, 2020

jonspock referenced this in commit 9e3a589a7b on Apr 17, 2020

jonspock referenced this in commit d1de7ac22a on May 23, 2020

jonspock referenced this in commit 77854652a2 on May 25, 2020

jonspock referenced this in commit f55b496522 on Jul 9, 2020

jonspock referenced this in commit e9500aeb7a on Jul 10, 2020

jonspock referenced this in commit ce18076151 on Jul 17, 2020

jonspock referenced this in commit 62df10714f on Jul 17, 2020

jonspock referenced this in commit 53bf489bfb on Jul 20, 2020

jonspock referenced this in commit b9a1c8ff5d on Jul 29, 2020

jonspock referenced this in commit 413b68d3fc on Jul 31, 2020

jonspock referenced this in commit 8802326ebb on Aug 5, 2020

jonspock referenced this in commit ca0774d285 on Aug 6, 2020

jonspock referenced this in commit 7abbc4e12f on Aug 7, 2020

proteanx referenced this in commit 2240b98003 on Aug 8, 2020

UdjinM6 referenced this in commit 6ae41b09a9 on Jun 29, 2021

UdjinM6 referenced this in commit 73a65e7a83 on Jun 29, 2021

UdjinM6 referenced this in commit 545705f40a on Jul 1, 2021

UdjinM6 referenced this in commit 98cba23a48 on Jul 2, 2021

UdjinM6 referenced this in commit 74a717d556 on Jul 2, 2021

MarcoFalke locked this on Sep 8, 2021

Add createwallet “disableprivatekeys” option: a sane mode for watchonly-wallets #9662

@jonasschnelli commented on this pull request.