Skip to content

fix(rpc): return correct error codes in upgradetohd rpc #6797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 4, 2025
Merged

fix(rpc): return correct error codes in upgradetohd rpc #6797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b19f463
fix(rpc): return correct error codes in `upgradetohd` rpc
UdjinM6 Aug 3, 2025
746e5f8
fix: always leave the wallet in a locked state afer `upgradetohd`
UdjinM6 Aug 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 83 additions & 36 deletions src/wallet/rpc/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,51 +371,98 @@ static RPCHelpMan upgradetohd()
if (!pwallet) return NullUniValue;

bool generate_mnemonic = request.params[0].isNull() || request.params[0].get_str().empty();
SecureString secureWalletPassphrase;
secureWalletPassphrase.reserve(100);

if (request.params[2].isNull()) {
if (pwallet->IsCrypted()) {
throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: Wallet encrypted but passphrase not supplied to RPC.");
{
LOCK(pwallet->cs_wallet);

SecureString secureWalletPassphrase;
secureWalletPassphrase.reserve(100);

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();
}
} 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);
if (!generate_mnemonic) {
secureMnemonic = request.params[0].get_str().c_str();
}
SecureString secureMnemonic;
secureMnemonic.reserve(256);
if (!generate_mnemonic) {
secureMnemonic = request.params[0].get_str().c_str();
}

SecureString secureMnemonicPassphrase;
secureMnemonicPassphrase.reserve(256);
if (!request.params[1].isNull()) {
secureMnemonicPassphrase = request.params[1].get_str().c_str();
}
SecureString secureMnemonicPassphrase;
secureMnemonicPassphrase.reserve(256);
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;
const int previous_version{pwallet->GetVersion()};
if (is_v21 && previous_version >= FEATURE_HD) {
return JSONRPCError(RPC_WALLET_ERROR, "Already at latest version. Wallet version unchanged.");
}
// TODO: breaking changes kept for v21!
// instead upgradetohd let's use more straightforward 'sethdseed'
constexpr bool is_v21 = false;
const int previous_version{pwallet->GetVersion()};
if (is_v21 && previous_version >= FEATURE_HD) {
return JSONRPCError(RPC_WALLET_ERROR, "Already at latest version. Wallet version unchanged.");
}

bilingual_str error;
const bool wallet_upgraded{pwallet->UpgradeToHD(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase, error)};
// Do not do anything to HD wallets
if (pwallet->IsHDEnabled()) {
throw JSONRPCError(RPC_WALLET_ERROR, "Cannot upgrade a wallet to HD if it is already upgraded to HD");
}

if (!secureWalletPassphrase.empty() && !pwallet->IsCrypted()) {
if (!pwallet->EncryptWallet(secureWalletPassphrase)) {
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet");
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Private keys are disabled for this wallet");
}
}

if (!wallet_upgraded) {
throw JSONRPCError(RPC_WALLET_ERROR, error.original);
}
pwallet->WalletLogPrintf("Upgrading wallet to HD\n");
pwallet->SetMinVersion(FEATURE_HD);

if (pwallet->IsCrypted()) {
if (secureWalletPassphrase.empty()) {
throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: Wallet encrypted but supplied empty wallet passphrase");
}

// We are intentionally re-locking the wallet so we can validate passphrase
// by verifying if it can unlock the wallet
pwallet->Lock();

// Unlock the wallet
if (!pwallet->Unlock(secureWalletPassphrase)) {
throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect");
Copy link
Collaborator

@knst knst Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once this PR (#6797) and (#6792) are meged, I would like to update error message like that:

        if (secureWalletPassphrase.find('\0') == std::string::npos) {
            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect.");
        } else {
            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The old wallet passphrase entered is incorrect. "
                                                                "It contains a null character (ie - a zero byte). "
                                                                "If the old passphrase was set with a version of this software prior to 23.0, "
                                                                "please try again with only the characters up to — but not including — "
                                                                "the first null character.");

}
}

if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
pwallet->SetupDescriptorScriptPubKeyMans(secureMnemonic, secureMnemonicPassphrase);
} else {
auto spk_man = pwallet->GetLegacyScriptPubKeyMan();
if (!spk_man) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error: Legacy ScriptPubKeyMan is not available");
}

if (pwallet->IsCrypted()) {
pwallet->WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, encryption_key);
return true;
});
} else {
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
}
}

if (pwallet->IsCrypted()) {
// Relock encrypted wallet
pwallet->Lock();
} else if (!secureWalletPassphrase.empty()) {
// Encrypt non-encrypted wallet
if (!pwallet->EncryptWallet(secureWalletPassphrase)) {
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet");
}
}
} // pwallet->cs_wallet

// If you are generating new mnemonic it is assumed that the addresses have never gotten a transaction before, so you don't need to rescan for transactions
bool rescan = request.params[3].isNull() ? !generate_mnemonic : request.params[3].get_bool();
Expand Down
118 changes: 10 additions & 108 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,11 +353,17 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
// TODO: drop this condition after removing option to create non-HD wallets
// related backport bitcoin#11250
if (wallet->GetVersion() >= FEATURE_HD) {
if (!wallet->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", passphrase)) {
error = Untranslated("Error: Failed to generate encrypted HD wallet");
status = DatabaseStatus::FAILED_CREATE;
return nullptr;
auto spk_man = wallet->GetLegacyScriptPubKeyMan();
if (!spk_man) {
error = Untranslated("Error: Legacy ScriptPubKeyMan is not available");
status = DatabaseStatus::FAILED_ENCRYPT;
return nullptr;
}

wallet->WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
spk_man->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", encryption_key);
return true;
});
}
}

Expand Down Expand Up @@ -3203,52 +3209,6 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error)
return true;
}

bool CWallet::UpgradeToHD(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase, bilingual_str& error)
{
LOCK(cs_wallet);

// Do not do anything to HD wallets
if (IsHDEnabled()) {
error = Untranslated("Cannot upgrade a wallet to HD if it is already upgraded to HD.");
return false;
}

if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
error = Untranslated("Private keys are disabled for this wallet");
return false;
}

WalletLogPrintf("Upgrading wallet to HD\n");
SetMinVersion(FEATURE_HD);

if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
if (IsCrypted()) {
if (secureWalletPassphrase.empty()) {
error = Untranslated("Error: Wallet encrypted but supplied empty wallet passphrase");
return false;
}

// Unlock the wallet
if (!Unlock(secureWalletPassphrase)) {
error = Untranslated("Error: The wallet passphrase entered was incorrect");
return false;
}
}
SetupDescriptorScriptPubKeyMans(secureMnemonic, secureMnemonicPassphrase);

if (IsCrypted()) {
// Relock the wallet
Lock();
}
} else {
if (!GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) {
error = Untranslated("Failed to generate HD wallet");
return false;
}
}
return true;
}

const CAddressBookData* CWallet::FindAddressBookEntry(const CTxDestination& dest, bool allow_change) const
{
const auto& address_book_it = m_address_book.find(dest);
Expand Down Expand Up @@ -3779,64 +3739,6 @@ void CWallet::ConnectScriptPubKeyManNotifiers()
}
}

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 (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);
}

return true;
}

void CWallet::UpdateProgress(const std::string& title, int nProgress)
{
ShowProgress(title, nProgress);
Expand Down
7 changes: 0 additions & 7 deletions src/wallet/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,10 +915,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
/* Returns true if HD is enabled */
bool IsHDEnabled() const;

// TODO: move it to scriptpubkeyman
/* Generates a new HD chain */
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;

Expand Down Expand Up @@ -975,9 +971,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
/** Upgrade the wallet */
bool UpgradeWallet(int version, bilingual_str& error);

/** Upgrade non-HD wallet to HD wallet */
bool UpgradeToHD(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase, bilingual_str& error);

//! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers
std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;

Expand Down
5 changes: 1 addition & 4 deletions test/functional/wallet_upgradetohd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,7 @@ def run_test(self):
node.wait_until_stopped()
self.start_node(0, extra_args=['-rescan'])
assert_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic)
if not self.options.descriptors:
assert_raises_rpc_error(-1, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
else:
assert_raises_rpc_error(-4, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
assert_raises_rpc_error(-14, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
assert node.upgradetohd(mnemonic, "", walletpass)
if not self.options.descriptors:
assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.dumphdinfo)
Expand Down
Loading