refactor: remove ::vpwallets and related global variables

ryanofsky commented at 10:10 pm on May 28, 2020: member

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 added the label Build system on May 28, 2020

DrahtBot added the label GUI on May 28, 2020

DrahtBot added the label Refactoring on May 28, 2020

DrahtBot added the label RPC/REST/ZMQ on May 28, 2020

DrahtBot added the label Tests on May 28, 2020

DrahtBot added the label Wallet on May 28, 2020

DrahtBot commented at 6:45 am on May 29, 2020: member

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

DrahtBot added the label Needs rebase on May 29, 2020

ryanofsky referenced this in commit 8569817a43 on May 29, 2020

ryanofsky force-pushed on May 29, 2020

ryanofsky commented at 1:08 pm on May 29, 2020: member

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

DrahtBot removed the label Needs rebase on May 29, 2020

ryanofsky referenced this in commit 934a8ae4b4 on May 29, 2020

promag commented at 11:38 pm on May 31, 2020: member

Concept ACK, need to update commits in OP.

ryanofsky referenced this in commit cfbf0bb2da on Jun 4, 2020

ryanofsky force-pushed on Jun 4, 2020

ryanofsky force-pushed on Jun 5, 2020

DrahtBot added the label Needs rebase on Jun 5, 2020

ryanofsky force-pushed on Jun 5, 2020

DrahtBot removed the label Needs rebase on Jun 5, 2020

DrahtBot added the label Needs rebase on Jun 9, 2020

ryanofsky referenced this in commit d2c9cd790b on Jun 10, 2020

ryanofsky referenced this in commit 4d3df42503 on Jun 10, 2020

ryanofsky force-pushed on Jun 10, 2020

DrahtBot removed the label Needs rebase on Jun 10, 2020

MarcoFalke referenced this in commit 7a24cca829 on Jun 11, 2020

DrahtBot added the label Needs rebase on Jun 11, 2020

sidhujag referenced this in commit 4085e64c21 on Jun 11, 2020

ryanofsky referenced this in commit ab1736cd92 on Jun 19, 2020

ryanofsky referenced this in commit 2502a44d04 on Jun 19, 2020

ryanofsky force-pushed on Jun 19, 2020

DrahtBot removed the label Needs rebase on Jun 19, 2020

DrahtBot added the label Needs rebase on Jun 29, 2020

ryanofsky force-pushed on Jul 1, 2020

DrahtBot removed the label Needs rebase on Jul 1, 2020

DrahtBot added the label Needs rebase on Jul 7, 2020

ryanofsky referenced this in commit fa522f9b9b on Jul 8, 2020

ryanofsky referenced this in commit 4c3d7416e5 on Jul 8, 2020

ryanofsky referenced this in commit 12135dd28e on Jul 8, 2020

ryanofsky force-pushed on Jul 10, 2020

DrahtBot removed the label Needs rebase on Jul 10, 2020

DrahtBot added the label Needs rebase on Jul 11, 2020

ryanofsky referenced this in commit 055cd7d2a1 on Jul 13, 2020

ryanofsky force-pushed on Jul 14, 2020

DrahtBot removed the label Needs rebase on Jul 14, 2020

DrahtBot added the label Needs rebase on Jul 23, 2020

ryanofsky force-pushed on Jul 30, 2020

DrahtBot removed the label Needs rebase on Jul 30, 2020

DrahtBot added the label Needs rebase on Aug 7, 2020

ryanofsky force-pushed on Aug 11, 2020

DrahtBot removed the label Needs rebase on Aug 11, 2020

DrahtBot added the label Needs rebase on Aug 13, 2020

ryanofsky force-pushed on Aug 28, 2020

DrahtBot removed the label Needs rebase on Aug 28, 2020

MarcoFalke referenced this in commit 269a7ccb27 on Aug 31, 2020

DrahtBot added the label Needs rebase on Aug 31, 2020

sidhujag referenced this in commit bde41caa8d on Aug 31, 2020

ryanofsky force-pushed on Sep 1, 2020

DrahtBot removed the label Needs rebase on Sep 1, 2020

DrahtBot added the label Needs rebase on Sep 3, 2020

ryanofsky force-pushed on Sep 3, 2020

DrahtBot removed the label Needs rebase on Sep 3, 2020

MarcoFalke removed the label Build system on Sep 5, 2020

MarcoFalke removed the label GUI on Sep 5, 2020

MarcoFalke removed the label RPC/REST/ZMQ on Sep 5, 2020

MarcoFalke removed the label Tests on Sep 5, 2020

MarcoFalke removed the label Wallet on Sep 5, 2020

DrahtBot added the label Needs rebase on Sep 7, 2020

ryanofsky force-pushed on Sep 25, 2020

DrahtBot removed the label Needs rebase on Sep 25, 2020

ryanofsky commented at 11:08 am on September 28, 2020: member

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

DrahtBot added the label Needs rebase on Oct 15, 2020

ryanofsky force-pushed on Mar 31, 2021

DrahtBot removed the label Needs rebase on Mar 31, 2021

DrahtBot added the label Needs rebase on Mar 31, 2021

ryanofsky force-pushed on Apr 1, 2021

DrahtBot removed the label Needs rebase on Apr 1, 2021

ryanofsky force-pushed on Apr 3, 2021

DrahtBot added the label Needs rebase on Apr 7, 2021

ryanofsky force-pushed on Apr 8, 2021

DrahtBot removed the label Needs rebase on Apr 8, 2021

ryanofsky force-pushed on Apr 9, 2021

DrahtBot added the label Needs rebase on Apr 13, 2021

ryanofsky force-pushed on Apr 13, 2021

DrahtBot removed the label Needs rebase on Apr 13, 2021

ryanofsky referenced this in commit 90369e5271 on Apr 30, 2021

ryanofsky referenced this in commit a5d176c023 on Apr 30, 2021

DrahtBot added the label Needs rebase on May 19, 2021

ryanofsky referenced this in commit 3287cff0b9 on May 19, 2021

ryanofsky force-pushed on May 19, 2021

DrahtBot removed the label Needs rebase on May 19, 2021

DrahtBot added the label Needs rebase on May 27, 2021

ryanofsky force-pushed on Jun 7, 2021

ryanofsky referenced this in commit fe45c37989 on Jun 7, 2021

DrahtBot removed the label Needs rebase on Jun 7, 2021

in src/qt/bitcoin.cpp:357 in fe45c37989 outdated

351@@ -352,6 +352,17 @@ void BitcoinApplication::requestShutdown()
352     window->setClientModel(nullptr);
353     pollShutdownTimer->stop();
354 
355+#ifdef ENABLE_WALLET
356+    // Delete wallet controller here manually, instead of relying on Qt
357+    // reference counting. This makes sure walletmodel m_handle_* notification

promag commented at 8:43 pm on June 7, 2021:

fe45c379898e1a418af3847974a55f936814ca2a

instead of relying on Qt reference counting.

What Qt reference counting?

ryanofsky commented at 10:00 pm on June 7, 2021:

re: #19101 (review)

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

in src/qt/bitcoin.cpp:353 in fe45c37989 outdated

357+    // reference counting. This makes sure walletmodel m_handle_* notification
358+    // handlers are deleted before wallets are unloaded, which can simplify
359+    // wallet implementations. It also avoids these notifications having to be
360+    // handled while GUI objects are being destroyed, making GUI code less
361+    // fragile as well.
362+    delete m_wallet_controller;

promag commented at 8:44 pm on June 7, 2021:

fe45c379898e1a418af3847974a55f936814ca2a

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

in src/wallet/context.h:37 in 4be544c7ec outdated

33@@ -23,6 +34,9 @@ class Chain;
34 struct WalletContext {
35     interfaces::Chain* chain{nullptr};
36     ArgsManager* args{nullptr};
37+    RecursiveMutex wallets_mutex;

promag commented at 9:17 pm on June 7, 2021:

4be544c7ec08a81952fd3f4349151cbb8bdb60e8

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

ryanofsky commented at 10:18 pm on June 7, 2021:

re: #19101 (review)

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 commented at 10:37 pm on June 7, 2021:

I also tested Mutex and built it with TSAN, LGTM

promag commented at 9:24 pm on June 7, 2021: member

Tested ACK 4be544c7ec08a81952fd3f4349151cbb8bdb60e8.

ryanofsky referenced this in commit d7bafb5065 on Jun 7, 2021

ryanofsky force-pushed on Jun 7, 2021

ryanofsky commented at 10:34 pm on June 7, 2021: member

Thanks for review! Made some updates based on comments.

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

promag commented at 11:03 pm on June 7, 2021: member

Tested ACK eb389b377d8e17cc146a5e71d83358d88c762297.

kiminuo commented at 1:44 pm on June 9, 2021: contributor

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

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

to:

0WalletClient& walletClient = *node.walletClient();
1walletClient.AddWallet(wallet);
2// WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get()); // not sure here
3walletClient.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 commented at 4:09 pm on June 9, 2021: member

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 commented at 8:01 pm on June 9, 2021: contributor

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 commented at 10:39 pm on June 9, 2021: member

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.

ryanofsky referenced this in commit 93cc53a2b2 on Jun 10, 2021

in src/qt/test/addressbooktests.cpp:115 in eb389b377d outdated

113-    WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get());
114-    RemoveWallet(wallet, std::nullopt);
115+    WalletContext& context = *node.walletClient().context();
116+    AddWallet(context, wallet);
117+    WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
118+    RemoveWallet(context, wallet, std::nullopt);

kiminuo commented at 8:00 pm on June 10, 2021:

nit: If this needs to be touched, may be useful for consistency:

0    RemoveWallet(context, wallet, std::nullopt /* load_on_start */);

(It is on more places but I don’t want to spam.)

ryanofsky commented at 10:27 pm on June 10, 2021:

re: #19101 (review)

nit: If this needs to be touched, may be useful for consistency:

(It is on more places but I don’t want to spam.)

Thanks! Added change all places

in src/qt/test/wallettests.cpp:170 in eb389b377d outdated

168-    WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get());
169-    RemoveWallet(wallet, std::nullopt);
170+    WalletContext& context = *node.walletClient().context();
171+    AddWallet(context, wallet);
172+    WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
173+    RemoveWallet(context, wallet, std::nullopt);

kiminuo commented at 8:22 pm on June 10, 2021:

(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.)

ryanofsky commented at 10:47 pm on June 10, 2021:

re: #19101 (review)

(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.)

kiminuo commented at 6:53 am on June 11, 2021:

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

in src/wallet/interfaces.cpp:572 in eb389b377d outdated

563     std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
564     {
565-        return HandleLoadWallet(std::move(fn));
566+        return HandleLoadWallet(m_context, std::move(fn));
567     }
568+    WalletContext* context() override  { return &m_context; }

kiminuo commented at 8:53 pm on June 10, 2021:

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.

ryanofsky commented at 10:45 pm on June 10, 2021:

re: #19101 (review)

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.

in src/wallet/context.h:37 in eb389b377d outdated

33@@ -23,6 +34,9 @@ class Chain;
34 struct WalletContext {
35     interfaces::Chain* chain{nullptr};
36     ArgsManager* args{nullptr};
37+    Mutex wallets_mutex;

kiminuo commented at 9:14 pm on June 10, 2021:

off topic: I noticed a very nice comment https://github.com/bitcoin/bitcoin/blob/1704bbf2263f16c720604cfab4ccb775315df690/src/node/context.h#L49 in src/node/context.h. Would it make sense to add the comment on L36, too?

ryanofsky commented at 10:34 pm on June 10, 2021:

re: #19101 (review)

off topic: I noticed a very nice comment

https://github.com/bitcoin/bitcoin/blob/1704bbf2263f16c720604cfab4ccb775315df690/src/node/context.h#L49 in src/node/context.h. Would it make sense to add the comment on L36, too?

Sure, added this for consistency

kiminuo commented at 6:53 am on June 11, 2021:

Thanks!

ryanofsky force-pushed on Jun 10, 2021

ryanofsky commented at 11:29 pm on June 10, 2021: member

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

DrahtBot added the label Needs rebase on Jun 12, 2021

ryanofsky force-pushed on Jun 15, 2021

DrahtBot removed the label Needs rebase on Jun 15, 2021

DrahtBot added the label Needs rebase on Jul 27, 2021

hebasto referenced this in commit 439e58c4d8 on Aug 12, 2021

ryanofsky force-pushed on Aug 13, 2021

ryanofsky commented at 2:54 pm on August 13, 2021: member

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

DrahtBot removed the label Needs rebase on Aug 13, 2021

stratospher referenced this in commit 34e8efb5bb on Aug 14, 2021

meshcollider commented at 4:18 am on August 15, 2021: contributor

Code review ACK 6e6a7e1fe874f0d93de7254d0884a1b9a95f6b19

Thanks Russ for your continued great work!

meshcollider added the label Wallet on Aug 15, 2021

DrahtBot added the label Needs rebase on Aug 15, 2021

refactor: remove ::vpwallets and related global variables

Move global wallet variables to WalletContext struct

62a09a3077

ryanofsky force-pushed on Aug 17, 2021

ryanofsky commented at 10:28 pm on August 17, 2021: member

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

DrahtBot removed the label Needs rebase on Aug 18, 2021

meshcollider commented at 0:06 am on August 18, 2021: contributor

re-utACK 62a09a30772141ef4add2f10d29927211abf57eb

achow101 commented at 1:13 am on August 19, 2021: member

ACK 62a09a30772141ef4add2f10d29927211abf57eb

fanquake merged this on Aug 19, 2021

fanquake closed this on Aug 19, 2021

fanquake commented at 1:45 am on August 19, 2021: member

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

kiminuo commented at 7:22 am on August 19, 2021: contributor

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

Yes, will do. Thanks for the notification.

in src/wallet/load.cpp:134 in 62a09a3077

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

MarcoFalke commented at 1:15 pm on August 19, 2021:

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

sidhujag referenced this in commit 82ecad72c6 on Aug 20, 2021

sidhujag referenced this in commit 5270986998 on Aug 22, 2021

sidhujag referenced this in commit 07a9f6726c on Aug 22, 2021

sidhujag referenced this in commit e29e2534da on Aug 22, 2021

MarcoFalke referenced this in commit cea38b491f on Aug 26, 2021

sidhujag referenced this in commit a9f957b9d7 on Aug 28, 2021

fanquake referenced this in commit 9424e78f34 on Sep 16, 2021

janus referenced this in commit f21dd521fa on Oct 29, 2021

PastaPastaPasta referenced this in commit aeae851740 on Jun 7, 2022

PastaPastaPasta referenced this in commit c6f2a4f5c8 on Jun 7, 2022

PastaPastaPasta referenced this in commit d6f0c804ef on Jun 10, 2022

PastaPastaPasta referenced this in commit 4151b12ed6 on Jun 14, 2022

DrahtBot locked this on Jan 2, 2023

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

Conflicts