Skip to content

Commit b19f463

Browse files
UdjinM6UdjinM6
committed
fix(rpc): return correct error codes in upgradetohd rpc
1 parent 88231b1 commit b19f463

File tree

4 files changed

+97
-155
lines changed

4 files changed

+97
-155
lines changed

‎src/wallet/rpc/wallet.cpp‎

Lines changed: 86 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -371,51 +371,101 @@ static RPCHelpMan upgradetohd()
371371
if (!pwallet) return NullUniValue;
372372

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

377-
if (request.params[2].isNull()) {
378-
if (pwallet->IsCrypted()) {
379-
throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: Wallet encrypted but passphrase not supplied to RPC.");
375+
{
376+
LOCK(pwallet->cs_wallet);
377+
378+
SecureString secureWalletPassphrase;
379+
secureWalletPassphrase.reserve(100);
380+
381+
if (request.params[2].isNull()) {
382+
if (pwallet->IsCrypted()) {
383+
throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: Wallet encrypted but passphrase not supplied to RPC.");
384+
}
385+
} else {
386+
// TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
387+
// Alternately, find a way to make request.params[0] mlock()'d to begin with.
388+
secureWalletPassphrase = request.params[2].get_str().c_str();
380389
}
381-
} else {
382-
// TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
383-
// Alternately, find a way to make request.params[0] mlock()'d to begin with.
384-
secureWalletPassphrase = request.params[2].get_str().c_str();
385-
}
386390

387-
SecureString secureMnemonic;
388-
secureMnemonic.reserve(256);
389-
if (!generate_mnemonic) {
390-
secureMnemonic = request.params[0].get_str().c_str();
391-
}
391+
SecureString secureMnemonic;
392+
secureMnemonic.reserve(256);
393+
if (!generate_mnemonic) {
394+
secureMnemonic = request.params[0].get_str().c_str();
395+
}
392396

393-
SecureString secureMnemonicPassphrase;
394-
secureMnemonicPassphrase.reserve(256);
395-
if (!request.params[1].isNull()) {
396-
secureMnemonicPassphrase = request.params[1].get_str().c_str();
397-
}
397+
SecureString secureMnemonicPassphrase;
398+
secureMnemonicPassphrase.reserve(256);
399+
if (!request.params[1].isNull()) {
400+
secureMnemonicPassphrase = request.params[1].get_str().c_str();
401+
}
398402

399-
// TODO: breaking changes kept for v21!
400-
// instead upgradetohd let's use more straightforward 'sethdseed'
401-
constexpr bool is_v21 = false;
402-
const int previous_version{pwallet->GetVersion()};
403-
if (is_v21 && previous_version >= FEATURE_HD) {
404-
return JSONRPCError(RPC_WALLET_ERROR, "Already at latest version. Wallet version unchanged.");
405-
}
403+
// TODO: breaking changes kept for v21!
404+
// instead upgradetohd let's use more straightforward 'sethdseed'
405+
constexpr bool is_v21 = false;
406+
const int previous_version{pwallet->GetVersion()};
407+
if (is_v21 && previous_version >= FEATURE_HD) {
408+
return JSONRPCError(RPC_WALLET_ERROR, "Already at latest version. Wallet version unchanged.");
409+
}
406410

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

410-
if (!secureWalletPassphrase.empty() && !pwallet->IsCrypted()) {
411-
if (!pwallet->EncryptWallet(secureWalletPassphrase)) {
412-
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet");
416+
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
417+
throw JSONRPCError(RPC_WALLET_ERROR, "Private keys are disabled for this wallet");
413418
}
414-
}
415419

416-
if (!wallet_upgraded) {
417-
throw JSONRPCError(RPC_WALLET_ERROR, error.original);
418-
}
420+
pwallet->WalletLogPrintf("Upgrading wallet to HD\n");
421+
pwallet->SetMinVersion(FEATURE_HD);
422+
423+
bool is_locked = pwallet->IsLocked();
424+
425+
if (pwallet->IsCrypted()) {
426+
if (secureWalletPassphrase.empty()) {
427+
throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: Wallet encrypted but supplied empty wallet passphrase");
428+
}
429+
430+
// We are intentionally re-locking the wallet so we can validate passphrase
431+
// by verifying if it can unlock the wallet
432+
pwallet->Lock();
433+
434+
// Unlock the wallet
435+
if (!pwallet->Unlock(secureWalletPassphrase)) {
436+
throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect");
437+
}
438+
}
439+
440+
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
441+
pwallet->SetupDescriptorScriptPubKeyMans(secureMnemonic, secureMnemonicPassphrase);
442+
} else {
443+
auto spk_man = pwallet->GetLegacyScriptPubKeyMan();
444+
if (!spk_man) {
445+
throw JSONRPCError(RPC_WALLET_ERROR, "Error: Legacy ScriptPubKeyMan is not available");
446+
}
447+
448+
if (pwallet->IsCrypted()) {
449+
pwallet->WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
450+
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, encryption_key);
451+
return true;
452+
});
453+
} else {
454+
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
455+
}
456+
}
457+
458+
if (is_locked) {
459+
// Relock the wallet
460+
pwallet->Lock();
461+
}
462+
463+
if (!secureWalletPassphrase.empty() && !pwallet->IsCrypted()) {
464+
if (!pwallet->EncryptWallet(secureWalletPassphrase)) {
465+
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet");
466+
}
467+
}
468+
} // pwallet->cs_wallet
419469

420470
// 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
421471
bool rescan = request.params[3].isNull() ? !generate_mnemonic : request.params[3].get_bool();

‎src/wallet/wallet.cpp‎

Lines changed: 10 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -353,11 +353,17 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
353353
// TODO: drop this condition after removing option to create non-HD wallets
354354
// related backport bitcoin#11250
355355
if (wallet->GetVersion() >= FEATURE_HD) {
356-
if (!wallet->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", passphrase)) {
357-
error = Untranslated("Error: Failed to generate encrypted HD wallet");
358-
status = DatabaseStatus::FAILED_CREATE;
359-
return nullptr;
356+
auto spk_man = wallet->GetLegacyScriptPubKeyMan();
357+
if (!spk_man) {
358+
error = Untranslated("Error: Legacy ScriptPubKeyMan is not available");
359+
status = DatabaseStatus::FAILED_ENCRYPT;
360+
return nullptr;
360361
}
362+
363+
wallet->WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
364+
spk_man->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", encryption_key);
365+
return true;
366+
});
361367
}
362368
}
363369

@@ -3203,52 +3209,6 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error)
32033209
return true;
32043210
}
32053211

3206-
bool CWallet::UpgradeToHD(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase, bilingual_str& error)
3207-
{
3208-
LOCK(cs_wallet);
3209-
3210-
// Do not do anything to HD wallets
3211-
if (IsHDEnabled()) {
3212-
error = Untranslated("Cannot upgrade a wallet to HD if it is already upgraded to HD.");
3213-
return false;
3214-
}
3215-
3216-
if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
3217-
error = Untranslated("Private keys are disabled for this wallet");
3218-
return false;
3219-
}
3220-
3221-
WalletLogPrintf("Upgrading wallet to HD\n");
3222-
SetMinVersion(FEATURE_HD);
3223-
3224-
if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
3225-
if (IsCrypted()) {
3226-
if (secureWalletPassphrase.empty()) {
3227-
error = Untranslated("Error: Wallet encrypted but supplied empty wallet passphrase");
3228-
return false;
3229-
}
3230-
3231-
// Unlock the wallet
3232-
if (!Unlock(secureWalletPassphrase)) {
3233-
error = Untranslated("Error: The wallet passphrase entered was incorrect");
3234-
return false;
3235-
}
3236-
}
3237-
SetupDescriptorScriptPubKeyMans(secureMnemonic, secureMnemonicPassphrase);
3238-
3239-
if (IsCrypted()) {
3240-
// Relock the wallet
3241-
Lock();
3242-
}
3243-
} else {
3244-
if (!GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) {
3245-
error = Untranslated("Failed to generate HD wallet");
3246-
return false;
3247-
}
3248-
}
3249-
return true;
3250-
}
3251-
32523212
const CAddressBookData* CWallet::FindAddressBookEntry(const CTxDestination& dest, bool allow_change) const
32533213
{
32543214
const auto& address_book_it = m_address_book.find(dest);
@@ -3779,64 +3739,6 @@ void CWallet::ConnectScriptPubKeyManNotifiers()
37793739
}
37803740
}
37813741

3782-
bool CWallet::GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase)
3783-
{
3784-
auto spk_man = GetLegacyScriptPubKeyMan();
3785-
if (!spk_man) {
3786-
throw std::runtime_error(strprintf("%s: spk_man is not available", __func__));
3787-
}
3788-
3789-
if (IsCrypted()) {
3790-
if (secureWalletPassphrase.empty()) {
3791-
throw std::runtime_error(strprintf("%s: encrypted but supplied empty wallet passphrase", __func__));
3792-
}
3793-
3794-
bool is_locked = IsLocked();
3795-
3796-
CCrypter crypter;
3797-
CKeyingMaterial vMasterKey;
3798-
3799-
// We are intentionally re-locking the wallet so we can validate vMasterKey
3800-
// by verifying if it can unlock the wallet
3801-
Lock();
3802-
3803-
LOCK(cs_wallet);
3804-
for (const auto& [_, master_key] : mapMasterKeys) {
3805-
CKeyingMaterial _vMasterKey;
3806-
if (!crypter.SetKeyFromPassphrase(secureWalletPassphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod)) {
3807-
return false;
3808-
}
3809-
// Try another key if it cannot be decrypted or the key is incapable of encrypting
3810-
if (!crypter.Decrypt(master_key.vchCryptedKey, _vMasterKey) || _vMasterKey.size() != WALLET_CRYPTO_KEY_SIZE) {
3811-
continue;
3812-
}
3813-
// The likelihood of the plaintext being gibberish but also of the expected size is low but not zero.
3814-
// If it can unlock the wallet, it's a good key.
3815-
if (Unlock(_vMasterKey)) {
3816-
vMasterKey = _vMasterKey;
3817-
break;
3818-
}
3819-
}
3820-
3821-
// We got a gibberish key...
3822-
if (vMasterKey.empty()) {
3823-
// Mimicking the error message of RPC_WALLET_PASSPHRASE_INCORRECT as it's possible
3824-
// that the user may see this error when interacting with the upgradetohd RPC
3825-
throw std::runtime_error("Error: The wallet passphrase entered was incorrect");
3826-
}
3827-
3828-
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, vMasterKey);
3829-
3830-
if (is_locked) {
3831-
Lock();
3832-
}
3833-
} else {
3834-
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
3835-
}
3836-
3837-
return true;
3838-
}
3839-
38403742
void CWallet::UpdateProgress(const std::string& title, int nProgress)
38413743
{
38423744
ShowProgress(title, nProgress);

‎src/wallet/wallet.h‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -915,10 +915,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
915915
/* Returns true if HD is enabled */
916916
bool IsHDEnabled() const;
917917

918-
// TODO: move it to scriptpubkeyman
919-
/* Generates a new HD chain */
920-
bool GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase = "");
921-
922918
/* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */
923919
bool CanGetAddresses(bool internal = false) const;
924920

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

978-
/** Upgrade non-HD wallet to HD wallet */
979-
bool UpgradeToHD(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase, bilingual_str& error);
980-
981974
//! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers
982975
std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;
983976

‎test/functional/wallet_upgradetohd.py‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -203,10 +203,7 @@ def run_test(self):
203203
node.wait_until_stopped()
204204
self.start_node(0, extra_args=['-rescan'])
205205
assert_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic)
206-
if not self.options.descriptors:
207-
assert_raises_rpc_error(-1, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
208-
else:
209-
assert_raises_rpc_error(-4, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
206+
assert_raises_rpc_error(-14, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
210207
assert node.upgradetohd(mnemonic, "", walletpass)
211208
if not self.options.descriptors:
212209
assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.dumphdinfo)

0 commit comments

Comments
 (0)