feat: enable HD wallets by default #5807

knst · 2024-01-09T11:41:18Z

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:

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

kwvg

LGTM, minor notes

kwvg · 2024-01-29T17:27:38Z

src/wallet/wallet.cpp

@@ -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


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?

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

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.

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)

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.

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.

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.

kwvg · 2024-01-29T18:04:14Z

src/wallet/rpcwallet.cpp

-            throw JSONRPCError(RPC_WALLET_ERROR, "Cannot downgrade wallet");
+    SecureString secureWalletPassphrase;
+    secureWalletPassphrase.reserve(100);
+    // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)


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.

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

kwvg · 2024-01-29T18:07:24Z

src/wallet/rpcwallet.cpp

-        if (!pwallet->SetMaxVersion(FEATURE_HD)) {
-            throw JSONRPCError(RPC_WALLET_ERROR, "Cannot downgrade wallet");
+    SecureString secureWalletPassphrase;
+    secureWalletPassphrase.reserve(100);


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.

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

kwvg · 2024-01-29T18:11:16Z

src/wallet/wallettool.cpp

+    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("", "");


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

Minor nit

spk_man->GenerateNewHDChain("" /* secureMnemonic */, "" /* secureMnemonicPassphrase */);

Need to use correct format at least :)

spk_man->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"" );

To be fair, we seem to use every possible variant though the last one is what upstream seems to prefer now even if I find it unsightly. Was only suggesting that there be an inline comment and wrote one for convenience...

test/functional/wallet_upgradewallet.py

RPC upgradetohd keeps only rpc's related code same as upgradewallet RPC same as bitcoin#15761

kwvg

LGTM, utACK

kwvg · 2024-02-01T10:13:16Z

test/functional/wallet_upgradewallet.py

-            ["-usehd=0"]             # v0.15.2 wallet
+            [],                      # current wallet version
+            ["-usehd=1"],            # v18.2.2 wallet
+            ["-usehd=0"]             # v0.16.1.1 wallt


Minor nit, spelling.

UdjinM6

LGTM overall, check comments below for 2 small issues (fixes: cda12bd)

src/wallet/wallet.cpp

Version is changed from 19.3.0 to 0.16.1.1 Even older doesn't support importing private coinbase key at the moment. Version 20.x is also changed to 18.2 to the latest which do not create wallets by default for sack of unification with bitcoin and to fix issue with `createwallet` rpc which is subject to change once descriptor wallets would be backported.

UdjinM6

light ACK

ogabrielides

LGTM in general

PastaPastaPasta

Tested; looks good. But needs release notes :)

doc/release-notes-5807.md

UdjinM6 · 2024-02-07T18:38:07Z

LGTM but I'll wait for @thephez :)

thephez

Release notes look okay to me 👍

UdjinM6

utACK

ogabrielides

LGTM in general, good job +1

PastaPastaPasta

utACK

…t HD feature a392be6 fix: actually run rpc_fundrawtransaction with and without HD feature (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Due to double initialization of `extra_args` rpc_fundrawtransaction always test with enabled or disabled HD wallets (which is enabled by default). ## What was done? Fixes double initialization and inversion of condition due to hd wallets enabled by default since #5807 ## How Has This Been Tested? Run functional tests and watch `ps aux` during running to ensure that `-usehd` is set properly. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] 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 Top commit has no ACKs. Tree-SHA512: 6e595b4073c50f0355d9f64b966b1a0613a7bb8dae3406523fae737e54725ffc652aa9db30daf46c9640d98f44883780bd075074d56b7e8baeeb0a1d2d007c20

…itcoin#18787, bitcoin#18805, bitcoin#18888, bitcoin#19502, bitcoin#19077, bitcoin#20125, bitcoin#20153, bitcoin#20198, bitcoin#20262, bitcoin#20266, bitcoin#23608, bitcoin#29510 - native descriptor wallets 6b71f27 Merge bitcoin#29510: wallet: `getrawchangeaddress` and `getnewaddress` failures should not affect keypools for descriptor wallets (Ava Chow) 85fa370 refactor: use Params().ExtCoinType() for descriptor wallets (Konstantin Akimov) da8e563 fix: skip functional tests which requires BDB if no bdb (see 20267) (Konstantin Akimov) 4ba44fa fix: skip interface_zmq.py which is not ready to work without bdb (Konstantin Akimov) 45fc8a4 fix: autobackup influences an exclusive locks made by SQLite (Konstantin Akimov) e542cd2 fix: missing changes from bitcoin#21634 (Konstantin Akimov) 2de7aec Merge bitcoin#19502: Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks (Samuel Dobson) c172605 Merge bitcoin#19077: wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets (Samuel Dobson) 2439247 Merge bitcoin#23608: test: fix `feature_rbf.py --descriptors` and add to test runner (fanquake) f6b3614 fix: descriptor wallets follow-up to merge bitcoin#20202: Make BDB support optional (Konstantin Akimov) a340ad6 Merge bitcoin#20262: tests: Skip --descriptor tests if sqlite is not compiled (Samuel Dobson) 7d55046 Merge bitcoin#20125: rpc, wallet: Expose database format in getwalletinfo (Samuel Dobson) 343d4b0 fix: descriptor wallets follow-up for bitcoin#20156: Make sqlite support optional (compile-time) (Konstantin Akimov) fa30777 Merge bitcoin#20198: Show name, format and if uses descriptors in bitcoin-wallet tool (MarcoFalke) 14121ec Merge bitcoin#18888: test: Remove RPCOverloadWrapper boilerplate (MarcoFalke) b18351e Merge bitcoin#20153: wallet: do not import a descriptor with hardened derivations into a watch-only wallet (Wladimir J. van der Laan) c995e5d Merge bitcoin#20266: wallet: fix change detection of imported internal descriptors (Wladimir J. van der Laan) c864582 Merge bitcoin#18787: wallet: descriptor wallet release notes and cleanups (Samuel Dobson) 0949c08 Merge bitcoin#18782: wallet: Make sure no DescriptorScriptPubKeyMan or WalletDescriptor members are left uninitialized after construction (Samuel Dobson) baa6959 Merge bitcoin#18805: tests: Add missing sync_all to wallet_importdescriptors.py (MarcoFalke) 76e08f9 Merge bitcoin#18027: "PSBT Operations" dialog (Samuel Dobson) c1b94b6 fix: wallet should be unlocked before generating keys for Descriptor wallet (Konstantin Akimov) f293c04 Merge bitcoin#16528: Native Descriptor Wallets using DescriptorScriptPubKeyMan (Andrew Chow) 4064334 fix: get receiving address for Descriptor Wallets (Konstantin Akimov) bdbd0b1 chore: dashification of descriptor implementation in dash (UdjinM6) b02fc0b fix: counting calculation of internal keys for Descriptor Wallets (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented This PR is a batch of backports and related fixes to add a support of native descriptor wallets to Dash Core. There're more related backports, but this PR is a minimal package of backports to get descriptor wallets working and unit/functional tests to succeed. To do: bitcoin#20226, bitcoin#21049, bitcoin#18788, bitcoin#20267, bitcoin#19230, bitcoin#19239, bitcoin#19441, bitcoin#19568, bitcoin#19979, bitcoin-core/gui#96, bitcoin#19136, bitcoin#21277, bitcoin#21063, bitcoin#21302, bitcoin#19651, bitcoin#20191, bitcoin#22446 and other. Prior work: - #5580 - #5807 ## What was done? backports: - bitcoin#16528 - bitcoin#18027 - bitcoin#18805 - bitcoin#18782 - bitcoin#18787 - bitcoin#20266 - bitcoin#20153 - bitcoin#18888 - bitcoin#20198 - bitcoin#20125 - bitcoin#20262 - bitcoin#23608 - bitcoin#19077 - bitcoin#19502 - bitcoin#29510 and extra fixes and missing changes for bitcoin#20156, bitcoin#20202, bitcoin#20267, bitcoin#21634 + fix of auto-backup for sqlite wallets. ## How Has This Been Tested? There're 2 new functional tests: `wallet_importdescriptors.py` and `wallet_descriptor.py` Beside that many functional tests run twice now: using legacy wallet and descriptor wallets: `wallet_hd.py`, `wallet_basic.py`, `wallet_labels.py`, `wallet_keypool_topup.py`, `wallet_avoidreuse.py`, `rpc_psbt.py`, `wallet_keypool_hd.py`, `rpc_createmultisig.py`, `wallet_encryption.py`. With bitcoin#18788 expected to more tests run. ## Breaking Changes N/A ## 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 - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: Rebase looks good; utACK 6b71f27 PastaPastaPasta: utACK 6b71f27 UdjinM6: utACK 6b71f27 kwvg: utACK 6b71f27 Tree-SHA512: 776c5dfe1eec2b5bebc8d606476cd981c810ac81965b348e78c13e96fff23be500c495ae68c93f669403941c96eccdd3775f2b96572163c34175900e15549b5d

knst marked this pull request as draft January 10, 2024 09:37

knst force-pushed the default-usehd branch from 1132f18 to 3e2ba10 Compare January 24, 2024 18:56

feat: use HD wallets by default

4ae116d

knst force-pushed the default-usehd branch from 3e2ba10 to 973518e Compare January 25, 2024 20:14

knst added this to the 20.1 milestone Jan 25, 2024

knst changed the title ~~WIP! feat: enable HD wallets by default WIP!~~ feat: enable HD wallets by default Jan 25, 2024

knst force-pushed the default-usehd branch from 973518e to a4bdcfd Compare January 25, 2024 20:30

knst marked this pull request as ready for review January 25, 2024 20:30

knst requested review from UdjinM6, ogabrielides, PastaPastaPasta and kwvg January 25, 2024 20:30

knst force-pushed the default-usehd branch from a4bdcfd to 96d0e2f Compare January 25, 2024 20:38

kwvg reviewed Jan 29, 2024

View reviewed changes

knst force-pushed the default-usehd branch from 6a6939b to 2a10245 Compare January 29, 2024 18:57

knst added a commit to knst/dash that referenced this pull request Jan 31, 2024

feat: cummulative changes from dashpay#5807 use HD wallets by default

09fd498

knst mentioned this pull request Jan 31, 2024

refactor: move HD code from CWallet to LegacyScriptPubKeyMan #5854

Merged

5 tasks

knst added 4 commits February 1, 2024 04:33

fix: now HD wallet can be created from the scratch by default

4620a0e

fix: functional test wallet_upgradewallet.py works now!

2a5a08d

feat: forbid wallet upgrade from non-HD to HD by upgradewallet

3de3c7e

refactor: move upgradetohd implementation from rpcwallet to wallet.

bfec081

RPC upgradetohd keeps only rpc's related code same as upgradewallet RPC same as bitcoin#15761

knst force-pushed the default-usehd branch from 2a10245 to cf03c33 Compare January 31, 2024 21:43

knst requested a review from kwvg February 1, 2024 09:58

kwvg approved these changes Feb 1, 2024

View reviewed changes

UdjinM6 requested changes Feb 1, 2024

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

knst requested a review from UdjinM6 February 1, 2024 16:07

knst and others added 4 commits February 2, 2024 00:06

chore: encourage users to encrypt wallets

b698a9f

chore: postpone breaking changes of wallet_upgradewallet to v21

af49d5a

suggestions

8c29608

knst force-pushed the default-usehd branch from 4ea68b3 to 8c29608 Compare February 1, 2024 17:06

UdjinM6 previously approved these changes Feb 1, 2024

View reviewed changes

ogabrielides previously approved these changes Feb 1, 2024

View reviewed changes

PastaPastaPasta requested changes Feb 7, 2024

View reviewed changes

knst dismissed stale reviews from ogabrielides and UdjinM6 via 801eaef February 7, 2024 18:02

knst requested review from thephez and PastaPastaPasta February 7, 2024 18:03

UdjinM6 reviewed Feb 7, 2024

View reviewed changes

doc/release-notes-5807.md Outdated Show resolved Hide resolved

docs: add release notes

73b5372

knst force-pushed the default-usehd branch from 801eaef to 73b5372 Compare February 7, 2024 18:36

knst requested a review from UdjinM6 February 7, 2024 18:37

thephez approved these changes Feb 7, 2024

View reviewed changes

UdjinM6 approved these changes Feb 7, 2024

View reviewed changes

ogabrielides approved these changes Feb 7, 2024

View reviewed changes

PastaPastaPasta approved these changes Feb 9, 2024

View reviewed changes

PastaPastaPasta merged commit 8dba655 into dashpay:develop Feb 9, 2024
6 of 10 checks passed

knst mentioned this pull request Feb 12, 2024

backport: bitcoin#16528, #18027, #18782, #18787, #18805, #18888, #19502, #19077, #20125, #20153, #20198, #20262, #20266, #23608, #29510 - native descriptor wallets #5579

Merged

5 tasks

knst mentioned this pull request Feb 28, 2024

fix: actually run rpc_fundrawtransaction with and without HD feature #5907

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable HD wallets by default #5807

feat: enable HD wallets by default #5807

knst commented Jan 9, 2024 •

edited

Loading

kwvg left a comment

kwvg Jan 29, 2024

knst Jan 29, 2024

kwvg Jan 29, 2024 •

edited

Loading

knst Jan 29, 2024

kwvg Jan 29, 2024 •

edited

Loading

knst Jan 29, 2024

kwvg Jan 29, 2024 •

edited

Loading

kwvg Jan 29, 2024

knst Jan 31, 2024

kwvg Jan 29, 2024

knst Jan 31, 2024

kwvg Jan 29, 2024

PastaPastaPasta Jan 29, 2024

kwvg Jan 29, 2024 •

edited

Loading

kwvg left a comment

kwvg Feb 1, 2024

UdjinM6 left a comment

UdjinM6 left a comment

ogabrielides left a comment

PastaPastaPasta left a comment

UdjinM6 commented Feb 7, 2024

thephez left a comment

UdjinM6 left a comment

ogabrielides left a comment

PastaPastaPasta left a comment

feat: enable HD wallets by default #5807

feat: enable HD wallets by default #5807

Conversation

knst commented Jan 9, 2024 • edited Loading

Issue being fixed or feature implemented

What was done?

What is not done?

How Has This Been Tested?

Breaking Changes

Checklist:

kwvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwvg Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwvg Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwvg Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwvg Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

kwvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

ogabrielides left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 commented Feb 7, 2024

thephez left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

ogabrielides left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

knst commented Jan 9, 2024 •

edited

Loading

kwvg Jan 29, 2024 •

edited

Loading

kwvg Jan 29, 2024 •

edited

Loading

kwvg Jan 29, 2024 •

edited

Loading

kwvg Jan 29, 2024 •

edited

Loading