Remove `gArgs` from `wallet.h` and `wallet.cpp`

kiminuo commented at 8:25 pm on June 7, 2021: contributor

The PR attempts to move us an inch towards the goal by using WalletContext in wallet.{h|cpp} code instead of relying on the global state (i.e. gArgs).

Edit: The PR builds on #19101.

ryanofsky commented at 8:37 pm on June 7, 2021: member

The effort here seems good, but it seems to me this approach adds as many (or more?) gArgs usages as it removes. I think a better approach would build on #19101 to use WalletContext::args instead of gArgs. #19101 is pretty simple PR and some more review would be welcome

kiminuo commented at 8:58 pm on June 7, 2021: contributor

The effort here seems good, but it seems to me this approach adds as many (or more?) gArgs usages as it removes.

The idea of this PR is just to “move gArgs” one level up. That’s not that interesting result but it’s very easy to review and to decide whether people see a benefit in the change in general.

Your PR seems like a more robust approach to the problem. 👍 I’ll try to have a look at #19101 the day after tomorrow.

ryanofsky commented at 9:09 pm on June 7, 2021: member

I guess it’s possible the two PR’s only overlap in some places and are mostly orthogonal since #19101 does not update the CWallet constructor and this PR does, but:

I think no new uses of gArgs should be necessary here, it should always be possible to pass WalletContext::args instead.
For the places where #19101 passes a WalletContext argument and this PR just passes an ArgsManager argument, I think it would be nice if this PR just passed a WalletContext so the same parts of code wouldn’t need to churn in both PRs

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

#22787 (refactor: actual immutable pointing by kallewoof)
#22766 (refactor: Clarify and disable unused ArgsManager flags 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 GUI on Jun 7, 2021

DrahtBot added the label RPC/REST/ZMQ on Jun 7, 2021

DrahtBot added the label Wallet on Jun 7, 2021

kiminuo commented at 10:25 am on June 8, 2021: contributor

I guess it’s possible the two PR’s only overlap in some places and are mostly orthogonal since #19101 does not update the CWallet constructor and this PR does

It seems so. I would like to avoid competing with your PR so I will try to review your PR tomorrow, I’ll leave this PR as a draft and I’ll try to make an alternative PR based on yours to show how it would look like.

If anyone else has a different opinion on this, please share it to avoid unnecessary work. :)

ryanofsky commented at 12:49 pm on June 8, 2021: member

Thanks. Just to be clear about the previous comment #22183 (comment), I am saying I think the overlap between two PRs actually not so great, and my first suggestion there to avoid adding lots of new references to gArgs variable and instead pass *context.args, should be applicable with or without #19101. My second suggestion would just be to resolve the few conflicts there are between this PR and #19101 where #19101 adds WalletContext& parameters and this PR adds ArgsManager& parameters by favoring #19101 to avoid code churn. (This could also be rebased on top of #19101 to accomplish the same thing)

kiminuo force-pushed on Aug 22, 2021

kiminuo commented at 6:41 pm on August 22, 2021: contributor

@ryanofsky This PR is very modest follow-up to your #19101 at the moment as there are still some occurrences of gArgs in src/wallet/wallet.cpp file.

To remove the rest of gArgs from wallet.cpp, it seems I would need to pass WalletContext parameter to a CWallet constructor. I wonder what you think about this idea.

in src/wallet/wallet.cpp:2593 in 1153ad33ff outdated

2589@@ -2590,28 +2590,28 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2590         }
2591     }
2592 
2593-    if (!gArgs.GetArg("-addresstype", "").empty()) {
2594-        std::optional<OutputType> parsed = ParseOutputType(gArgs.GetArg("-addresstype", ""));
2595+    if (!context.args->GetArg("-addresstype", "").empty()) {

ryanofsky commented at 10:51 pm on August 22, 2021:

In commit “Use context.args in CWallet::Create instead of gArgs.” (1153ad33ffc821917c76777d971f0074f7af7590)

Just as a style suggestion I might write:

0ArgsManager& args = *Assert(context.args);

above and use args instead of context.args throughout this function for less verbosity and for a clearer assert error.

kiminuo commented at 1:00 pm on August 23, 2021:

Thank you 👍, I had something similar in mind.

ryanofsky approved

ryanofsky commented at 10:57 pm on August 22, 2021: member

Code review ACK 1153ad33ffc821917c76777d971f0074f7af7590, and this seems acceptable to merge even though it’s still marked as draft. It also looks like it would be pretty straightforward to extend this and remove gArgs from wallet/load.cpp (but it would require a rebase to avoid conflicts from recent #22217 merge).

To remove the rest of gArgs from wallet.cpp, it seems I would need to pass WalletContext parameter to a CWallet constructor. I wonder what you think about this idea.

I think you could do this, but I’d suggest passing ArgsManager* parameter instead of WalletContext. In that case, it would also make sense to add a CWallet ArgsManager* m_args member.

The difference between passing ArgsManager* and WalletContext is not hugely significant, but I suggest it because the wallet context struct is really meant to hold the state needed to load/save/manage multiple wallets, and shouldn’t be required to deal with just a single wallet. It’s nice if CWallet objects don’t have access to unnecessary state and less state is needed to create them.

(Related to the topic of ArgsManager cleanup @kiminuo, I’d also be happy if you could review #22766, in case you’re inclined)

kiminuo force-pushed on Aug 23, 2021

kiminuo marked this as ready for review on Aug 23, 2021

kiminuo commented at 1:14 pm on August 23, 2021: contributor

It also looks like it would be pretty straightforward to extend this and remove gArgs from wallet/load.cpp (but it would require a rebase to avoid conflicts from recent #22217 merge).

I’ll have a look. Thanks.

To remove the rest of gArgs from wallet.cpp, it seems I would need to pass WalletContext parameter to a CWallet constructor. I wonder what you think about this idea.

I think you could do this, but I’d suggest passing ArgsManager* parameter instead of WalletContext. In that case, it would also make sense to add a CWallet ArgsManager* m_args member.

When passing ArgsManager*, one would get to the situation that on this line, one would have two options: Either get ArgsManager from WalletContext (passed by parameter to CWallet::Create) or use CWallet ArgsManager* m_args member. Do you find it acceptable? It feels not so great. Still it would be better than to use gArgs.

(Related to the topic of ArgsManager cleanup @kiminuo, I’d also be happy if you could review #22766, in case you’re inclined)

I’ll attempt to have a look when I have a chance.

ryanofsky commented at 2:53 pm on August 23, 2021: member

Code review ACK b1c01bc11d6869787eda10571439d01cc3e79d04. Only change since last review is using ArgsManager& args = *Assert(context.args); suggestion. Thanks for the update!

When passing ArgsManager*, one would get to the situation that on this line, one would have two options: Either get ArgsManager from WalletContext (passed by parameter to CWallet::Create) or use CWallet ArgsManager* m_args member. Do you find it acceptable?

CWallet::Create is a static member, so there should be no conflict on that line because a this->m_args member does not exist at all, and a walletInstance->m_args instance will not exist until a few lines below. So the whole function should be unaffected by the suggestion to prefer using ArgsManager* instead of WalletContext when replacing gArgs elsewhere in the CWallet class.

MarcoFalke removed the label GUI on Aug 23, 2021

MarcoFalke removed the label Wallet on Aug 23, 2021

MarcoFalke removed the label RPC/REST/ZMQ on Aug 23, 2021

MarcoFalke added the label Refactoring on Aug 23, 2021

DrahtBot added the label Needs rebase on Aug 24, 2021

Fix typo in bitcoin-cli.cpp aa5e7c9471

Use `context.args` in `CWallet::Create` instead of `gArgs`. 25de4e77fe

kiminuo force-pushed on Aug 24, 2021

DrahtBot removed the label Needs rebase on Aug 24, 2021

Use `context.args` in `src/wallet/load.cpp`. c3c213215b

kiminuo commented at 7:36 pm on August 25, 2021: contributor

It also looks like it would be pretty straightforward to extend this and remove gArgs from wallet/load.cpp (but it would require a rebase to avoid conflicts from recent #22217 merge). @ryanofsky I have added a commit that fixes this up. Thanks for the suggestion.

ryanofsky approved

ryanofsky commented at 0:52 am on August 26, 2021: member

Code review ACK c3c213215b25f3e6f36d46b1d49dfcc3040cee1c. Changes since last review: just rebasing and adding wallet load commit

MarcoFalke merged this on Aug 26, 2021

MarcoFalke closed this on Aug 26, 2021

kiminuo deleted the branch on Aug 26, 2021

kallewoof commented at 8:53 am on August 26, 2021: member

Post-review ACK c3c213215b25f3e6f36d46b1d49dfcc3040cee1c

As a side-thought, it seems like WalletContext could use an args-parameter constructor alternative.

sidhujag referenced this in commit a9f957b9d7 on Aug 28, 2021

ryanofsky commented at 3:37 pm on September 8, 2021: member

Saw this in IRC logs:

[21:05:16] <achow101> ryanofsky: is it possible to get a WalletContext from within a CWallet? @achow101, it’s intentional for CWallet not to have access to WalletContext. WalletContext is container for multiple wallets, and only meant to be used by RPC code and init code, not meant to be used by core wallet code. An individual CWallet is only meant to have access to the state it needs: a chain interface pointer, maybe an argsmanager pointer, it’s own database, not to the list of other wallets or other global state. WalletContext is analogous to NodeContext. NodeContext is meant to be used by init and rpc code, not by validation code. Most normal code should not depend on or have access to the global state in these context structs.

MarcoFalke referenced this in commit f63bf05e73 on Nov 10, 2021

DrahtBot locked this on Oct 30, 2022

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

Conflicts

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