Skip to content
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.

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

feat: enable HD wallets by default #5807

Merged
merged 10 commits into from
Feb 9, 2024
2 changes: 1 addition & 1 deletion src/qt/forms/createwalletdialog.ui
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
<string>Encrypt Wallet</string>
</property>
<property name="checked">
<bool>false</bool>
<bool>true</bool>
</property>
</widget>
</item>
Expand Down
97 changes: 37 additions & 60 deletions src/wallet/rpcwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2681,76 +2681,53 @@ 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");
SecureString secureWalletPassphrase;
secureWalletPassphrase.reserve(100);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The GUI reserves MAX_PASSPHRASE_SIZE for the wallet.

It's unlikely someone will actually come around with a password >100 characters but since we allow longer passwords in the GUI, we should have RPC behave in the same way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This call reserve has been introduced before introducing SecureString (which use custom allocator).
That call is needed to be sure that string is not accidentally re-allocated and it would be impossible to wipe a data by rewriting bytes.

This call is just for legacy and completely useless now. I'd better to consider removing reserve completely rather than keep it here.

btw, that's not new code, I just moved it a bit; I will consider removing reserve at all, not planning to refactor it in this PR

// TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: This review comment is not a blocker for this pull request

This TODO has been lingering around for ~12 years in Bitcoin Core (as of this writing) and it seems to be finally removed with bitcoin#27068. Might be worth to backport OOO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will do it in the next PRs after #5807 (this one) and #5854 (HD wallet refactorings).

Alongs bitcoin#27068, I'd like to introduce std::optional in hdchain and couple more changes

// 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");
}
}

if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet");
}
EnsureWalletIsUnlocked(pwallet.get());

bool prev_encrypted = pwallet->IsCrypted();
SecureString secureMnemonic;
secureMnemonic.reserve(256);
if (!generate_mnemonic) {
secureMnemonic = request.params[0].get_str().c_str();
}

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 {
secureWalletPassphrase = request.params[2].get_str().c_str();
if (!pwallet->Unlock(secureWalletPassphrase)) {
throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "The wallet passphrase entered was incorrect");
}
}
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.");
}

SecureString secureMnemonic;
secureMnemonic.reserve(256);
if (!generate_mnemonic) {
secureMnemonic = request.params[0].get_str().c_str();
}
bilingual_str error;
const bool wallet_upgraded{pwallet->UpgradeToHD(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase, error)};

SecureString secureMnemonicPassphrase;
secureMnemonicPassphrase.reserve(256);
if (!request.params[1].isNull()) {
secureMnemonicPassphrase = request.params[1].get_str().c_str();
if (!secureWalletPassphrase.empty() && !pwallet->IsCrypted()) {
if (!pwallet->EncryptWallet(secureWalletPassphrase)) {
throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet");
}
}

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");
}
} else {
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
if (!secureWalletPassphrase.empty()) {
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
Expand Down
78 changes: 58 additions & 20 deletions src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -306,29 +310,25 @@ 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");
status = DatabaseStatus::FAILED_ENCRYPT;
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;
Expand Down Expand Up @@ -4601,6 +4601,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);
Expand Down Expand Up @@ -4630,10 +4634,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");
Expand Down Expand Up @@ -4897,7 +4897,45 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error)
return false;
}

// TODO: consider discourage users to skip passphrase for HD wallets for v21
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: This review comment is not a blocker for this pull request

IMO users should not be allowed at all to have unencrypted wallets unless running with some sort of debug flag. It's a lingering security risk and I doubt most users delete their unencrypted wallet file by first overwriting its content with random data and then deleting it ("shredding").

I've noticed when using third party light clients, they usually don't allow unencrypted export, they might let you pick a weak password but they won't let you not have one at all.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An user of dash-core can already have air-gapped device, or it can be encrypted disk, virtual machine, etc.

Forcing user to use passphrase for light client maybe a good idea, but for case of cold wallet it may lead to lost funds if user will forget passphrase after creating one "just do not bother me, here's a simple password" that he may not remember because uses device once in awhile. so, users should be discourage, but not forced IMO

Copy link
Collaborator

@kwvg kwvg Jan 29, 2024

Choose a reason for hiding this comment

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

There is definitely a risk of forgetting a password but that's inherent to any form of encryption, that be using Cryptomator, BitLocker or FileVault, all three urge you to have a physical copy of the key for recovery should you forget your password.

Since we're dealing with user funds here, assuming a mildly clumsy pair of hands (someone who forgot to delete unencrypted copies of their wallet) instead of a cautious pair of hands (a seasoned user who has encryption schemes for their protected environment in lieu of not using a password for their wallet) would be reducing user funds at risk, IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

someone who forgot to delete unencrypted copies of their wallet

you may delete it, but they are still written on disk, until you re-write that sector of disk (what is close to impossible for ssd)

Copy link
Collaborator

@kwvg kwvg Jan 29, 2024

Choose a reason for hiding this comment

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

I doubt most users delete their unencrypted wallet file by first overwriting its content with random data and then deleting it ("shredding")

I did mention most people don't "shred" the file and opt to simply delete it but my example of a clumsy user would be one who didn't delete the file at all, because even if the file isn't "shredded", a deleted file overwritten with new data in the now freed up region overwriting (maybe parts of it) is still more frustrating to recover than one that's intact and forgotten about, which can just be copied over.

what is close to impossible for ssd

Writing repeated patterns is something SSDs try to "optimize away" and there is some truth to wear optimization complicating things but overwriting the file with random data and then deleting it would frustrate recovery attempts much more significantly than plain deletion. (Edit: This is incorrect, see comment below.)


The bottom line is, I think security policies should be stricter with opt-out ability, rather than having softer policies and an opt-in to stricter policies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Writing repeated patterns is something SSDs try to "optimize away" and there is some truth to wear optimization complicating things but overwriting the file with random data and then deleting it would frustrate recovery attempts much more significantly than plain deletion.

not exactly; I meant that writing on SSD same logical block happen everytime to different physical blocks; and when you trying to shred something 100 times - it would be 100 different blocks written, but not your original one due to wear optimization; logical blocks no need to be mapped next-to-each other as on hdd, they can be complely different physical blocks each recording.

Copy link
Collaborator

@kwvg kwvg Jan 29, 2024

Choose a reason for hiding this comment

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

Then I seem to be mistaken about how "shredding" works on SSDs in that it doesn't work at all due to fundamental decoupling of logical and physical blocks with wear levelling taking the wheel. There are methods that claim to do the job but I can't substantiate them, the only "proper" means seem to be using ATA Secure Erase but that applies to the whole drive, not just the file.

Though this seems to point in the direction that we should avoid writing unencrypted data at all since overwriting the file alone after encrypting it after-the-fact isn't feasible on SSDs.

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

Expand Down
6 changes: 5 additions & 1 deletion src/wallet/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1278,6 +1278,7 @@ 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 GenerateNewHDChainEncrypted(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase);

Expand Down Expand Up @@ -1330,6 +1331,9 @@ 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
8 changes: 5 additions & 3 deletions src/wallet/wallettool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
16 changes: 1 addition & 15 deletions test/functional/tool_wallet.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,21 +97,6 @@ def test_tool_wallet_info(self):
# 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
===========
Expand All @@ -122,6 +107,7 @@ def test_tool_wallet_info(self):
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)
Expand Down
4 changes: 1 addition & 3 deletions test/functional/wallet_createwallet.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,7 @@ def run_test(self):
# 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.')
Expand Down
8 changes: 5 additions & 3 deletions test/functional/wallet_upgradetohd.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@
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()

Expand Down Expand Up @@ -69,7 +70,8 @@ def run_test(self):
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()
Expand Down Expand Up @@ -188,7 +190,7 @@ def run_test(self):
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)
Expand Down
Loading
Loading