Remove gArgs from wallet.h and wallet.cpp

[kiminuo]

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]

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]

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]

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 refactor: remove ::vpwallets and related global variables #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]

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.

[kiminuo]

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]

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]

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

[ryanofsky]

Code review ACK 1153ad3, 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]

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]

Code review ACK b1c01bc. 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.

[kiminuo]

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]

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

[kallewoof]

Post-review ACK c3c2132

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

[ryanofsky]

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.