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

ryanofsky · 2020-05-28T22:10:51Z

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

DrahtBot · 2020-05-29T06:45:30Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22100 (refactor: Clean up new wallet spend, receive files added MOVEONLY: CWallet transaction code out of wallet.cpp/.h #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.

Qt tests currently are currently using two NodeContext structs at the same time, one in interfaces::NodeImpl::m_context, and the other in BasicTestingSetup::m_node, and the tests have hacks transferring state between them. Fix this by getting rid of the NodeImpl::m_context struct and making it a pointer. This way a common BitcoinApplication object can be used for all qt tests, but they can still have their own testing setups. Non-test code is changing but non-test behavior is still the same as before. Benefit of this PR is to being able to remove the existing "std::move(test.m_node.connman)" and mempool hacks. Motivation for this PR is to let qt wallet tests continue working when the ::vpwallets global variable is removed bitcoin#19101 and not have to add even more complicated hacks exposing duplicate WalletContext and WalletClient instances in addition to the duplicate NodeContext instances and having to manipulate them in the test setup.

ryanofsky · 2020-05-29T13:08:31Z

Rebased 197a403 -> 7272686 (pr/novp.1 -> pr/novp.2, compare) with updated base prs and fixed walletcontroller lifetime bug in test
Rebased 7272686 -> 924561d (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 924561d -> f2d9af6 (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 f2d9af6 -> de89a09 (pr/novp.4 -> pr/novp.5, compare) with no changes after base pr #19096 merged
Rebased de89a09 -> e805b16 (pr/novp.5 -> pr/novp.6, compare) on rebased base pr #19099 pr/wclient.4
Rebased e805b16 -> 0b60ae5 (pr/novp.6 -> pr/novp.7, compare) due to conflicts with #19250 and #19261 on rebased base pr #19099 pr/wclient.5
Rebased 0b60ae5 -> 6c9a117 (pr/novp.7 -> pr/novp.8, compare) due to conflicts with #19300
Rebased 6c9a117 -> d22a053 (pr/novp.8 -> pr/novp.9, compare) on top of #19099 pr/wclient.6
Rebased d22a053 -> 6f799b1 (pr/novp.9 -> pr/novp.10, compare) on top of #19099 pr/wclient.7
Rebased 6f799b1 -> 58c9550 (pr/novp.10 -> pr/novp.11, compare) due to conflict with #19334
Rebased 58c9550 -> a00fc53 (pr/novp.11 -> pr/novp.12, compare) on top of #19099 pr/wclient.8
Rebased a00fc53 -> 7fa4e3b (pr/novp.12 -> pr/novp.13, compare) after #19099 merge and conflicts due to bitcoin-core/gui#35 and #19099
Rebased 7fa4e3b -> 9070db9 (pr/novp.13 -> pr/novp.14, compare) due to conflicts with #19717 and #19671
Rebased 9070db9 -> 3df8f44 (pr/novp.14 -> pr/novp.15, compare) due to conflict with #19754

Qt tests currently are currently using two NodeContext structs at the same time, one in interfaces::NodeImpl::m_context, and the other in BasicTestingSetup::m_node, and the tests have hacks transferring state between them. Fix this by getting rid of the NodeImpl::m_context struct and making it a pointer. This way a common BitcoinApplication object can be used for all qt tests, but they can still have their own testing setups. Non-test code is changing but non-test behavior is still the same as before. Benefit of this PR is to being able to remove the existing "std::move(test.m_node.connman)" and mempool hacks. Motivation for this PR is to let qt wallet tests continue working when the ::vpwallets global variable is removed bitcoin#19101 and not have to add even more complicated hacks exposing duplicate WalletContext and WalletClient instances in addition to the duplicate NodeContext instances and having to manipulate them in the test setup.

promag · 2020-05-31T23:38:44Z

Concept ACK, need to update commits in OP.

This change was originally part of both bitcoin#10102 and bitcoin#19101 and is required for both because it avoids the IPC wallet implementation in bitcoin#10102 and the WalletContext implementation in bitcoin#19101 needing to deal with notification objects that have stale pointers to deleted wallets.

meshcollider

Code review ACK 6e6a7e1

Thanks Russ for your continued great work!

Move global wallet variables to WalletContext struct

ryanofsky · 2021-08-17T22:28:13Z

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

meshcollider

re-utACK 62a09a3

achow101 · 2021-08-19T01:13:29Z

ACK 62a09a3

fanquake · 2021-08-19T01:45:34Z

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

kiminuo · 2021-08-19T07:22:51Z

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

Yes, will do. Thanks for the notification.

maflcko · 2021-08-19T13:15:07Z

src/wallet/load.cpp

        pwallet->postInitProcess();
    }

    // Schedule periodic wallet flushes and tx rebroadcasts
-    if (args.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
-        scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
+    if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {


Previously this was an Assert, now it is UB. Obviously doesn't matter, but I wanted to point out the difference.

…variables

break nodecontext usages inside of wallet rpc's and use chain interface functions to get node specific information

…variables 62a09a3 refactor: remove ::vpwallets and related global variables (Russell Yanofsky) Pull request description: Get rid of global wallet list variables by moving them to WalletContext struct - [`cs_wallets`](https://github.com/bitcoin/bitcoin/blob/e638acf6970394f8eb1957366ad2d39512f33b31/src/wallet/wallet.cpp#L56) is now [`WalletContext::wallet_mutex`](https://github.com/ryanofsky/bitcoin/blob/4be544c7ec08a81952fd3f4349151cbb8bdb60e8/src/wallet/context.h#L37) - [`vpwallets`](https://github.com/bitcoin/bitcoin/blob/e638acf6970394f8eb1957366ad2d39512f33b31/src/wallet/wallet.cpp#L57) is now [`WalletContext::wallets`](https://github.com/ryanofsky/bitcoin/blob/4be544c7ec08a81952fd3f4349151cbb8bdb60e8/src/wallet/context.h#L38) - [`g_load_wallet_fns`](https://github.com/bitcoin/bitcoin/blob/e638acf6970394f8eb1957366ad2d39512f33b31/src/wallet/wallet.cpp#L58) is now [`WalletContext::wallet_load_fns`](https://github.com/ryanofsky/bitcoin/blob/4be544c7ec08a81952fd3f4349151cbb8bdb60e8/src/wallet/context.h#L39) ACKs for top commit: achow101: ACK 62a09a3 meshcollider: re-utACK 62a09a3 Tree-SHA512: 74428180d57b4214c3d96963e6ff43e8778f6f23b6880262d1272f2de67d02714fdc3ebb558f62e48655b221a642c36f80ef37c8f89d362e2d66fd93cbf03b8f

… global variables" This reverts commit 07a9f67.

c3c2132 Use `context.args` in `src/wallet/load.cpp`. (Kiminuo) 25de4e7 Use `context.args` in `CWallet::Create` instead of `gArgs`. (Kiminuo) aa5e7c9 Fix typo in bitcoin-cli.cpp (Kiminuo) Pull request description: The PR attempts to move us an inch towards the [goal](#21244 (comment)) by using `WalletContext` in `wallet.{h|cpp}` code instead of relying on the global state (i.e. `gArgs`). Edit: The PR builds on #19101. ACKs for top commit: ryanofsky: Code review ACK c3c2132. Changes since last review: just rebasing and adding wallet load commit Tree-SHA512: 2b436f5a219e32c2d529f55a89edca086d949396cebf9e089a21aa7b1c180e3c0fb17468f415dfc834f8e1614f8b3914c7e9a0bd33b95e7e0199c0dfe5ca9490

c3c2132 Use `context.args` in `src/wallet/load.cpp`. (Kiminuo) 25de4e7 Use `context.args` in `CWallet::Create` instead of `gArgs`. (Kiminuo) aa5e7c9 Fix typo in bitcoin-cli.cpp (Kiminuo) Pull request description: The PR attempts to move us an inch towards the [goal](bitcoin#21244 (comment)) by using `WalletContext` in `wallet.{h|cpp}` code instead of relying on the global state (i.e. `gArgs`). Edit: The PR builds on bitcoin#19101. ACKs for top commit: ryanofsky: Code review ACK c3c2132. Changes since last review: just rebasing and adding wallet load commit Tree-SHA512: 2b436f5a219e32c2d529f55a89edca086d949396cebf9e089a21aa7b1c180e3c0fb17468f415dfc834f8e1614f8b3914c7e9a0bd33b95e7e0199c0dfe5ca9490

865ee1a Fix Qt test broken by #22219 (Russell Yanofsky) Pull request description: It looks like this should have been caught by CI but there might have been a conflict with a recently merged PR like #19101. Failure was reported by fanquake #22219 (comment) Fix avoids null WalletClient pointer dereference in address book test by adding MakeWalletClient call and making address book test initialization more consistent with wallet test initialization: https://github.com/bitcoin/bitcoin/blob/865ee1af201238f48671e3b79b8215f896db7a05/src/qt/test/addressbooktests.cpp#L63-L66 https://github.com/bitcoin/bitcoin/blob/865ee1af201238f48671e3b79b8215f896db7a05/src/qt/test/wallettests.cpp#L141-L144 ACKs for top commit: fanquake: ACK 865ee1a - I'm merging this now to unbreak the build. Tree-SHA512: 1f32b7fc79fa79fcf8600d23063896cbc7f8bbcff39d95747ecd546e754581f0f36ece3098ddecded175afccbb3709b4232da39a400dda23b7e550f361b515fb

This change was originally part of both bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 and is required for both because it avoids the IPC wallet implementation in bitcoin/bitcoin#10102 and the WalletContext implementation in bitcoin/bitcoin#19101 needing to deal with notification objects that have stale pointers to deleted wallets.

… node.h 24bf176 gui refactor: Inline SplashScreen::ConnectWallet (Russell Yanofsky) e4f4350 refactor: Move wallet methods out of chain.h and node.h (Russell Yanofsky) b266b3e refactor: Create interfaces earlier during initialization (Russell Yanofsky) Pull request description: Add WalletClient interface so node interface is cleaner and don't need wallet-specific methods. The new NodeContext::wallet_client pointer will also be needed to eliminate global wallet variables like ::vpwallets in bitcoin#19101, because createWallet(), loadWallet(), getWallets(), etc methods called by the GUI need a way to get a reference to the list of open wallets if it is no longer a global variable. ACKs for top commit: promag: Code review ACK 24bf176. MarcoFalke: ACK 24bf176 🐚 Tree-SHA512: a70d3776cd6723093db8912028c50075ec5fa0a48b961cb1a945f922658f5363754f8380dbb8378ed128c8c858913024f8264740905b8121a35c0d63bfaed7cf

DrahtBot added Build system GUI Refactoring RPC/REST/ZMQ Tests Wallet labels May 28, 2020

DrahtBot mentioned this pull request May 29, 2020

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

Merged

DrahtBot added the Needs rebase label May 29, 2020

ryanofsky mentioned this pull request May 29, 2020

test: Remove duplicate NodeContext hacks #19098

Merged

DrahtBot mentioned this pull request May 29, 2020

qt, refactor: Make BitcoinUnits::Unit a scoped enum #17877

Closed

ryanofsky mentioned this pull request May 29, 2020

refactor: Combine GetWalletForJSONRPCRequest and EnsureWalletIsAvailable functions #19100

Merged

ryanofsky force-pushed the pr/novp branch from 197a403 to 7272686 Compare May 29, 2020 13:06

DrahtBot removed the Needs rebase label May 29, 2020

This was referenced Jun 1, 2020

gui: Drop RecentRequestsTableModel dependency to WalletModel #18618

Closed

wallettool: Add dump and createfromdump commands #19137

Merged

meshcollider reviewed Aug 15, 2021

View reviewed changes

meshcollider added the Wallet label Aug 15, 2021

DrahtBot added the Needs rebase label Aug 15, 2021

refactor: remove ::vpwallets and related global variables

62a09a3

Move global wallet variables to WalletContext struct

ryanofsky force-pushed the pr/novp branch from 6e6a7e1 to 62a09a3 Compare August 17, 2021 22:27

DrahtBot removed the Needs rebase label Aug 18, 2021

meshcollider reviewed Aug 18, 2021

View reviewed changes

fanquake merged commit 638855a into bitcoin:master Aug 19, 2021

maflcko reviewed Aug 19, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 20, 2021

Merge bitcoin#19101: refactor: remove ::vpwallets and related global …

82ecad7

…variables

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 22, 2021

refactor after bitcoin#19101

5270986

break nodecontext usages inside of wallet rpc's and use chain interface functions to get node specific information

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 22, 2021

Revert "Merge bitcoin#19101: refactor: remove ::vpwallets and related…

e29e253

… global variables" This reverts commit 07a9f67.

This was referenced Sep 16, 2021

multiprocess: Start using init makeNode, makeChain, etc methods #22219

Merged

Fix Qt test broken by #22219 #22992

Merged

hebasto mentioned this pull request Oct 17, 2022

Replace all of the RecursiveMutex instances with the Mutex ones #19303

Open

36 tasks

bitcoin locked and limited conversation to collaborators Jan 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

ryanofsky commented May 28, 2020 •

edited

Loading

Uh oh!

DrahtBot commented May 29, 2020 •

edited

Loading

Uh oh!

ryanofsky commented May 29, 2020 •

edited

Loading

Uh oh!

promag commented May 31, 2020

Uh oh!

meshcollider left a comment •

edited

Loading

Uh oh!

ryanofsky commented Aug 17, 2021

Uh oh!

meshcollider left a comment

Uh oh!

achow101 commented Aug 19, 2021

Uh oh!

fanquake commented Aug 19, 2021

Uh oh!

kiminuo commented Aug 19, 2021

Uh oh!

maflcko Aug 19, 2021

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

ryanofsky commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

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

Uh oh!

Uh oh!

promag commented May 31, 2020

Uh oh!

meshcollider left a comment • 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!

ryanofsky commented Aug 17, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 19, 2021

Uh oh!

fanquake commented Aug 19, 2021

Uh oh!

kiminuo commented Aug 19, 2021

Uh oh!

maflcko Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky commented May 28, 2020 •

edited

Loading

DrahtBot commented May 29, 2020 •

edited

Loading

ryanofsky commented May 29, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading