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

TheCharlatan · 2023-05-04T21:36:34Z

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 #27125 #25527 #25487 #25290

DrahtBot · 2023-05-04T21:36:37Z

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.

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.

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.

This is done in the context of the libbitcoinkernel project, wherein reliance of libbitcoinkernel code on the global gArgs is incrementally removed.

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

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

hebasto

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

hebasto · 2023-06-05T12:59:52Z

src/Makefile.am

  clientversion.cpp \
  coins.cpp \
-  common/args.cpp \
-  common/config.cpp \


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.

maflcko · 2023-06-08T05:23:27Z

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

lgtm ACK db77f87 🍄

Show signature

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

ryanofsky

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

ryanofsky · 2023-06-09T18:43:47Z

src/test/validation_chainstatemanager_tests.cpp

@@ -184,7 +184,7 @@ struct SnapshotTestSetup : TestChain100Setup {
        {
            LOCK(::cs_main);
            BOOST_CHECK(!chainman.IsSnapshotValidated());
-            BOOST_CHECK(!node::FindSnapshotChainstateDir());
+            BOOST_CHECK(!node::FindSnapshotChainstateDir(m_args.GetDataDirNet()));


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

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

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

ryanofsky

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

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

…nparams…

…s in chainstatemanager test d54819d scripted-diff: Use datadir from options in chainstatemanager test (TheCharlatan) Pull request description: This should make the test less reliant on argument state from the test setup. This is a follow-up PR as requested in bitcoin/bitcoin#27576 (comment). ACKs for top commit: achow101: ACK d54819d MarcoFalke: lgtm ACK d54819d kevkevinpal: ACK bitcoin/bitcoin@d54819d ryanofsky: Code review ACK d54819d Tree-SHA512: 939fde2505c5585d993545a3d05d3a00caec40f860c74fa002caebdf4c1b70e774cfb028a8a8f780525f8968844157d2c568d9f2c8dd5ec32b093173d8644c34

…instatemanager test d54819d scripted-diff: Use datadir from options in chainstatemanager test (TheCharlatan) Pull request description: This should make the test less reliant on argument state from the test setup. This is a follow-up PR as requested in bitcoin#27576 (comment). ACKs for top commit: achow101: ACK d54819d MarcoFalke: lgtm ACK d54819d kevkevinpal: ACK bitcoin@d54819d ryanofsky: Code review ACK d54819d Tree-SHA512: 939fde2505c5585d993545a3d05d3a00caec40f860c74fa002caebdf4c1b70e774cfb028a8a8f780525f8968844157d2c568d9f2c8dd5ec32b093173d8644c34

The following changes considered: - bitcoin/bitcoin#27419 - bitcoin/bitcoin#27491 - bitcoin/bitcoin#27576 - bitcoin/bitcoin#27636

Pull request description: Sync with the main repo up to the latest bitcoin/bitcoin@ab42b2e, which includes the recent changes in the CI. There is no downloadable artifacts support for now. It will be done in a separated PR(s). Additionally: - The code was adjusted to reflect changes from [PR27419](bitcoin/bitcoin#27419), [PR27491](bitcoin/bitcoin#27491), [PR27576](bitcoin/bitcoin#27576) and [PR27636](bitcoin/bitcoin#27636). - Fixed `modernize-use-default-member-init` clang-tidy warnings. - The ARM task has been temporarily disabled until the issue with the depends cache is resolved. Guix builds: ``` e92b8c4c3298165edb1a0e85ee516d52c81af1269405dcbc6520e63069de2363 guix-build-b3261144c892/output/aarch64-linux-gnu/SHA256SUMS.part 939c6c002490d5649bdbfabacd20cd2270b41b20b7b3a254c9fcd5780209900d guix-build-b3261144c892/output/aarch64-linux-gnu/bitcoin-b3261144c892-aarch64-linux-gnu-debug.tar.gz b3c1383fb394997378997bdd2933965cf4ecc694143b4703108ff6ecb946696c guix-build-b3261144c892/output/aarch64-linux-gnu/bitcoin-b3261144c892-aarch64-linux-gnu.tar.gz f43fedf3af666d35e83b84e63cfe19f315f74f01296982f47d8c159385c3b03c guix-build-b3261144c892/output/arm-linux-gnueabihf/SHA256SUMS.part 73b89b0487e8eee474a6c9c96ae0e7ad635cccc332fc062eb5d4ff5555356c3e guix-build-b3261144c892/output/arm-linux-gnueabihf/bitcoin-b3261144c892-arm-linux-gnueabihf-debug.tar.gz b4518dd9396f316de8d7de5181b8b5d1083e0afa9081625c37117472d2559380 guix-build-b3261144c892/output/arm-linux-gnueabihf/bitcoin-b3261144c892-arm-linux-gnueabihf.tar.gz 0213e754408e2a032cef61a946354656f5b5f755f85aeac1ce4b37f1d22528e6 guix-build-b3261144c892/output/arm64-apple-darwin/SHA256SUMS.part 11bc1be1f53dad337565f3c556dd69abc2d702a31e661359daad6ff89225c794 guix-build-b3261144c892/output/arm64-apple-darwin/bitcoin-b3261144c892-arm64-apple-darwin-unsigned.dmg 558d8e805420c7a348759df6f559ca349953646aa28840efafe5a3d245ea917f guix-build-b3261144c892/output/arm64-apple-darwin/bitcoin-b3261144c892-arm64-apple-darwin-unsigned.tar.gz e679ce3f1c80aff11a5eab8890efbd0d396a851875fbd6f93f32eef5cdf06813 guix-build-b3261144c892/output/arm64-apple-darwin/bitcoin-b3261144c892-arm64-apple-darwin.tar.gz 0cb346390dc6620593b1af5b6669ddc3c1a8d2219a51b1697747c5ab24069c27 guix-build-b3261144c892/output/dist-archive/bitcoin-b3261144c892.tar.gz ac8bd2d58d9d0ebe2da1c8efa2d57bd97c3ef2b2590c758edbc4919808c528c5 guix-build-b3261144c892/output/powerpc64-linux-gnu/SHA256SUMS.part cdf8252fa8aca6da61ff6926de5c7e2e6560ab046049c84c26ba44823f83236a guix-build-b3261144c892/output/powerpc64-linux-gnu/bitcoin-b3261144c892-powerpc64-linux-gnu-debug.tar.gz 3b8b5f53d365b5bf962ecd7def9f06b6f13af0e5c9ef69c6d028f1ed772459be guix-build-b3261144c892/output/powerpc64-linux-gnu/bitcoin-b3261144c892-powerpc64-linux-gnu.tar.gz b44e688d233dcb46a7d6d0b1d97979335d3cc559d16190cc5cd647add79298d2 guix-build-b3261144c892/output/powerpc64le-linux-gnu/SHA256SUMS.part ae5c19afefd523cdc171a3f9aa9f707870fd99749c01c01166086619dfd95ece guix-build-b3261144c892/output/powerpc64le-linux-gnu/bitcoin-b3261144c892-powerpc64le-linux-gnu-debug.tar.gz bb581b1444fa1686f8889248af13d1859f2915091cd640bc522185d5ad83e13d guix-build-b3261144c892/output/powerpc64le-linux-gnu/bitcoin-b3261144c892-powerpc64le-linux-gnu.tar.gz bdca0a3c19b5a9a5c72b2b43b07050678d960009d3fa80cf7e0689d508346974 guix-build-b3261144c892/output/riscv64-linux-gnu/SHA256SUMS.part b0b9c91abe2ad0b5ab3b0bfd10c90133d8d75b50aef0a6a98ac2c2ae4219eaa8 guix-build-b3261144c892/output/riscv64-linux-gnu/bitcoin-b3261144c892-riscv64-linux-gnu-debug.tar.gz fcce0ea00f1d9df136dd677cbc468183faa92bd4bfcd4a77cd1c70f1b894b5f0 guix-build-b3261144c892/output/riscv64-linux-gnu/bitcoin-b3261144c892-riscv64-linux-gnu.tar.gz 7be84969950bb9570522be5a37551c01698cd3fb65eca3988fc9bd6867460552 guix-build-b3261144c892/output/x86_64-apple-darwin/SHA256SUMS.part 25203f50aa6a344ad1c6c4a44a48082440bb0af9bf38f0d60506569f216d1672 guix-build-b3261144c892/output/x86_64-apple-darwin/bitcoin-b3261144c892-x86_64-apple-darwin-unsigned.dmg 16c5baaf6d00ed43b0611c86c2d4555d500b3896daa1daac6a567bc2611c39f6 guix-build-b3261144c892/output/x86_64-apple-darwin/bitcoin-b3261144c892-x86_64-apple-darwin-unsigned.tar.gz 86662f39c29b013b576e6555ecb6cbbc98eaa08532a541e22a7ed6b1baf87209 guix-build-b3261144c892/output/x86_64-apple-darwin/bitcoin-b3261144c892-x86_64-apple-darwin.tar.gz fbbc0ad2376431fdc5b214fd63f24a6da907d87f6f11e0833def50c0d45772cd guix-build-b3261144c892/output/x86_64-linux-gnu/SHA256SUMS.part cba8d700f746a6063809570e45d6dc3d5e60ad5f1a28e0f41f8beed8b546a7b1 guix-build-b3261144c892/output/x86_64-linux-gnu/bitcoin-b3261144c892-x86_64-linux-gnu-debug.tar.gz 0a32985a1e26e13ce883a85e4a92cc68bf51ce096f2f6d74ea499a9fa662d7d0 guix-build-b3261144c892/output/x86_64-linux-gnu/bitcoin-b3261144c892-x86_64-linux-gnu.tar.gz 0bd4cc64cd6ad733cdef87cd74d5034e79dd250b72795cebf9c2c63500509457 guix-build-b3261144c892/output/x86_64-w64-mingw32/SHA256SUMS.part 6ed8f2e6c6cf1992d156672707cd2c254754051f88223dd052a9cd9078d84789 guix-build-b3261144c892/output/x86_64-w64-mingw32/bitcoin-b3261144c892-win64-debug.zip 1ea6d7660652e20b2b1529e406be1f606745d35f6a179b006335a19a19aa9a5b guix-build-b3261144c892/output/x86_64-w64-mingw32/bitcoin-b3261144c892-win64-setup-unsigned.exe 41b0f8cbac614e8c555921de60b25a73a75e6bed025de98ca40d3db48c5db6b1 guix-build-b3261144c892/output/x86_64-w64-mingw32/bitcoin-b3261144c892-win64-unsigned.tar.gz 5c68d711782e76f9e4be93b5468c505f022b72ca299532b200e58fe1e51343b1 guix-build-b3261144c892/output/x86_64-w64-mingw32/bitcoin-b3261144c892-win64.zip ``` Top commit has no ACKs. Tree-SHA512: dd18cfb2cfd6fd45b35bef8a0397bccc0752ce946b304bae986006ff09a9a183d6222b0f607e4dd3373992814ae0e61d5ba63cb54fef9a288152edef3d7ea81d

DrahtBot added the Validation label May 4, 2023

DrahtBot added the Needs rebase label May 5, 2023

DrahtBot mentioned this pull request May 5, 2023

fuzz: BIP 30, CVE-2018-17144 #17860

Merged

TheCharlatan force-pushed the rmKernelArgs branch from 0936f0c to f41da64 Compare May 6, 2023 07:20

TheCharlatan force-pushed the rmKernelArgs branch from 9974f06 to aae6b95 Compare May 24, 2023 14:35

TheCharlatan added 6 commits May 30, 2023 16:52

refactor: Add path argument to FindSnapshotChainstateDir

8789b11

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.

refactor: Remove gArgs access from validation.cpp

05870b1

This is done in the context of the libbitcoinkernel project, wherein reliance of libbitcoinkernel code on the global gArgs is incrementally removed.

kernel: Remove chainparams, chainparamsbase, args, settings from kern…

c2dae5d

…el library

TheCharlatan force-pushed the rmKernelArgs branch from aae6b95 to db77f87 Compare May 30, 2023 15:27

DrahtBot added the CI failed label May 30, 2023

TheCharlatan marked this pull request as ready for review May 30, 2023 16:35

TheCharlatan changed the title ~~kernel: Remove args, chainparams, chainparamsbase from kernel library~~ kernel: Remove args, settings, chainparams, chainparamsbase from kernel library May 30, 2023

DrahtBot removed the CI failed label May 31, 2023

DrahtBot mentioned this pull request Jun 5, 2023

Renamed UniValue::__pushKV to UniValue::pushKVEnd. #27822

Merged

hebasto approved these changes Jun 5, 2023

View reviewed changes

fanquake requested a review from maflcko June 7, 2023 13:13

DrahtBot removed the request for review from maflcko June 8, 2023 05:23

ryanofsky approved these changes Jun 9, 2023

View reviewed changes

ryanofsky merged commit 153a688 into bitcoin:master Jun 9, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 12, 2023

Merge bitcoin#27576: kernel: Remove args, settings, chainparams, chai…

8f438f6

…nparams…

TheCharlatan mentioned this pull request Jun 13, 2023

test: (refactor) Use datadir from options in chainstatemanager test #27876

Merged

hebasto added a commit to hebasto/gui-qml that referenced this pull request Aug 29, 2023

Adjust code according to changes in the main repository

592da16

The following changes considered: - bitcoin/bitcoin#27419 - bitcoin/bitcoin#27491 - bitcoin/bitcoin#27576 - bitcoin/bitcoin#27636

hebasto added a commit to hebasto/gui-qml that referenced this pull request Aug 29, 2023

Adjust code according to changes in the main repository

c77e8b5

The following changes considered: - bitcoin/bitcoin#27419 - bitcoin/bitcoin#27491 - bitcoin/bitcoin#27576 - bitcoin/bitcoin#27636

hebasto added a commit to hebasto/gui-qml that referenced this pull request Aug 29, 2023

Adjust code according to changes in the main repository

8b02563

The following changes considered: - bitcoin/bitcoin#27419 - bitcoin/bitcoin#27491 - bitcoin/bitcoin#27576 - bitcoin/bitcoin#27636

hebasto mentioned this pull request Aug 30, 2023

Sync with the main repo bitcoin-core/gui-qml#359

Merged

bitcoin locked and limited conversation to collaborators Jun 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

TheCharlatan commented May 4, 2023 •

edited

Loading

Uh oh!

DrahtBot commented May 4, 2023 •

edited

Loading

Uh oh!

hebasto left a comment

Uh oh!

hebasto Jun 5, 2023

Uh oh!

maflcko commented Jun 8, 2023

Uh oh!

ryanofsky left a comment •

edited

Loading

Uh oh!

ryanofsky Jun 9, 2023 •

edited

Loading

Uh oh!

fanquake Jun 12, 2023

Uh oh!

TheCharlatan Jun 12, 2023

Uh oh!

TheCharlatan Jun 13, 2023

Uh oh!

ryanofsky left a comment •

edited

Loading

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

TheCharlatan commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 8, 2023

Uh oh!

ryanofsky left a comment • 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 Jun 9, 2023 • 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!

fanquake Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • 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!

Uh oh!

TheCharlatan commented May 4, 2023 •

edited

Loading

DrahtBot commented May 4, 2023 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky Jun 9, 2023 •

edited

Loading

ryanofsky left a comment •

edited

Loading