refactor: wallet, remove global 'ArgsManager' dependency #26889

pull furszy wants to merge 4 commits into bitcoin:master from furszy:2022_spkm_keypool_size changing 27 files +146 −122
  1. 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.

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

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  3. DrahtBot added the label Wallet on Jan 13, 2023
  4. DrahtBot cross-referenced this on Jan 13, 2023 from issue wallet: Migrate non-HD keys with single combo containing a list of keys by achow101
  5. DrahtBot cross-referenced this on Jan 13, 2023 from issue wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101
  6. DrahtBot cross-referenced this on Jan 13, 2023 from issue wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101
  7. DrahtBot cross-referenced this on Jan 13, 2023 from issue [WIP] wallet: standardize change output detection process by furszy
  8. DrahtBot cross-referenced this on Jan 13, 2023 from issue wallet: rpc to add automatically generated descriptors by achow101
  9. DrahtBot cross-referenced this on Jan 14, 2023 from issue wallet: group independent db writes on single batched db transaction by furszy
  10. 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
  11. furszy force-pushed on Jan 15, 2023
  12. 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
  13. 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:

             : m_args(args),
           m_chain(chain),
           m_name(name),
-          m_database(std::move(database))
+          m_database(std::move(database)),
+          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!

  14. TheCharlatan commented at 9:35 PM on January 16, 2023: contributor

    Concept ACK

  15. furszy renamed this:
    wallet: set keypool size instead of access global args manager from spkm
    refactor: remove global 'ArgsManager' dependency
    on Jan 18, 2023
  16. furszy force-pushed on Jan 18, 2023
  17. furszy renamed this:
    refactor: remove global 'ArgsManager' dependency
    refactor: wallet, remove global 'ArgsManager' dependency
    on Jan 18, 2023
  18. furszy force-pushed on Jan 18, 2023
  19. furszy force-pushed on Jan 18, 2023
  20. DrahtBot cross-referenced this on Jan 18, 2023 from issue Introduce `MockableDatabase` for wallet unit tests by achow101
  21. DrahtBot cross-referenced this on Jan 18, 2023 from issue wallet: bugfix, 'wallet_load_ckey' unit test fails with bdb by furszy
  22. DrahtBot cross-referenced this on Jan 18, 2023 from issue wallet: Implement independent BDB parser by achow101
  23. DrahtBot cross-referenced this on Jan 19, 2023 from issue wallet: Load database records in a particular order by achow101
  24. DrahtBot cross-referenced this on Jan 19, 2023 from issue RPC/Wallet: Add "use_txids" to output of getaddressinfo by luke-jr
  25. S3RK commented at 8:12 AM on January 19, 2023: contributor

    Awesome. Approach ACK

  26. DrahtBot cross-referenced this on Jan 19, 2023 from issue wallet, gui: bugfix, getAvailableBalance skips selected coins by furszy
  27. DrahtBot cross-referenced this on Jan 20, 2023 from issue wallet: coin selection, don't return results that exceed the max allowed weight by furszy
  28. DrahtBot cross-referenced this on Jan 21, 2023 from issue refactor: Fix clang-tidy readability-const-return-type violations by maflcko
  29. DrahtBot cross-referenced this on Jan 21, 2023 from issue refactor: Use util::Result class for wallet loading by ryanofsky
  30. fanquake commented at 4:15 PM on January 22, 2023: member

    Concept ACK

  31. DrahtBot cross-referenced this on Jan 26, 2023 from issue [Draft / POC] Silent Payments by w0xlt
  32. DrahtBot added the label Needs rebase on Feb 1, 2023
  33. furszy force-pushed on Feb 1, 2023
  34. furszy commented at 4:04 PM on February 1, 2023: member

    rebased, conflicts solved.

  35. DrahtBot removed the label Needs rebase on Feb 1, 2023
  36. 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.

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

  38. 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!

  39. pablomartin4btc approved
  40. pablomartin4btc commented at 9:10 PM on February 10, 2023: member

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

  41. DrahtBot cross-referenced this on Feb 11, 2023 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
  42. DrahtBot cross-referenced this on Feb 11, 2023 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky
  43. hebasto commented at 1:35 PM on February 13, 2023: member

    Concept ACK.

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

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

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

  45. 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:

        uint64_t m_keypool_size GUARDED_BY(cs_KeyStore){DEFAULT_KEYPOOL_SIZE};
    furszy commented at 2:49 PM on February 13, 2023:

    pushed

  46. 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:

        uint64_t m_keypool_size GUARDED_BY(cs_desc_man){DEFAULT_KEYPOOL_SIZE};
  47. 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:

        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

  48. 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?

    #include <util/string.h>
#include <util/system.h>
    furszy commented at 2:49 PM on February 13, 2023:

    pushed

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

  50. furszy force-pushed on Feb 13, 2023
  51. furszy force-pushed on Feb 13, 2023
  52. furszy force-pushed on Feb 13, 2023
  53. 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?

    diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 64253789ec..d9c6755bcb 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -15,7 +15,6 @@
 #include <script/standard.h>
 #include <sync.h>
 #include <util/bip32.h>
-#include <util/system.h>
 #include <util/time.h>
 #include <util/translation.h>
 #include <wallet/rpc/util.h>
@@ -1480,7 +1479,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
             } else {
                 warnings.push_back("Range not given, using default keypool range");
                 range_start = 0;
-                range_end = gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE);
+                range_end = wallet.m_keypool_size;
             }
             next_index = range_start;
    furszy commented at 4:50 PM on February 13, 2023:

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

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

  55. furszy force-pushed on Feb 13, 2023
  56. 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

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

  58. TheCharlatan changes_requested
  59. furszy force-pushed on Feb 13, 2023
  60. furszy commented at 10:31 PM on February 13, 2023: member

    Updated per feedback. Thanks. Tiny diff.

  61. TheCharlatan approved
  62. 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.

  63. DrahtBot requested review from pablomartin4btc on Feb 14, 2023
  64. 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:

        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!

  65. furszy force-pushed on Feb 14, 2023
  66. furszy commented at 12:52 PM on February 14, 2023: member

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

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

        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

  68. hebasto approved
  69. hebasto commented at 2:05 PM on February 15, 2023: member

    ACK d90b54b4aa18d60faa0075348fbb29a513558098

  70. maflcko cross-referenced this on Feb 15, 2023 from issue thread 'actix-rt|system:0|arbiter:0' panicked at 'called `Result::unwrap()` on an `Err` value: GitHubError(GitHub { source: GitHubError { documentation_url: Some("https://docs.github.com/rest/reference/pulls#request-reviewers-for-a-pull-request"), errors: None, message: "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the bitcoin/bitcoin repository." }, backtrace: Backtrace( 0: <snafu::backtrace_shim::Backtrace as snafu::GenerateImplicitData>::generate by maflcko
  71. furszy force-pushed on Feb 15, 2023
  72. hebasto approved
  73. hebasto commented at 3:29 PM on February 15, 2023: member

    re-ACK fa1ce3dd62be739b579373287c5501d99fcf9c17

  74. TheCharlatan approved
  75. TheCharlatan commented at 4:15 PM on February 15, 2023: contributor

    re-ACK fa1ce3dd62be739b579373287c5501d99fcf9c17

  76. fanquake commented at 4:16 PM on February 15, 2023: member

    @achow101 can you take a look here

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

  78. wallet: set keypool_size instead of access global args manager 3477a28dd3
  79. wallet: set '-walletnotify' script instead of access global args manager d8f5fc4462
  80. refactor: wallet, remove global 'ArgsManager' access
    we are not using it anymore
    6c9b342c30
  81. 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
  82. furszy force-pushed on Feb 15, 2023
  83. furszy commented at 6:51 PM on February 15, 2023: member

    Updated per feedback. Thanks. Single line removal diff.

  84. TheCharlatan approved
  85. TheCharlatan commented at 9:33 AM on February 16, 2023: contributor

    Re-ACK 52f4d567d69425dfd514489079db80483024a80d

  86. furszy removed review request from pablomartin4btc on Feb 17, 2023
  87. furszy requested review from hebasto on Feb 17, 2023
  88. hebasto approved
  89. hebasto commented at 4:24 PM on February 17, 2023: member

    re-ACK 52f4d567d69425dfd514489079db80483024a80d

  90. DrahtBot requested review from pablomartin4btc on Feb 17, 2023
  91. achow101 commented at 5:38 PM on February 17, 2023: member

    ACK 52f4d567d69425dfd514489079db80483024a80d

  92. achow101 merged this on Feb 17, 2023
  93. achow101 closed this on Feb 17, 2023
  94. pablomartin4btc commented at 6:43 PM on February 17, 2023: member

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

  95. sidhujag referenced this in commit f7a5a46489 on Feb 17, 2023
  96. TheCharlatan cross-referenced this on Feb 19, 2023 from issue Remove blockstorage args by TheCharlatan
  97. TheCharlatan cross-referenced this on Feb 19, 2023 from issue refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan
  98. 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.

  99. furszy deleted the branch on Feb 27, 2023
  100. furszy cross-referenced this on Mar 3, 2023 from issue wallet: group outputs only once, decouple it from Coin Selection by furszy
  101. fanquake referenced this in commit c2f2abd0a4 on May 11, 2023
  102. bitcoin locked this on Apr 30, 2024
Contributors
furszyDrahtBotTheCharlatanS3RKfanquakepablomartin4btchebastoachow101
Labels
Wallet
Linked (view graph)
#21005 Remove gArgs#25806 wallet: group outputs only once, decouple it from Coin Selection#27125 refactor, kernel: Decouple ArgsManager from blockstorage
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC