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

[3/3] key, wallet: HD wallets #2540

Merged
merged 19 commits into from
Dec 4, 2022

Conversation

div72
Copy link
Member

@div72 div72 commented Jul 7, 2022

Adds HD wallet support. Introduces two new RPCs: sethdseed and upgradewallet(previously a cli argument).

This PR does not introduce BIP39/seed phrases support.

Upgrade Instructions

Old pre-HD wallets will not automatically be upgraded and will continue to work as normal. HD can be enabled for old wallets like this:

  1. Unlock wallet if locked.
  2. Call upgradewallet (5040101 can be given as a second argument if the wallet is not meant to be fully upgraded; does not affect anything for now.)
  3. Wait. (The generation and the flush of the keys might take a few minutes.)

Note: New wallets are automatically created as HD wallets and do not need to be updated.
Note 2: Unused keys in the key pool will be removed when doing this action. When dealing with multiple out of sync wallet.dat files caution must be taken care as losing the other files can cause loss of keys/funds.

Backup/Restore Instructions

A backup of the wallet.dat file is enough as the seed is contained in the file. Alternatively the private key associated with the seed can be backed up like this:

  1. Do a walletdump. (walletdump <file>)
  2. Get the key marked with hdmaster=1 or hdseed=1 in the file.
  3. Backup that key.

and restored with:
sethdseed true {key}

Note: This operation can also take a few minutes.
Note 2: The wallet dump files contain unencrypted private keys and should be removed after the backup.
Note 3: If the sethdseed command is used from a shell, cleaning the shell's $HISTFILE might be a good idea.

Testing checklist

  • Ability to re-create private keys from a given seed.
  • Old wallets are able to generate private keys without an issue.
  • Receiving and sending is not impacted.

Fixes #570.
Fixes #2325.

@div72 div72 force-pushed the key-bip32 branch 3 times, most recently from e7aea8d to 459699f Compare July 7, 2022 21:00
@div72 div72 self-assigned this Jul 7, 2022
@div72 div72 marked this pull request as ready for review July 7, 2022 23:11
@jamescowens
Copy link
Member

Do we want to try to get this in Kermit's Mom?

@div72
Copy link
Member Author

div72 commented Jul 17, 2022

It's a nice to have but it carries a non-insignificant bug risk and without upgradewallet being an RPC, encrypted wallets can't be upgraded.

@div72 div72 force-pushed the key-bip32 branch 2 times, most recently from b20b598 to afcf9c3 Compare August 1, 2022 18:43
@div72 div72 requested a review from jamescowens August 21, 2022 15:43
Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

In general this works really well. I don't have any comments on the coding style. Based on my testing I have the following suggestions:

  1. The new commit which raises the keypool (re)fill size to 1000 should only apply for wallets that have not been upgraded. Post upgrade wallets should use a much smaller keypool size because otherwise it takes 2-4 minutes to upgrade the wallet and/or generate a new one on first wallet startup and or setting a new seed. Since the BIP32 keys are deterministic, a large keypool by default post upgrade is not necessary. (Having calls take this long with no feedback is asking for trouble as well, but cutting the keypool size down should solve this problem I think.)
  2. We need to make it clear in the documentation that an upgraded wallet is a HYBRID wallet. The hdmaster key will only restore the keys associated with the hdmaster. The pre-BIP32 keys must still be retained as well. (i.e. for a hybrid wallet, the actual set of keys to be retained for restoration purposes are all important pre-BIP32 keys + the hdmaster key.)
  3. A hybrid wallet should be able to be restored in one step by doing importwallet on a file containing pre BIP32 keys + the hdmaster key. Right now the restoration requires two steps. Import the wallet file, where the pre-BIP32 keys are pulled in, and then do the sethdseed with the hdmaster key. (In other words the hdmaster key in the import file is actually ignored - we should have it automatically processed and do the sethdseed behind the scenes.)

Actually the hdmaster key is not ignored, rather it is imported as a normal (loose) key, which then prevents resetting the seed to that key. We need to fix this...

div72 added a commit to div72/Gridcoin-Research that referenced this pull request Sep 29, 2022
Rationale:
    As Jim pointed out in gridcoin-community#2540 (review)
    post-HD wallets have essentially no risk of losing funds as long as
    the wallet file is intact, therefore having the default keypool
    refill size being high as the pre-HD and having to wait for the
    keypool refill is unnecesarry.

Interestingly, I originally formatted the change in src/init.cpp to be
a separate line for the DEFAULT_KEYPOOL_SIZE_PRE_HD argument but
clang-format seems to prefer a single line. Oh well.
Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

ACK

@jamescowens jamescowens added this to the LaVerne milestone Oct 3, 2022
@jamescowens
Copy link
Member

I will merge this right after I do the 5.4.1.0 point release.

@jamescowens
Copy link
Member

@div72 you want to rebase this so we can merge it?

div72 and others added 6 commits December 4, 2022 17:42
Rationale:
    Due to a test order issue, BIP32 tests fail under Wine as this test
    does not set the chain back to mainnet which BIP32 tests expect.
Rationale:
    As Jim pointed out in gridcoin-community#2540 (review)
    post-HD wallets have essentially no risk of losing funds as long as
    the wallet file is intact, therefore having the default keypool
    refill size being high as the pre-HD and having to wait for the
    keypool refill is unnecesarry.

Interestingly, I originally formatted the change in src/init.cpp to be
a separate line for the DEFAULT_KEYPOOL_SIZE_PRE_HD argument but
clang-format seems to prefer a single line. Oh well.
Rationale:
    After an importwallet the hdmaster key is imported but not set
    as seed causing the user to be unable to use it as a seed. This
    commit changes it so that existing keys can be used as seed so
    that users will be able to use their seeds after importing until
    the interface is improved.
@jamescowens jamescowens merged commit 5d91b93 into gridcoin-community:development Dec 4, 2022
jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Mar 26, 2023
Added
 - key, wallet: HD wallets gridcoin-community#2540 (@div72)
 - ARMv8 SHA2 Intrinsics gridcoin-community#2612 (@barton2526)
 - build: vendor bdb 5.3 gridcoin-community#2620 (@div72)
 - scraper, gui: Add external adapter projects indication gridcoin-community#2625 (@jamescowens)
 - gui: implement INSUFFICIENT_MATURE_FUNDS status for the mrcmodel gridcoin-community#2628 (@jamescowens)
 - gui, accrual: Implement accrual limit warning gridcoin-community#2636 (@jamescowens)
 - rpc: add `getnodeaddresses` gridcoin-community#2646 (@Pythonix)
 - consensus: Add new checkpoints gridcoin-community#2651 (@barton2526)

Changed
 - voting: Optimize poll locks gridcoin-community#2619 (@jamescowens)
 - util: move threadinterrupt.{cpp,h} to util gridcoin-community#2613 (@Pythonix)
 - gui, voting: Update pool cpids and avw rules gridcoin-community#2624 (@jamescowens)
 - ci: bump python and setup-python action version gridcoin-community#2626 (@div72)
 - gui: Change text from username to name (real name or nickname) gridcoin-community#2633 (@jamescowens)
 - locale: Translation update, phase 1 gridcoin-community#2637 (@jamescowens)
 - gui: Change MRC too soon to submit error to be less confusing gridcoin-community#2645 (@jamescowens)
 - locale: Update translations prior to release (phase 2/2) gridcoin-community#2658 (@jamescowens)
 - gui: Enhance MRC request form to avoid fee boost field confusion gridcoin-community#2659 (@jamescowens)

Removed
none

Fixed
 - net: Turn net structures into dumb storage classes (backport) gridcoin-community#2561 (@Pythonix)
 - build: Include native_X.mk before X.mk gridcoin-community#2609 (@barton2526)
 - depends: fix OpenSSL for Darwin builds gridcoin-community#2610 (@div72)
 - build: Change actions runner image to Focal, Force Lint to use 22.04, Change cd runner version gridcoin-community#2611 (@barton2526)
 - gui: don't show datadir error msgbox if arg isn't specified gridcoin-community#2617 (@div72)
 - rpc: Repair auditsnapshotaccrual rpc function gridcoin-community#2621 (@jamescowens)
 - gui: Correct updateBeaconIcon() function in bitcoingui.cpp gridcoin-community#2622 (@jamescowens)
 - wallet: Strengthen CWalletTx::RevalidateTransactions gridcoin-community#2627 (@jamescowens)
 - test: Fix Wambiguous-reversed-operator compiler warning, drop boost::assign gridcoin-community#2632 (@barton2526)
 - gui: Fix wallet overview displaying lower-case poll name gridcoin-community#2640 (@delta1513)
 - Fix and optimize ResendWalletTransactions gridcoin-community#2642 (@jamescowens)
 - build(nsis): Write registry keys to HKLM instead of HKCU, Install shortcuts for all users, Fix INSTALLDIR removal bug gridcoin-community#2643 (@sitiom)
 - gui: Fix TransactionRecord::decomposeTransaction to properly display self-sidestake gridcoin-community#2647 (@jamescowens)
 - rpc: Fixed the RPC error when running `help voting` while syncing gridcoin-community#2649 (@delta1513)
 - build: Fix compilation with GCC 13 gridcoin-community#2653 (@theMarix)
 - rpc: Formatting - typo correction rpc help for listresearcheraccounts gridcoin-community#2654 (@PrestackI)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An option to disable automatic keypool refill Feature Request: Implement deterministic wallets
3 participants