Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101 · Pull Request #17261 · bitcoin/bitcoin

achow101 · 2019-10-26T05:16:00Z

Continuation of wallet boxes project.

Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies.

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.

achow101 · 2019-10-26T05:17:46Z

The final diff is very similar to #16341 just with some things dropped as they were deemed unnecessary.

Sjors · 2019-10-26T08:35:58Z

For comparison, my last ACK on the original was for c37be15. That code was identical to @ryanofsky's branch at 299296e, just using a different set of commits to get there. This PR rebases that on master @25d7e2e78137d07eb612c44d19b0d496050c947a, with the last two commits (952ba99 & 299296e) dropped. The result is f47ffe0a88111a63e1d7d98c25fbe2184215dbbe here (minus two linter changes).

DrahtBot · 2019-10-26T11:55:59Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17954 (wallet: Remove calls to Chain::Lock methods by ryanofsky)
#17878 (wip: zmq: Support -zmqpubwallettx by promag)
#17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
#17681 (wallet: Keep inactive seeds after sethdseed and derive keys from them as needed by achow101)
#17484 (wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload by ariard)
#17443 (Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard)
#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)
#16698 (Mempool: rework rebroadcast logic to improve privacy by amitiuttarwar)
#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#16440 (BIP-322: Generic signed message format by kallewoof)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

Suggested by MarcoFalke <[email protected]> bitcoin#17260 (comment) The same check was also added in the followup commit "Refactor: Move SetupGeneration code out of CWallet" 9727a4d from bitcoin#17261, but it's safer to start checking now without waiting for that followup.

ryanofsky · 2019-10-29T18:39:58Z

achow101 · 2019-10-29T19:03:56Z

@ryanofsky Done as #17304

maflcko · 2019-10-29T20:15:09Z

fanquake added this to Blockers in High-priority for review 5 hours ago

Why? I thought that high-prio could only be used once per week per contributor.

fanquake · 2019-10-29T20:24:32Z

Achow requested it via IRC. He didn't have anything in high-prio.

…

On Tue, 29 Oct 2019 at 16:15, MarcoFalke ***@***.***> wrote: fanquake added this to Blockers in High-priority for review 5 hours ago Why? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#17261?email_source=notifications&email_token=AAGS34SKBDHCRX7QCH2LWVLQRCKWDA5CNFSM4JFL4BW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECR5VXY#issuecomment-547609311>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGS34RIQ5JLGSG4BKV3SCTQRCKWDANCNFSM4JFL4BWQ> .

maflcko · 2019-10-30T00:36:21Z

#17260 (comment) was in for this week, but fair enough

Sjors · 2020-01-19T11:23:14Z

re-utACK 126d6945e38cdc47b1227cdacfaa456714efcdf5

laanwj · 2020-01-20T19:03:02Z

src/outputtype.h

Doesn't defining the value of non-primitive constants in the header file potentially cause linker issues in C++ (or duplication, as this literal data will be included in multiple files)?. I think it's better to only declare the variable here then initialize the value in the cpp.

I don't think this would cause any issues since it's const. I also haven't seen any linker warnings or errors about this. I think there would only be some duplication, but isn't that the same for any constant global variable that is defined in a header?

If you are fine trusting the word of Microsoft,

In C, constant values default to external linkage, so they can appear only in source files. In C++, constant values default to internal linkage, which allows them to appear in header files.

Another source said C++ does not normally create storage for consts, which I assume means they work like #defines, in which case there shouldn't be wasted duplicate data entries either.

If you replace the {...} initializer with a function call, and then have the constant in multiple translation units, the function will get called once for each.

I moved the initialization to the cpp file

This commit only affects locking behavior and doesn't have other changes.

In CWallet::LoadWallet, use this to detect and empty wallet with no keys This commit does not change behavior.

This avoids repeaded upgrades when support for more multiple keyman references is added in the next commit: bitcoin#16341 (comment)

Instead of having a uint256 representations of one scattered throughout where it is used, define it globally in uint256.h

Add wallet logic for dealing with multiple ScriptPubKeyMan instances. This doesn't change current behavior because there is still only a single LegacyScriptPubKeyMan. But in the future the new logic will be used to support descriptor wallets.

…eyMan This commit does not change behavior.

This commit does not change behavior.

Needed for future ScriptPubKeyMans which may need to create SigningProviders dynamically and thus a normal pointer is not enough This commit does not change behavior.

instagibbs · 2020-01-23T22:15:36Z

re-utACK 3f37365

Sjors · 2020-01-24T08:38:23Z

re-utACK 3f37365 (it still compiles on macOS after #17261 (comment))

meshcollider · 2020-01-30T04:20:22Z

Tested re-ACK 3f37365

I think this is ready to go in

ryanofsky

Post-merge code review ACK 3f37365. I did post two questions in review comments below that I think would be good to resolve.

Changes since last review (72df6f1):

Dropped commit daf6d548de3efdfdb974bb52948f6086fb0e2ecc "Set state to WATCH_ONLY if we can get the pubkey"
Added EnsureLegacyScriptPubKeyMan create argument to make RPCs like sethdseed work on empty wallets
coinselector_tests setup fix
OUTPUT_TYPES definition move
AddAndGetDestinationForScript formatting tweaks
Split commit 01b4511 "Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan" out of commit 264917ff4343b678007d89373aba6e99d521001c "Add multiple keyman maps and loops"
LegacyScriptPubKeyMan::Upgrade hd split version check no longer added
Replacement hd seed generation skipped in CWallet::EncryptWallet for nonlegacy wallets
CWallet::GetAllScriptPubKeyMans method added and used by CreateWalletFromFile to not skip inactive keyman instances when figuring out first key time
CWallet::GetScriptPubKeyMan internal true/false code deduped
CWallet::SetupLegacyScriptPubKeyMan asserts removed
LegacySigningProvider m_spk_man member rename and comment tweak

ryanofsky · 2020-02-05T18:08:25Z

src/wallet/scriptpubkeyman.cpp

    }
    // Upgrade to HD chain split if necessary
-    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) {
+    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT) && CHDChain::VERSION_HD_CHAIN_SPLIT) {


In commit "HD Split: Avoid redundant upgrades" (415afcc):

This seems buggy and the change looks like it has no effect. Is this supposed to say && hdChain.nVersion < CHDChain::VERSION_HD_CHAIN_SPLIT?

ryanofsky · 2020-02-05T18:21:00Z

src/wallet/wallet.cpp

re: #17261 (comment)

No, descriptor wallets have a different way of doing it.

In commit "Box the wallet: Add multiple keyman maps and loops" (c729afd):

It seems bad to do nothing at all and keep using unencrypted seeds for non-legacy wallets, and the comment here doesn't here doesn't explain any legacy/nonlegacy distinction. It would seem safer to trigger some kind of error for non-legacy wallets if planned behavior hasn't been implemented yet, than to just do nothing.

If there is supposed to be legacy / non-legacy distinction, it would seem better to move legacy code out of CWallet into LegacyScriptPubKeyMan in a PostEncrypt or similar ScriptPubKeyMan virtual method

achow101 force-pushed the wallet-box-pr-2 branch from 1b62c69 to c81a129 Compare October 26, 2019 05:52

DrahtBot added Build system GUI RPC/REST/ZMQ Tests Utils/log/libs Wallet labels Oct 26, 2019

fanquake added Refactoring and removed Build system GUI RPC/REST/ZMQ Utils/log/libs labels Oct 26, 2019

Sjors mentioned this pull request Oct 26, 2019

Split some CWallet functions into new LegacyScriptPubKeyMan #17260

Merged

achow101 force-pushed the wallet-box-pr-2 branch from f47ffe0 to 4f559f7 Compare October 26, 2019 16:24

ryanofsky mentioned this pull request Oct 29, 2019

LegacyScriptPubKeyMan code cleanups #17300

Merged

achow101 force-pushed the wallet-box-pr-2 branch from 4f559f7 to 7250fab Compare October 29, 2019 18:22

Sjors mentioned this pull request Oct 29, 2019

External signer support - Wallet Box edition #16546

Merged

2 tasks

achow101 force-pushed the wallet-box-pr-2 branch from 7250fab to f3c3e9e Compare October 29, 2019 19:03

ryanofsky mentioned this pull request Oct 29, 2019

refactor: Move many functions into LegacyScriptPubKeyMan and further separate it from CWallet #17304

Merged

DrahtBot added the Needs rebase label Oct 30, 2019

achow101 force-pushed the wallet-box-pr-2 branch from f3c3e9e to 35b0f7b Compare October 30, 2019 16:12

laanwj reviewed Jan 20, 2020

View reviewed changes

promag and others added 13 commits January 23, 2020 16:34

wallet: Improve CWallet:MarkDestinationsDirty

f5be479

Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman

fadc08a

This commit only affects locking behavior and doesn't have other changes.

Refactor: Allow LegacyScriptPubKeyMan to be null

eb81fc3

In CWallet::LoadWallet, use this to detect and empty wallet with no keys This commit does not change behavior.

List output types in an array in order to be iterated over

81610ed

Always try to sign for all pubkeys in multisig

501acb5

Store p2sh scripts in AddAndGetDestinationForScript

4a7e43e

Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan

01b4511

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)

refactor: define a UINT256_ONE global constant

4977c30

Instead of having a uint256 representations of one scattered throughout where it is used, define it globally in uint256.h

Refactor: Copy CWallet signals and print function to LegacyScriptPubK…

e2f02aa

…eyMan This commit does not change behavior.

Cleanup: Drop unused GUI learnRelatedScripts method

3afe53c

This commit does not change behavior.

Refactor: Replace SigningProvider pointers with unique_ptrs

3f37365

Needed for future ScriptPubKeyMans which may need to create SigningProviders dynamically and thus a normal pointer is not enough This commit does not change behavior.

ajtowns mentioned this pull request Jan 30, 2020

psbt_wallet_tests: use unique_ptr for GetSigningProvider #18026

Merged

This was referenced Jan 30, 2020

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

gui: Bilingual GUI error messages #16224

Merged

gwillen mentioned this pull request Jan 30, 2020

"PSBT Operations" dialog #18027

Merged

This was referenced Feb 4, 2020

wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition #18067

Merged

wallet: #17261 regression: addmultisigaddress addresses not treated as solvable #18075

Closed

ryanofsky reviewed Feb 5, 2020

View reviewed changes

achow101 mentioned this pull request Feb 11, 2020

wallet: Pass in transactions and messages for signing instead of exporting the private keys #18115

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky mentioned this pull request Apr 27, 2020

Add fee_est tool for debugging fee estimation code #10443

Closed

achow101 mentioned this pull request Jun 12, 2020

[RFC] Wallet Class Structure Changes #16165

Closed

ajtowns mentioned this pull request Sep 25, 2020

uint256: 1 is a constant #20016

Merged

Conversation

achow101 commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Oct 26, 2019

Uh oh!

Sjors commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Oct 29, 2019

Uh oh!

maflcko commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 29, 2019 via email

Uh oh!

maflcko commented Oct 30, 2019

Uh oh!

Sjors commented Jan 19, 2020

Uh oh!

laanwj Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Jan 20, 2020

Choose a reason for hiding this comment

Uh oh!

kallewoof Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

sipa Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jan 23, 2020

Uh oh!

Sjors commented Jan 24, 2020

Uh oh!

meshcollider commented Jan 30, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

achow101 commented Oct 26, 2019 •

edited

Loading

Sjors commented Oct 26, 2019 •

edited

Loading

DrahtBot commented Oct 26, 2019 •

edited

Loading

ryanofsky commented Oct 29, 2019 •

edited

Loading

maflcko commented Oct 29, 2019 •

edited

Loading

laanwj Jan 20, 2020 •

edited

Loading

sipa Jan 23, 2020 •

edited

Loading