Add `-disablehot` mode: a sane mode for watchonly-wallets #9662

Open
wants to merge 6 commits into
from
@@ -93,6 +93,10 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)
SLOT(recentRequestsView_selectionChanged(QItemSelection, QItemSelection)));
// Last 2 columns are set by the columnResizingFixer, when the table geometry is ready.
columnResizingFixer = new GUIUtil::TableViewLastColumnResizingFixer(tableView, AMOUNT_MINIMUM_COLUMN_WIDTH, DATE_COLUMN_WIDTH, this);
+
+ if (_model->hotKeysDisabled()) {
+ ui->receiveButton->setEnabled(false);
+ }
}
}
View
@@ -703,6 +703,11 @@ bool WalletModel::hdEnabled() const
return wallet->IsHDEnabled();
}
+bool WalletModel::hotKeysDisabled() const
+{
+ return (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS));
+}
+
int WalletModel::getDefaultConfirmTarget() const
{
return nTxConfirmTarget;
View
@@ -210,6 +210,7 @@ class WalletModel : public QObject
static bool isWalletEnabled();
bool hdEnabled() const;
+ bool hotKeysDisabled() const;
int getDefaultConfirmTarget() const;
View
@@ -102,6 +102,10 @@ UniValue importprivkey(const JSONRPCRequest& request)
);
+ if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
+ throw JSONRPCError(RPC_WALLET_ERROR, "Error: Hot keys are disabled (-disablehot)");
+ }
+
LOCK2(cs_main, pwallet->cs_wallet);
EnsureWalletIsUnlocked(pwallet);
@@ -470,6 +474,10 @@ UniValue importwallet(const JSONRPCRequest& request)
if (fPruneMode)
throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled in pruned mode");
+ if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
+ throw JSONRPCError(RPC_WALLET_ERROR, "Error: Hot keys are disabled (-disablehot)");
+ }
+
LOCK2(cs_main, pwallet->cs_wallet);
EnsureWalletIsUnlocked(pwallet);
View
@@ -129,6 +129,10 @@ UniValue getnewaddress(const JSONRPCRequest& request)
+ HelpExampleRpc("getnewaddress", "")
);
+ if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
+ throw JSONRPCError(RPC_WALLET_ERROR, "Error: Hot keys are disabled (-disablehot)");
+ }
+
LOCK2(cs_main, pwallet->cs_wallet);
// Parse the account first so we don't generate a key if there's an error
@@ -216,6 +220,10 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request)
+ HelpExampleRpc("getrawchangeaddress", "")
);
+ if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
+ throw JSONRPCError(RPC_WALLET_ERROR, "Error: Hot keys are disabled (-disablehot)");
+ }
+
LOCK2(cs_main, pwallet->cs_wallet);
if (!pwallet->IsLocked()) {
@@ -1960,6 +1968,10 @@ UniValue keypoolrefill(const JSONRPCRequest& request)
+ HelpExampleRpc("keypoolrefill", "")
);
+ if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
+ throw JSONRPCError(RPC_WALLET_ERROR, "Error: Hot keys are disabled (-disablehot)");
+ }
+
LOCK2(cs_main, pwallet->cs_wallet);
// 0 is interpreted by TopUpKeyPool() as the default keypool size given by -keypool
View
@@ -89,6 +89,7 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const
CPubKey CWallet::GenerateNewKey(bool internal)
{
+ assert(!IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS));
AssertLockHeld(cs_wallet); // mapKeyMetadata
bool fCompressed = CanSupportFeature(FEATURE_COMPRPUBKEY); // default to compressed public keys if we want 0.6.0 wallets
@@ -1293,6 +1294,7 @@ CAmount CWallet::GetChange(const CTransaction& tx) const
CPubKey CWallet::GenerateNewHDMasterKey()
{
+ assert(!IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS));
CKey key;
key.MakeNewKey(true);
@@ -1350,6 +1352,25 @@ bool CWallet::IsHDEnabled() const
return !hdChain.masterKeyID.IsNull();
}
+void CWallet::AddWalletFlag(uint64_t flags)
+{
+ walletFlags |= flags;
+ if (!CWalletDB(*dbw).WriteWalletFlags(walletFlags))
+ throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");
+}
+
+bool CWallet::IsWalletFlagSet(uint64_t flag)
+{
+ return (walletFlags & flag);
+}
+
+void CWallet::SetWalletFlags(uint64_t overwriteFlags, bool memonly)
+{
+ walletFlags = overwriteFlags;
+ if (!memonly && !CWalletDB(*dbw).WriteWalletFlags(walletFlags))
+ throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");
+}
+
int64_t CWalletTx::GetTxTime() const
{
int64_t n = nTimeSmart;
@@ -2497,6 +2518,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
// post-backup change.
// Reserve a new key pair from key pool
+ if (IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
+ strFailReason = _("Can't generate a change-address key. Hot keys are disabled for this wallet (-disablehot).");
+ return false;
+ }
CPubKey vchPubKey;
bool ret;
ret = reservekey.GetReservedKey(vchPubKey, true);
@@ -2814,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

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

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);
+ fFirstRunRet = false;
+ }
uiInterface.LoadWallet(this);
@@ -2938,6 +2966,9 @@ bool CWallet::SetDefaultKey(const CPubKey &vchPubKey)
*/
bool CWallet::NewKeyPool()
{
+ if (IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
@instagibbs

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?

+ return false;
+ }
{
LOCK(cs_wallet);
CWalletDB walletdb(*dbw);
@@ -2978,6 +3009,9 @@ size_t CWallet::KeypoolCountExternalKeys()
bool CWallet::TopUpKeyPool(unsigned int kpSize)
{
+ if (IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
+ return false;
+ }
{
LOCK(cs_wallet);
@@ -3078,6 +3112,10 @@ void CWallet::ReturnKey(int64_t nIndex)
bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
{
+ if (IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
+ return false;
+ }
+
int64_t nIndex = 0;
CKeyPool keypool;
{
@@ -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));
@jnewbery

jnewbery Jul 10, 2017

Member

nit: place alphabetically (above -disablewallet)

strUsage += HelpMessageOpt("-keypool=<n>", strprintf(_("Set key pool size to <n> (default: %u)"), DEFAULT_KEYPOOL_SIZE));
strUsage += HelpMessageOpt("-fallbackfee=<amt>", strprintf(_("A fee rate (in %s/kB) that will be used when fee estimation has insufficient data (default: %s)"),
CURRENCY_UNIT, FormatMoney(DEFAULT_FALLBACK_FEE)));
@@ -3698,6 +3737,9 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
if (!walletInstance->SetHDMasterKey(masterPubKey))
throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
}
+ if (GetBoolArg("-disablehot", DEFAULT_DISABLE_HOT_WALLET)) {
+ walletInstance->AddWalletFlag(WALLET_FLAG_DISABLE_HOT_KEYS);
+ }
CPubKey newDefaultKey;
if (walletInstance->GetKeyFromPool(newDefaultKey, false)) {
walletInstance->SetDefaultKey(newDefaultKey);
@@ -3709,6 +3751,27 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
walletInstance->SetBestChain(chainActive.GetLocator());
}
+ else if (IsArgSet("-disablehot")) {
+ bool disableHot = GetBoolArg("-disablehot", DEFAULT_DISABLE_HOT_WALLET);
+ LOCK(walletInstance->cs_wallet);
+ if (walletInstance->vchDefaultKey.IsValid() && disableHot) {
@NicolasDorier

NicolasDorier Mar 13, 2017

Member

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

@jonasschnelli

jonasschnelli Mar 13, 2017

Member

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

@NicolasDorier

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

NicolasDorier Mar 13, 2017

Member

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

+ InitError(strprintf(_("Error loading %s: You can't disable hot keys if your wallet already contains hot keys"), walletFile));
+ return NULL;
+ }
+ 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 (use -disablehot instead)"), walletFile));
+ return NULL;
+ }
+ }
+ else if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_HOT_KEYS)) {
+ 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

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.

+ return NULL;
+ }
+ }
else if (IsArgSet("-usehd")) {
bool useHD = GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET);
if (walletInstance->IsHDEnabled() && !useHD) {
@@ -3839,6 +3902,10 @@ bool CWallet::ParameterInteraction()
if (GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET))
return true;
+ if (GetBoolArg("-disablehot", DEFAULT_DISABLE_HOT_WALLET) && SoftSetBoolArg("-usehd", false)) {
+ LogPrintf("%s: parameter interaction: -disablehot=1 -> setting -usehd=0\n", __func__);
+ }
+
if (GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY) && SoftSetBoolArg("-walletbroadcast", false)) {
LogPrintf("%s: parameter interaction: -blocksonly=1 -> setting -walletbroadcast=0\n", __func__);
}
View
@@ -61,6 +61,7 @@ static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 6;
static const bool DEFAULT_WALLET_RBF = false;
static const bool DEFAULT_WALLETBROADCAST = true;
static const bool DEFAULT_DISABLE_WALLET = false;
+static const bool DEFAULT_DISABLE_HOT_WALLET = false;
//! if set, all keys will be derived by using BIP32
static const bool DEFAULT_USE_HD_WALLET = true;
@@ -91,6 +92,10 @@ enum WalletFeature
FEATURE_LATEST = FEATURE_COMPRPUBKEY // HD is optional, use FEATURE_COMPRPUBKEY as latest version
};
+enum WalletFlags : uint64_t {
+ // will enforce the rule that the wallet can't contain any private keys (only watch-only/pubkeys)
+ WALLET_FLAG_DISABLE_HOT_KEYS = (1 << 0),
+};
/** A key pool entry */
class CKeyPool
@@ -696,6 +701,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
/* HD derive new child key (on internal or external chain) */
void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal = false);
+ std::atomic<uint64_t> walletFlags;
+
std::set<int64_t> setKeyPool;
int64_t nTimeFirstKey;
@@ -791,6 +798,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
nRelockTime = 0;
fAbortRescan = false;
fScanningWallet = false;
+ walletFlags = 0;
}
std::map<uint256, CWalletTx> mapWallet;
@@ -1104,6 +1112,15 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
caller must ensure the current wallet version is correct before calling
this function). */
bool SetHDMasterKey(const CPubKey& key);
+
+ /* set a single wallet flag */
+ void AddWalletFlag(uint64_t flags);
+
+ /* check if a certain wallet flag is set */
+ bool IsWalletFlagSet(uint64_t flag);
+
+ /* overwrite all flags by the given unit64_t */
+ void SetWalletFlags(uint64_t overwriteFlags, bool memOnly);
};
/** A key allocated from the key pool. */
View
@@ -537,6 +537,12 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
return false;
}
}
+ else if (strType == "flags")
+ {
+ uint64_t flags;
+ ssValue >> flags;
+ pwallet->SetWalletFlags(flags, true);
+ }
} catch (...)
{
return false;
@@ -871,6 +877,12 @@ bool CWalletDB::WriteHDChain(const CHDChain& chain)
return batch.Write(std::string("hdchain"), chain);
}
+bool CWalletDB::WriteWalletFlags(const uint64_t flags)
+{
+ nWalletDBUpdateCounter++;
+ return batch.Write(std::string("flags"), flags);
+}
+
void CWalletDB::IncrementUpdateCounter()
{
nWalletDBUpdateCounter++;
View
@@ -212,6 +212,7 @@ class CWalletDB
//! write the hdchain model (external chain child index counter)
bool WriteHDChain(const CHDChain& chain);
+ bool WriteWalletFlags(const uint64_t flags);
static void IncrementUpdateCounter();
static unsigned int GetUpdateCounter();
@@ -0,0 +1,67 @@
+#!/usr/bin/env python3
@jnewbery

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

jnewbery Jul 10, 2017

Member

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

+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+
+class DisableHotKeysTest(BitcoinTestFramework):
+
+ def __init__(self):
+ super().__init__()
+ self.setup_clean_chain = True
+ self.num_nodes = 2
+
+ def setup_network(self, split=False):
@jnewbery

jnewbery Jul 10, 2017

Member

You can avoid having to override this method by setting:

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

in the __init__() method

+ self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, [[], ['-disablehot']])
+
+ connect_nodes_bi(self.nodes,0,1)
+
+ self.is_network_split=False
+ self.sync_all()
+
+ def run_test(self):
+ assert_equal(self.nodes[1].getwalletinfo()['keypoolsize'], 0)
+
+ print("Mining blocks...")
@jnewbery

jnewbery Jul 10, 2017

Member

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

+ self.sync_all()
+ self.nodes[0].generate(101)
+ self.sync_all()
+
+ n0addrR = self.nodes[0].getnewaddress()
+ n0pubkR = self.nodes[0].validateaddress(n0addrR)['pubkey']
+ n0addrC = self.nodes[0].getnewaddress()
+ n0pubkC = self.nodes[0].validateaddress(n0addrC)['pubkey']
+ n0addr = self.nodes[0].getnewaddress()
+ self.nodes[0].sendtoaddress(n0addrR, 10);
@jnewbery

jnewbery Jul 10, 2017

Member

no ; please 🙂

+ self.nodes[0].generate(6)
+ self.sync_all()
+
+ assert_raises_jsonrpc(-4,"Error: Hot keys are disabled (-disablehot)", self.nodes[1].getnewaddress)
+ assert_raises_jsonrpc(-4,"Error: Hot keys are disabled (-disablehot)", self.nodes[1].getrawchangeaddress)
+ assert_raises_jsonrpc(-4,"Error: Hot keys are disabled (-disablehot)", self.nodes[1].importprivkey, "92e6XLo5jVAVwrQKPNTs93oQco8f8sDNBcpv73Dsrs397fQtFQn")
+ assert_raises_jsonrpc(-4,"Error: Hot keys are disabled (-disablehot)", self.nodes[1].importwallet, "dummy")
+
+ self.nodes[1].importpubkey(n0pubkR) #TODO switch to importmulti
+ self.nodes[1].importpubkey(n0pubkC)
+ assert_equal(self.nodes[1].getbalance("*", 1, True), 10)
+ rawtx = self.nodes[1].createrawtransaction([], {n0addr: 1.0})
+ frawtx= self.nodes[1].fundrawtransaction(rawtx, {"changeAddress": n0addrC, "includeWatching": True})
+
+ # check if we can re-enable private keys
+ assert_equal(self.nodes[1].signrawtransaction(frawtx['hex'])['complete'], False)
+ print("restarting node with disablehot=0 (try to re-enable private keys which must not be possible)")
+ stop_nodes(self.nodes)
+ assert_start_raises_init_error(1, self.options.tmpdir, ['-disablehot=0'], 'can\'t enable hot keys')
+
+ #remove wallet
+ os.remove(self.options.tmpdir + "/node1/regtest/wallet.dat")
+ print("start node, create normal private key wallet")
+ self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, [[], ['-disablehot=0']])
+ stop_nodes(self.nodes)
+ assert_start_raises_init_error(1, self.options.tmpdir, ['-disablehot=1'], 'You can\'t disable hot keys')
+ self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, [[], ['-disablehot=0']])
+
+if __name__ == '__main__':
+ DisableHotKeysTest().main()
@@ -110,6 +110,7 @@
'rpcnamedargs.py',
'listsinceblock.py',
'p2p-leaktests.py',
+ 'disablehot.py',
]
EXTENDED_SCRIPTS = [