refactor: Remove `gArgs` from `wallet.h` and `wallet.cpp` (2)

kiminuo commented at 6:25 am on September 9, 2021: contributor

This is a follow-up PR to #22183 and is related to #21005 issue.

fanquake added the label Wallet on Sep 9, 2021

kiminuo renamed this:
~~Remove `gArgs` from `wallet.h` and `wallet.cpp` (2)~~
refactor: Remove `gArgs` from `wallet.h` and `wallet.cpp` (2)
on Sep 9, 2021

kiminuo marked this as ready for review on Sep 9, 2021

DrahtBot commented at 12:51 pm on September 9, 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:

#22372 (Support multiple -*notify commands by luke-jr)
#21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option 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.

in src/wallet/test/ismine_tests.cpp:37 in d7b904551b outdated

33@@ -34,7 +34,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard)
34 
35     // P2PK compressed
36     {
37-        CWallet keystore(chain.get(), "", CreateDummyWalletDatabase());
38+        CWallet keystore(chain.get(), "", gArgs, CreateDummyWalletDatabase());

ryanofsky commented at 3:52 pm on September 15, 2021:

In commit “Remove gArgs from wallet.h and wallet.cpp” (d7b904551b4ee337a5b02ebddb1d27fd86d21a8f)

I think it’s mistake to be adding gArgs references so many places when the goal is to not use gArgs. Most of these uses can be replaced by m_args. Would suggest the following change:

Diff: https://github.com/ryanofsky/bitcoin/compare/review.22928.1..review.22928.1-edit
Tag: review.22928.1-edit.1
Commit: bb118a81a51de93a59497638d8f51ee561f60a1f

kiminuo commented at 4:32 pm on September 15, 2021:

Thank you for the feedback! My approach overall is to progress from the bottom up. I find the way I did it here very conservative so that we are sure that nothing breaks. You are right that m_args can be used here, I wanted to do that later but if you find it better use m_args right away, then great.

ryanofsky commented at 4:45 pm on September 15, 2021:

Thanks! Normally I’d agree with being incremental and conservative, but if it is important to do this transition away from gArgs I think it is also important to try to make transition painless as possible to reviewers, and to other PR authors working on conflicting PRs by avoiding changing the same code multiple times in multiple PRs.

Maybe changing the same code in different PRs makes sense if one change is a bulk change and the second change is a manual change. But if two changes are both bulk changes, then splitting into conservative and non-conservative PRs just delays the inevitable non-conservative part of the change and prolongs the transition for no benefit. Also, these are changes to test code so there is basically no need to be conservative because it will be obvious if the tests are broken.

ryanofsky commented at 4:51 pm on September 15, 2021:

In general it would be great to be to see a consolidated final PR with all the future changes in one place. Or if that is too much work, it would be good to have a roadmap describing what the future changes will be. At very least if a PR has changes that are known to be temporary, it would be good to identify which parts of the PR are temporary, and which parts are permanent in the PR description.

kiminuo commented at 9:16 pm on September 15, 2021:

Makes sense. I’ll see what I can do.

in src/wallet/wallet.cpp:2752 in d7b904551b outdated

2748@@ -2749,7 +2749,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2749     return walletInstance;
2750 }
2751 
2752-bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interfaces::Chain& chain, bilingual_str& error, std::vector<bilingual_str>& warnings)
2753+bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, const ArgsManager& args, interfaces::Chain& chain, bilingual_str& error, std::vector<bilingual_str>& warnings)

ryanofsky commented at 3:58 pm on September 15, 2021:

In commit “Remove gArgs from wallet.h and wallet.cpp” (d7b904551b4ee337a5b02ebddb1d27fd86d21a8f)

args argument is redundant here and should be dropped. Just say ArgsManager& args = walletInstance->m_args inside the function.

kiminuo commented at 8:29 pm on September 15, 2021:

You are right. Thanks.

ryanofsky approved

ryanofsky commented at 4:15 pm on September 15, 2021: member

Code review ACK d7b904551b4ee337a5b02ebddb1d27fd86d21a8f, but I left two review comments below that would be good to address.

Also it would be good if PR description linked to issue #21005 as the true motivation for this PR, instead of saying that I suggested this. I think the current description which says the PR “follows the suggestion by ryanofsky” is a misleading, even though it is technically true, because I don’t have any opinion on whether gArgs should be removed from wallet code, and I didn’t suggest doing that. My suggestions have only been about how to pass ArgsManager references if you did decide it was important to get rid of gArgs.

kiminuo force-pushed on Sep 15, 2021

kiminuo commented at 6:33 pm on September 15, 2021: contributor

Also it would be good if PR description linked to issue #21005 as the true motivation for this PR, instead of saying that I suggested this. I think the current description which says the PR “follows the suggestion by ryanofsky” is a misleading, even though it is technically true, because I don’t have any opinion on whether gArgs should be removed from wallet code, and I didn’t suggest doing that. My suggestions have only been about how to pass ArgsManager references if you did decide it was important to get rid of gArgs.

Sorry for putting words to your mouth 😐

ryanofsky approved

ryanofsky commented at 5:57 pm on September 16, 2021: member

Code review ACK 8762663a31727d7fce578dff6c3a02307b59a967. Only changes since previous review were removing gArgs usages in tests and simplifying AttachChain arguments.

Sorry for putting words to your mouth

Sorry that was my fault. You referenced the post correctly. I just didn’t make the context clear that it was an “if you need to do this” change suggestion, not a change request!

DrahtBot added the label Needs rebase on Sep 30, 2021

kiminuo force-pushed on Oct 1, 2021

DrahtBot removed the label Needs rebase on Oct 1, 2021

DrahtBot added the label Needs rebase on Oct 22, 2021

kiminuo force-pushed on Oct 23, 2021

DrahtBot removed the label Needs rebase on Oct 23, 2021

DrahtBot added the label Needs rebase on Oct 29, 2021

ryanofsky approved

ryanofsky commented at 5:21 pm on October 29, 2021: member

Code review ACK d815b50edc057424b09e4310543904f28f0f6dd2. No changes since last review other than rebase

kiminuo force-pushed on Oct 29, 2021

DrahtBot removed the label Needs rebase on Oct 29, 2021

ryanofsky approved

ryanofsky commented at 5:22 pm on November 8, 2021: member

Code review ACK 29554afb7305c8ff2d262ca78e61a67e10615427. No changes since last review other than rebase

kiminuo commented at 6:19 pm on November 8, 2021: contributor

🙏

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

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

2ec38bdebb

in src/wallet/wallet.cpp:2735 in 29554afb73 outdated

2731@@ -2732,11 +2732,11 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2732         walletInstance->m_default_max_tx_fee = max_fee.value();
2733     }
2734 
2735-    if (gArgs.IsArgSet("-consolidatefeerate")) {
2736-        if (std::optional<CAmount> consolidate_feerate = ParseMoney(gArgs.GetArg("-consolidatefeerate", ""))) {
2737+    if (args.IsArgSet("-consolidatefeerate")) {

MarcoFalke commented at 9:30 am on November 9, 2021:

This hunk can be removed via a rebase

kiminuo commented at 10:38 am on November 9, 2021:

You are right. I have rebased the PR. Thanks.

kiminuo force-pushed on Nov 9, 2021

ryanofsky approved

ryanofsky commented at 6:40 pm on November 10, 2021: member

Code review ACK 2ec38bdebbdfd3f932eaa85c98b617d7b9326399. No changes since last review, just rebase

MarcoFalke merged this on Nov 10, 2021

MarcoFalke closed this on Nov 10, 2021

in src/bench/coin_selection.cpp:35 in 2ec38bdebb

31@@ -32,7 +32,7 @@ static void CoinSelection(benchmark::Bench& bench)
32 {
33     NodeContext node;
34     auto chain = interfaces::MakeChain(node);
35-    CWallet wallet(chain.get(), "", CreateDummyWalletDatabase());
36+    CWallet wallet(chain.get(), "", gArgs, CreateDummyWalletDatabase());

MarcoFalke commented at 6:46 pm on November 10, 2021:

Can’t this use node.args directly? (Same for all instances below)

kiminuo deleted the branch on Nov 10, 2021

sidhujag referenced this in commit c26338f288 on Nov 10, 2021

DrahtBot locked this on Nov 10, 2022

refactor: Remove gArgs from wallet.h and wallet.cpp (2) #22928

Conflicts

refactor: Remove `gArgs` from `wallet.h` and `wallet.cpp` (2) #22928