kernel: Remove args, settings, chainparams, chainparamsbase from kernel library #27576

pull TheCharlatan wants to merge 6 commits into bitcoin:master from TheCharlatan:rmKernelArgs changing 27 files +167 −171
  1. TheCharlatan commented at 9:36 PM on May 4, 2023: contributor

    This pull request is part of the libbitcoinkernel project #27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".

    This completes the removal of the node's chainparams, chainparamsbase, args and settings files and their respective classes from the kernel library. This is the last pull request in a long series working towards decoupling the ArgsManager and the gArgs global from kernel code. These prior pull requests are: #26177 https://github.com/bitcoin/bitcoin/pull/27125 #25527 https://github.com/bitcoin/bitcoin/pull/25487 #25290

  2. DrahtBot commented at 9:36 PM on May 4, 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 hebasto, MarcoFalke, ryanofsky

    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:

    • #27822 (Renamed UniValue::__pushKV to UniValue::pushKVEnd. by Brotcrunsher)
    • #27711 (kernel: Remove shutdown globals from kernel library by TheCharlatan)
    • #26762 (refactor: Make CCheckQueue RAII-styled by hebasto)
    • #24479 (Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg by luke-jr)

    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 Validation on May 4, 2023
  4. DrahtBot cross-referenced this on May 5, 2023 from issue Allow accepting non-standard transactions on mainnet by benthecarman
  5. DrahtBot cross-referenced this on May 5, 2023 from issue ci: Remove CI_EXEC bloat in test/06_script_b.sh by maflcko
  6. DrahtBot cross-referenced this on May 5, 2023 from issue ci: Run iwyu on all src files by maflcko
  7. DrahtBot cross-referenced this on May 5, 2023 from issue refactor: Remove need to pass chainparams to BlockManager methods by maflcko
  8. DrahtBot cross-referenced this on May 5, 2023 from issue rpc: add 'getnetmsgstats', new rpc to view network message statistics by satsie
  9. DrahtBot cross-referenced this on May 5, 2023 from issue Remove now-unnecessary poll, fcntl includes from net(base).cpp by Empact
  10. DrahtBot cross-referenced this on May 5, 2023 from issue blockstorage: do not flush block to disk if it is already there by pinheadmz
  11. DrahtBot cross-referenced this on May 5, 2023 from issue reduce cs_main scope, guard block index 'nFile' under a local mutex by furszy
  12. DrahtBot cross-referenced this on May 5, 2023 from issue index: blockfilter initial sync speedup, parallelize process by furszy
  13. DrahtBot cross-referenced this on May 5, 2023 from issue bugfix: Make `CCheckQueue` RAII-styled (attempt 2) by hebasto
  14. DrahtBot cross-referenced this on May 5, 2023 from issue test: test_bitcoin: allow -testdatadir=<datadir> by LarryRuane
  15. DrahtBot cross-referenced this on May 5, 2023 from issue policy: Ephemeral anchors by instagibbs
  16. DrahtBot cross-referenced this on May 5, 2023 from issue net: don't lock cs_main while reading blocks in net processing by andrewtoth
  17. DrahtBot cross-referenced this on May 5, 2023 from issue Enable -Wstring-concatenation and -Wstring-conversion on clang builds by Empact
  18. DrahtBot cross-referenced this on May 5, 2023 from issue refactor: Replace `std::optional<bilingual_str>` with `util::Result` by ryanofsky
  19. DrahtBot cross-referenced this on May 5, 2023 from issue p2p: remove adjusted time by fanquake
  20. DrahtBot cross-referenced this on May 5, 2023 from issue indexes: Read the locator's top block during init, allow interaction with reindex-chainstate by mzumsande
  21. DrahtBot cross-referenced this on May 5, 2023 from issue policy: nVersion=3 and Package RBF by glozow
  22. DrahtBot cross-referenced this on May 5, 2023 from issue indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky
  23. DrahtBot cross-referenced this on May 5, 2023 from issue util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky
  24. DrahtBot cross-referenced this on May 5, 2023 from issue refactor: Remove settings merge reverse precedence code by ryanofsky
  25. DrahtBot cross-referenced this on May 5, 2023 from issue refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky
  26. DrahtBot cross-referenced this on May 5, 2023 from issue util: Forbid ambiguous multiple assignments in config file by ryanofsky
  27. DrahtBot cross-referenced this on May 5, 2023 from issue refactor: Implement missing error checking for ArgsManager flags by ryanofsky
  28. DrahtBot cross-referenced this on May 5, 2023 from issue assumeutxo by jamesob
  29. DrahtBot added the label Needs rebase on May 5, 2023
  30. DrahtBot cross-referenced this on May 5, 2023 from issue fuzz: BIP 30, CVE-2018-17144 by maflcko
  31. TheCharlatan force-pushed on May 6, 2023
  32. DrahtBot removed the label Needs rebase on May 6, 2023
  33. DrahtBot added the label Needs rebase on May 6, 2023
  34. TheCharlatan force-pushed on May 8, 2023
  35. TheCharlatan force-pushed on May 11, 2023
  36. fanquake commented at 9:48 AM on May 11, 2023: member 
    src/common/settings.h seems to be missing the expected include guard:
  #ifndef BITCOIN_COMMON_SETTINGS_H
  #define BITCOIN_COMMON_SETTINGS_H
  ...
  #endif // BITCOIN_COMMON_SETTINGS_H
  37. DrahtBot removed the label Needs rebase on May 11, 2023
  38. DrahtBot added the label CI failed on May 11, 2023
  39. TheCharlatan force-pushed on May 11, 2023
  40. hebasto commented at 2:12 PM on May 11, 2023: member

    Concept ACK.

  41. DrahtBot removed the label CI failed on May 11, 2023
  42. DrahtBot cross-referenced this on May 11, 2023 from issue refactor: Use util::Result class for wallet loading by ryanofsky
  43. DrahtBot cross-referenced this on May 11, 2023 from issue refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky
  44. DrahtBot cross-referenced this on May 12, 2023 from issue kernel: Remove util/system from kernel library, interface_ui from validation. by TheCharlatan
  45. TheCharlatan force-pushed on May 14, 2023
  46. DrahtBot cross-referenced this on May 14, 2023 from issue assumeutxo (2) by jamesob
  47. DrahtBot cross-referenced this on May 14, 2023 from issue rpc: Add importmempool RPC by maflcko
  48. DrahtBot cross-referenced this on May 14, 2023 from issue refactor: extract CCheckQueue's data handling into a separate container "Bag" by martinus
  49. DrahtBot cross-referenced this on May 14, 2023 from issue Improve display address handling for external signer by Sjors
  50. DrahtBot cross-referenced this on May 14, 2023 from issue assumeutxo: net_processing changes by jamesob
  51. DrahtBot cross-referenced this on May 14, 2023 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky
  52. DrahtBot cross-referenced this on May 14, 2023 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
  53. DrahtBot cross-referenced this on May 14, 2023 from issue Add fee_est tool for debugging fee estimation code by ryanofsky
  54. DrahtBot added the label Needs rebase on May 17, 2023
  55. TheCharlatan force-pushed on May 17, 2023
  56. DrahtBot removed the label Needs rebase on May 17, 2023
  57. DrahtBot added the label Needs rebase on May 17, 2023
  58. TheCharlatan force-pushed on May 20, 2023
  59. DrahtBot removed the label Needs rebase on May 20, 2023
  60. DrahtBot cross-referenced this on May 20, 2023 from issue index: make startup more efficient by furszy
  61. DrahtBot cross-referenced this on May 20, 2023 from issue Return EXIT_FAILURE on post-init fatal errors by furszy
  62. TheCharlatan force-pushed on May 21, 2023
  63. TheCharlatan cross-referenced this on May 21, 2023 from issue kernel: Remove shutdown globals from kernel library by TheCharlatan
  64. TheCharlatan force-pushed on May 24, 2023
  65. refactor: Add stop_at_height option in ChainstateManager
    Remove access to the global gArgs for the stopatheight argument and
replace it by adding a field to the existing ChainstateManager Options
struct.

This should eventually allow users of the ChainstateManager to not rely
on the global gArgs and instead pass in their own options.
    ef95be334f
  66. refactor: Add path argument to FindSnapshotChainstateDir
    Remove access to the global gArgs for getting the directory in
utxo_snapshot.

This is done in the context of the libbitcoinkernel project, wherein
reliance of libbitcoinkernel code on the global gArgs is incrementally
removed.
    8789b11114
  67. refactor: Remove gArgs access from validation.cpp
    This is done in the context of the libbitcoinkernel project, wherein
reliance of libbitcoinkernel code on the global gArgs is incrementally
removed.
    05870b1c92
  68. kernel: Remove chainparams, chainparamsbase, args, settings from kernel library c2dae5d7d8
  69. move-only: Move settings to the common library
    The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from code that is not strictly required by it.
The settings code belongs into the common library and namespace, since
the kernel library should not depend on it. See doc/design/libraries.md
for more information on this rationale.

Changing the namespace of the moved functions is scripted in the
following commit.
    c27e4bdc35
  70. scripted-diff: move settings to common namespace
    -BEGIN VERIFY SCRIPT-
sed -i 's/namespace\ util/namespace\ common/g' src/common/settings.cpp src/common/settings.h
sed -i 's/util\:\:GetSetting/common\:\:GetSetting/g' $( git grep -l 'util\:\:GetSetting')
sed -i 's/util\:\:Setting/common\:\:Setting/g' $( git grep -l 'util\:\:Setting')
sed -i 's/util\:\:FindKey/common\:\:FindKey/g' $( git grep -l 'util\:\:FindKey')
sed -i 's/util\:\:ReadSettings/common\:\:ReadSettings/g' $( git grep -l 'util\:\:ReadSettings')
sed -i 's/util\:\:WriteSettings/common\:\:WriteSettings/g' $( git grep -l 'util\:\:WriteSettings')
-END VERIFY SCRIPT-
    db77f87c63
  71. TheCharlatan force-pushed on May 30, 2023
  72. DrahtBot added the label CI failed on May 30, 2023
  73. TheCharlatan marked this as ready for review on May 30, 2023
  74. TheCharlatan renamed this:
    kernel: Remove args, chainparams, chainparamsbase from kernel library
    kernel: Remove args, settings, chainparams, chainparamsbase from kernel library
    on May 30, 2023
  75. DrahtBot removed the label CI failed on May 31, 2023
  76. DrahtBot cross-referenced this on Jun 5, 2023 from issue Renamed UniValue::__pushKV to UniValue::pushKVEnd. by Brotcrunsher
  77. in src/Makefile.am:920 in db77f87c63 
     911 | @@ -912,12 +912,8 @@ libbitcoinkernel_la_SOURCES = \
 912 |    kernel/bitcoinkernel.cpp \
 913 |    arith_uint256.cpp \
 914 |    chain.cpp \
 915 | -  chainparamsbase.cpp \
 916 | -  chainparams.cpp \
 917 |    clientversion.cpp \
 918 |    coins.cpp \
 919 | -  common/args.cpp \
 920 | -  common/config.cpp \
    hebasto commented at 12:59 PM on June 5, 2023:

    Side note: Removing of common/config.cpp is correct but not directly related to this PR changes as it can be done even on the current master branch.

  78. hebasto approved
  79. hebasto commented at 12:59 PM on June 5, 2023: member

    ACK db77f87c6365cb5f414036d6bfb1a12705772028, I have reviewed the code and it looks OK.

  80. fanquake requested review from maflcko on Jun 7, 2023
  81. maflcko commented at 5:23 AM on June 8, 2023: member

    nit: In the last commit, any reason to use \ and \:? Seems to pass with just and : for me.

    lgtm ACK db77f87c6365cb5f414036d6bfb1a12705772028 🍄

    <details><summary>Show signature</summary>

    Signature:

    untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK db77f87c6365cb5f414036d6bfb1a12705772028 🍄
KqZtB8gKRVdhu2hRGnc62rXYkD1VZ6jPpAiKmJTULaR+TMukUea0QvkzFq3lg5Z0B00R4fjYSZHUc0oJ/M5qAw==

    </details>

  82. DrahtBot removed review request from maflcko on Jun 8, 2023
  83. in src/test/validation_chainstatemanager_tests.cpp:187 in 8789b11114 outdated 
     183 | @@ -184,7 +184,7 @@ struct SnapshotTestSetup : TestChain100Setup {
 184 |          {
 185 |              LOCK(::cs_main);
 186 |              BOOST_CHECK(!chainman.IsSnapshotValidated());
 187 | -            BOOST_CHECK(!node::FindSnapshotChainstateDir());
 188 | +            BOOST_CHECK(!node::FindSnapshotChainstateDir(m_args.GetDataDirNet()));
    ryanofsky commented at 6:43 PM on June 9, 2023:

    In commit "refactor: Add path argument to FindSnapshotChainstateDir" (8789b11114b4bd6c7ee727dffbc75a6bdf20dd27)

    Throughout this file, it would make tests clearer and less reliant on details of test setup if the new m_args.GetDataDirNet() calls were replaced with chainman.m_options.datadir

    fanquake commented at 3:53 PM on June 12, 2023:

    @TheCharlatan did you want to open a followup for this?

    TheCharlatan commented at 3:59 PM on June 12, 2023:

    Yes.

    TheCharlatan commented at 1:22 PM on June 13, 2023:

    https://github.com/bitcoin/bitcoin/pull/27876

  84. ryanofsky approved
  85. ryanofsky commented at 6:48 PM on June 9, 2023: contributor

    Code review ACK db77f87c6365cb5f414036d6bfb1a12705772028. Looks great!

    I left a suggestion to clean up a unit test in a possible followup, but I will just go ahead and merge this now if tests pass locally.

  86. ryanofsky approved
  87. ryanofsky commented at 6:53 PM on June 9, 2023: contributor

    In commit "scripted-diff: move settings to common namespace" (db77f87c6365cb5f414036d6bfb1a12705772028)

    Not important but I noticed the scripted diff is escaping space and colon characters. I think there is not actually a need for all those backslashes, and they don't do anything

    EDIT: just noticed Marco left the same comment. Great minds, I guess

  88. ryanofsky merged this on Jun 9, 2023
  89. ryanofsky closed this on Jun 9, 2023
  90. sidhujag referenced this in commit 8f438f615d on Jun 12, 2023
  91. TheCharlatan cross-referenced this on Jun 13, 2023 from issue test: (refactor) Use datadir from options in chainstatemanager test by TheCharlatan
  92. achow101 referenced this in commit 427853ab49 on Jun 13, 2023
  93. sidhujag referenced this in commit 0cdc15f1aa on Jun 15, 2023
  94. hebasto referenced this in commit 592da16150 on Aug 29, 2023
  95. hebasto referenced this in commit c77e8b5c38 on Aug 29, 2023
  96. hebasto referenced this in commit 8b025633e4 on Aug 29, 2023
  97. hebasto cross-referenced this on Aug 30, 2023 from issue Sync with the main repo by hebasto
  98. hebasto referenced this in commit 14ddf61693 on Aug 31, 2023
  99. bitcoin locked this on Jun 12, 2024
Contributors
TheCharlatanDrahtBotfanquakehebastomaflckoryanofsky
Labels
Validation
Linked (view graph)
#24303 The `libbitcoinkernel` Project#25290 [kernel 3a/n] Decouple `CTxMemPool` from `ArgsManager`#25487 [kernel 3b/n] Decouple `{Dump,Load}Mempool` from `ArgsManager`#25527 [kernel 3c/n] Decouple validation cache initialization from `ArgsManager`#26177 refactor / kernel: Move non-gArgs chainparams functionality to kernel#27125 refactor, kernel: Decouple ArgsManager from blockstorage#27491 refactor: Move chain constants to the util library#27587 Bitcoin Kernel Library Project Tracking#27711 kernel: Remove shutdown globals from kernel library#27876 test: (refactor) Use datadir from options in chainstatemanager test
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-19 06:52 UTC