refactor: remove ::vpwallets and related global variables by ryanofsky · Pull Request #19101 · bitcoin/bitcoin

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.

promag

Tested ACK 4be544c.

promag · 2021-06-07T20:43:51Z

src/qt/bitcoin.cpp


+#ifdef ENABLE_WALLET
+    // Delete wallet controller here manually, instead of relying on Qt
+    // reference counting. This makes sure walletmodel m_handle_* notification


fe45c37

instead of relying on Qt reference counting.

What Qt reference counting?

re: #19101 (comment)

fe45c37

instead of relying on Qt reference counting.

What Qt reference counting?

Thanks, changed "reference counting" to "object tracking" and linked to https://doc.qt.io/qt-5/objecttrees.html

promag · 2021-06-07T20:44:38Z

src/qt/bitcoin.cpp

+    // wallet implementations. It also avoids these notifications having to be
+    // handled while GUI objects are being destroyed, making GUI code less
+    // fragile as well.
+    delete m_wallet_controller;


fe45c37

Just noting that its not necessary to check for WalletModel::isWalletEnabled()), delete nullptr is safe.

promag · 2021-06-07T21:17:26Z

src/wallet/context.h

 struct WalletContext {
    interfaces::Chain* chain{nullptr};
    ArgsManager* args{nullptr};
+    RecursiveMutex wallets_mutex;


4be544c

It seems this can be a Mutex, just a suggestion for follow-up.

re: #19101 (comment)

4be544c

It seems this can be a Mutex, just a suggestion for follow-up.

Thanks went ahead and made the change here. It's not an immediately obvious change because the mutex is held while calling wallet_load_fns callbacks, but looking at the callbacks registered in GUI code it seems like these will never change the list of wallets so it seems good change to get rid of the recursive mutex now to prevent more complexity in the future

I also tested Mutex and built it with TSAN, LGTM

ryanofsky

Thanks for review! Made some updates based on comments.

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

ryanofsky · 2021-06-07T22:00:06Z

src/qt/bitcoin.cpp


+#ifdef ENABLE_WALLET
+    // Delete wallet controller here manually, instead of relying on Qt
+    // reference counting. This makes sure walletmodel m_handle_* notification


re: #19101 (comment)

fe45c37

instead of relying on Qt reference counting.

What Qt reference counting?

Thanks, changed "reference counting" to "object tracking" and linked to https://doc.qt.io/qt-5/objecttrees.html

ryanofsky · 2021-06-07T22:18:33Z

src/wallet/context.h

 struct WalletContext {
    interfaces::Chain* chain{nullptr};
    ArgsManager* args{nullptr};
+    RecursiveMutex wallets_mutex;


re: #19101 (comment)

4be544c

It seems this can be a Mutex, just a suggestion for follow-up.

Thanks went ahead and made the change here. It's not an immediately obvious change because the mutex is held while calling wallet_load_fns callbacks, but looking at the callbacks registered in GUI code it seems like these will never change the list of wallets so it seems good change to get rid of the recursive mutex now to prevent more complexity in the future

promag

Tested ACK eb389b3.

kiminuo · 2021-06-09T13:44:49Z

@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.

ryanofsky · 2021-06-09T16:09:00Z

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.

kiminuo · 2021-06-09T20:01:15Z

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.

ryanofsky · 2021-06-09T22:39:33Z

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.

src/qt/test/addressbooktests.cpp

kiminuo · 2021-06-10T20:22:52Z

src/qt/test/wallettests.cpp

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


(This snippet is exactly the same as in src/qt/test/addressbooktests.cpp but I must say I rather double checked with my editor's text search.)

re: #19101 (comment)

(This snippet is exactly the same as in src/qt/test/addressbooktests.cpp but I must say I rather double checked with my editor's text search.)

Duplication is good to note, but it precedes this PR, and I think trying to simplify or dedup here would be more invasive and make more sense as a separate change. (This PR is just adding function parameters. Trying to replace the actual function calls would be a bigger change.)

Yes, that was just an observation. I agree that it is better to address that in another PR if at all.

kiminuo · 2021-06-10T20:53:44Z

src/wallet/interfaces.cpp

-        return HandleLoadWallet(std::move(fn));
+        return HandleLoadWallet(m_context, std::move(fn));
    }
+    WalletContext* context() override  { return &m_context; }


Would it be possible to pass WalletContext via constructor to WalletClientImpl? It seems that in that case, this function may not be necessary and a test can still create WalletClientImpl.

re: #19101 (comment)

Would it be possible to pass WalletContext via constructor to WalletClientImpl? It seems that in that case, this function may not be necessary and a test can still create WalletClientImpl.

I think that could work but it seems a little better to me if instead of passing around WalletClient / WalletContext pairs, that if tests are already using WalletClient interfaces, they prefer using them, and only access the internal context as a fallback so they are less tied to implementation details. I think having a context method does a good job of encouraging using the external interface while making internal access still possible.

src/wallet/context.h

ryanofsky

Thanks for review! Responded and implemented some of the suggestions

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

src/qt/test/addressbooktests.cpp

src/wallet/context.h

ryanofsky · 2021-06-10T22:45:03Z

src/wallet/interfaces.cpp

-        return HandleLoadWallet(std::move(fn));
+        return HandleLoadWallet(m_context, std::move(fn));
    }
+    WalletContext* context() override  { return &m_context; }


re: #19101 (comment)

Would it be possible to pass WalletContext via constructor to WalletClientImpl? It seems that in that case, this function may not be necessary and a test can still create WalletClientImpl.

I think that could work but it seems a little better to me if instead of passing around WalletClient / WalletContext pairs, that if tests are already using WalletClient interfaces, they prefer using them, and only access the internal context as a fallback so they are less tied to implementation details. I think having a context method does a good job of encouraging using the external interface while making internal access still possible.

ryanofsky · 2021-06-10T22:47:43Z

src/qt/test/wallettests.cpp

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


re: #19101 (comment)

(This snippet is exactly the same as in src/qt/test/addressbooktests.cpp but I must say I rather double checked with my editor's text search.)

Duplication is good to note, but it precedes this PR, and I think trying to simplify or dedup here would be more invasive and make more sense as a separate change. (This PR is just adding function parameters. Trying to replace the actual function calls would be a bigger change.)

ryanofsky · 2021-08-13T14:54:18Z

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

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

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

    // 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.

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

DrahtBot mentioned this pull request Apr 8, 2021

tests: Skip SQLite fsyncs while testing #21634

Merged

DrahtBot mentioned this pull request Apr 23, 2021

Add -flushwalletinterval command line arg #21760

Closed

DrahtBot mentioned this pull request May 6, 2021

[Bundle 7/7] validation: Farewell, global Chainstate! #21866

Merged

ryanofsky mentioned this pull request Jun 7, 2021

Remove gArgs from wallet.h and wallet.cpp #22183

Merged

promag reviewed Jun 7, 2021

View reviewed changes

ryanofsky commented Jun 7, 2021

View reviewed changes

promag reviewed Jun 7, 2021

View reviewed changes

DrahtBot mentioned this pull request Jun 7, 2021

refactor: Clean up new wallet spend, receive files added #21207 #22100

Merged

ryanofsky mentioned this pull request Jun 10, 2021

Unregister wallet notifications before unloading wallets bitcoin-core/gui#360

Merged

kiminuo reviewed Jun 10, 2021

View reviewed changes

ryanofsky commented Jun 10, 2021

View reviewed changes

DrahtBot mentioned this pull request Jul 2, 2021

wallet: Do not set fInMempool in transactionAddedToMempool when tx is not in the mempool #22359

Merged

DrahtBot mentioned this pull request Jul 15, 2021

wallet test: Add test for subtract fee from recipient behavior #22155

Merged

DrahtBot mentioned this pull request Aug 14, 2021

Add a new RPC command: restorewallet #22541

Merged

meshcollider reviewed Aug 15, 2021

View reviewed changes

refactor: remove ::vpwallets and related global variables

62a09a3

Move global wallet variables to WalletContext struct

meshcollider reviewed Aug 18, 2021

View reviewed changes

maflcko reviewed Aug 19, 2021

View reviewed changes

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

35 tasks

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!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Jun 7, 2021 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

kiminuo commented Jun 9, 2021

Uh oh!

ryanofsky commented Jun 9, 2021

Uh oh!

kiminuo commented Jun 9, 2021

Uh oh!

ryanofsky commented Jun 9, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Aug 13, 2021

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

ryanofsky commented May 28, 2020 •

edited

Loading

DrahtBot commented May 29, 2020 •

edited

Loading

ryanofsky commented May 29, 2020 •

edited

Loading

promag Jun 7, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading

meshcollider left a comment •

edited

Loading