diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index c939ebb1fd4..b865a7a7bb9 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -542,7 +542,7 @@ static LoadResult LoadRecords(CWallet* pwallet, DatabaseBatch& batch, const std: return LoadRecords(pwallet, batch, key, prefix, load_func); } -static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, int last_client) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) +static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, WalletBatch& batch, int last_client) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) { AssertLockHeld(pwallet->cs_wallet); DBErrors result = DBErrors::LOAD_OK; @@ -555,7 +555,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, DataStream prefix; prefix << type; - std::unique_ptr cursor = batch.GetNewPrefixCursor(prefix); + std::unique_ptr cursor = batch.m_batch->GetNewPrefixCursor(prefix); if (!cursor) { pwallet->WalletLogPrintf("Error getting database cursor for '%s' records\n", type); return DBErrors::CORRUPT; @@ -573,28 +573,28 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, // Load HD Chain // Note: There should only be one HDCHAIN record with no data following the type - LoadResult hd_chain_res = LoadRecords(pwallet, batch, DBKeys::HDCHAIN, + LoadResult hd_chain_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::HDCHAIN, [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { return LoadHDChain(pwallet, value, err) ? DBErrors:: LOAD_OK : DBErrors::CORRUPT; }); result = std::max(result, hd_chain_res.m_result); // Load unencrypted keys - LoadResult key_res = LoadRecords(pwallet, batch, DBKeys::KEY, + LoadResult key_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::KEY, [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { return LoadKey(pwallet, key, value, err) ? DBErrors::LOAD_OK : DBErrors::CORRUPT; }); result = std::max(result, key_res.m_result); // Load encrypted keys - LoadResult ckey_res = LoadRecords(pwallet, batch, DBKeys::CRYPTED_KEY, + LoadResult ckey_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::CRYPTED_KEY, [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { return LoadCryptedKey(pwallet, key, value, err) ? DBErrors::LOAD_OK : DBErrors::CORRUPT; }); result = std::max(result, ckey_res.m_result); // Load scripts - LoadResult script_res = LoadRecords(pwallet, batch, DBKeys::CSCRIPT, + LoadResult script_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::CSCRIPT, [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& strErr) { uint160 hash; key >> hash; @@ -617,13 +617,13 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, // Load keymeta std::map hd_chains; - LoadResult keymeta_res = LoadRecords(pwallet, batch, DBKeys::KEYMETA, - [&hd_chains] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& strErr) { + std::vector updated_metas; + LoadResult keymeta_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::KEYMETA, + [&hd_chains, &updated_metas] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& strErr) { CPubKey vchPubKey; key >> vchPubKey; CKeyMetadata keyMeta; value >> keyMeta; - pwallet->GetOrCreateLegacyDataSPKM()->LoadKeyMetadata(vchPubKey.GetID(), keyMeta); // Extract some CHDChain info from this metadata if it has any if (keyMeta.nVersion >= CKeyMetadata::VERSION_WITH_HDDATA && !keyMeta.hd_seed_id.IsNull() && keyMeta.hdKeypath.size() > 0) { @@ -634,13 +634,32 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, uint32_t index = 0; if (keyMeta.hdKeypath != "s" && keyMeta.hdKeypath != "m") { std::vector path; - if (keyMeta.has_key_origin) { - // We have a key origin, so pull it from its path vector - path = keyMeta.key_origin.path; - } else { - // No key origin, have to parse the string - if (!ParseHDKeypath(keyMeta.hdKeypath, path)) { - strErr = "Error reading wallet database: keymeta with invalid HD keypath"; + // Always parse the path string + if (!ParseHDKeypath(keyMeta.hdKeypath, path)) { + strErr = "Error reading wallet database: keymeta with invalid HD keypath"; + return DBErrors::NONCRITICAL_ERROR; + } + // If there is a key origin, make sure that path matches + if (keyMeta.has_key_origin && path != keyMeta.key_origin.path) { + // Detect if this metadata has a bad derivation path from a bug in inactive hd key derivation that was present in 0.21 and 22.x + // See https://github.com/bitcoin/bitcoin/issues/29109 + // Markers for bad derivation: + // - 6 indexes + // - path[5] == path[2] + 1 + // - path[0:2] == path[3:5] + // - path[3:6] matches hdKeypath + if (keyMeta.key_origin.path.size() == 6 + && keyMeta.key_origin.path[5] == keyMeta.key_origin.path[2] + 1 + && keyMeta.key_origin.path[0] == keyMeta.key_origin.path[3] + && keyMeta.key_origin.path[1] == keyMeta.key_origin.path[4] + && std::equal(keyMeta.key_origin.path.begin() + 3, keyMeta.key_origin.path.end(), path.begin(), path.end()) + ) { + // Matches the pattern, just reset it to the parsed path + pwallet->WalletLogPrintf("Repairing derivation path for %s\n", HexStr(vchPubKey)); + keyMeta.key_origin.path = path; + updated_metas.push_back(vchPubKey); + } else { + strErr = "keymeta with mismatched hdkeypath string and key origin derivation path"; return DBErrors::NONCRITICAL_ERROR; } } @@ -684,10 +703,26 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, chain.nExternalChainCounter = std::max(chain.nExternalChainCounter, index + 1); } } + + pwallet->GetOrCreateLegacyDataSPKM()->LoadKeyMetadata(vchPubKey.GetID(), keyMeta); + return DBErrors::LOAD_OK; }); result = std::max(result, keymeta_res.m_result); + // Update any changed metadata + if (!updated_metas.empty()) { + LegacyDataSPKM* spkm = pwallet->GetLegacyDataSPKM(); + Assert(spkm); + LOCK(spkm->cs_KeyStore); + for (const auto& pubkey : updated_metas) { + if (!batch.WriteKeyMetadata(spkm->mapKeyMetadata.at(pubkey.GetID()), pubkey, true)) { + pwallet->WalletLogPrintf("Unable to write repaired keymeta for %s\n", HexStr(pubkey)); + result = std::max(result, DBErrors::NONCRITICAL_ERROR); + } + } + } + // Set inactive chains if (!hd_chains.empty()) { LegacyDataSPKM* legacy_spkm = pwallet->GetLegacyDataSPKM(); @@ -704,7 +739,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, } // Load watchonly scripts - LoadResult watch_script_res = LoadRecords(pwallet, batch, DBKeys::WATCHS, + LoadResult watch_script_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::WATCHS, [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { CScript script; key >> script; @@ -718,7 +753,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, result = std::max(result, watch_script_res.m_result); // Load watchonly meta - LoadResult watch_meta_res = LoadRecords(pwallet, batch, DBKeys::WATCHMETA, + LoadResult watch_meta_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::WATCHMETA, [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { CScript script; key >> script; @@ -730,7 +765,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, result = std::max(result, watch_meta_res.m_result); // Load keypool - LoadResult pool_res = LoadRecords(pwallet, batch, DBKeys::POOL, + LoadResult pool_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::POOL, [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { int64_t nIndex; key >> nIndex; @@ -747,7 +782,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, // We don't want or need the default key, but if there is one set, // we want to make sure that it is valid so that we can detect corruption // Note: There should only be one DEFAULTKEY with nothing trailing the type - LoadResult default_key_res = LoadRecords(pwallet, batch, DBKeys::DEFAULTKEY, + LoadResult default_key_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::DEFAULTKEY, [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { CPubKey default_pubkey; try { @@ -765,7 +800,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, result = std::max(result, default_key_res.m_result); // "wkey" records are unsupported, if we see any, throw an error - LoadResult wkey_res = LoadRecords(pwallet, batch, DBKeys::OLD_KEY, + LoadResult wkey_res = LoadRecords(pwallet, *batch.m_batch, DBKeys::OLD_KEY, [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { err = "Found unsupported 'wkey' record, try loading with version 0.18"; return DBErrors::LOAD_FAIL; @@ -1198,7 +1233,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) #endif // Load legacy wallet keys - result = std::max(LoadLegacyWalletRecords(pwallet, *m_batch, last_client), result); + result = std::max(LoadLegacyWalletRecords(pwallet, *this, last_client), result); // Load descriptors result = std::max(LoadDescriptorWalletRecords(pwallet, *m_batch, last_client), result); diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 70d69870126..bb8fb59bd35 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -303,8 +303,8 @@ public: //! Registers db txn callback functions void RegisterTxnListener(const DbTxnListener& l); -private: std::unique_ptr m_batch; +private: WalletDatabase& m_database; // External functions listening to the current db txn outcome. diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py index 62db32d806a..0a3506ec2af 100755 --- a/test/functional/wallet_backwards_compatibility.py +++ b/test/functional/wallet_backwards_compatibility.py @@ -41,7 +41,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework): ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v25.0 ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v24.0.1 ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v23.0 - ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v22.0 + ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1", "-keypool=10"], # v22.0 ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v0.21.0 ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v0.20.1 ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v0.19.1 @@ -128,6 +128,79 @@ class BackwardsCompatibilityTest(BitcoinTestFramework): wallet = node_v19.get_wallet_rpc("w1_v19") assert wallet.getaddressinfo(address_18075)["solvable"] + + def test_v22_inactivehdchain_path(self): + LAST_KEYPOOL_INDEX = 9 # Index of the last derived address with the keypool size of 10 + + self.log.info("Testing inactive hd chain bad derivation path cleanup") + # 0.21.x and 22.x would both produce bad derivation paths when topping up an inactive hd chain + # Make sure that this is being detected and automatically cleaned up + node_master = self.nodes[1] + node_v22 = self.nodes[self.num_nodes - 6] + wallet_name = "bad_deriv_path" + node_v22.createwallet(wallet_name=wallet_name, descriptors=False) + wallet = node_v22.get_wallet_rpc(wallet_name) + + # Make a dump of the wallet to get an unused address + dump_path = node_v22.wallets_path / f"{wallet_name}.dump" + wallet.dumpwallet(dump_path) + addr = None + seed = None + with open(dump_path, encoding="utf8") as f: + for line in f: + if f"hdkeypath=m/0'/0'/{LAST_KEYPOOL_INDEX}'" in line: + addr = line.split(" ")[4].split("=")[1] + elif " hdseed=1 " in line: + seed = line.split(" ")[0] + assert addr is not None + assert seed is not None + # Rotate seed and unload + wallet.sethdseed() + wallet.unloadwallet() + # Receive at addr to trigger inactive chain topup on next load + self.nodes[0].sendtoaddress(addr, 1) + self.generate(self.nodes[0], 1, sync_fun=self.no_op) + self.sync_all(nodes=[self.nodes[0], node_master, node_v22]) + node_v22.loadwallet(wallet_name) + + # Dump again to find bad hd keypath + bad_deriv_path = f"m/0'/0'/{LAST_KEYPOOL_INDEX}'/0'/0'/{LAST_KEYPOOL_INDEX + 1}'" + good_deriv_path = f"m/0h/0h/{LAST_KEYPOOL_INDEX + 1}h" + os.unlink(dump_path) + wallet.dumpwallet(dump_path) + bad_path_addr = None + with open(dump_path, encoding="utf8") as f: + for line in f: + if f"hdkeypath={bad_deriv_path}" in line: + bad_path_addr = line.split(" ")[4].split("=")[1] + assert bad_path_addr is not None + assert_equal(wallet.getaddressinfo(bad_path_addr)["hdkeypath"], bad_deriv_path) + + # Verify that this addr is actually at m/0'/0'/10' by making a new wallet with the same seed + node_v22.createwallet(wallet_name="path_verify", descriptors=False, blank=True) + verify_wallet = node_v22.get_wallet_rpc("path_verify") + verify_wallet.sethdseed(True, seed) + # Bad addr is after keypool, so need to generate it by refilling + verify_wallet.keypoolrefill(LAST_KEYPOOL_INDEX + 2) + assert_equal(verify_wallet.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path.replace("h", "'")) + + # Load into master, the path should now be fixed + backup_path = node_v22.wallets_path / f"{wallet_name}.bak" + wallet.backupwallet(backup_path) + wallet_dir_master = os.path.join(node_master.wallets_path, wallet_name) + os.makedirs(wallet_dir_master, exist_ok=True) + shutil.copy(backup_path, os.path.join(wallet_dir_master, "wallet.dat")) + with node_master.assert_debug_log(expected_msgs=["Repairing derivation path for"]): + node_master.migratewallet(wallet_name) + wallet_master = node_master.get_wallet_rpc(wallet_name) + assert_equal(wallet_master.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path) + + # Check persistence + wallet_master.unloadwallet() + with node_master.assert_debug_log(expected_msgs=[], unexpected_msgs=["Repairing derivation path for"]): + node_master.loadwallet(wallet_name) + assert_equal(wallet_master.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path) + def run_test(self): node_miner = self.nodes[0] node_master = self.nodes[1] @@ -385,5 +458,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework): info = wallet_res.getaddressinfo(address) assert_equal(info, addr_info) + self.test_v22_inactivehdchain_path() + if __name__ == '__main__': BackwardsCompatibilityTest(__file__).main()