Skip to content

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

Closed
achow101 wants to merge 70 commits intobitcoin:masterfrom
achow101:box-the-wallet
Closed

Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)#16341
achow101 wants to merge 70 commits intobitcoin:masterfrom
achow101:box-the-wallet

Conversation

@achow101
Copy link
Member

@achow101 achow101 commented Jul 5, 2019

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

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
@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
Copy link
Member

Sjors commented Jul 11, 2019

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 and others added 19 commits October 21, 2019 11:33
@achow101
Copy link
Member Author

Rebased

@Sjors
Copy link
Member

Sjors commented Oct 25, 2019

Code review re-ACK c37be15

@achow101
Copy link
Member Author

As per the wallet meeting today, we will be using @ryanofsky's branch (with some modification maybe) and split it up into multiple PRs in an attempt to reduce review burden.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.