Issue being fixed or feature implemented

What was done?

That is a next backport related to CWallet refactoring. It split to many commits (same as original pull request) for easier conflict resolving and code review, but should be squashed before merging.

✔️ Merge bitcoin#16227: Refactor CWallet's inheritance chain
✔️ Merge bitcoin#17260: Split some CWallet functions into new LegacyScriptPubKeyMan
✔️ Merge bitcoin#17300: LegacyScriptPubKeyMan code cleanups
✔️ Merge bitcoin#16237: Have the wallet give out destinations instead of keys
✔️ Merge bitcoin#16301: Use CWallet::Import* functions in all import* RPCs
✔️ Merge bitcoin#16383: rpcwallet: default include_watchonly to true for watchonly wallets
✔️ Merge bitcoin#16798: Refactor rawtransaction_util's SignTransaction to separate prevtx parsing
✔️ Merge bitcoin#16900: doc: Fix doxygen comment for SignTransaction in rpc/rawtransaction_util
↻ Merge bitcoin#17304: refactor: Move many functions into LegacyScriptPubKeyMan and further separate it from CWallet
☐ Merge bitcoin#17381: LegacyScriptPubKeyMan code cleanups
☐ Merge bitcoin#17371: Refactor: Require scriptPubKey to get wallet SigningProvider
☐ Merge bitcoin#17237: wallet: LearnRelatedScripts only if KeepDestination
☐ Merge bitcoin#17373: wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet
☐ Merge bitcoin#17369: Refactor: Move encryption code between KeyMan and Wallet
☐ Merge bitcoin#17537: wallet: Cleanup and move opportunistic and superfluous TopUp()
☐ Merge bitcoin#17677: Activate watchonly wallet behavior for LegacySPKM only
☐ Merge bitcoin#17924: Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash
☐ Merge bitcoin#17261: Make ScriptPubKeyMan an actual interface and the wallet to have multiple
☐ Merge bitcoin#18026: psbt_wallet_tests: use unique_ptr for GetSigningProvider
☐ Merge bitcoin#18067: wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition
☐ Merge bitcoin#18193: scripted-diff: Wallet: Rename incorrectly named *UsedDestination

Therefore 2 commits are skipped because code is not relevant:

• 8b0d82b Refactor: Move Upgrade code out of CWallet::CreateWalletFromFile
• 0eac708 Refactor: Move SetupGeneration code out of CWallet

commit bbc9e41
Merge: fba574c 152b0a0
Author: Wladimir J. van der Laan [email protected]
Date: Mon Nov 4 16:01:30 2019 +0100

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

152b0a00d8e681dd098f6b548447b82ab54ebe3c Refactor: Move nTimeFirstKey accesses out of CWallet (Andrew Chow)
7ef47b88e67718766c92d23973742d08436176e0 Refactor: Move GetKeypoolSize code out of CWallet (Andrew Chow)
089e17d45c8147bd0303bcbf02dc0f7d6b387f2a Refactor: Move RewriteDB code out of CWallet (Andrew Chow)
0eac7088ab878372a72f85f9c5f7662a14199083 Refactor: Move SetupGeneration code out of CWallet (Andrew Chow)
f45d12b36cee05aa3c2685b951d27bd8a58539ae Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile (Andrew Chow)
8b0d82bb428de9e7f1da7c61574e7a8376a62d43 Refactor: Move Upgrade code out of CWallet::CreateWalletFromFile (Andrew Chow)
46865ec958b6b9bde04a827de598975f14bdb5e7 Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe (Andrew Chow)
a18edd7b383d667b15b6d4b87aa3a055a9fa5051 Refactor: Move GetMetadata code out of getaddressinfo (Andrew Chow)
9716bbe0f8081814cbf052e8211d1c6a838e5262 Refactor: Move LoadKey LegacyScriptPubKeyMan method definition (Andrew Chow)
67be6b9e213da1fd7b2e8438c445c5a64092e831 Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys (Andrew Chow)
fc2867fdf50f47e5ad5f43445e500e3a477a8074 refactor: Replace UnsetWalletFlagWithDB with UnsetBlankWalletFlag in ScriptPubKeyMan (Andrew Chow)
78e7cbc7ba5938ab2ae6347141ca793a8fc09853 Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed (Andrew Chow)
0391aba52dcc33613295fd6099c804ac7134b063 Remove SetWalletFlag from WalletStorage (Andrew Chow)
4c5491f99c6b8ca779c48be63eb2ba18d98ee52a Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata (Andrew Chow)
769acef85730a6c0e2f5603fbd673b176c2d34d0 Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetNewDestination (Andrew Chow)
acedc5b8230ed9ad07f96f51f0ef862ab3a41d5e Refactor: Add new ScriptPubKeyMan virtual methods (Andrew Chow)
533d8b364f4e589aa1acb28cdea5e6e6e80d34dc Refactor: Declare LegacyScriptPubKeyMan methods as virtual (Andrew Chow)
b4cb18bce3c9a6adab2fa32f3dad4ad966b33be0 MOVEONLY: Reorder LegacyScriptPubKeyMan methods (Andrew Chow)

Pull request description:

  Moves several more key management and metadata functions into LegacyScriptPubKeyMan from CWallet to further separate the two.

  Note to reviewers: All of the `if (auto spk_man = walletInstance->m_spk_man.get()) {` blocks will be replaced with for loops in the next PR so you may see some things in those blocks that don't necessarily make sense with an `if` but will with a `for`.

How Has This Been Tested?

By running unit/functional tests.

Please, pay extra attention during code review to these 4 snippet:

• should this method has EXCLUSIVE_LOCKS_REQUIRED or not? bitcoin does not have
  (even some other methods do)

int64_t GetTimeFirstKey() const override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

• Please, help to check method

LegacyScriptPubKeyMan::MarkUnusedAddresses

Code seems correct, but I am not sure that is it right place to do it (code with bitcoin slightly different here)

• Why method 'CWallet::ZapSelectTx` doesn't have this line?

    nKeysLeftSinceAutoBackup = 0;

Seems as it was missing in past but I am not 100% sure. Can anyone confirm it?

Breaking Changes

Seems compatible, should be none as underlying backport is a "refactor"

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone