diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index f9b29f28e5..2107cec953 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2779,11 +2779,13 @@ static RPCHelpMan upgradetohd() { return RPCHelpMan{"upgradetohd", "\nUpgrades non-HD wallets to HD.\n" + "\nIf your wallet is encrypted, the wallet passphrase must be supplied. Supplying an incorrect" + "\npassphrase may result in your wallet getting locked.\n" "\nWarning: You will need to make a new backup of your wallet after setting the HD wallet mnemonic.\n", { {"mnemonic", RPCArg::Type::STR, /* default */ "", "Mnemonic as defined in BIP39 to use for the new HD wallet. Use an empty string \"\" to generate a new random mnemonic."}, {"mnemonicpassphrase", RPCArg::Type::STR, /* default */ "", "Optional mnemonic passphrase as defined in BIP39"}, - {"walletpassphrase", RPCArg::Type::STR, /* default */ "", "If your wallet is encrypted you must have your wallet passphrase here. If your wallet is not encrypted specifying wallet passphrase will trigger wallet encryption."}, + {"walletpassphrase", RPCArg::Type::STR, /* default */ "", "If your wallet is encrypted you must have your wallet passphrase here. If your wallet is not encrypted, specifying wallet passphrase will trigger wallet encryption."}, {"rescan", RPCArg::Type::BOOL, /* default */ "false if mnemonic is empty", "Whether to rescan the blockchain for missing transactions or not"}, }, RPCResult{ @@ -2793,6 +2795,7 @@ static RPCHelpMan upgradetohd() HelpExampleCli("upgradetohd", "") + HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\"") + HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\" \"mnemonicpassphrase\"") + + HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\" \"\" \"walletpassphrase\"") + HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\" \"mnemonicpassphrase\" \"walletpassphrase\"") }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue @@ -2803,16 +2806,16 @@ static RPCHelpMan upgradetohd() bool generate_mnemonic = request.params[0].isNull() || request.params[0].get_str().empty(); SecureString secureWalletPassphrase; secureWalletPassphrase.reserve(100); - // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) - // Alternately, find a way to make request.params[0] mlock()'d to begin with. - if (!request.params[2].isNull()) { - secureWalletPassphrase = request.params[2].get_str().c_str(); - if (!pwallet->Unlock(secureWalletPassphrase)) { - throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "The wallet passphrase entered was incorrect"); - } - } - EnsureWalletIsUnlocked(pwallet.get()); + if (request.params[2].isNull()) { + if (pwallet->IsCrypted()) { + throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: Wallet encrypted but passphrase not supplied to RPC."); + } + } else { + // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) + // Alternately, find a way to make request.params[0] mlock()'d to begin with. + secureWalletPassphrase = request.params[2].get_str().c_str(); + } SecureString secureMnemonic; secureMnemonic.reserve(256); @@ -2825,6 +2828,7 @@ static RPCHelpMan upgradetohd() if (!request.params[1].isNull()) { secureMnemonicPassphrase = request.params[1].get_str().c_str(); } + // TODO: breaking changes kept for v21! // instead upgradetohd let's use more straightforward 'sethdseed' constexpr bool is_v21 = false; diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 678022e1f2..631fcac566 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -377,56 +377,42 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata() } } -void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, CKeyingMaterial vMasterKey) -{ - assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); - - - CHDChain hdChainTmp; - - // NOTE: an empty mnemonic means "generate a new one for me" - // NOTE: default mnemonic passphrase is an empty string - if (!hdChainTmp.SetMnemonic(secureMnemonic, secureMnemonicPassphrase, true)) { - throw std::runtime_error(std::string(__func__) + ": SetMnemonic failed"); - } - - // add default account - hdChainTmp.AddAccount(); - - // We need to safe chain for validation further - CHDChain hdChainPrev = hdChainTmp; - bool res = EncryptHDChain(vMasterKey, hdChainTmp); - assert(res); - res = LoadHDChain(hdChainTmp); - assert(res); - - CHDChain hdChainCrypted; - res = GetHDChain(hdChainCrypted); - assert(res); - - // ids should match, seed hashes should not - assert(hdChainPrev.GetID() == hdChainCrypted.GetID()); - assert(hdChainPrev.GetSeedHash() != hdChainCrypted.GetSeedHash()); - - if (!AddHDChainSingle(hdChainCrypted)) { - throw std::runtime_error(std::string(__func__) + ": AddHDChainSingle failed"); - } -} - -void LegacyScriptPubKeyMan::GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase) +void LegacyScriptPubKeyMan::GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, std::optional vMasterKeyOpt) { assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); CHDChain newHdChain; // NOTE: an empty mnemonic means "generate a new one for me" // NOTE: default mnemonic passphrase is an empty string - if (!newHdChain.SetMnemonic(secureMnemonic, secureMnemonicPassphrase, true)) { + if (!newHdChain.SetMnemonic(secureMnemonic, secureMnemonicPassphrase, /* fUpdateID = */ true)) { throw std::runtime_error(std::string(__func__) + ": SetMnemonic failed"); } - // add default account + // Add default account newHdChain.AddAccount(); + // Encryption routine if vMasterKey has been supplied + if (vMasterKeyOpt.has_value()) { + auto vMasterKey = vMasterKeyOpt.value(); + if (vMasterKey.size() != WALLET_CRYPTO_KEY_SIZE) { + throw std::runtime_error(strprintf("%s : invalid vMasterKey size, got %zd (expected %lld)", __func__, vMasterKey.size(), WALLET_CRYPTO_KEY_SIZE)); + } + + // Maintain an unencrypted copy of the chain for sanity checking + CHDChain prevHdChain{newHdChain}; + + bool res = EncryptHDChain(vMasterKey, newHdChain); + assert(res); + res = LoadHDChain(newHdChain); + assert(res); + res = GetHDChain(newHdChain); + assert(res); + + // IDs should match, seed hashes should not + assert(prevHdChain.GetID() == newHdChain.GetID()); + assert(prevHdChain.GetSeedHash() != newHdChain.GetSeedHash()); + } + if (!AddHDChainSingle(newHdChain)) { throw std::runtime_error(std::string(__func__) + ": AddHDChainSingle failed"); } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 9c639b73eb..5ebb757628 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -463,8 +463,7 @@ public: bool GetDecryptedHDChain(CHDChain& hdChainRet); /* Generates a new HD chain */ - void GenerateNewCryptedHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, CKeyingMaterial vMasterKey); - void GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase); + void GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, std::optional vMasterKey = std::nullopt); /** * Explicitly make the wallet learn the related scripts for outputs to the diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 29d6104270..36cdf3f171 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -337,7 +337,7 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, interfaces::Coin // TODO: drop this condition after removing option to create non-HD wallets // related backport bitcoin#11250 if (wallet->GetVersion() >= FEATURE_HD) { - if (!wallet->GenerateNewHDChainEncrypted(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", passphrase)) { + if (!wallet->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", passphrase)) { error = Untranslated("Error: Failed to generate encrypted HD wallet"); status = DatabaseStatus::FAILED_CREATE; return nullptr; @@ -5022,18 +5022,9 @@ bool CWallet::UpgradeToHD(const SecureString& secureMnemonic, const SecureString WalletLogPrintf("Upgrading wallet to HD\n"); SetMinVersion(FEATURE_HD); - // TODO: replace to GetLegacyScriptPubKeyMan() when `sethdseed` is backported - auto spk_man = GetOrCreateLegacyScriptPubKeyMan(); - bool prev_encrypted = IsCrypted(); - // TODO: unify encrypted and plain chains usages here - if (prev_encrypted) { - if (!GenerateNewHDChainEncrypted(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) { - error = Untranslated("Failed to generate encrypted HD wallet"); - return false; - } - Lock(); - } else { - spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase); + if (!GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) { + error = Untranslated("Failed to generate HD wallet"); + return false; } return true; } @@ -5651,42 +5642,61 @@ void CWallet::ConnectScriptPubKeyManNotifiers() } } -bool CWallet::GenerateNewHDChainEncrypted(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase) +bool CWallet::GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase) { auto spk_man = GetLegacyScriptPubKeyMan(); if (!spk_man) { throw std::runtime_error(strprintf("%s: spk_man is not available", __func__)); } - if (!HasEncryptionKeys()) { - return false; - } - - CCrypter crypter; - CKeyingMaterial vMasterKey; - - LOCK(cs_wallet); - for (const CWallet::MasterKeyMap::value_type& pMasterKey : mapMasterKeys) { - if (!crypter.SetKeyFromPassphrase(secureWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod)) { - return false; - } - // get vMasterKey to encrypt new hdChain - if (crypter.Decrypt(pMasterKey.second.vchCryptedKey, vMasterKey)) { - break; + if (IsCrypted()) { + if (secureWalletPassphrase.empty()) { + throw std::runtime_error(strprintf("%s: encrypted but supplied empty wallet passphrase", __func__)); } + bool is_locked = IsLocked(); + + CCrypter crypter; + CKeyingMaterial vMasterKey; + + // We are intentionally re-locking the wallet so we can validate vMasterKey + // by verifying if it can unlock the wallet + Lock(); + + LOCK(cs_wallet); + for (const auto& [_, master_key] : mapMasterKeys) { + CKeyingMaterial _vMasterKey; + if (!crypter.SetKeyFromPassphrase(secureWalletPassphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod)) { + return false; + } + // Try another key if it cannot be decrypted or the key is incapable of encrypting + if (!crypter.Decrypt(master_key.vchCryptedKey, _vMasterKey) || _vMasterKey.size() != WALLET_CRYPTO_KEY_SIZE) { + continue; + } + // The likelihood of the plaintext being gibberish but also of the expected size is low but not zero. + // If it can unlock the wallet, it's a good key. + if (Unlock(_vMasterKey)) { + vMasterKey = _vMasterKey; + break; + } + } + + // We got a gibberish key... + if (vMasterKey.empty()) { + // Mimicking the error message of RPC_WALLET_PASSPHRASE_INCORRECT as it's possible + // that the user may see this error when interacting with the upgradetohd RPC + throw std::runtime_error("Error: The wallet passphrase entered was incorrect"); + } + + spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, vMasterKey); + + if (is_locked) { + Lock(); + } + } else { + spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase); } - spk_man->GenerateNewCryptedHDChain(secureMnemonic, secureMnemonicPassphrase, vMasterKey); - - Lock(); - if (!Unlock(secureWalletPassphrase)) { - // this should never happen - throw std::runtime_error(std::string(__func__) + ": Unlock failed"); - } - if (!spk_man->NewKeyPool()) { - throw std::runtime_error(std::string(__func__) + ": NewKeyPool failed"); - } return true; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index b2f967cf11..0ac9666989 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1343,7 +1343,7 @@ public: // TODO: move it to scriptpubkeyman /* Generates a new HD chain */ - bool GenerateNewHDChainEncrypted(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase); + bool GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase = ""); /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */ bool CanGetAddresses(bool internal = false) const; diff --git a/test/functional/wallet_upgradetohd.py b/test/functional/wallet_upgradetohd.py index 10bb44f6b6..b1d09534b8 100755 --- a/test/functional/wallet_upgradetohd.py +++ b/test/functional/wallet_upgradetohd.py @@ -190,8 +190,8 @@ class WalletUpgradeToHDTest(BitcoinTestFramework): node.stop() node.wait_until_stopped() self.start_node(0, extra_args=['-rescan']) - assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.upgradetohd, mnemonic) - assert_raises_rpc_error(-14, "The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass") + assert_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic) + assert_raises_rpc_error(-1, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass") assert node.upgradetohd(mnemonic, "", walletpass) assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.dumphdinfo) node.walletpassphrase(walletpass, 100)