refactor: wallet, remove global 'ArgsManager' dependency

furszy commented at 2:45 pm on January 13, 2023: member

Structurally, the wallet class shouldn’t access the global ArgsManager class, its internal behavior shouldn’t be coupled to a global command line args parsing object.

So this PR migrates the only two places where we depend on it: (1) the keypool size, and (2) the “-walletnotify” script. And cleans up the, now unneeded, wallet ArgsManager ref member.

Extra note: In the process of removing the args ref member, discovered and fixed files that were invalidly depending on the wallet header including util/system.h.

DrahtBot commented at 2:45 pm on January 13, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, hebasto
Concept ACK	fanquake
Approach ACK	S3RK
Stale ACK	pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26720 (wallet: coin selection, don’t return results that exceed the max allowed weight by furszy)
#26715 (Introduce MockableDatabase for wallet unit tests by achow101)
#26699 (wallet, gui: bugfix, getAvailableBalance skips selected coins by furszy)
#26644 (wallet: bugfix, ‘wallet_load_ckey’ unit test fails with bdb by furszy)
#26627 (wallet: Migrate non-HD keys with single combo containing a list of keys by achow101)
#26606 (wallet: Implement independent BDB parser by achow101)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25297 (wallet: group independent db writes on single batched db transaction by furszy)
#24897 ([Draft / POC] Silent Payments by w0xlt)
#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.

DrahtBot added the label Wallet on Jan 13, 2023

furszy renamed this:
~~wallet: set keypool size instead of access global args manager from spkm~~
[WIP] wallet: set keypool size instead of access global args manager from spkm
on Jan 14, 2023

furszy force-pushed on Jan 15, 2023

furszy renamed this:
~~[WIP] wallet: set keypool size instead of access global args manager from spkm~~
wallet: set keypool size instead of access global args manager from spkm
on Jan 15, 2023

in src/wallet/wallet.cpp:2918 in d5f494db6d outdated

2882@@ -2883,6 +2883,9 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2883     // TODO: Can't use std::make_shared because we need a custom deleter but
2884     // should be possible to use std::allocate_shared.
2885     std::shared_ptr<CWallet> walletInstance(new CWallet(chain, name, args, std::move(database)), ReleaseWallet);
2886+    walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1);

TheCharlatan commented at 9:34 pm on January 16, 2023:

There might be a good reason for this, but why not initialize this member field in the header?

furszy commented at 10:11 pm on January 16, 2023:

where in the header exactly? constructor?

TheCharlatan commented at 5:29 pm on January 17, 2023:

Yes:

0         : m_args(args),
1           m_chain(chain),
2           m_name(name),
3-          m_database(std::move(database))
4+          m_database(std::move(database)),
5+          m_keypool_size(std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1))

furszy commented at 1:23 pm on January 18, 2023:

Structurally speaking, the wallet class shouldn’t have access to the global ArgsManager class, its internal behavior shouldn’t be coupled to a global command line args parsing object. So setting this on the constructor would just hide the non-desire coupling.

Could actually go further here and totally remove the m_args member from the wallet now. Just need to keep track of the “-walletnotify” script. Let me see how big this change is.

furszy commented at 2:41 pm on January 18, 2023:

Updated the PR with this changes.

TheCharlatan commented at 10:09 pm on January 19, 2023:

Great, that seems conceptually sound to me!

TheCharlatan commented at 9:35 pm on January 16, 2023: contributor

Concept ACK

furszy renamed this:
~~wallet: set keypool size instead of access global args manager from spkm~~
refactor: remove global 'ArgsManager' dependency
on Jan 18, 2023

furszy force-pushed on Jan 18, 2023

furszy renamed this:
~~refactor: remove global 'ArgsManager' dependency~~
refactor: wallet, remove global 'ArgsManager' dependency
on Jan 18, 2023

furszy force-pushed on Jan 18, 2023

S3RK commented at 8:12 am on January 19, 2023: contributor

Awesome. Approach ACK

fanquake commented at 4:15 pm on January 22, 2023: member

Concept ACK

DrahtBot added the label Needs rebase on Feb 1, 2023

furszy force-pushed on Feb 1, 2023

furszy commented at 4:04 pm on February 1, 2023: member

rebased, conflicts solved.

DrahtBot removed the label Needs rebase on Feb 1, 2023

pablomartin4btc commented at 10:09 pm on February 8, 2023: member

code review ACK, pending of testing it.

Instead of removing it all in once you could extract the gui bit/ qt part (6/26 ~ 20%) into another PR in /bitcoin-core/gui, so you reduce the amount of files modified touched by the PR - perhaps other splits can be performed as well.

furszy commented at 10:44 pm on February 8, 2023: member

Instead of removing it all in once you could extract the gui bit/ qt part (6/26 ~ 20%) into another PR in /bitcoin-core/gui, so you reduce the amount of files modified touched by the PR - perhaps other splits can be performed as well.

There are two commits that slightly touch the GUI.

6b578a37 because the GUI tests create a CWallet, object whose constructor signature has changed (due the removal of the global args object ref). In this scenario, nothing can be done, the GUI test must be changed within the same commit.

Then 9022473, whose GUI modifications were made as a result of removing the no longer necessary util/system.h include from the wallet.h file. In this situation, separating the changes into a separate GUI-only PR would mean the GUI PR would impact the backend code, which is typically not recommended. Furthermore, given the limited scope of the changes in that specific commit, it makes more sense to keep it in this PR rather than creating a separate follow-up.

pablomartin4btc commented at 10:51 pm on February 9, 2023: member

In this situation, separating the changes into a separate GUI-only PR would mean the GUI PR would impact the backend code, which is typically not recommended. Furthermore, given the limited scope of the changes in that specific commit, it makes more sense to keep it in this PR rather than creating a separate follow-up.

I see, yeah, I understand now it would take some time to split the PRs and as you said the scope is very specific, but for future reference, perhaps you can consider this, as some reviewers could take the amount of files being touched into account. I like this refactoring in general, as I said, I’ll test it asap. Thanks!

pablomartin4btc approved

pablomartin4btc commented at 9:10 pm on February 10, 2023: member

tested ACK 9022473ff167f2677ccad27d98705c3887bf9f96 (unit tests, functionals extended & qt tests)

hebasto commented at 1:35 pm on February 13, 2023: member

Concept ACK.

in src/wallet/wallet.h:642 in 9022473ff1 outdated

637@@ -642,6 +638,12 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
638     /** Absolute maximum transaction fee (in satoshis) used by default for the wallet */
639     CAmount m_default_max_tx_fee{DEFAULT_TRANSACTION_MAXFEE};
640 
641+    /** Number of pre-generated keys/scripts by each spkm (part of the look-ahead process, used to detect payments) */
642+    uint64_t m_keypool_size{DEFAULT_KEYPOOL_SIZE};

hebasto commented at 2:02 pm on February 13, 2023:

Any specific reason to use unsigned type here (as such types are appropriate for bit-wise operations only)?

furszy commented at 2:42 pm on February 13, 2023:

Why only for bit-wise operations? It’s just a standard data type that represents an unsigned 64-bit integer. Didn’t have any specific reason, just that is more concise and specific. Could change it to a bare unsigned int too.

hebasto commented at 3:31 pm on February 13, 2023:

See:

Suggesting:

0    int64_t m_keypool_size{DEFAULT_KEYPOOL_SIZE};

furszy commented at 4:35 pm on February 13, 2023:

ah, you are suggesting to change it to a signed type. Sure. Pushed.

in src/wallet/scriptpubkeyman.h:290 in 9022473ff1 outdated

285@@ -286,6 +286,9 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
286 
287     int64_t nTimeFirstKey GUARDED_BY(cs_KeyStore) = 0;
288 
289+    //! Number of pre-generated keys/scripts (part of the look-ahead process, used to detect payments)
290+    uint64_t m_keypool_size GUARDED_BY(cs_KeyStore) {DEFAULT_KEYPOOL_SIZE};

hebasto commented at 2:19 pm on February 13, 2023:

e0cbd87583cae174d5331ef6fb994f46013f2988, style nit:

0    uint64_t m_keypool_size GUARDED_BY(cs_KeyStore){DEFAULT_KEYPOOL_SIZE};

furszy commented at 2:49 pm on February 13, 2023:

pushed

in src/wallet/scriptpubkeyman.h:564 in 9022473ff1 outdated

559@@ -555,6 +560,9 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
560     //! keeps track of whether Unlock has run a thorough check before
561     bool m_decryption_thoroughly_checked = false;
562 
563+    //! Number of pre-generated keys/scripts (part of the look-ahead process, used to detect payments)
564+    uint64_t m_keypool_size GUARDED_BY(cs_desc_man) {DEFAULT_KEYPOOL_SIZE};

hebasto commented at 2:19 pm on February 13, 2023:

e0cbd87583cae174d5331ef6fb994f46013f2988, style nit:

0    uint64_t m_keypool_size GUARDED_BY(cs_desc_man){DEFAULT_KEYPOOL_SIZE};

in src/wallet/wallet.cpp:2918 in 9022473ff1 outdated

2913@@ -2913,7 +2914,11 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2914     const auto start{SteadyClock::now()};
2915     // TODO: Can't use std::make_shared because we need a custom deleter but
2916     // should be possible to use std::allocate_shared.
2917-    std::shared_ptr<CWallet> walletInstance(new CWallet(chain, name, args, std::move(database)), ReleaseWallet);
2918+    std::shared_ptr<CWallet> walletInstance(new CWallet(chain, name, std::move(database)), ReleaseWallet);
2919+    walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1);

hebasto commented at 2:21 pm on February 13, 2023:

e0cbd87583cae174d5331ef6fb994f46013f2988, style nit:

0    walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t)1);

furszy commented at 2:49 pm on February 13, 2023:

pushed

in src/wallet/wallet.cpp:36 in 9022473ff1 outdated

32@@ -33,6 +33,7 @@
33 #include <util/fees.h>
34 #include <util/moneystr.h>
35 #include <util/rbf.h>
36+#include <util/system.h>
37 #include <util/string.h>

hebasto commented at 2:23 pm on February 13, 2023:

9022473ff167f2677ccad27d98705c3887bf9f96, nit: sort?

0#include <util/string.h>
1#include <util/system.h>

furszy commented at 2:49 pm on February 13, 2023:

pushed

hebasto commented at 2:26 pm on February 13, 2023: member

Approach ACK 9022473ff167f2677ccad27d98705c3887bf9f96, I’ve reviewed code and verified changes in the included headers with the IWYU tool.

A few style nits have been suggested by clang-format-diff.py.

furszy force-pushed on Feb 13, 2023

in src/wallet/rpc/backup.cpp:1481 in 1ff1b9c4cf outdated

1477@@ -1478,7 +1478,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
1478             } else {
1479                 warnings.push_back("Range not given, using default keypool range");
1480                 range_start = 0;
1481-                range_end = gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE);
1482+                range_end = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t)1);

TheCharlatan commented at 4:49 pm on February 13, 2023:

I might have missed something, but why not use the m_keypool_size from the available wallet here?

 0diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
 1index 64253789ec..d9c6755bcb 100644
 2--- a/src/wallet/rpc/backup.cpp
 3+++ b/src/wallet/rpc/backup.cpp
 4@@ -15,7 +15,6 @@
 5 #include <script/standard.h>
 6 #include <sync.h>
 7 #include <util/bip32.h>
 8-#include <util/system.h>
 9 #include <util/time.h>
10 #include <util/translation.h>
11 #include <wallet/rpc/util.h>
12@@ -1480,7 +1479,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
13             } else {
14                 warnings.push_back("Range not given, using default keypool range");
15                 range_start = 0;
16-                range_end = gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE);
17+                range_end = wallet.m_keypool_size;
18             }
19             next_index = range_start;

furszy commented at 4:50 pm on February 13, 2023:

ha, because I’m still sleepy, good catch. Pushing it..

furszy commented at 4:49 pm on February 13, 2023: member

Updated per feedback. Thanks hebasto and TheCharlatan 👌🏼.

Plus added 1ff1b9c to protect importdescriptors from negative ‘-keypool’ value. Applied the same approach used in the wallet code to handle a negative keypool size. Currently, the keypool size is set to 1 in such cases, although it could also be handled by throwing an initialization error but.. the objective was to implement the smallest change in this PR to avoid extending its scope.

furszy force-pushed on Feb 13, 2023

in src/wallet/rpc/backup.cpp:1480 in 1bd4e8dad9 outdated

1477@@ -1478,7 +1478,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
1478             } else {
1479                 warnings.push_back("Range not given, using default keypool range");
1480                 range_start = 0;
1481-                range_end = gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE);
1482+                range_end = wallet.m_keypool_size;

TheCharlatan commented at 9:57 pm on February 13, 2023:

You can also get rid of the system.h include in this file.

furszy commented at 10:30 pm on February 13, 2023:

done

in src/wallet/test/coinselector_tests.cpp:935 in ebb14064a0 outdated

933@@ -934,7 +934,7 @@ BOOST_AUTO_TEST_CASE(effective_value_test)
934 
935 static util::Result<SelectionResult> select_coins(const CAmount& target, const CoinSelectionParams& cs_params, const CCoinControl& cc, std::function<CoinsResult(CWallet&)> coin_setup, interfaces::Chain* chain, const ArgsManager& args)

TheCharlatan commented at 10:11 pm on February 13, 2023:

I think you can remove the ArgsManager from the arguments here.

furszy commented at 10:30 pm on February 13, 2023:

done

TheCharlatan changes_requested

furszy force-pushed on Feb 13, 2023

furszy commented at 10:31 pm on February 13, 2023: member

Updated per feedback. Thanks. Tiny diff.

TheCharlatan approved

TheCharlatan commented at 8:32 am on February 14, 2023: contributor

Code review ACK fafaa7a7e15a49da0214ce9546a42117c63e611f

I would take this one step further though in a follow-up PR and replace the ArgsManager in the WalletContext with an options struct that gets passed to each CWallet instance.

DrahtBot requested review from pablomartin4btc on Feb 14, 2023

in src/wallet/scriptpubkeyman.h:564 in fafaa7a7e1 outdated

559@@ -555,6 +560,9 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
560     //! keeps track of whether Unlock has run a thorough check before
561     bool m_decryption_thoroughly_checked = false;
562 
563+    //! Number of pre-generated keys/scripts (part of the look-ahead process, used to detect payments)
564+    uint64_t m_keypool_size GUARDED_BY(cs_desc_man){DEFAULT_KEYPOOL_SIZE};

hebasto commented at 9:37 am on February 14, 2023:

Sorry for not being explicit in my previous comments. There are more cases where uint64_t could be replaced with int64_t:

0    int64_t m_keypool_size GUARDED_BY(cs_desc_man){DEFAULT_KEYPOOL_SIZE};

and parameter keypool_size in (member) functions.

furszy commented at 12:45 pm on February 14, 2023:

np, small miscommunication. Done now, all of them tackled now. Thanks!

furszy force-pushed on Feb 14, 2023

furszy commented at 12:52 pm on February 14, 2023: member

Feedback tackled. Moved remaining variables to signed type. Small diff.

in src/wallet/wallet.cpp:2918 in d90b54b4aa outdated

2913@@ -2913,7 +2914,11 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2914     const auto start{SteadyClock::now()};
2915     // TODO: Can't use std::make_shared because we need a custom deleter but
2916     // should be possible to use std::allocate_shared.
2917-    std::shared_ptr<CWallet> walletInstance(new CWallet(chain, name, args, std::move(database)), ReleaseWallet);
2918+    std::shared_ptr<CWallet> walletInstance(new CWallet(chain, name, std::move(database)), ReleaseWallet);
2919+    walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t)1);

hebasto commented at 2:02 pm on February 15, 2023:

nanonit: slightly prefer

0    walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{1});

as it was before, and is used recently in our code base.

furszy commented at 3:19 pm on February 15, 2023:

done

hebasto approved

hebasto commented at 2:05 pm on February 15, 2023: member

ACK d90b54b4aa18d60faa0075348fbb29a513558098

furszy force-pushed on Feb 15, 2023

hebasto approved

hebasto commented at 3:29 pm on February 15, 2023: member

re-ACK fa1ce3dd62be739b579373287c5501d99fcf9c17

TheCharlatan approved

TheCharlatan commented at 4:15 pm on February 15, 2023: contributor

re-ACK fa1ce3dd62be739b579373287c5501d99fcf9c17

fanquake commented at 4:16 pm on February 15, 2023: member

@achow101 can you take a look here

in src/wallet/scriptpubkeyman.h:369 in e86214692d outdated

367@@ -365,6 +368,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
368 public:
369     using ScriptPubKeyMan::ScriptPubKeyMan;
370 
371+    LegacyScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : ScriptPubKeyMan(storage), m_keypool_size(keypool_size) {}

achow101 commented at 4:57 pm on February 15, 2023:

In e86214692d3fe7a8c835e07f7ebd47a9afb9fec9 “wallet: set keypool_size instead of access global args manager”

The using ScriptPubKeyMan::ScriptPubKeyMan; line above should be removed so that this new constructor is the only constructor to avoid possible programming errors where the keypool size is not passed in.

furszy commented at 6:50 pm on February 15, 2023:

done

wallet: set keypool_size instead of access global args manager 3477a28dd3

wallet: set '-walletnotify' script instead of access global args manager d8f5fc4462

refactor: wallet, remove global 'ArgsManager' access

we are not using it anymore

6c9b342c30

refactor: remove <util/system.h> include from wallet.h

Since we no longer store a ref to the global `ArgsManager`
inside the wallet, we can move the util/system.h
include to the cpp.

This dependency removal opened a can of worms, as few
other places were, invalidly, depending on the wallet's
header including it.

52f4d567d6

furszy force-pushed on Feb 15, 2023

furszy commented at 6:51 pm on February 15, 2023: member

Updated per feedback. Thanks. Single line removal diff.

TheCharlatan approved

TheCharlatan commented at 9:33 am on February 16, 2023: contributor

Re-ACK 52f4d567d69425dfd514489079db80483024a80d

furszy removed review request from pablomartin4btc on Feb 17, 2023

furszy requested review from hebasto on Feb 17, 2023

hebasto approved

hebasto commented at 4:24 pm on February 17, 2023: member

re-ACK 52f4d567d69425dfd514489079db80483024a80d

DrahtBot requested review from pablomartin4btc on Feb 17, 2023

achow101 commented at 5:38 pm on February 17, 2023: member

ACK 52f4d567d69425dfd514489079db80483024a80d

achow101 merged this on Feb 17, 2023

achow101 closed this on Feb 17, 2023

pablomartin4btc commented at 6:43 pm on February 17, 2023: member

re-ACK 52f4d567d69425dfd514489079db80483024a80d; checked all updates, good stuff!

sidhujag referenced this in commit f7a5a46489 on Feb 17, 2023

pablomartin4btc commented at 4:11 pm on February 20, 2023: member

Found an issue where @MarcoFalke mentioned the need of removing gArgs from dfferent places and @ryanofsky’s comment regarding the GUI specifically, which seem very interesting so I wanted to add the links here.

furszy deleted the branch on Feb 27, 2023

fanquake referenced this in commit c2f2abd0a4 on May 11, 2023

bitcoin locked this on Apr 30, 2024

refactor: wallet, remove global ‘ArgsManager’ dependency #26889

Reviews

Conflicts