[Wallet] HD Wallet - v2. #1327

furszy · 2020-02-10T14:22:22Z

HD Wallet implementation (BIP44 derivation path) streamlined to upstream flow and db object's serialization storage.

Summarizing few important changes of this work:

Restructured over latest upstream's architecture.

Meaning that this new wallet code organization will make easier most of the upcoming backports, having it as the base structure for every feature that we have and new ones that we will be implementing. Introducing the ScriptPubKeyMan (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys, more information about the wallet class architecture can be found here and here.

Supporting pre and post HD wallet keys.

This means that old wallets that want to upgrade to HD wallet, can do it directly within the wallet. No need to move the balance to a fresh new wallet. Can import single private keys too, the wallet will continue storing and managing them in parallel of the hd seed derived keys.

Key Metadata Upgrade

The key metadata has been upgraded to support key origin (hd path) and the hd seed identifier.

HD Master Seed.

The master seed is being managed by the keystore, same as with new and old keys (Single flow for both of them).

Staking addresses derivation path.

The "change" level in the BIP44 definition has been extended to support staking addresses generation. We will have three different purposes:

External receiving addresses.
Internal receiving addresses.
Staking addresses, (external receiving addresses starting with an 'S' for staking contracts).

A really short view of the HD classes structure:

CWallet ---> SPKM ---> HDChain —> chain_counters.

Upgrade to HD wallet flow.

The automatic upgrade can be done starting the node with the flag -upgradewallet or going to settings and pressing on the Upgrade Wallet button.

RPC commands:

New commands:

sethdseed set a new HD wallet seed.
getaddressinfo obtains the address information, including the key derivation path.

Upgraded commands:

dumpwallet : HD wallet export included into the flow.

Tests:

Most of the previous functional test has been upgraded to support and verify HD functionality.

wallet_backup.py
wallet_keypool.py
wallet_topup_keypool.py
wallet_basic.py
wallet_dump.py

New functional test created:

wallet_hd.py .
upgrade_wallet.py.

Next PRs

Improve scripts management (multisig - P2CS).
Further wallet.h/cpp cleanup. Continue decoupling the keys/scripts management to the SPKM box. (This work will need several PRs more).
Upgrade from encrypted wallet.

Fuzzbawls

Not an in-depth review, but a reminder of a previously discussed topic.

looks like init.cpp's help message was changed previously (and in error, at the time) to reflect a much smaller number of default keypairs. but now that keypairs are tied to a deterministic "seed", there is no real benefit in pre-generating 1000 keypairs.

src/wallet/wallet.cpp

random-zebra

Code ACK with few questions.
Fantastic job 🍻

Will give it some more live testing.

src/wallet/scriptpubkeyman.h

src/wallet/scriptpubkeyman.cpp

src/wallet/rpcwallet.cpp

src/wallet/scriptpubkeyman.cpp

test/functional/test_runner.py

random-zebra · 2020-02-21T15:42:33Z

src/wallet/rpcdump.cpp

-            if (boost::algorithm::starts_with(vstr[nStr], "label=")) {
+            else if (type == "hdseed")
+                fLabel = false;
+            if (boost::algorithm::starts_with(type, "label=")) {


guess this can be else if too

src/wallet/hdchain.h

src/wallet/rpcdump.cpp

random-zebra · 2020-02-21T16:47:08Z

This change is pretty important and user-facing so, imo, there should be a separate paragraph for it in the release notes (with indications for upgrade/backup best practice).
Also the rpc section needs to be completed:

new rpc: sethdseed and getaddressinfo
indication for getinfo: keypoolsize only counts external keys

src/wallet/rpcwallet.cpp

src/wallet/wallet.h

furszy · 2020-02-22T14:46:55Z

Updated following @random-zebra' comments 👌 .

test/functional/test_runner.py

src/wallet/scriptpubkeyman.cpp

furszy · 2020-02-23T14:35:26Z

I skipped the last comment, sorry. Yeah @random-zebra, will write a separate release notes section for this. Agree that it's needed.

Fuzzbawls

couple styling changes, and a question

src/wallet/rpcwallet.cpp

src/wallet/wallet.h

Fuzzbawls

looks like some missing upgrade functionality

src/init.cpp

akshaynexus · 2020-02-25T21:16:30Z

is there any method to extract the mnemonic via the rpc commands if any?

furszy · 2020-02-25T21:22:55Z

@akshaynexus there is no mnemonic. BIP39 was not implemented.

If you meant the seed, you can export it calling dumpwallet. It will export pre and post HD keys all together, including the master seed.

akshaynexus · 2020-02-25T21:24:59Z

Oh ok

furszy · 2020-03-01T18:57:14Z

Upgrade wallet from non-HD to HD functional test included, PR details updated. All good, travis passing.

…s (wallet)

… convention. [Test] bip32_tests key.SetSeed() changed to the new SetMaster refactor

furszy · 2020-03-04T14:54:28Z

PR rebased, had conflicts after #1337 merge.

random-zebra

Few nits here and there. Think this is pretty close to merge. Still missing release notes

src/wallet/scriptpubkeyman.cpp

src/wallet/wallet.cpp

test/functional/wallet_upgrade.py

random-zebra

Good opportunity to fix the paths in wallet_dump.py as well.

test/functional/wallet_dump.py

…her old comments cleanup.

furszy · 2020-03-06T19:55:06Z

PR updated with the release notes. Let me know about any typo or needed change there.

Fuzzbawls · 2020-03-09T07:43:26Z

Seems like there may have been some commits left out. Reading your changes to the release notes; i'm not seeing anything during running or code review that adds a new topbar icon or a GUI notification of running a non-HD wallet. There also isn't the upgradewallet RPC command that is referenced.

furszy · 2020-03-09T12:48:35Z

@Fuzzbawls yes, working in another branch. I just cherry-picked the release notes change here to merge this PR and not continue making it bigger.
The plan is push the upgradewallet from encrypted file and the GUI upgrade notification flow in a subsequent PR.

random-zebra

sethdseed can't be used with arguments at the moment, as it doesn't interpret the first one as boolean.
Need to add {"sethdseed", 0} to vRPCConvertParams in rpc/client.cpp

furszy · 2020-03-09T15:59:52Z

@random-zebra done.

random-zebra · 2020-03-09T16:08:12Z

Yes, this looks quite good by now. I think it can be merged and anything else can be addressed in successive pull requests.

random-zebra

ACK 818dac7

Fuzzbawls

ACK 818dac7

0e969f1 [GUI] Launch upgrade wallet dialog at the wallet's startup. (furszy) 7c468ce [Trivial] Method brackets jump lines. (furszy) 42ed283 [GUI] Upgrade wallet to HD functionality connected. (furszy) c29d34e [GUI] isHDEnabled & upgradeWallet methods created. (furszy) Pull request description: This was built on top of PIVX-Project#1399. Essentially connecting the HD wallet upgrade to the GUI as it was defined in the release-notes in PIVX-Project#1327. ACKs for top commit: random-zebra: ACK 0e969f1 Fuzzbawls: ACK 0e969f1 Tree-SHA512: 8d6ee4a5c53b2bead2303b8fd241ef7404ee5cb62599118e3328073b7ce6b5b63d7555bb5d26e5d3b266514476d81d1deb3f42c738216ea788c87babc019cf88

furszy self-assigned this Feb 10, 2020

Fuzzbawls requested changes Feb 11, 2020

View reviewed changes

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

furszy added the Wallet label Feb 12, 2020

furszy force-pushed the 2020_hd_wallet_rebased_7_feb branch from 18b3a6b to 3549d2a Compare February 12, 2020 19:15

furszy changed the title ~~[WIP] HD Wallet - v2.~~ HD Wallet - v2. Feb 14, 2020

random-zebra reviewed Feb 21, 2020

View reviewed changes

akshaynexus reviewed Feb 21, 2020

View reviewed changes

src/wallet/rpcdump.cpp Show resolved Hide resolved

random-zebra added this to the 4.1.0 milestone Feb 21, 2020

random-zebra added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Feb 21, 2020

random-zebra reviewed Feb 21, 2020

View reviewed changes

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

random-zebra reviewed Feb 21, 2020

View reviewed changes

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

furszy force-pushed the 2020_hd_wallet_rebased_7_feb branch from e45d2c2 to ff77b5a Compare February 22, 2020 14:51

random-zebra reviewed Feb 22, 2020

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

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

furszy force-pushed the 2020_hd_wallet_rebased_7_feb branch from ff77b5a to f9e929c Compare February 22, 2020 17:30

Fuzzbawls requested changes Feb 25, 2020

View reviewed changes

src/wallet/rpcwallet.cpp Show resolved Hide resolved

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

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

Fuzzbawls reviewed Feb 25, 2020

View reviewed changes

src/init.cpp Show resolved Hide resolved

furszy force-pushed the 2020_hd_wallet_rebased_7_feb branch from 50ee7cb to ff2fefa Compare February 29, 2020 21:18

furszy requested review from Fuzzbawls and random-zebra February 29, 2020 21:21

furszy added 4 commits March 4, 2020 11:21

[Crypter] vMasterKey protected to be able to get it in the child clas…

4198824

…s (wallet)

[Refactor] CExtKey, SetSeed changed to SetMaster following upstream's…

5276621

… convention. [Test] bip32_tests key.SetSeed() changed to the new SetMaster refactor

KeyOriginInfo struct created.

922a831

memory.h file created, upstream backport.

c731f26

furszy force-pushed the 2020_hd_wallet_rebased_7_feb branch from ff2fefa to 5748d39 Compare March 4, 2020 14:53

random-zebra previously approved these changes Mar 4, 2020

View reviewed changes

random-zebra reviewed Mar 4, 2020

View reviewed changes

test/functional/wallet_dump.py Outdated Show resolved Hide resolved

[Wallet] Upgrade to HD from a locked wallet error notification + furt…

7f7ffae

…her old comments cleanup.

furszy dismissed random-zebra’s stale review via 7f7ffae March 5, 2020 23:21

furszy added 2 commits March 5, 2020 21:18

[Tests] Replace invalid / separator for OS specific separator.

2072ebc

[Doc] HD Wallet release notes.

1cccdea

furszy force-pushed the 2020_hd_wallet_rebased_7_feb branch from 29f040c to 1cccdea Compare March 6, 2020 19:31

[Lint] Do not check for trailing spaces in release-notes.md file.

a40cc8a

furszy requested a review from random-zebra March 6, 2020 19:53

random-zebra added enhancement GUI RPC and removed Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes labels Mar 8, 2020

random-zebra suggested changes Mar 9, 2020

View reviewed changes

[RPC] Missing sethdseed.

818dac7

random-zebra approved these changes Mar 9, 2020

View reviewed changes

Fuzzbawls approved these changes Mar 11, 2020

View reviewed changes

Fuzzbawls changed the title ~~HD Wallet - v2.~~ [Wallet] HD Wallet - v2. Mar 11, 2020

furszy merged commit d58fd6f into PIVX-Project:master Mar 11, 2020

furszy mentioned this pull request Mar 13, 2020

[GUI] Upgrade to HD wallet functionality. #1405

Merged

random-zebra mentioned this pull request Mar 21, 2020

Add BIP39/44 functionality #378

Closed

furszy deleted the 2020_hd_wallet_rebased_7_feb branch November 29, 2022 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wallet] HD Wallet - v2. #1327

[Wallet] HD Wallet - v2. #1327

furszy commented Feb 10, 2020 •

edited

Loading

Fuzzbawls left a comment

random-zebra left a comment

random-zebra Feb 21, 2020

random-zebra commented Feb 21, 2020

furszy commented Feb 22, 2020

furszy commented Feb 23, 2020

Fuzzbawls left a comment

Fuzzbawls left a comment

akshaynexus commented Feb 25, 2020

furszy commented Feb 25, 2020

akshaynexus commented Feb 25, 2020

furszy commented Mar 1, 2020

furszy commented Mar 4, 2020

random-zebra left a comment

random-zebra left a comment

furszy commented Mar 6, 2020 •

edited

Loading

Fuzzbawls commented Mar 9, 2020

furszy commented Mar 9, 2020

random-zebra left a comment

furszy commented Mar 9, 2020

random-zebra commented Mar 9, 2020

random-zebra left a comment

Fuzzbawls left a comment

[Wallet] HD Wallet - v2. #1327

[Wallet] HD Wallet - v2. #1327

Conversation

furszy commented Feb 10, 2020 • edited Loading

Summarizing few important changes of this work:

Restructured over latest upstream's architecture.

Supporting pre and post HD wallet keys.

Key Metadata Upgrade

HD Master Seed.

Staking addresses derivation path.

A really short view of the HD classes structure:

Upgrade to HD wallet flow.

RPC commands:

New commands:

Upgraded commands:

Tests:

Next PRs

Fuzzbawls left a comment

Choose a reason for hiding this comment

random-zebra left a comment

Choose a reason for hiding this comment

random-zebra Feb 21, 2020

Choose a reason for hiding this comment

random-zebra commented Feb 21, 2020

furszy commented Feb 22, 2020

furszy commented Feb 23, 2020

Fuzzbawls left a comment

Choose a reason for hiding this comment

Fuzzbawls left a comment

Choose a reason for hiding this comment

akshaynexus commented Feb 25, 2020

furszy commented Feb 25, 2020

akshaynexus commented Feb 25, 2020

furszy commented Mar 1, 2020

furszy commented Mar 4, 2020

random-zebra left a comment

Choose a reason for hiding this comment

random-zebra left a comment

Choose a reason for hiding this comment

furszy commented Mar 6, 2020 • edited Loading

Fuzzbawls commented Mar 9, 2020

furszy commented Mar 9, 2020

random-zebra left a comment

Choose a reason for hiding this comment

furszy commented Mar 9, 2020

random-zebra commented Mar 9, 2020

random-zebra left a comment

Choose a reason for hiding this comment

Fuzzbawls left a comment

Choose a reason for hiding this comment

furszy commented Feb 10, 2020 •

edited

Loading

furszy commented Mar 6, 2020 •

edited

Loading