Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)#16341
Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)#16341achow101 wants to merge 70 commits intobitcoin:masterfrom
Conversation
|
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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
3fdfd70 to
1404f8c
Compare
|
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. |
1dfa1c8 to
d34d213
Compare
|
Concept ACK, this abstraction is definitely the right path for the wallet to take IMO as we work toward descriptor wallets. |
|
Approach ACK. To make some of the commits easier to review, instead of having one commit that adds a function to In other words, start with empty Are you sure you want to move I brainstormed how to apply this to hardware wallets: #14912 (comment). The unsollicited keypool topup in |
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.
They are global to the wallet. They are moved to
I guess it should fail to unlock everything? That would indicate some corruption has occurred.
Agreed.
I suppose it should actually be in |
…functions in ScriptPubKeyMans
…ing or creating a wallet
Co-authored-by: Hugo Nguyen <[email protected]>
|
Rebased |
|
Code review re-ACK c37be15 |
|
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. |
Introducing the
ScriptPubKeyMan(short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over fromCWallet. Instead,CWalletwill have a pointer to aScriptPubKeyManfor every possible address type, internal and external. It will fetch the correctScriptPubKeyManas necessary. When fetching new addresses, it chooses theScriptPubKeyManbased on address type and whether it is change. For signing, it takes the script and asks eachScriptPubKeyManfor whether thatScriptPubKeyManconsiders that scriptIsMine, whether it has that script, or whether it is able to produce a signature for it. If so, theScriptPubKeyManwill provide aSigningProviderto the caller which will use that in order to sign.There is currently one
ScriptPubKeyMan- theLegacyScriptPubKeyMan. EachCWalletwill have only oneLegacyScriptPubKeyManwith the pointers for all of the address types and change pointing to thisLegacyScriptPubKeyMan. It is created when the wallet is loaded and all keys and metadata are loaded into it instead ofCWallet. TheLegacyScriptPubKeyManis primarily made up of all of the key and script management that used to be inCWallet. For convenience,CWallethas aGetLegacyScriptPubKeyManwhich will return theLegacyScriptPubKeyManor anullptrif it does not have one (not yet implemented, but callers will check for thenullptr). For purposes of signing,LegacyScriptPubKeyMan'sGetSigningProviderwill return itself rather than a separateSigningProvider. This will be different for futureScriptPubKeyMans.The
LegacyScriptPubKeyManwill also handle the importing and exporting of keys and scripts instead ofCWallet. As such, a number of RPCs have been limited to work only if aLegacyScriptPubKeyMancan be retrieved from the wallet. These RPCs aresethdseed,addmultisigaddress,importaddress,importprivkey,importpubkey,importmulti,dumpprivkey, anddumpwallet. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take theSigningProviderretrieved from theScriptPubKeyManfor 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
CWalletfunctions that have been removed.This PR is the last step in the Wallet Structure Changes.
The commits are organized as follows:
ScriptPubKeyManintegrationLegacyScriptPubKeyManby copying existing code fromCWallet. These will pass all tests and do not affectCWalletCWalletfunctions and RPC things with calls toScriptPubKeyManorLegacyScriptPubKeyMan. These will compile but are not expected to pass tests hence the[ci skip].CWalletfunctions.