refactor: remove ::vpwallets and related global variables #19101

Get rid of global wallet list variables by moving them to WalletContext struct

• cs_wallets is now WalletContext::wallet_mutex
• vpwallets is now WalletContext::wallets
• g_load_wallet_fns is now WalletContext::wallet_load_fns

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22100 (refactor: Clean up new wallet spend, receive files added #21207 by ryanofsky)
• #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
• #15719 (Wallet passive startup 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.

Rebased 197a40317f1240d2d94a6092ccb51bbf51b9e3b1 -> 7272686491f39b31e59d9d56d93ba1c57f7a732f (pr/novp.1 -> pr/novp.2, compare) with updated base prs and fixed walletcontroller lifetime bug in test Rebased 7272686491f39b31e59d9d56d93ba1c57f7a732f -> 924561d98958a9ef95417b195200f3c54a6e6bfd (pr/novp.2 -> pr/novp.3, compare) to fix UndefinedBehaviorSanitizer error https://travis-ci.org/github/bitcoin/bitcoin/jobs/692548214#L4281 from #19098, also making changes to fix disable wallet build errors https://travis-ci.org/github/bitcoin/bitcoin/jobs/692548213#L3864 and importwallet_rescan segfault https://travis-ci.org/github/bitcoin/bitcoin/jobs/692548218#L10769 Rebased 924561d98958a9ef95417b195200f3c54a6e6bfd -> f2d9af627f9fce89c7a5acb763f13a82a5ca4b48 (pr/novp.3 -> pr/novp.4, compare) with no changes to avoid travis ARM buster "sudo: unable to resolve host travis-job-bitcoin-bitcoin-69472326" error https://travis-ci.org/github/bitcoin/bitcoin/jobs/694723263 reported in #19171 Rebased f2d9af627f9fce89c7a5acb763f13a82a5ca4b48 -> de89a0916424f2376a7466829166109febfde431 (pr/novp.4 -> pr/novp.5, compare) with no changes after base pr #19096 merged Rebased de89a0916424f2376a7466829166109febfde431 -> e805b165cc51ee6e42e49d1807e8e1627834200c (pr/novp.5 -> pr/novp.6, compare) on rebased base pr #19099 pr/wclient.4 Rebased e805b165cc51ee6e42e49d1807e8e1627834200c -> 0b60ae5da0b1424ee060af49e528a71e5644c758 (pr/novp.6 -> pr/novp.7, compare) due to conflicts with #19250 and #19261 on rebased base pr #19099 pr/wclient.5 Rebased 0b60ae5da0b1424ee060af49e528a71e5644c758 -> 6c9a117438ec84b56ab255abd5d88112f6a7b615 (pr/novp.7 -> pr/novp.8, compare) due to conflicts with #19300 Rebased 6c9a117438ec84b56ab255abd5d88112f6a7b615 -> d22a0535c69acd87f11b431373a3e27b11d3d4de (pr/novp.8 -> pr/novp.9, compare) on top of #19099 pr/wclient.6 Rebased d22a0535c69acd87f11b431373a3e27b11d3d4de -> 6f799b15203e176446fcd323255ae84265df5280 (pr/novp.9 -> pr/novp.10, compare) on top of #19099 pr/wclient.7 Rebased 6f799b15203e176446fcd323255ae84265df5280 -> 58c9550da715a26f32c7c4f568edbc8ca8f7f0bf (pr/novp.10 -> pr/novp.11, compare) due to conflict with #19334 Rebased 58c9550da715a26f32c7c4f568edbc8ca8f7f0bf -> a00fc5348b60db885abd1f37265f6ce710ea3a6f (pr/novp.11 -> pr/novp.12, compare) on top of #19099 pr/wclient.8 Rebased a00fc5348b60db885abd1f37265f6ce710ea3a6f -> 7fa4e3b98ed94f81a33f240691e069b8258ed3f6 (pr/novp.12 -> pr/novp.13, compare) after #19099 merge and conflicts due to bitcoin-core/gui#35 and #19099 Rebased 7fa4e3b98ed94f81a33f240691e069b8258ed3f6 -> 9070db91d2d688ed73bdd1f829827b6d17a2d437 (pr/novp.13 -> pr/novp.14, compare) due to conflicts with #19717 and #19671 Rebased 9070db91d2d688ed73bdd1f829827b6d17a2d437 -> 3df8f4480bf3dec79c2acbd1068554ed513d918c (pr/novp.14 -> pr/novp.15, compare) due to conflict with #19754

Concept ACK, need to update commits in OP.

Rebased 3df8f4480bf3dec79c2acbd1068554ed513d918c -> 22efdc8bccbe0c050eaa3b3dd19b274b0bb35489 (pr/novp.15 -> pr/novp.16, compare) due to conflict with #19619 Rebased 22efdc8bccbe0c050eaa3b3dd19b274b0bb35489 -> 5927d2f8ba51b8bf4d5cf76550596da836662a3e (pr/novp.16 -> pr/novp.17, compare) due to various conflicts Rebased 5927d2f8ba51b8bf4d5cf76550596da836662a3e -> 86102b657ca9d3f8a373b80bafff8a44a7367ba8 (pr/novp.17 -> pr/novp.18, compare) due to conflict with #21366 Updated 86102b657ca9d3f8a373b80bafff8a44a7367ba8 -> f940e2260ab1a1459656038c9b64c622a0eefbff (pr/novp.18 -> pr/novp.19, compare) including #21572 and #21574 to fix appveyor failure https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882 Rebased f940e2260ab1a1459656038c9b64c622a0eefbff -> dd50d58bf462e1193927535aefc76105b11847f5 (pr/novp.19 -> pr/novp.20, compare) after #21572 and #21574 merged Rebased dd50d58bf462e1193927535aefc76105b11847f5 -> d97043e047380744ccda839996ab9b73a080d3ee (pr/novp.20 -> pr/novp.21, compare) to fix fuzz timeout https://cirrus-ci.com/task/6394351371681792 #21639 Rebased d97043e047380744ccda839996ab9b73a080d3ee -> 6a68685d2d1f87827a51b7a5389bad4dcbd94e9b (pr/novp.21 -> pr/novp.22, compare) due to conflict with #21634 Rebased 6a68685d2d1f87827a51b7a5389bad4dcbd94e9b -> 7aca6d3ced892dea96d50708eb8cd113705a915e (pr/novp.22 -> pr/novp.23, compare) due to conflict with #20773 Rebased 7aca6d3ced892dea96d50708eb8cd113705a915e -> 4be544c7ec08a81952fd3f4349151cbb8bdb60e8 (pr/novp.23 -> pr/novp.24, compare) due to conflict with bitcoin-core/gui#346 and #21207

Tested ACK 4be544c7ec08a81952fd3f4349151cbb8bdb60e8.

Thanks for review! Made some updates based on comments.

Updated 4be544c7ec08a81952fd3f4349151cbb8bdb60e8 -> eb389b377d8e17cc146a5e71d83358d88c762297 (pr/novp.24 -> pr/novp.25, compare) with review tweaks

Tested ACK eb389b377d8e17cc146a5e71d83358d88c762297.

@ryanofsky I find your PR good. It's easier to follow for me than the code in master branch. Concept ACK.

I can see that #15638 of yours added load.h and load.cpp with functions like VerifyWallets(..), LoadWallets(..), etc. Now this PR adds WalletContext struct and WalletClient now contains WalletContext* context() function.

I had a look where the functions from load.h are used and it looks like they are used only in WalletClientImpl (please correct me if I'm wrong). This brings me to my original question I had when I first glanced at your PR: Is it necessary to have the context struct? I mean what would happen if the functions from load.h were moved to WalletClientImpl and a test like the following one (https://github.com/bitcoin/bitcoin/pull/19101/files#diff-0bd88ab96ab5927299eb4e0d92d63ecbf9c8ee5753b918edc4c0eef38ce5d2c7) was replaced from:

WalletContext& context = *node.walletClient().context();
AddWallet(context, wallet);
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
RemoveWallet(context, wallet, std::nullopt);

to:

WalletClient& walletClient = *node.walletClient();
walletClient.AddWallet(wallet);
// WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get()); // not sure here
walletClient.RemoveWallet(wallet, std::nullopt);

I don't really know if it is possible at all, I would just love to hear your opinion on this.

You could merge WalletContext and interfaces::WalletClientImpl and move code from load.cpp to interfaces.cpp. If anything though, I'd like to do the opposite and make interfaces.cpp contain as little functionality as possible so interfaces::Wallet just serves the limited purpose of exposing a well-defined interface to the wallet from external GUI code, and interfaces::WalletClient is only used to expose an interface to the wallet from external node code. I think ideally internal wallet would remain more modular (database code separate from key management code separate from syncing code separate from balance tracking code), and not reference these interfaces which tie everything together.

I think I'm not understanding the advantages of the specific Qt apptest change you mentioned. Qt test code does access internals of many layers because some checks are easier to do at the GUI layer, other are easier to do at the wallet layer, etc. I think it's important for this to be possible, but in general the interfaces should be designed more to hide internals than expose them.

As for wallet load code it's true it is only called in one place. Probably it is missing some test coverage. I would like to move more wallet loading code from wallet.cpp to load.cpp and I think improve way it handles race conditions and handles errors as discussed in #19300. But whatever improvements can be made to wallet/load.cpp I think it does make sense for it to exist as a separate file.

I think ideally internal wallet would remain more modular (database code separate from key management code separate from syncing code separate from balance tracking code), and not reference these interfaces which tie everything together.

I certainly do not suggest to make code worse from modularity point of view. I wrote my comment to understand better how/why you implemented this PR as you did. Thank you for the explanation. Also I did not write the comment to suggest any modifications to this PR but to understand what would happen if instead of function(context, parameters) (e.g. AddWallet(context, wallet);) one would have object.function(parameters) with no context whatsoever. And again it's just a question. Your approach is certainly straightforward.

I spent several hours trying to get familiar with various wallet classes and it still feels like I only scratched the surface, so that's why your insights here may be very valuable to other reviewers as well.

Thanks for asking these questions! I'm very happy to answer them, and I hope I didn't give the impression that I was disagreeing with anything. I was trying to answer "Is it necessary to have the context struct?" and say I don't think it's necessary, but I do think it's good because I think it's more lightweight than WalletClient and shouldn't get in the way of modularity.

Thanks for review! Responded and implemented some of the suggestions

Rebased eb389b377d8e17cc146a5e71d83358d88c762297 -> 6ef719f02219bf4e4525fc24da89fb5ca86ef3dd (pr/novp.25 -> pr/novp.26, compare) on top of bitcoin-core/gui#360, with review suggestions Rebased 6ef719f02219bf4e4525fc24da89fb5ca86ef3dd -> df609b1a44bad295f44bdbda3fa32a49fd745233 (pr/novp.26 -> pr/novp.27, compare) due to conflict with #21866

Base PR bitcoin-core/gui#360 is now merged so this PR doesn't have any dependencies

Rebased df609b1a44bad295f44bdbda3fa32a49fd745233 -> 6e6a7e1fe874f0d93de7254d0884a1b9a95f6b19 (pr/novp.27 -> pr/novp.28, compare) after base PR bitcoin-core/gui#360 merged, also fixing conflict with #22359

Code review ACK 6e6a7e1fe874f0d93de7254d0884a1b9a95f6b19

Thanks Russ for your continued great work!

Rebased 6e6a7e1fe874f0d93de7254d0884a1b9a95f6b19 -> 62a09a30772141ef4add2f10d29927211abf57eb (pr/novp.28 -> pr/novp.29, compare) due to conflict with #22541

re-utACK 62a09a30772141ef4add2f10d29927211abf57eb

ACK 62a09a30772141ef4add2f10d29927211abf57eb

@kiminuo maybe you'll want to follow up here given some of the comments up thread?

@kiminuo maybe you'll want to follow up here given some of the comments up thread?

Yes, will do. Thanks for the notification.