From 0652ee73ec880a66ec88bde007ee03c0b9d1b074 Mon Sep 17 00:00:00 2001 From: Rob Fielding Date: Thu, 14 Oct 2021 17:32:05 +1300 Subject: [PATCH 1/5] Add size check on meta.key_origin.path Resolves segfault on legacy wallet Log warning when meta.key_origin.path is below expected size --- src/wallet/scriptpubkeyman.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1769429efec..5a9755d32b8 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -382,11 +382,18 @@ std::vector LegacyScriptPubKeyMan::MarkUnusedAddresses(const if (it != mapKeyMetadata.end()){ CKeyMetadata meta = it->second; if (!meta.hd_seed_id.IsNull() && meta.hd_seed_id != m_hd_chain.seed_id) { - bool internal = (meta.key_origin.path[1] & ~BIP32_HARDENED_KEY_LIMIT) != 0; - int64_t index = meta.key_origin.path[2] & ~BIP32_HARDENED_KEY_LIMIT; + if (meta.key_origin.path.size() < 3) { + WalletLogPrintf("%s: Adding inactive seed keys failed, insufficient path size: %d, has_key_origin: %s\n", + __func__, + meta.key_origin.path.size(), + meta.has_key_origin); + } else { + bool internal = (meta.key_origin.path[1] & ~BIP32_HARDENED_KEY_LIMIT) != 0; + int64_t index = meta.key_origin.path[2] & ~BIP32_HARDENED_KEY_LIMIT; - if (!TopUpInactiveHDChain(meta.hd_seed_id, index, internal)) { - WalletLogPrintf("%s: Adding inactive seed keys failed\n", __func__); + if (!TopUpInactiveHDChain(meta.hd_seed_id, index, internal)) { + WalletLogPrintf("%s: Adding inactive seed keys failed\n", __func__); + } } } } From 961b9e4e40019a87eaa11c8a9c3305870f7a6d75 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 18 Oct 2021 14:43:26 -0400 Subject: [PATCH 2/5] wallet: Parse hdKeypath if key_origin is not available When topping up an inactive HD chain, either key_origin will be available and we can use the path given there, or we need to figure out the path from the string hdKeypath. --- src/wallet/scriptpubkeyman.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 5a9755d32b8..7cca5fba20b 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -382,14 +382,22 @@ std::vector LegacyScriptPubKeyMan::MarkUnusedAddresses(const if (it != mapKeyMetadata.end()){ CKeyMetadata meta = it->second; if (!meta.hd_seed_id.IsNull() && meta.hd_seed_id != m_hd_chain.seed_id) { - if (meta.key_origin.path.size() < 3) { - WalletLogPrintf("%s: Adding inactive seed keys failed, insufficient path size: %d, has_key_origin: %s\n", + std::vector path; + if (meta.has_key_origin) { + path = meta.key_origin.path; + } else if (!ParseHDKeypath(meta.hdKeypath, path)) { + WalletLogPrintf("%s: Adding inactive seed keys failed, invalid hdKeypath: %s\n", __func__, - meta.key_origin.path.size(), + meta.hdKeypath); + } + if (path.size() != 3) { + WalletLogPrintf("%s: Adding inactive seed keys failed, invalid path size: %d, has_key_origin: %s\n", + __func__, + path.size(), meta.has_key_origin); } else { - bool internal = (meta.key_origin.path[1] & ~BIP32_HARDENED_KEY_LIMIT) != 0; - int64_t index = meta.key_origin.path[2] & ~BIP32_HARDENED_KEY_LIMIT; + bool internal = (path[1] & ~BIP32_HARDENED_KEY_LIMIT) != 0; + int64_t index = path[2] & ~BIP32_HARDENED_KEY_LIMIT; if (!TopUpInactiveHDChain(meta.hd_seed_id, index, internal)) { WalletLogPrintf("%s: Adding inactive seed keys failed\n", __func__); From 70134eb34f58f0c572e7c3775e292d408f03b5ab Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 18 Oct 2021 18:22:59 -0400 Subject: [PATCH 3/5] wallet: Properly set hd chain counters when loading When build CHDChains out of CKeyMetadata, the chain counters are actually 1 based, not 0 based, so 1 must be added to each index. --- src/wallet/walletdb.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index f392649bd9f..3e99d145d89 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -556,9 +556,9 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, } if (internal) { chain.nVersion = CHDChain::VERSION_HD_CHAIN_SPLIT; - chain.nInternalChainCounter = std::max(chain.nInternalChainCounter, index); + chain.nInternalChainCounter = std::max(chain.nInternalChainCounter, index + 1); } else { - chain.nExternalChainCounter = std::max(chain.nExternalChainCounter, index); + chain.nExternalChainCounter = std::max(chain.nExternalChainCounter, index + 1); } } } else if (strType == DBKeys::WATCHMETA) { From 8077862c5e8a3ed501f0baabc33536eb16922ceb Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 18 Oct 2021 15:27:27 -0400 Subject: [PATCH 4/5] wallet: Refactor TopUp to be able to top up inactive chains too Refactors TopUp so that it also tops up inactive chains. The bulk of TopUp is moved to TopUpChain. CHDChain also has 2 new in memory variables to track its highest used indexes. This is used only for inactive hd chains so that they can be topped up later in the same session (e.g. if the wallet is encrypted and not unlocked at the time of MarkUnusedAddresses). --- src/wallet/scriptpubkeyman.cpp | 122 ++++++++++++++++++--------------- src/wallet/scriptpubkeyman.h | 1 + src/wallet/walletdb.h | 2 + 3 files changed, 69 insertions(+), 56 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 7cca5fba20b..507951134b2 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -321,8 +321,6 @@ bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t i { LOCK(cs_KeyStore); - if (m_storage.IsLocked()) return false; - auto it = m_inactive_hd_chains.find(seed_id); if (it == m_inactive_hd_chains.end()) { return false; @@ -330,27 +328,14 @@ bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t i CHDChain& chain = it->second; - // Top up key pool - int64_t target_size = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1); - - // "size" of the keypools. Not really the size, actually the difference between index and the chain counter - // Since chain counter is 1 based and index is 0 based, one of them needs to be offset by 1. - int64_t kp_size = (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter) - (index + 1); - - // make sure the keypool fits the user-selected target (-keypool) - int64_t missing = std::max(target_size - kp_size, (int64_t) 0); - - if (missing > 0) { - WalletBatch batch(m_storage.GetDatabase()); - for (int64_t i = missing; i > 0; --i) { - GenerateNewKey(batch, chain, internal); - } - if (internal) { - WalletLogPrintf("inactive seed with id %s added %d internal keys\n", HexStr(seed_id), missing); - } else { - WalletLogPrintf("inactive seed with id %s added %d keys\n", HexStr(seed_id), missing); - } + if (internal) { + chain.m_next_internal_index = std::max(chain.m_next_internal_index, index + 1); + } else { + chain.m_next_external_index = std::max(chain.m_next_external_index, index + 1); } + + TopUpChain(chain, 0); + return true; } @@ -1273,47 +1258,72 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize) if (!CanGenerateKeys()) { return false; } - { - LOCK(cs_KeyStore); - if (m_storage.IsLocked()) return false; - - // Top up key pool - unsigned int nTargetSize; - if (kpSize > 0) - nTargetSize = kpSize; - else - nTargetSize = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0); - - // count amount of available keys (internal, external) - // make sure the keypool of external and internal keys fits the user selected target (-keypool) - int64_t missingExternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - (int64_t)setExternalKeyPool.size(), (int64_t) 0); - int64_t missingInternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - (int64_t)setInternalKeyPool.size(), (int64_t) 0); - - if (!IsHDEnabled() || !m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) - { - // don't create extra internal keys - missingInternal = 0; - } - bool internal = false; - WalletBatch batch(m_storage.GetDatabase()); - for (int64_t i = missingInternal + missingExternal; i--;) - { - if (i < missingInternal) { - internal = true; - } - - CPubKey pubkey(GenerateNewKey(batch, m_hd_chain, internal)); - AddKeypoolPubkeyWithDB(pubkey, internal, batch); - } - if (missingInternal + missingExternal > 0) { - WalletLogPrintf("keypool added %d keys (%d internal), size=%u (%u internal)\n", missingInternal + missingExternal, missingInternal, setInternalKeyPool.size() + setExternalKeyPool.size() + set_pre_split_keypool.size(), setInternalKeyPool.size()); + if (!TopUpChain(m_hd_chain, kpSize)) { + return false; + } + for (auto& [chain_id, chain] : m_inactive_hd_chains) { + if (!TopUpChain(chain, kpSize)) { + return false; } } NotifyCanGetAddressesChanged(); return true; } +bool LegacyScriptPubKeyMan::TopUpChain(CHDChain& chain, unsigned int kpSize) +{ + LOCK(cs_KeyStore); + + if (m_storage.IsLocked()) return false; + + // Top up key pool + unsigned int nTargetSize; + if (kpSize > 0) { + nTargetSize = kpSize; + } else { + nTargetSize = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{0}); + } + int64_t target = std::max((int64_t) nTargetSize, int64_t{1}); + + // count amount of available keys (internal, external) + // make sure the keypool of external and internal keys fits the user selected target (-keypool) + int64_t missingExternal; + int64_t missingInternal; + if (chain == m_hd_chain) { + missingExternal = std::max(target - (int64_t)setExternalKeyPool.size(), int64_t{0}); + missingInternal = std::max(target - (int64_t)setInternalKeyPool.size(), int64_t{0}); + } else { + missingExternal = std::max(target - (chain.nExternalChainCounter - chain.m_next_external_index), int64_t{0}); + missingInternal = std::max(target - (chain.nInternalChainCounter - chain.m_next_internal_index), int64_t{0}); + } + + if (!IsHDEnabled() || !m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) { + // don't create extra internal keys + missingInternal = 0; + } + bool internal = false; + WalletBatch batch(m_storage.GetDatabase()); + for (int64_t i = missingInternal + missingExternal; i--;) { + if (i < missingInternal) { + internal = true; + } + + CPubKey pubkey(GenerateNewKey(batch, chain, internal)); + if (chain == m_hd_chain) { + AddKeypoolPubkeyWithDB(pubkey, internal, batch); + } + } + if (missingInternal + missingExternal > 0) { + if (chain == m_hd_chain) { + WalletLogPrintf("keypool added %d keys (%d internal), size=%u (%u internal)\n", missingInternal + missingExternal, missingInternal, setInternalKeyPool.size() + setExternalKeyPool.size() + set_pre_split_keypool.size(), setInternalKeyPool.size()); + } else { + WalletLogPrintf("inactive seed with id %s added %d external keys, %d internal keys\n", HexStr(chain.seed_id), missingExternal, missingInternal); + } + } + return true; +} + void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const bool internal, WalletBatch& batch) { LOCK(cs_KeyStore); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index ebe064fa0a0..e97f0bb40c4 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -354,6 +354,7 @@ private: */ bool TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal); + bool TopUpChain(CHDChain& chain, unsigned int size); public: using ScriptPubKeyMan::ScriptPubKeyMan; diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 9c752623b3b..428f3d36ec0 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -90,6 +90,8 @@ public: uint32_t nExternalChainCounter; uint32_t nInternalChainCounter; CKeyID seed_id; //!< seed hash160 + int64_t m_next_external_index{0}; // Next index in the keypool to be used. Memory only. + int64_t m_next_internal_index{0}; // Next index in the keypool to be used. Memory only. static const int VERSION_HD_BASE = 1; static const int VERSION_HD_CHAIN_SPLIT = 2; From c4d76c6faa3adf06f192649e169ca860ce420d30 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 18 Oct 2021 16:24:24 -0400 Subject: [PATCH 5/5] tests: Tests for inactive HD chains test cases are added for inactive HD chains: a basic case, a case where the wallet is encrypted, and a case for the 21605 segfault. --- test/functional/test_framework/test_node.py | 3 + test/functional/test_runner.py | 1 + test/functional/wallet_inactive_hdchains.py | 147 ++++++++++++++++++++ 3 files changed, 151 insertions(+) create mode 100755 test/functional/wallet_inactive_hdchains.py diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index b3279666b20..e3ceb0b1186 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -698,6 +698,9 @@ class RPCOverloadWrapper(): def __getattr__(self, name): return getattr(self.rpc, name) + def createwallet_passthrough(self, *args, **kwargs): + return self.__getattr__("createwallet")(*args, **kwargs) + def createwallet(self, wallet_name, disable_private_keys=None, blank=None, passphrase='', avoid_reuse=None, descriptors=None, load_on_startup=None, external_signer=None): if descriptors is None: descriptors = self.descriptors diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 0a764a17556..0cd0c7c3a5d 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -279,6 +279,7 @@ BASE_SCRIPTS = [ 'wallet_send.py --descriptors', 'wallet_create_tx.py --descriptors', 'wallet_taproot.py', + 'wallet_inactive_hdchains.py', 'p2p_fingerprint.py', 'feature_uacomment.py', 'feature_init.py', diff --git a/test/functional/wallet_inactive_hdchains.py b/test/functional/wallet_inactive_hdchains.py new file mode 100755 index 00000000000..e1dad008764 --- /dev/null +++ b/test/functional/wallet_inactive_hdchains.py @@ -0,0 +1,147 @@ +#!/usr/bin/env python3 +# Copyright (c) 2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +Test Inactive HD Chains. +""" +import os +import shutil +import time + +from test_framework.authproxy import JSONRPCException +from test_framework.test_framework import BitcoinTestFramework +from test_framework.wallet_util import ( + get_generate_key, +) + + +class InactiveHDChainsTest(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 2 + self.extra_args = [["-keypool=10"], ["-nowallet", "-keypool=10"]] + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + self.skip_if_no_bdb() + self.skip_if_no_previous_releases() + + def setup_nodes(self): + self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[ + None, + 170200, # 0.17.2 Does not have the key metadata upgrade + ]) + + self.start_nodes() + self.init_wallet(node=0) + + def prepare_wallets(self, wallet_basename, encrypt=False): + self.nodes[0].createwallet(wallet_name=f"{wallet_basename}_base", descriptors=False, blank=True) + self.nodes[0].createwallet(wallet_name=f"{wallet_basename}_test", descriptors=False, blank=True) + base_wallet = self.nodes[0].get_wallet_rpc(f"{wallet_basename}_base") + test_wallet = self.nodes[0].get_wallet_rpc(f"{wallet_basename}_test") + + # Setup both wallets with the same HD seed + seed = get_generate_key() + base_wallet.sethdseed(True, seed.privkey) + test_wallet.sethdseed(True, seed.privkey) + + if encrypt: + # Encrypting will generate a new HD seed and flush the keypool + test_wallet.encryptwallet("pass") + else: + # Generate a new HD seed on the test wallet + test_wallet.sethdseed() + + return base_wallet, test_wallet + + def do_inactive_test(self, base_wallet, test_wallet, encrypt=False): + default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + # The first address should be known by both wallets. + addr1 = base_wallet.getnewaddress() + assert test_wallet.getaddressinfo(addr1)["ismine"] + # The address at index 9 is the first address that the test wallet will not know initially + for _ in range(0, 9): + base_wallet.getnewaddress() + addr2 = base_wallet.getnewaddress() + assert not test_wallet.getaddressinfo(addr2)["ismine"] + + # Send to first address on the old seed + txid = default.sendtoaddress(addr1, 10) + self.generate(self.nodes[0], 1) + + # Wait for the test wallet to see the transaction + while True: + try: + test_wallet.gettransaction(txid) + break + except JSONRPCException: + time.sleep(0.1) + + if encrypt: + # The test wallet will not be able to generate the topped up keypool + # until it is unlocked. So it still should not know about the second address + assert not test_wallet.getaddressinfo(addr2)["ismine"] + test_wallet.walletpassphrase("pass", 1) + + # The test wallet should now know about the second address as it + # should have generated it in the inactive chain's keypool + assert test_wallet.getaddressinfo(addr2)["ismine"] + + # Send to second address on the old seed + txid = default.sendtoaddress(addr2, 10) + self.generate(self.nodes[0], 1) + test_wallet.gettransaction(txid) + + def test_basic(self): + self.log.info("Test basic case for inactive HD chains") + self.do_inactive_test(*self.prepare_wallets("basic")) + + def test_encrypted_wallet(self): + self.log.info("Test inactive HD chains when wallet is encrypted") + self.do_inactive_test(*self.prepare_wallets("enc", encrypt=True), encrypt=True) + + def test_without_upgraded_keymeta(self): + # Test that it is possible to top up inactive hd chains even if there is no key origin + # in CKeyMetadata. This tests for the segfault reported in + # https://github.com/bitcoin/bitcoin/issues/21605 + self.log.info("Test that topping up inactive HD chains does not need upgraded key origin") + + self.nodes[0].createwallet(wallet_name="keymeta_base", descriptors=False, blank=True) + # Createwallet is overridden in the test framework so that the descriptor option can be filled + # depending on the test's cli args. However we don't want to do that when using old nodes that + # do not support descriptors. So we use the createwallet_passthrough function. + self.nodes[1].createwallet_passthrough(wallet_name="keymeta_test") + base_wallet = self.nodes[0].get_wallet_rpc("keymeta_base") + test_wallet = self.nodes[1].get_wallet_rpc("keymeta_test") + + # Setup both wallets with the same HD seed + seed = get_generate_key() + base_wallet.sethdseed(True, seed.privkey) + test_wallet.sethdseed(True, seed.privkey) + + # Encrypting will generate a new HD seed and flush the keypool + test_wallet.encryptwallet("pass") + + # Copy test wallet to node 0 + test_wallet.unloadwallet() + test_wallet_dir = os.path.join(self.nodes[1].datadir, "regtest/wallets/keymeta_test") + new_test_wallet_dir = os.path.join(self.nodes[0].datadir, "regtest/wallets/keymeta_test") + shutil.copytree(test_wallet_dir, new_test_wallet_dir) + self.nodes[0].loadwallet("keymeta_test") + test_wallet = self.nodes[0].get_wallet_rpc("keymeta_test") + + self.do_inactive_test(base_wallet, test_wallet, encrypt=True) + + def run_test(self): + self.generate(self.nodes[0], 101) + + self.test_basic() + self.test_encrypted_wallet() + self.test_without_upgraded_keymeta() + + +if __name__ == '__main__': + InactiveHDChainsTest().main()