Skip to content

Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) #16341

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 70 commits into from
Closed

Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) #16341

wants to merge 70 commits into from

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Jul 5, 2019 •
edited
Loading

Introducing the ScriptPubKeyMan (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from CWallet. Instead, CWallet will have a pointer to a ScriptPubKeyMan for every possible address type, internal and external. It will fetch the correct ScriptPubKeyMan as necessary. When fetching new addresses, it chooses the ScriptPubKeyMan based on address type and whether it is change. For signing, it takes the script and asks each ScriptPubKeyMan for whether that ScriptPubKeyMan considers that script IsMine, whether it has that script, or whether it is able to produce a signature for it. If so, the ScriptPubKeyMan will provide a SigningProvider to the caller which will use that in order to sign.

There is currently one ScriptPubKeyMan - the LegacyScriptPubKeyMan. Each CWallet will have only one LegacyScriptPubKeyMan with the pointers for all of the address types and change pointing to this LegacyScriptPubKeyMan. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of CWallet. The LegacyScriptPubKeyMan is primarily made up of all of the key and script management that used to be in CWallet. For convenience, CWallet has a GetLegacyScriptPubKeyMan which will return the LegacyScriptPubKeyMan or a nullptr if it does not have one (not yet implemented, but callers will check for the nullptr). For purposes of signing, LegacyScriptPubKeyMan's GetSigningProvider will return itself rather than a separate SigningProvider. This will be different for future ScriptPubKeyMans.

The LegacyScriptPubKeyMan will also handle the importing and exporting of keys and scripts instead of CWallet. As such, a number of RPCs have been limited to work only if a LegacyScriptPubKeyMan can be retrieved from the wallet. These RPCs are sethdseed, addmultisigaddress, importaddress, importprivkey, importpubkey, importmulti, dumpprivkey, and dumpwallet. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the SigningProvider retrieved from the ScriptPubKeyMan for a given script.

Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing CWallet functions that have been removed.

This PR is the last step in the Wallet Structure Changes.

The commits are organized as follows:

  • Miscellaneous changes that don't necessarily make sense outside of this PR
    • Move wallet enums to walletutil.h
    • List output types in an array in order to be iterated over
    • Always try to sign for all pubkeys in multisig
  • Interface definitions and miscellaneous changes in preparation for ScriptPubKeyMan integration
    • Introduce both ScriptPubKeyMan as an interface and LegacyScriptPubKeyMan as a dummy class
    • Add LegacyScriptPubKeyMan to CWallet
    • Add function callbacks for wallet flags and versions and wallet database
    • Fetch the SigningProvider for a script from the wallet
    • Fetch the ScriptPubKeyMan for given output type and internal-ness, or a given script, or ScriptPubKeyMan id
  • Implementation of LegacyScriptPubKeyMan by copying existing code from CWallet. These will pass all tests and do not affect CWallet
    • Implement GetSigningProvider in LegacyScriptPubKeyMan
    • Implement function to connect ScriptPubKeyMan's NotifyCanGetAddessesChanged and NotifyWatchOnlyChanged to CWallet's
    • Implement IsLocked and IsCrypted in LegacyScriptPubKeyMan
    • Implement LoadCryptedKey and AddCryptedKey in LegacyScriptPubKeyMan
    • Implement UpdateTimeFirstKey, and GetTimeFirstKey in LegacyScriptPubKeyMan
    • Implement AddWatchOnly, RemoveWatchOnly, HaveWatchOnly, and LoadWatchOnly in LegacyScriptPubKeyMan
    • Implement AddKeyPubKey and LoadKey in LegacyScriptPubKeyMan
    • Implement WalletLogPrintf in LegacyScriptPubKeyMan
    • Implement SetHDCHain, and IsHDEnabled in LegacyScriptPubKeyMan
    • Implement LoadCScript in LegacyScriptPubKeyMan
    • Implement LoadKeyMetadata and LoadScriptMetadata in LegacyScriptPubKeyMan
    • Implement GetKey, HaveKey, and GetPubKey in LegacyScriptPubKeyMan
    • Implement GenerateNewKey in LegacyScriptPubKeyMan
    • Implement LoadKeyPool in LegacyScriptPubKeyMan
    • Implement GetOldestKeyPoolTime, KeypoolCountExternalKeys, and GetKeypoolSize in LegacyScriptPubKeyMan
    • Implement CanGetAddresses, CanGenerateKeys, and HavePrivateKeys in LegacyScriptPubKeyMan
    • Implement GenerateNewSeed, DeriveNewSeed, and SetHDSeed for LegacyScriptPubKeyMan
    • Implement TopUpKeypool, TopUp, and NewKeyPool in LegacyScriptPubKeyMan
    • Implement ReturnAddress, and KeepKey in LegacyScriptPubKeyMan
    • Implement GetNewAddress, and GetReservedAddress in LegacyScriptPubKeyMan
    • Implement MarkUnusedAddresses in LegacyScriptPubKeyMan
    • Implement IsMine in LegacyScriptPubKeyMan
    • Implement UpgradeKeyMetaData, SetupGeneration, IsFirstRun, Upgrade, RewriteDB in LegacyScriptPubKeyMan
    • Implement Unlock, Lock, and Encrypt and LegacyScriptPubKeyMan
    • Implement ImporScripts, ImportPrivKeys, ImportPubKeys, and ImportScriptPubKeys in LegacyScriptPubKeyMan
    • Implement GetMetadata in LegacyScriptPubKeyMan
    • Implement GetKeyOrigin in LegacyScriptPubKeyMan
    • Implement actually loading everything into LegacyScriptPubKeyMan
    • Implement CanProvide in LegacyScriptPubKeyMan
  • Replacing CWallet functions and RPC things with calls to ScriptPubKeyMan or LegacyScriptPubKeyMan. These will compile but are not expected to pass tests hence the [ci skip].
    • [ci skip] Remove CWallet from IsMine and have CWallet always use ScriptPubKeyMan's IsMine
    • [ci skip] moveonly: move ismine stuff to be a module of LegacyScriptPubKeyMan
    • [ci skip] Have GetNewAddress, GetNewChangeAddress, and ReserveAddress use ScriptPubKeyMan
    • [ci skip] Mark used addresses in ScriptPubKeyMan
    • [ci skip] Call UpgradeKeyMetaData for each ScriptPubKeyMan
    • [ci skip] Sign using SigningProvider from ScriptPubKeyMan when signing within CWallet
    • [ci skip] Do not allow import*, dump*, and addmultisigaddress RPCs when wallet is not backed by LegacyScriptPubKeyMan
    • [ci skip] Change Imports to use LegacyScriptPubKeyMan Imports
    • [ci skip] Use SigningProviders and ScriptPubKeyMans in listunspent, signmessage, signrawtransactionwithwallet, and getaddressinfo
    • [ci skip] Use LegacyScriptPubKeyMan in addmultisigaddress and sethdseed
    • [ci skip] Use LegacyScriptPubKeyMan for hdseedid in getwalletinfo
    • [ci skip] Change KeypoolCountExternal and GetKeypoolSize to get aggregate sizes from ScriptPubKeyMans
    • [ci skip] Have IsHDEnabled fetch from ScriptPubKeyMans
    • [ci skip] Fetch oldest keypool time from ScriptPubKeyMans
    • [ci skip] have TopUpKeyPool call TopUp in each ScriptPubKeyMan
    • [ci skip] Have EncryptWallet, Lock, and Unlock call their respective functions in ScriptPubKeyMans
    • [ci skip] Use LegacyScriptPubKeyMan throughout psbt_wallet_tests
    • [ci skip] Use LegacyScriptPubKeyMan throughout wallettool
    • [ci skip] Use ScriptPubKeyMans' Setup and Upgrade functions when loading or creating a wallet
    • [ci skip] Define first run as having no ScriptPubKeyMans
    • [ci skip] Use RewriteDB action when DB needs rewrite
    • [ci skip] Use GetTimeFirstKey instead of nTimeFirstKey
    • [ci skip] Use LegacyScriptPubKeyMan for in wallet_tests
    • [ci skip] Use LegacyScriptPubKeyMan in dumpprivkey and dumpwallet
    • [ci skip] Change CanGetAddresses to fetch from ScriptPubKeyMan
    • [ci skip] Fetch the correct SigningProvider for signing PSBTs
    • [ci skip] Use LegacyScriptPubKeyMan in test util
    • [ci skip] Use LegacyScriptPubKeyMan in some parts of getbalances and createwallet
    • [ci skip] Have getPubKey and getPrivKey use SigningProvider
    • [ci skip] Use LegacyScriptPubKeyMan in benchmarks involving the wallet
    • [ci skip] Store p2sh scripts in AddAndGetDestinationForScript
  • Tying everything together and removing the CWallet functions.
    • Remove unused functions and switch CWallet to use ScriptPubKeyMan

@achow101
Copy link
Member Author

achow101 commented Jul 5, 2019

In the coming days I will try to squash together some of these commits so there aren't so many of them. I just wanted to get this open now for people to begin reviewing the changes.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 5, 2019 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17246 (wallet: avoid knapsack when there's no change by Sjors)
  • #17237 (wallet: LearnRelatedScripts only if KeepDestination by promag)
  • #17219 (wallet: allow transaction without change if keypool is empty by Sjors)
  • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
  • #16946 (wallet: include a checksum of encrypted private keys by achow101)
  • #16910 (wallet: reduce loading time by using unordered maps by achow101)
  • #16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
  • #16440 (BIP-322: Generic signed message format by kallewoof)
  • #16037 (rpc: Enable wallet import on pruned nodes by promag)
  • #15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
  • #15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
  • #15294 ([moveonly] wallet: Extract RipeMd160 by Empact)
  • #14942 (wallet: Make scan / abort status private to CWallet by Empact)
  • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
  • #14384 (Fire TransactionRemovedFromMempool callbacks from mempool by l2a5b1)
  • #14144 (Refactoring: Clarify code using encrypted_batch in CWallet by domob1812)
  • #10785 (Serialization improvements by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@achow101 achow101 force-pushed the box-the-wallet branch 6 times, most recently from 3fdfd70 to 1404f8c Compare July 5, 2019 22:16
fqlx
fqlx reviewed Jul 6, 2019
View reviewed changes
@fanquake fanquake changed the title Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) Jul 6, 2019
@fanquake
Copy link
Member

fanquake commented Jul 6, 2019

Everyone commenting here, note that this PR is a work in progress, and is built on top of multiple open PRs.

For now we should try and keep the discussion here at a Concept ACK/NACK & design level, rather than individual nit-picks and code review.

If you would like to review the code, please do so in the open base PRs. That will not only save the concept level discussion here from being drowned out in inline comments, but will also prevent duplicate review. i.e What you're commenting on here might have already been addressed/pointed out in the base PRs.

@achow101 achow101 force-pushed the box-the-wallet branch 4 times, most recently from 1dfa1c8 to d34d213 Compare July 6, 2019 06:30
@meshcollider
Copy link
Contributor

Concept ACK, this abstraction is definitely the right path for the wallet to take IMO as we work toward descriptor wallets.

@Sjors Sjors mentioned this pull request Jul 11, 2019
Closed
@Sjors
Copy link
Member

Sjors commented Jul 11, 2019 •
edited
Loading

Approach ACK.

To make some of the commits easier to review, instead of having one commit that adds a function to LegacyScriptPubKeyMan (e.g. LegacyScriptPubKeyMan::LoadKey), one commit that deletes it from CWallet (Remove unused functions and switch CWallet to use ScriptPubKeyMan) and one commit that moves the call over (Implement actually loading everything into LegacyScriptPubKeyMan), try to combine those. That lets you get rid of the CI Skip commits, and makes it easier to verify move-only parts with --color-moved=dimmed-zebra.

In other words, start with empty (Legacy)ScriptPubKeyMan and then migrate one function per commit into it.

Are you sure you want to move WalletFeature and WalletFlags into LegacyScriptPubKeyMan? I would prefer to keep these settings global to the wallet. It does make sense for each ScriptPubKeyMan to have its own IsLocked() state, so that brings up the question what to do if a single one fails to unlock. But I'd rather not add RPC methods to unlock specific PubKeyMans in a given wallet.

I brainstormed how to apply this to hardware wallets: #14912 (comment). The unsollicited keypool topup in GetReservedDestination is annoying in that context. Can ScriptPubKeyMan have a flag to opt out of that?

@achow101
Copy link
Member Author

To make some of the commits easier to review, instead of having one commit that adds a function to LegacyScriptPubKeyMan (e.g. LegacyScriptPubKeyMan::LoadKey), one commit that deletes it from CWallet (Remove unused functions and switch CWallet to use ScriptPubKeyMan) and one commit that moves the call over (Implement actually loading everything into LegacyScriptPubKeyMan), try to combine those. That lets you get rid of the CI Skip commits, and makes it easier to verify move-only parts with --color-moved=dimmed-zebra.

In other words, start with empty (Legacy)ScriptPubKeyMan and then migrate one function per commit into it.

Doing that will cause the entirety of the PR to fail all tests (so all need ci skip) except for the very last commit. If other reviewers want that, I could reorganize it.

Are you sure you want to move WalletFeature and WalletFlags into LegacyScriptPubKeyMan? I would prefer to keep these settings global to the wallet.

They are global to the wallet. They are moved to walletutil.h. Both LegacyScriptPubKeyMan and CWallet require access to both of these enums, but having scriptpubkeyman including wallet.h would be a circular dependency, so I moved it to walletutil.h.

It does make sense for each ScriptPubKeyMan to have its own IsLocked() state, so that brings up the question what to do if a single one fails to unlock.

I guess it should fail to unlock everything? That would indicate some corruption has occurred.

But I'd rather not add RPC methods to unlock specific PubKeyMans in a given wallet.

Agreed.

The unsollicited keypool topup in GetReservedDestination is annoying in that context. Can ScriptPubKeyMan have a flag to opt out of that?\

I suppose it should actually be in LegacyScriptPubKeyMan::GetReservedDestination rather than CWallet's. However, TopUp can just do nothing and that won't effect this behavior. Every ScriptPubKeyMan needs to implement TopUp anyways.

achow101 added a commit to achow101/bitcoin that referenced this pull request Dec 6, 2019
@achow101
HD Split: Avoid redundant upgrades
4099344 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Dec 12, 2019
@achow101
HD Split: Avoid redundant upgrades
f28008c 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Dec 13, 2019
@achow101
HD Split: Avoid redundant upgrades
a3e5ffe 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Dec 17, 2019
@achow101
HD Split: Avoid redundant upgrades
216f655 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Dec 17, 2019
@achow101
HD Split: Avoid redundant upgrades
eca2793 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Dec 30, 2019
@achow101
HD Split: Avoid redundant upgrades
ecdbacc 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 6, 2020
@achow101
HD Split: Avoid redundant upgrades
c6a061d 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 6, 2020
@achow101
HD Split: Avoid redundant upgrades
05e08b8 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 6, 2020
@achow101
HD Split: Avoid redundant upgrades
b890cac 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 15, 2020
@achow101
HD Split: Avoid redundant upgrades
c5842c1 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 15, 2020
@achow101
HD Split: Avoid redundant upgrades
8540dcb 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 17, 2020
@achow101
HD Split: Avoid redundant upgrades
d27e13f 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 17, 2020
@achow101
HD Split: Avoid redundant upgrades
7a9fef7 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 17, 2020
@achow101
HD Split: Avoid redundant upgrades
5b58007 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 23, 2020
@achow101
HD Split: Avoid redundant upgrades
415afcc 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
JeremyRubin pushed a commit that referenced this pull request Feb 7, 2020
@achow101 @JeremyRubin
HD Split: Avoid redundant upgrades
7f08a0e 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
#16341 (comment)
@furszy furszy mentioned this pull request Feb 10, 2020
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Mar 11, 2020
@furszy
Merge #1327: [Wallet] HD Wallet - v2.
d58fd6f 
818dac7 [RPC] Missing sethdseed. (furszy)
a40cc8a [Lint] Do not check for trailing spaces in release-notes.md file. (furszy)
1cccdea [Doc] HD Wallet release notes. (furszy)
2072ebc [Tests] Replace invalid `/` separator for OS specific separator. (furszy)
7f7ffae [Wallet] Upgrade to HD from a locked wallet error notification + further old comments cleanup. (furszy)
5748d39 [Test] HD wallet upgrade functional test. (furszy)
cc2f0eb [Wallet] Bugfix, CReservedKey internal flag initialized. (furszy)
1f426ac [Wallet] Not used wallet feature versions commented (plus init bug fix). (furszy)
051a4c5 [Bug] Return key to the proper keypool. (furszy)
5b94a2f [Wallet] GetOldestKeyPoolTime implemented inside the SPKM. (furszy)
69e57c2 [Wallet] throw error if staking key was misclassified. (furszy)
025c02a [Cleanup] Removing not used methods. (furszy)
805ed41 [RPC] sethdseed help text corrections. (furszy)
cc1a60b [Wallet] Upgrade to HD flow with -upgradewallet flag. (furszy)
25ee29b Cleanup: dumpwallet never empty str check removed + duplicated line clean. (furszy)
d0325eb [RPC] getwalletinfo keypoolsize_hd_staking count added. (furszy)
2580cab [Wallet] CanSupportFeature(FEATURE_HD_SPLIT) moved to IsHDEnabled. (furszy)
deef0b3 [Wallet] Default keypool size decreased to 100. (furszy)
a7ea0b0 keyOrigin size validation fix. (furszy)
0f87145 [Cleanup] Removing unused NewKeyPool from the wallet class. (furszy)
4a79a08 [Wallet] GetKeyFromPool using the new keypool type instead of the internal boolean. (furszy)
7a8a1fd [Unittest] Remove rpc_wallet_tests.cpp. (furszy)
403ff69 [SPKM] Staking addresses derivation path. (furszy)
9a7d0fd [Wallet] HD Wallet, staking chain counter. (furszy)
811fb29 linux link issue fix (furszy)
e0621f4 KeyIO namespace. (furszy)
8377ef4 [Cleanup] Unifying internal/external chain counter increment. (furszy)
5fb1b7f [Build] missing HD new files. (furszy)
70ba39b [RPC] `importwallet` hdseed key type added. (furszy)
84a8403 [RPC] `dumpwallet` exporting hd key path. (furszy)
2864d82 rpc: Prevent `dumpwallet` from overwriting files. (furszy)
ac16d22 [Test] Test runner, wallet_hd.py added. (furszy)
eebb3fc [Test] wallet_keypool_topup fix. (furszy)
bcc8796 [Tests] Backport HD wallet functional test. (furszy)
23aeeb5 [Wallet] Use internal key for tx change. (furszy)
198e70e key_io DecodeSecret backported. (furszy)
ac87b5c [Wallet] HD chain scan + mark unused addresses flow implemented. (furszy)
37d89f6 [RPC] sethdseed implemented. (furszy)
3e79435 [Wallet] SPKM single account. (furszy)
c791b7a [RPC] getaddressinfo implemented. (furszy)
80a8407 BIP44 derivation path + HD wallet functional tests passing. (furszy)
beb5fcb Remove DecodeBase58Check warning (furszy)
99f6071 test framework, at least 10 keys in the keypool to be able to generate blocks on demand. (furszy)
698bd64 [Backport] key_io.h/cpp (furszy)
155a8d9 HD Wallet connected to the wallet. (furszy)
ffb719f CHDChain class created. (furszy)
4b33cef [Backport] Simple GetScriptForRawPubKey coming from upstream. (furszy)
c731f26 memory.h file created, upstream backport. (furszy)
922a831 KeyOriginInfo struct created. (furszy)
5276621 [Refactor] CExtKey, SetSeed changed to SetMaster following upstream's convention. (furszy)
4198824 [Crypter] vMasterKey protected to be able to get it in the child class (wallet) (furszy)

Pull request description:

  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](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes) and  [here](bitcoin#16341).

  #### 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:
  1. External receiving addresses.
  2. Internal receiving addresses.
  3. 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.

ACKs for top commit:
  random-zebra:
    ACK 818dac7
  Fuzzbawls:
    ACK 818dac7

Tree-SHA512: 366da9c681c053d23a2aec6d1ea34f951c65e72b290f6807c22a91a40013bec4eab41011034299aa519a7bf86707231bd47f01557ac174f4151062f15a5606c2
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 17, 2020
@achow101
Key pool: Move CanGetAddresses call
b329fd5 
Call LegacyScriptPubKeyMan::CanGetAddresses directly instead of calling
CWallet::CanGetAddresses to only query the relevant key manager

This is a minor change in behavior: call now only happens if a new key needs to
be reserved, since if a key is already reserved it might fail unnecessarily.

This change also serves as a sanity check
bitcoin#16341 (comment)
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 17, 2020
@achow101
Key pool: Fix omitted pre-split count in GetKeyPoolSize
6c2fbd7 
This is a bugfix: bitcoin#16341 (comment)
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 17, 2020
@achow101
HD Split: Avoid redundant upgrades
5a0000a 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin#16341 (comment)
meshcollider added a commit that referenced this pull request Apr 27, 2020
@meshcollider
Merge #16528: Native Descriptor Wallets using DescriptorScriptPubKeyMan
eef90c1 
223588b Add a --descriptors option to various tests (Andrew Chow)
869f7ab tests: Add RPCOverloadWrapper which overloads some disabled RPCs (Andrew Chow)
cf06062 Correctly check for default wallet (Andrew Chow)
886e0d7 Implement CWallet::IsSpentKey for non-LegacySPKMans (Andrew Chow)
3c19fdd Return error when no ScriptPubKeyMan is available for specified type (Andrew Chow)
388ba94 Change wallet_encryption.py to use signmessage instead of dumpprivkey (Andrew Chow)
1346e14 Functional tests for descriptor wallets (Andrew Chow)
f193ea8 add importdescriptors RPC and tests for native descriptor wallets (Hugo Nguyen)
ce24a94 Add IsLegacy to CWallet so that the GUI knows whether to show watchonly (Andrew Chow)
1cb42b2 Generate new descriptors when encrypting (Andrew Chow)
82ae02b Be able to create new wallets with DescriptorScriptPubKeyMans as backing (Andrew Chow)
b713baa Implement GetMetadata in DescriptorScriptPubKeyMan (Andrew Chow)
8b9603b Change GetMetadata to use unique_ptr<CKeyMetadata> (Andrew Chow)
72a9540 Implement FillPSBT in DescriptorScriptPubKeyMan (Andrew Chow)
84b4978 Implement SignMessage for descriptor wallets (Andrew Chow)
bde7c9f Implement SignTransaction in DescriptorScriptPubKeyMan (Andrew Chow)
d50c8dd Implement GetSolvingProvider for DescriptorScriptPubKeyMan (Andrew Chow)
f1ca5fe Implement GetKeypoolOldestTime and only display it if greater than 0 (Andrew Chow)
586b57a Implement ReturnDestination in DescriptorScriptPubKeyMan (Andrew Chow)
f866957 Implement GetReservedDestination in DescriptorScriptPubKeyMan (Andrew Chow)
a775f7c Implement Unlock and Encrypt in DescriptorScriptPubKeyMan (Andrew Chow)
bfdd073 Implement GetNewDestination for DescriptorScriptPubKeyMan (Andrew Chow)
58c7651 Implement TopUp in DescriptorScriptPubKeyMan (Andrew Chow)
e014886 Implement SetupGeneration for DescriptorScriptPubKeyMan (Andrew Chow)
46dfb99 Implement writing descriptorkeys, descriptorckeys, and descriptors to wallet file (Andrew Chow)
4cb9b69 Implement several simple functions in DescriptorScriptPubKeyMan (Andrew Chow)
d1ec3e4 Add IsSingleType to Descriptors (Andrew Chow)
953feb3 Implement loading of keys for DescriptorScriptPubKeyMan (Andrew Chow)
2363e9f Load the descriptor cache from the wallet file (Andrew Chow)
46c46ae Implement GetID for DescriptorScriptPubKeyMan (Andrew Chow)
ec2f9e1 Implement IsHDEnabled in DescriptorScriptPubKeyMan (Andrew Chow)
741122d Implement MarkUnusedAddresses in DescriptorScriptPubKeyMan (Andrew Chow)
2db7ca7 Implement IsMine for DescriptorScriptPubKeyMan (Andrew Chow)
db7177a Add LoadDescriptorScriptPubKeyMan and SetActiveScriptPubKeyMan to CWallet (Andrew Chow)
78f8a92 Implement SetType in DescriptorScriptPubKeyMan (Andrew Chow)
834de03 Store WalletDescriptor in DescriptorScriptPubKeyMan (Andrew Chow)
d813266 Add a lock cs_desc_man for DescriptorScriptPubKeyMan (Andrew Chow)
3194a7f Introduce WalletDescriptor class (Andrew Chow)
6b13cd3 Create LegacyScriptPubKeyMan when not a descriptor wallet (Andrew Chow)
aeac157 Return nullptr from GetLegacyScriptPubKeyMan if descriptor wallet (Andrew Chow)
96accc7 Add WALLET_FLAG_DESCRIPTORS (Andrew Chow)
6b8119a Introduce DescriptorScriptPubKeyMan as a dummy class (Andrew Chow)
0662030 Introduce SetType function to tell ScriptPubKeyMans the type and internal-ness of it (Andrew Chow)

Pull request description:

  Introducing the wallet of the glorious future (again): native descriptor wallets. With native descriptor wallets, addresses are generated from descriptors. Instead of generating keys and deriving addresses from keys, addresses come from the scriptPubKeys produced by a descriptor. Native descriptor wallets will be optional for now and can only be created by using `createwallet`.

  Descriptor wallets will store descriptors, master keys from the descriptor, and descriptor cache entries. Keys are derived from descriptors on the fly. In order to allow choosing different address types, 6 descriptors are needed for normal use. There is a pair of primary and change descriptors for each of the 3 address types. With the default keypool size of 1000, each descriptor has 1000 scriptPubKeys and descriptor cache entries pregenerated. This has a side effect of making wallets large since 6000 pubkeys are written to the wallet by default, instead of the current 2000. scriptPubKeys are kept only in memory and are generated every time a descriptor is loaded. By default, we use the standard BIP 44, 49, 84 derivation paths with an external and internal derivation chain for each.

  Descriptors can also be imported with a new `importdescriptors` RPC.

  Native descriptor wallets use the `ScriptPubKeyMan` interface introduced in #16341 to add a `DescriptorScriptPubKeyMan`. This defines a different IsMine which uses the simpler model of "does this scriptPubKey exist in this wallet". Furthermore, `DescriptorScriptPubKeyMan` does not have watchonly, so with native descriptor wallets, it is not possible to have a wallet with both watchonly and non-watchonly things. Rather a wallet with `disable_private_keys` needs to be used for watchonly things.

  A `--descriptor` option was added to some tests (`wallet_basic.py`, `wallet_encryption.py`, `wallet_keypool.py`, `wallet_keypool_topup.py`, and `wallet_labels.py`) to allow for these tests to use descriptor wallets. Additionally, several RPCs are disabled for descriptor wallets (`importprivkey`, `importpubkey`, `importaddress`, `importmulti`, `addmultisigaddress`, `dumpprivkey`, `dumpwallet`, `importwallet`, and `sethdseed`).

ACKs for top commit:
  Sjors:
    utACK 223588b (rebased, nits addressed)
  jonatack:
    Code review re-ACK 223588b.
  fjahr:
    re-ACK 223588b
  instagibbs:
    light re-ACK 223588b
  meshcollider:
    Code review ACK 223588b

Tree-SHA512: 59bc52aeddbb769ed5f420d5d240d8137847ac821b588eb616b34461253510c1717d6a70bab8765631738747336ae06f45ba39603ccd17f483843e5ed9a90986
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 27, 2020
@meshcollider
Merge bitcoin#16528: Native Descriptor Wallets using DescriptorScript…
46ed584 
…PubKeyMan

223588b Add a --descriptors option to various tests (Andrew Chow)
869f7ab tests: Add RPCOverloadWrapper which overloads some disabled RPCs (Andrew Chow)
cf06062 Correctly check for default wallet (Andrew Chow)
886e0d7 Implement CWallet::IsSpentKey for non-LegacySPKMans (Andrew Chow)
3c19fdd Return error when no ScriptPubKeyMan is available for specified type (Andrew Chow)
388ba94 Change wallet_encryption.py to use signmessage instead of dumpprivkey (Andrew Chow)
1346e14 Functional tests for descriptor wallets (Andrew Chow)
f193ea8 add importdescriptors RPC and tests for native descriptor wallets (Hugo Nguyen)
ce24a94 Add IsLegacy to CWallet so that the GUI knows whether to show watchonly (Andrew Chow)
1cb42b2 Generate new descriptors when encrypting (Andrew Chow)
82ae02b Be able to create new wallets with DescriptorScriptPubKeyMans as backing (Andrew Chow)
b713baa Implement GetMetadata in DescriptorScriptPubKeyMan (Andrew Chow)
8b9603b Change GetMetadata to use unique_ptr<CKeyMetadata> (Andrew Chow)
72a9540 Implement FillPSBT in DescriptorScriptPubKeyMan (Andrew Chow)
84b4978 Implement SignMessage for descriptor wallets (Andrew Chow)
bde7c9f Implement SignTransaction in DescriptorScriptPubKeyMan (Andrew Chow)
d50c8dd Implement GetSolvingProvider for DescriptorScriptPubKeyMan (Andrew Chow)
f1ca5fe Implement GetKeypoolOldestTime and only display it if greater than 0 (Andrew Chow)
586b57a Implement ReturnDestination in DescriptorScriptPubKeyMan (Andrew Chow)
f866957 Implement GetReservedDestination in DescriptorScriptPubKeyMan (Andrew Chow)
a775f7c Implement Unlock and Encrypt in DescriptorScriptPubKeyMan (Andrew Chow)
bfdd073 Implement GetNewDestination for DescriptorScriptPubKeyMan (Andrew Chow)
58c7651 Implement TopUp in DescriptorScriptPubKeyMan (Andrew Chow)
e014886 Implement SetupGeneration for DescriptorScriptPubKeyMan (Andrew Chow)
46dfb99 Implement writing descriptorkeys, descriptorckeys, and descriptors to wallet file (Andrew Chow)
4cb9b69 Implement several simple functions in DescriptorScriptPubKeyMan (Andrew Chow)
d1ec3e4 Add IsSingleType to Descriptors (Andrew Chow)
953feb3 Implement loading of keys for DescriptorScriptPubKeyMan (Andrew Chow)
2363e9f Load the descriptor cache from the wallet file (Andrew Chow)
46c46ae Implement GetID for DescriptorScriptPubKeyMan (Andrew Chow)
ec2f9e1 Implement IsHDEnabled in DescriptorScriptPubKeyMan (Andrew Chow)
741122d Implement MarkUnusedAddresses in DescriptorScriptPubKeyMan (Andrew Chow)
2db7ca7 Implement IsMine for DescriptorScriptPubKeyMan (Andrew Chow)
db7177a Add LoadDescriptorScriptPubKeyMan and SetActiveScriptPubKeyMan to CWallet (Andrew Chow)
78f8a92 Implement SetType in DescriptorScriptPubKeyMan (Andrew Chow)
834de03 Store WalletDescriptor in DescriptorScriptPubKeyMan (Andrew Chow)
d813266 Add a lock cs_desc_man for DescriptorScriptPubKeyMan (Andrew Chow)
3194a7f Introduce WalletDescriptor class (Andrew Chow)
6b13cd3 Create LegacyScriptPubKeyMan when not a descriptor wallet (Andrew Chow)
aeac157 Return nullptr from GetLegacyScriptPubKeyMan if descriptor wallet (Andrew Chow)
96accc7 Add WALLET_FLAG_DESCRIPTORS (Andrew Chow)
6b8119a Introduce DescriptorScriptPubKeyMan as a dummy class (Andrew Chow)
0662030 Introduce SetType function to tell ScriptPubKeyMans the type and internal-ness of it (Andrew Chow)

Pull request description:

  Introducing the wallet of the glorious future (again): native descriptor wallets. With native descriptor wallets, addresses are generated from descriptors. Instead of generating keys and deriving addresses from keys, addresses come from the scriptPubKeys produced by a descriptor. Native descriptor wallets will be optional for now and can only be created by using `createwallet`.

  Descriptor wallets will store descriptors, master keys from the descriptor, and descriptor cache entries. Keys are derived from descriptors on the fly. In order to allow choosing different address types, 6 descriptors are needed for normal use. There is a pair of primary and change descriptors for each of the 3 address types. With the default keypool size of 1000, each descriptor has 1000 scriptPubKeys and descriptor cache entries pregenerated. This has a side effect of making wallets large since 6000 pubkeys are written to the wallet by default, instead of the current 2000. scriptPubKeys are kept only in memory and are generated every time a descriptor is loaded. By default, we use the standard BIP 44, 49, 84 derivation paths with an external and internal derivation chain for each.

  Descriptors can also be imported with a new `importdescriptors` RPC.

  Native descriptor wallets use the `ScriptPubKeyMan` interface introduced in bitcoin#16341 to add a `DescriptorScriptPubKeyMan`. This defines a different IsMine which uses the simpler model of "does this scriptPubKey exist in this wallet". Furthermore, `DescriptorScriptPubKeyMan` does not have watchonly, so with native descriptor wallets, it is not possible to have a wallet with both watchonly and non-watchonly things. Rather a wallet with `disable_private_keys` needs to be used for watchonly things.

  A `--descriptor` option was added to some tests (`wallet_basic.py`, `wallet_encryption.py`, `wallet_keypool.py`, `wallet_keypool_topup.py`, and `wallet_labels.py`) to allow for these tests to use descriptor wallets. Additionally, several RPCs are disabled for descriptor wallets (`importprivkey`, `importpubkey`, `importaddress`, `importmulti`, `addmultisigaddress`, `dumpprivkey`, `dumpwallet`, `importwallet`, and `sethdseed`).

ACKs for top commit:
  Sjors:
    utACK 223588b (rebased, nits addressed)
  jonatack:
    Code review re-ACK 223588b.
  fjahr:
    re-ACK 223588b
  instagibbs:
    light re-ACK 223588b
  meshcollider:
    Code review ACK 223588b

Tree-SHA512: 59bc52aeddbb769ed5f420d5d240d8137847ac821b588eb616b34461253510c1717d6a70bab8765631738747336ae06f45ba39603ccd17f483843e5ed9a90986
@ryanofsky ryanofsky mentioned this pull request May 14, 2020
Closed
8 tasks
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 10, 2020
@achow101 @majcosta
[backport#17261 8/13] HD Split: Avoid redundant upgrades
343d199 
Summary:
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin/bitcoin#16341 (comment)

bitcoin/bitcoin@415afcc

Depends on D7854

Partial backport of Core [[bitcoin/bitcoin#17261 | PR17261]]

Test Plan:
  ninja all check check-functional

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7855
van-orton pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Oct 30, 2020
@achow101
HD Split: Avoid redundant upgrades
1affd98 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin/bitcoin#16341 (comment)
backpacker69 pushed a commit to peercoin/peercoin that referenced this pull request Mar 28, 2021
@achow101 @backpacker69
Key pool: Move CanGetAddresses call
80c0e77 
Call LegacyScriptPubKeyMan::CanGetAddresses directly instead of calling
CWallet::CanGetAddresses to only query the relevant key manager

This is a minor change in behavior: call now only happens if a new key needs to
be reserved, since if a key is already reserved it might fail unnecessarily.

This change also serves as a sanity check
bitcoin/bitcoin#16341 (comment)
backpacker69 pushed a commit to peercoin/peercoin that referenced this pull request Mar 28, 2021
@achow101 @backpacker69
Key pool: Fix omitted pre-split count in GetKeyPoolSize
de5717d 
This is a bugfix: bitcoin/bitcoin#16341 (comment)
backpacker69 pushed a commit to peercoin/peercoin that referenced this pull request Mar 28, 2021
@achow101 @backpacker69
HD Split: Avoid redundant upgrades
88cdbf1 
This avoids repeaded upgrades when support for more multiple keyman references
is added in the next commit:
bitcoin/bitcoin#16341 (comment)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sipa sipa sipa left review comments

@ryanofsky ryanofsky ryanofsky approved these changes

+6 more reviewers

@Sjors Sjors Sjors approved these changes

@dandv dandv dandv left review comments

@fqlx fqlx fqlx left review comments

@elichai elichai elichai left review comments

@meshcollider meshcollider meshcollider left review comments

@instagibbs instagibbs instagibbs left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.