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

kiminuo · 2021-09-09T06:25:23Z

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

DrahtBot · 2021-09-09T12:51:47Z

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.

ryanofsky

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

ryanofsky · 2021-09-15T15:52:02Z

src/wallet/test/ismine_tests.cpp

    // P2PK compressed
    {
-        CWallet keystore(chain.get(), "", CreateDummyWalletDatabase());
+        CWallet keystore(chain.get(), "", gArgs, CreateDummyWalletDatabase());


In commit "Remove gArgs from wallet.h and wallet.cpp" (d7b9045)

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

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.

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.

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.

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

src/wallet/wallet.cpp

kiminuo · 2021-09-15T18:33:44Z

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

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!

ryanofsky

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

ryanofsky

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

kiminuo · 2021-11-08T18:19:29Z

🙏

maflcko · 2021-11-09T09:30:27Z

src/wallet/wallet.cpp

This hunk can be removed via a rebase

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

Co-authored-by: Russell Yanofsky <[email protected]>

ryanofsky

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

maflcko · 2021-11-10T18:46:28Z

src/bench/coin_selection.cpp

    NodeContext node;
    auto chain = interfaces::MakeChain(node);
-    CWallet wallet(chain.get(), "", CreateDummyWalletDatabase());
+    CWallet wallet(chain.get(), "", gArgs, CreateDummyWalletDatabase());


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

….cp…

fanquake added the Wallet label Sep 9, 2021

kiminuo changed the title ~~Remove gArgs from wallet.h and wallet.cpp (2)~~ refactor: Remove gArgs from wallet.h and wallet.cpp (2) Sep 9, 2021

kiminuo marked this pull request as ready for review September 9, 2021 10:52

ryanofsky approved these changes Sep 15, 2021

View reviewed changes

kiminuo force-pushed the feature/2021-09-wallet-gArgs branch from d7b9045 to 8762663 Compare September 15, 2021 18:33

ryanofsky approved these changes Sep 16, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 29, 2021

Remove -rescan startup parameter #23123

Closed

DrahtBot added the Needs rebase label Sep 30, 2021

kiminuo force-pushed the feature/2021-09-wallet-gArgs branch from 8762663 to 5b0d937 Compare October 1, 2021 09:55

DrahtBot removed the Needs rebase label Oct 1, 2021

DrahtBot mentioned this pull request Oct 15, 2021

tests: remove usage of LegacyScriptPubKeyMan from some wallet tests #23288

Merged

DrahtBot added the Needs rebase label Oct 22, 2021

kiminuo force-pushed the feature/2021-09-wallet-gArgs branch from 5b0d937 to d815b50 Compare October 23, 2021 17:59

DrahtBot removed the Needs rebase label Oct 23, 2021

DrahtBot mentioned this pull request Oct 26, 2021

refactor: Make CWalletTx sync state type-safe #21206

Merged

DrahtBot added the Needs rebase label Oct 29, 2021

ryanofsky approved these changes Oct 29, 2021

View reviewed changes

kiminuo force-pushed the feature/2021-09-wallet-gArgs branch from d815b50 to 29554af Compare October 29, 2021 18:04

DrahtBot removed the Needs rebase label Oct 29, 2021

ryanofsky approved these changes Nov 8, 2021

View reviewed changes

maflcko reviewed Nov 9, 2021

View reviewed changes

Remove gArgs from wallet.h and wallet.cpp

2ec38bd

Co-authored-by: Russell Yanofsky <[email protected]>

kiminuo force-pushed the feature/2021-09-wallet-gArgs branch from 29554af to 2ec38bd Compare November 9, 2021 10:38

ryanofsky approved these changes Nov 10, 2021

View reviewed changes

maflcko merged commit f63bf05 into bitcoin:master Nov 10, 2021

maflcko reviewed Nov 10, 2021

View reviewed changes

kiminuo deleted the feature/2021-09-wallet-gArgs branch November 10, 2021 18:50

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 10, 2021

Merge bitcoin#22928: refactor: Remove gArgs from wallet.h and `wallet…

c26338f

….cp…

kiminuo mentioned this pull request Dec 10, 2021

refactor: Remove gArgs from bdb.h and sqlite.h #23732

Merged

bitcoin locked and limited conversation to collaborators Nov 10, 2022

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

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

Uh oh!

Conversation

kiminuo commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kiminuo commented Sep 15, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

kiminuo commented Nov 8, 2021

Uh oh!

maflcko Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo Nov 9, 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!

maflcko Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

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

kiminuo commented Sep 9, 2021 •

edited

Loading

DrahtBot commented Sep 9, 2021 •

edited

Loading

kiminuo Nov 9, 2021 •

edited

Loading