feat: enable HD wallets by default (#5807)

## Issue being fixed or feature implemented
HD wallets are old-existsing feature, appeared in Dash years ago, but
enabling HD wallets is not trivial task that requires multiple steps and
command line/rpc calls.
Let's have them enabled by default.

## What was done?
- HD wallets are enabled by default. Currently behavior `dashd`,
`dash-qt` are similar to run with option `-usehd=1`
- the rpc `upgradewallet` do not let to upgrade from non-HD wallet to HD
wallet to don't encourage user use non-crypted wallets (postponed till
v21)
- the initialization of ScriptPubKey is updated to be sure that encypted
HD seed is never written on disk (if passphrase is provided)
- enabled and dashified a script `wallet_upgradewallet.py` which test
compatibility between different versions of wallet


## What is not done?
- wallet tool still does not support passhprase, HD seed can appear on
disk
- there's no dialog that show user a mnemonic phrase and encourage him
to make a paper backup
 
Before removing a command line 'usehd' (backport bitcoin#11250) need to
make at least one major release for fail-over option (if someone wish to
use non-HD wallets only).


## How Has This Been Tested?
Run unit and functional tests.
Enabled new functional test `wallet_upgradewallet.py` that has been
backported long time ago but waited this PR to be enabled.

## Breaking Changes
HD wallets are created by default. 

## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone

---------

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
This commit is contained in:
Konstantin Akimov 2024-02-10 00:36:14 +07:00 committed by GitHub
parent 19681d0f45
commit 8dba6559f6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 158 additions and 143 deletions

View File

@ -0,0 +1,7 @@
# Notable changes
## HD Wallets Enabled by Default
In this release, we are taking a significant step towards enhancing the Dash wallet's usability by enabling Hierarchical Deterministic (HD) wallets by default. This change aligns the behavior of `dashd` and `dash-qt` with the previously optional `-usehd=1` flag, making HD wallets the standard for all users.
While HD wallets are now enabled by default to improve user experience, Dash Core still allows for the creation of non-HD wallets by using the `-usehd=0` flag. However, users should be aware that this option is intended for legacy support and will be removed in future releases. Importantly, even with an HD wallet, users can still import non-HD private keys, ensuring flexibility in managing their funds.

View File

@ -50,7 +50,7 @@
<string>Encrypt Wallet</string>
</property>
<property name="checked">
<bool>false</bool>
<bool>true</bool>
</property>
</widget>
</item>

View File

@ -2681,49 +2681,23 @@ static UniValue upgradetohd(const JSONRPCRequest& request)
},
}.Check(request);
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
if (!wallet) return NullUniValue;
CWallet* const pwallet = wallet.get();
LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan();
if (!spk_man) {
throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
}
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
if (!pwallet) return NullUniValue;
bool generate_mnemonic = request.params[0].isNull() || request.params[0].get_str().empty();
{
LOCK(pwallet->cs_wallet);
// 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 (!pwallet->SetMaxVersion(FEATURE_HD)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Cannot downgrade wallet");
}
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet");
}
bool prev_encrypted = pwallet->IsCrypted();
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()) {
if (prev_encrypted) {
throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Cannot upgrade encrypted wallet to HD without the wallet passphrase");
}
} else {
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());
SecureString secureMnemonic;
secureMnemonic.reserve(256);
if (!generate_mnemonic) {
@ -2735,22 +2709,25 @@ static UniValue upgradetohd(const JSONRPCRequest& request)
if (!request.params[1].isNull()) {
secureMnemonicPassphrase = request.params[1].get_str().c_str();
}
pwallet->WalletLogPrintf("Upgrading wallet to HD\n");
pwallet->SetMinVersion(FEATURE_HD);
if (prev_encrypted) {
if (!pwallet->GenerateNewHDChainEncrypted(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Failed to generate encrypted HD wallet");
// 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.");
}
} else {
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
if (!secureWalletPassphrase.empty()) {
bilingual_str error;
const bool wallet_upgraded{pwallet->UpgradeToHD(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase, error)};
if (!secureWalletPassphrase.empty() && !pwallet->IsCrypted()) {
if (!pwallet->EncryptWallet(secureWalletPassphrase)) {
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet");
}
}
}
if (!wallet_upgraded) {
throw JSONRPCError(RPC_WALLET_ERROR, error.original);
}
// 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

View File

@ -297,6 +297,10 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, interfaces::Coin
status = DatabaseStatus::FAILED_CREATE;
return nullptr;
}
if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET)) {
wallet->WalletLogPrintf("Set HD by default\n");
wallet->SetMinVersion(FEATURE_HD);
}
// Encrypt the wallet
if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
@ -306,6 +310,18 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, interfaces::Coin
return nullptr;
}
if (!create_blank) {
{
// 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)) {
error = Untranslated("Error: Failed to generate encrypted HD wallet");
status = DatabaseStatus::FAILED_CREATE;
return nullptr;
}
}
}
// Unlock the wallet
if (!wallet->Unlock(passphrase)) {
error = Untranslated("Error: Wallet was encrypted but could not be unlocked");
@ -313,22 +329,6 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, interfaces::Coin
return nullptr;
}
// Set a HD chain for the wallet
// TODO: re-enable this and `keypoolsize_hd_internal` check in `wallet_createwallet.py`
// when HD is the default mode (make sure this actually works!)...
// if (auto spk_man = wallet->GetLegacyScriptPubKeyMan() {
// if (!spk_man->GenerateNewHDChainEncrypted("", "", passphrase)) {
// throw JSONRPCError(RPC_WALLET_ERROR, "Failed to generate encrypted HD wallet");
// }
// }
// ... and drop this
LOCK(wallet->cs_wallet);
wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
if (auto spk_man = wallet->GetLegacyScriptPubKeyMan()) {
spk_man->NewKeyPool();
}
// end TODO
// backup the wallet we just encrypted
if (!wallet->AutoBackupWallet("", error, warnings) && !error.original.empty()) {
status = DatabaseStatus::FAILED_ENCRYPT;
@ -4617,6 +4617,10 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, interfaces::C
if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET) && !walletInstance->IsHDEnabled()) {
std::string strSeed = gArgs.GetArg("-hdseed", "not hex");
// ensure this wallet.dat can only be opened by clients supporting HD
walletInstance->WalletLogPrintf("Upgrading wallet to HD\n");
walletInstance->SetMinVersion(FEATURE_HD);
if (gArgs.IsArgSet("-hdseed") && IsHex(strSeed)) {
CHDChain newHdChain;
std::vector<unsigned char> vchSeed = ParseHex(strSeed);
@ -4646,10 +4650,6 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, interfaces::C
}
}
// ensure this wallet.dat can only be opened by clients supporting HD
walletInstance->WalletLogPrintf("Upgrading wallet to HD\n");
walletInstance->SetMinVersion(FEATURE_HD);
// clean up
gArgs.ForceRemoveArg("hdseed");
gArgs.ForceRemoveArg("mnemonic");
@ -4913,7 +4913,45 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error)
return false;
}
// TODO: consider discourage users to skip passphrase for HD wallets for v21
if (false && nMaxVersion >= FEATURE_HD && !IsHDEnabled()) {
error = Untranslated("You should use upgradetohd RPC to upgrade non-HD wallet to HD");
return false;
}
SetMaxVersion(nMaxVersion);
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);
auto spk_man = GetOrCreateLegacyScriptPubKeyMan();
bool prev_encrypted = IsCrypted();
if (prev_encrypted) {
if (!GenerateNewHDChainEncrypted(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) {
error = Untranslated("Failed to generate encrypted HD wallet");
return false;
}
} else {
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
}
return true;
}

View File

@ -91,7 +91,7 @@ static const CAmount HIGH_TX_FEE_PER_KB = COIN / 100;
static const CAmount HIGH_MAX_TX_FEE = 100 * HIGH_TX_FEE_PER_KB;
//! if set, all keys will be derived by using BIP39/BIP44
static const bool DEFAULT_USE_HD_WALLET = false;
static const bool DEFAULT_USE_HD_WALLET = true;
class CCoinControl;
class CKey;
@ -1279,6 +1279,7 @@ public:
/* Returns true if HD is enabled */
bool IsHDEnabled() const;
// TODO: move it to scriptpubkeyman
/* Generates a new HD chain */
bool GenerateNewHDChainEncrypted(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase);
@ -1331,6 +1332,9 @@ public:
/** 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;

View File

@ -28,9 +28,11 @@ static void WalletCreate(CWallet* wallet_instance)
// generate a new HD seed
wallet_instance->SetupLegacyScriptPubKeyMan();
// NOTE: we do not yet create HD wallets by default
// auto spk_man = wallet_instance->GetLegacyScriptPubKeyMan();
// spk_man->GenerateNewHDChain("", "");
auto spk_man = wallet_instance->GetLegacyScriptPubKeyMan();
// NOTE: drop this condition after removing option to create non-HD wallets
if (spk_man->IsHDEnabled()) {
spk_man->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"");
}
tfm::format(std::cout, "Topping up keypool...\n");
wallet_instance->TopUpKeyPool();

View File

@ -97,21 +97,6 @@ class ToolWalletTest(BitcoinTestFramework):
# shasum_before = self.wallet_shasum()
timestamp_before = self.wallet_timestamp()
self.log.debug('Wallet file timestamp before calling info: {}'.format(timestamp_before))
out = textwrap.dedent('''\
Wallet info
===========
Encrypted: no
HD (hd seed available): no
Keypool Size: 1
Transactions: 0
Address Book: 1
''')
self.assert_tool_output(out, '-wallet=' + self.default_wallet_name, 'info')
self.start_node(0)
self.nodes[0].upgradetohd()
self.stop_node(0)
out = textwrap.dedent('''\
Wallet info
===========
@ -122,6 +107,7 @@ class ToolWalletTest(BitcoinTestFramework):
Address Book: 1
''')
self.assert_tool_output(out, '-wallet=' + self.default_wallet_name, 'info')
timestamp_after = self.wallet_timestamp()
self.log.debug('Wallet file timestamp after calling info: {}'.format(timestamp_after))
self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)

View File

@ -113,9 +113,7 @@ class CreateWalletTest(BitcoinTestFramework):
# There should only be 1 key
walletinfo = w6.getwalletinfo()
assert_equal(walletinfo['keypoolsize'], 1)
# TODO: re-enable this when HD is the default mode
# assert_equal(walletinfo['keypoolsize_hd_internal'], 1)
# end TODO
assert_equal(walletinfo['keypoolsize_hd_internal'], 1)
# Allow empty passphrase, but there should be a warning
resp = self.nodes[0].createwallet(wallet_name='w7', disable_private_keys=False, blank=False, passphrase='')
assert_equal(resp['warning'], 'Empty string given as passphrase, wallet will not be encrypted.')

View File

@ -21,12 +21,13 @@ from test_framework.util import (
class WalletUpgradeToHDTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
self.extra_args = [['-usehd=0']]
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
def setup_network(self):
self.add_nodes(self.num_nodes)
self.add_nodes(self.num_nodes, self.extra_args)
self.start_nodes()
self.import_deterministic_coinbase_privkeys()
@ -69,7 +70,8 @@ class WalletUpgradeToHDTest(BitcoinTestFramework):
self.log.info("Should no longer be able to start it with HD disabled")
self.stop_node(0)
node.assert_start_raises_init_error(['-usehd=0'], "Error: Error loading %s: You can't disable HD on an already existing HD wallet" % self.default_wallet_name)
self.start_node(0)
self.extra_args = []
self.start_node(0, [])
balance_after = node.getbalance()
self.recover_non_hd()
@ -188,7 +190,7 @@ class WalletUpgradeToHDTest(BitcoinTestFramework):
node.stop()
node.wait_until_stopped()
self.start_node(0, extra_args=['-rescan'])
assert_raises_rpc_error(-14, "Cannot upgrade encrypted wallet to HD without the wallet passphrase", node.upgradetohd, mnemonic)
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 node.upgradetohd(mnemonic, "", walletpass)
assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.dumphdinfo)

View File

@ -12,10 +12,11 @@ import os
import shutil
from test_framework.blocktools import COINBASE_MATURITY
from test_framework.test_framework import (BitcoinTestFramework, SkipTest)
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
assert_greater_than,
assert_greater_than_or_equal,
assert_is_hex_string,
)
@ -26,26 +27,21 @@ class UpgradeWalletTest(BitcoinTestFramework):
self.num_nodes = 3
self.extra_args = [
[], # current wallet version
["-usehd=1"], # v0.16.3 wallet
["-usehd=0"] # v0.15.2 wallet
["-usehd=1"], # v18.2.2 wallet
["-usehd=0"] # v0.16.1.1 wallet
]
self.wallet_names = [self.default_wallet_name]
self.wallet_names = [self.default_wallet_name, None, None]
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
self.skip_if_no_bdb()
self.skip_if_no_previous_releases()
# TODO: this test doesn't work yet
raise SkipTest("Test wallet_upgradewallet.py is not adapted for Dash Core yet.")
def setup_network(self):
self.setup_nodes()
def setup_nodes(self):
self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[
None,
160300,
150200,
18020200, # that's last version before `default wallets` are created
160101, # that's oldest version that support `import_deterministic_coinbase_privkeys`
])
self.start_nodes()
self.import_deterministic_coinbase_privkeys()
@ -61,13 +57,13 @@ class UpgradeWalletTest(BitcoinTestFramework):
Further info: https://github.com/bitcoin/bitcoin/pull/18774#discussion_r416967844
"""
node_from = self.nodes[0]
v16_3_node = self.nodes[1]
v18_2_node = self.nodes[1]
to_height = node_from.getblockcount()
height = self.nodes[1].getblockcount()
for i in range(height, to_height+1):
b = node_from.getblock(blockhash=node_from.getblockhash(i), verbose=0)
v16_3_node.submitblock(b)
assert_equal(v16_3_node.getblockcount(), to_height)
v18_2_node.submitblock(b)
assert_equal(v18_2_node.getblockcount(), to_height)
def run_test(self):
self.nodes[0].generatetoaddress(COINBASE_MATURITY + 1, self.nodes[0].getnewaddress())
@ -76,28 +72,28 @@ class UpgradeWalletTest(BitcoinTestFramework):
res = self.nodes[0].getblockchaininfo()
assert_equal(res['blocks'], COINBASE_MATURITY + 1)
node_master = self.nodes[0]
v16_3_node = self.nodes[1]
v15_2_node = self.nodes[2]
v18_2_node = self.nodes[1]
v16_1_node = self.nodes[2]
# Send coins to old wallets for later conversion checks.
v16_3_wallet = v16_3_node.get_wallet_rpc('wallet.dat')
v16_3_address = v16_3_wallet.getnewaddress()
node_master.generatetoaddress(COINBASE_MATURITY + 1, v16_3_address)
v18_2_wallet = v18_2_node.get_wallet_rpc(self.default_wallet_name)
v18_2_address = v18_2_wallet.getnewaddress()
node_master.generatetoaddress(COINBASE_MATURITY + 1, v18_2_address)
self.dumb_sync_blocks()
v16_3_balance = v16_3_wallet.getbalance()
v18_2_balance = v18_2_wallet.getbalance()
self.log.info("Test upgradewallet RPC...")
# Prepare for copying of the older wallet
node_master_wallet_dir = os.path.join(node_master.datadir, "regtest/wallets")
v16_3_wallet = os.path.join(v16_3_node.datadir, "regtest/wallets/wallet.dat")
v15_2_wallet = os.path.join(v15_2_node.datadir, "regtest/wallet.dat")
v18_2_wallet = os.path.join(v18_2_node.datadir, "regtest/wallets/wallet.dat")
v16_1_wallet = os.path.join(v16_1_node.datadir, "regtest/wallets/wallet.dat")
self.stop_nodes()
# Copy the 0.16.3 wallet to the last Dash Core version and open it:
shutil.rmtree(node_master_wallet_dir)
os.mkdir(node_master_wallet_dir)
shutil.copy(
v16_3_wallet,
v18_2_wallet,
node_master_wallet_dir
)
self.restart_node(0, ['-nowallet'])
@ -111,16 +107,16 @@ class UpgradeWalletTest(BitcoinTestFramework):
assert_equal(wallet.upgradewallet(), "")
new_version = wallet.getwalletinfo()["walletversion"]
# upgraded wallet version should be greater than older one
assert_greater_than(new_version, old_version)
assert_greater_than_or_equal(new_version, old_version)
# wallet should still contain the same balance
assert_equal(wallet.getbalance(), v16_3_balance)
assert_equal(wallet.getbalance(), v18_2_balance)
self.stop_node(0)
# Copy the 0.15.2 wallet to the last Dash Core version and open it:
# Copy the 19.3.0 wallet to the last Dash Core version and open it:
shutil.rmtree(node_master_wallet_dir)
os.mkdir(node_master_wallet_dir)
shutil.copy(
v15_2_wallet,
v16_1_wallet,
node_master_wallet_dir
)
self.restart_node(0, ['-nowallet'])
@ -131,12 +127,17 @@ class UpgradeWalletTest(BitcoinTestFramework):
assert_equal('hdseedid' in wallet.getwalletinfo(), False)
# calling upgradewallet with explicit version number
# should return nothing if successful
assert_equal(wallet.upgradewallet(169900), "")
assert_equal(wallet.upgradewallet(120200), "")
new_version = wallet.getwalletinfo()["walletversion"]
# upgraded wallet should have version 169900
assert_equal(new_version, 169900)
# after conversion master key hash should be present
assert_is_hex_string(wallet.getwalletinfo()['hdseedid'])
# upgraded wallet would not have 120200 version until HD seed actually appeared
assert_greater_than(120200, new_version)
# after conversion master key hash should not be present yet
assert 'hdchainid' not in wallet.getwalletinfo()
assert_equal(wallet.upgradetohd(), True)
new_version = wallet.getwalletinfo()["walletversion"]
assert_equal(new_version, 120200)
assert_is_hex_string(wallet.getwalletinfo()['hdchainid'])
if __name__ == '__main__':
UpgradeWalletTest().main()