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

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

    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 added the label Needs rebase on May 5, 2023
  5. TheCharlatan force-pushed on May 6, 2023
  6. DrahtBot removed the label Needs rebase on May 6, 2023
  7. DrahtBot added the label Needs rebase on May 6, 2023
  8. TheCharlatan force-pushed on May 8, 2023
  9. TheCharlatan force-pushed on May 11, 2023
  10. fanquake commented at 9:48 am on May 11, 2023: member 
    0src/common/settings.h seems to be missing the expected include guard:
1  #ifndef BITCOIN_COMMON_SETTINGS_H
2  #define BITCOIN_COMMON_SETTINGS_H
3  ...
4  #endif // BITCOIN_COMMON_SETTINGS_H
  11. DrahtBot removed the label Needs rebase on May 11, 2023
  12. DrahtBot added the label CI failed on May 11, 2023
  13. TheCharlatan force-pushed on May 11, 2023
  14. hebasto commented at 2:12 pm on May 11, 2023: member
    Concept ACK.
  15. DrahtBot removed the label CI failed on May 11, 2023
  16. TheCharlatan force-pushed on May 14, 2023
  17. DrahtBot added the label Needs rebase on May 17, 2023
  18. TheCharlatan force-pushed on May 17, 2023
  19. DrahtBot removed the label Needs rebase on May 17, 2023
  20. DrahtBot added the label Needs rebase on May 17, 2023
  21. TheCharlatan force-pushed on May 20, 2023
  22. DrahtBot removed the label Needs rebase on May 20, 2023
  23. TheCharlatan force-pushed on May 21, 2023
  24. TheCharlatan force-pushed on May 24, 2023
  25. 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
  26. 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
  27. 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
  28. kernel: Remove chainparams, chainparamsbase, args, settings from kernel library c2dae5d7d8
  29. 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
  30. 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
  31. TheCharlatan force-pushed on May 30, 2023
  32. DrahtBot added the label CI failed on May 30, 2023
  33. TheCharlatan marked this as ready for review on May 30, 2023
  34. TheCharlatan renamed this:
    kernel: Remove args, chainparams, chainparamsbase from kernel library
    kernel: Remove args, settings, chainparams, chainparamsbase from kernel library
    on May 30, 2023
  35. DrahtBot removed the label CI failed on May 31, 2023
  36. 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.
  37. hebasto approved
  38. hebasto commented at 12:59 pm on June 5, 2023: member
    ACK db77f87c6365cb5f414036d6bfb1a12705772028, I have reviewed the code and it looks OK.
  39. fanquake requested review from maflcko on Jun 7, 2023
  40. 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 🍄

    Signature:

    0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK db77f87c6365cb5f414036d6bfb1a12705772028 🍄
3KqZtB8gKRVdhu2hRGnc62rXYkD1VZ6jPpAiKmJTULaR+TMukUea0QvkzFq3lg5Z0B00R4fjYSZHUc0oJ/M5qAw==
  41. DrahtBot removed review request from maflcko on Jun 8, 2023
  42. 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
  43. ryanofsky approved
  44. 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.

  45. ryanofsky approved
  46. 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

  47. ryanofsky merged this on Jun 9, 2023
  48. ryanofsky closed this on Jun 9, 2023
  49. sidhujag referenced this in commit 8f438f615d on Jun 12, 2023
  50. achow101 referenced this in commit 427853ab49 on Jun 13, 2023
  51. sidhujag referenced this in commit 0cdc15f1aa on Jun 15, 2023
  52. hebasto referenced this in commit 592da16150 on Aug 29, 2023
  53. hebasto referenced this in commit c77e8b5c38 on Aug 29, 2023
  54. hebasto referenced this in commit 8b025633e4 on Aug 29, 2023
  55. hebasto referenced this in commit 14ddf61693 on Aug 31, 2023
  56. bitcoin locked this on Jun 12, 2024


TheCharlatan DrahtBot fanquake hebasto maflcko ryanofsky

Labels
Validation

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: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me