Page MenuHomePhabricator

Allow -upgradewallet to upgradewallets to HD and use a keypool of presplit keys after upgrading to hd chain split
ClosedPublic

Authored by nakihito on Thu, Oct 3, 17:39.

Details

Summary

(commit #1)
Changes the maximum upgradewallet version to the latest wallet version
number, 190700. Non-HD wallets will be upgraded to use HD derivation.
Non HD chain split wallets will be upgraded to HD chain split.

(commit #2)
After upgrading to HD chain split, we want to continue to use keys
from the old keypool. To do this, before we generate any new keys after
upgrading, we mark all of the keypool entries as being pre-chain
split and move them to a separate pre chain split keypool. Keys are
fetched from that keypool until it is emptied. Only then are the new
internal and external keypools used.

Since upgradewallet is effectively run during a first run, all of the
first run initial setup stuff is combined with the upgrade to HD

Partial backport of Core PR12560 (3/4)
https://github.com/bitcoin/bitcoin/pull/12560/commits/5c50e93d52c14fc7bc41130cdb1568f2c11e45de
https://github.com/bitcoin/bitcoin/pull/12560/commits/dfcd9f3e6abf3d53903227a085ff4cfecbfeb07f

Depends on D4202

Test Plan

Recommend to perform testing on regtest or else this process may take a very long time.

make check
./bitcoind -upgradewallet
./bitcoin-cli getwalletinfo > preupgradewalletinfo
./bitcoin-cli generate 1
Kill bitcoind
./bitcoind -printtoconsole -upgradewallet

bitcoind should output to console the two following lines:

Performing wallet upgrade to 190700
Upgrading wallet to use HD chain split

Compare previous preupgradewalletinfo with ./bitcoin-cli getwalletinfo post upgrade

preupgradewalletinfo:

{
  "walletname": "",
  "walletversion": 160300,
  "balance": 0.00000000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 0,
  "keypoololdest": 1551725443,
  "keypoolsize": 1000,
  "keypoolsize_hd_internal": 1000,
  "paytxfee": 0.00000000,
  "hdmasterkeyid": "c13ec93790a56f74edd2da42d231317eb6dbf02b"
}

post upgrade wallet info:

{
  "walletname": "",
  "walletversion": 190700,
  "balance": 0.00000000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 50.00000000,
  "txcount": 1,
  "keypoololdest": 1551725443,
  "keypoolsize": 1000,
  "keypoolsize_hd_internal": 1000,
  "paytxfee": 0.00000000,
  "hdmasterkeyid": "c13ec93790a56f74edd2da42d231317eb6dbf02b"
}

The newly upgraded wallet should not be able to be downgraded to a previous version

Kill bitcoind
./bitcoind -upgradewallet=160300

This should fail and cause bitcoind to shutdown with the following line:

Error: Cannot downgrade wallet

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Thu, Oct 3, 17:39
Owners added a reviewer: Restricted Owners Package.Thu, Oct 3, 17:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Oct 3, 17:39
nakihito planned changes to this revision.Thu, Oct 3, 17:39
nakihito edited the test plan for this revision. (Show Details)Thu, Oct 3, 17:41
nakihito edited the test plan for this revision. (Show Details)Thu, Oct 3, 17:50
nakihito edited the test plan for this revision. (Show Details)Thu, Oct 3, 18:25
nakihito updated this revision to Diff 13342.Thu, Oct 3, 18:37

Squashed together with this commit: https://github.com/bitcoin/bitcoin/pull/12560/commits/dfcd9f3e6abf3d53903227a085ff4cfecbfeb07f because the squashed commit's changes to behavior is prefered to the previous behavior.

nakihito planned changes to this revision.Thu, Oct 3, 18:37
nakihito retitled this revision from Allow -upgradewallet to upgradewallets to HD to Allow -upgradewallet to upgradewallets to HD and use a keypool of presplit keys after upgrading to hd chain split.Thu, Oct 3, 18:38
nakihito edited the summary of this revision. (Show Details)
nakihito edited the summary of this revision. (Show Details)
nakihito edited the test plan for this revision. (Show Details)Thu, Oct 3, 18:49
nakihito edited the test plan for this revision. (Show Details)Thu, Oct 3, 18:52
jasonbcox added inline comments.Fri, Oct 4, 21:35
src/wallet/wallet.h
88 ↗(On Diff #13342)

Note for other reviewers: https://github.com/bitcoin/bitcoin/pull/12560/commits/5c50e93d52c14fc7bc41130cdb1568f2c11e45de#r182192873
There was some discussion by Core that this may lead to compatibility issues where a default key is present, but not supported. Considering the default key was removed in D1051, it appears safe that this diff is landed separately from D4207.

jasonbcox accepted this revision.Fri, Oct 4, 21:45
This revision is now accepted and ready to land.Fri, Oct 4, 21:45
nakihito updated this revision to Diff 13370.Sun, Oct 6, 19:21

Rebased.