refactor / kernel: Move non-gArgs chainparams functionality to kernel #26177
Conversation
96c9c85 to
fc1df1b
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
52e1c9c to
9714fe9
Compare
There was a problem hiding this comment.
Concept ACK
Did a first-pass review. Perhaps @ajtowns and @kallewoof can help w/re the activation/chainparams/signet bits!
There was a problem hiding this comment.
Concept ACK. Having simple options structs as a buffer between code and gArgs is great IMO.
Some comments below; code implementing them, and some other ideas at https://github.com/ajtowns/bitcoin/commits/202209-kernelchainparams
There was a problem hiding this comment.
Just reviewed the first commit so far, but already I think some things are a little overcomplicated and potentially buggy. I would suggest having a 1:1 correspondence between arguments and options, treating empty arguments as unset options, and keeping PR a pure refactoring which does not change behavior.
Started review (will update list below with progress).
- 8c0a94f Decouple SigNetChainParams from ArgsManager (1/7)
- 9929969 Decouple RegTestChainParams from ArgsManager (2/7)
- 4ca849d Remove UpdateVersionBitsParameters (3/7)
- 5d70983 Add factory functions for Main/Test chainparams (4/7)
- c00f4f7 split non/kernel chainparamsbase (5/7)
- e461bd2 Split non/kernel chainparams (6/7)
- 9714fe9 Wrap kernel chainparamsbase functions in kernel namespace (7/7)
9714fe9 to
03468cb
Compare
|
Thank you all for the reviews, I reworked my commits to include your suggestions. @ajtowns I molded most of your suggested code into the existing commits. I did not include the complete refactor of the @ryanofsky the options structs are now instantiated with default values and read from the |
03468cb to
b50c2ea
Compare
There was a problem hiding this comment.
Nice. I left some non-blocking nits. Please advise maintainers if this should be merged and if you plan to address the nits (if any).
review ACK 600ab02 💒
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: review ACK 600ab02bf58e073a93936438a7e884b3a7110f1 💒
kzm7rHrxddPB+LjaomzYoCJON/6jVUm4ilbYIF+uPjDG2kXx6jB3bd9r+RtMuVEKCtA3jrp/uE0wa/yO1RytDA==
| if (signet_challenge.size() != 1) { | ||
| throw std::runtime_error(strprintf("%s: -signetchallenge cannot be multiple values.", __func__)); | ||
| } | ||
| options.challenge.emplace(ParseHex(signet_challenge[0])); |
There was a problem hiding this comment.
unrelated in f3225c9: Could use TryParseHex?
There was a problem hiding this comment.
To test, -signetchallenge=nonhex silently fails/passes
Yes, will address the nits. It think it's worth another round. |
|
Ok, but please don't address those that are marked "unrelated", as that would not be refactoring changes, but behavior changes/bugfixes. |
SigNet chain params can now be initialized by configuring a SigNetOptions struct, or with ArgsManager. This offers an interface for creating SigNetChainParams without a gArgs object.
RegTest chain params can now be initialized by configuring a RegTestOptions struct, or with ArgsManager. This offers an interface for creating RegTestChainParams without a gArgs object.
Moves setting struct member fields from a function to its call site. This improves readability by surfacing the code.
This normalizes the behavior of initializing Main/Test/Sig/Reg chainparams with RegTest/SigNet chainparams. These factory functions can also easily be used from a context without an instantiated ArgsManager, e.g. from libbitcoin kernel code, unlike the existing CreateChainParams method.
Moves chainparams code not using the ArgsManager to the kernel. Subsequently use the kernel chainparams header now where possible in order to further decouple chainparams call sites from gArgs.
The chainstatemanager m_options.chainparams member variable gets its value from the global chainparams in init.cpp. This allows validation.cpp to only include the the kernel chainparams file.
600ab02 to
b3e78dc
Compare
|
Thank you for the review! Updated 600ab02 -> b3e78dc (tc/2022-09-libbitcoinkernel-chainparams-args_7 -> tc/2022-09-libbitcoinkernel-chainparams-args_8, compare) to address @MarcoFalke's nits. |
|
re-ACK b3e78dc 🛁 Show signatureSignature: |
00e9b97 refactor: Move fs.* to util/fs.* (TheCharlatan) 106b46d Add missing fs.h includes (TheCharlatan) b202b3d Add missing cstddef include in assumptions.h (TheCharlatan) 18fb363 refactor: Extract util/fs_helpers from util/system (Ben Woosley) Pull request description: This pull request is part of the `libbitcoinkernel` project bitcoin/bitcoin#24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by empact and is taken from its parent PR #25152. #### Context There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (bitcoin/bitcoin#25290, bitcoin/bitcoin#25487, bitcoin/bitcoin#25527, bitcoin/bitcoin#25862, bitcoin/bitcoin#26177, and bitcoin/bitcoin#27125). The `ArgsManager` is defined in `system.h`. A similar pull request extracting functionality from `system.h` has been merged in bitcoin/bitcoin#27238. #### Changes Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving filesystem related functions out of the `system.*` files. There is already a pair of `fs.h` / `fs.cpp` in the top-level `src/` directory. They were not combined with the files introduced here, to keep the patch cleaner and more importantly because they are often included without the utility functions. The new files are therefore named `fs_helpers` and the existing `fs` files are moved into the util directory. Further commits splitting more functionality out of `system.h` are still in #25152 and will be submitted in separate PRs once this PR has been processed. ACKs for top commit: hebasto: ACK 00e9b97 Tree-SHA512: 31422f148d14ba3c843b99b1550a6fd77c77f350905ca324f93d4f97b652246bc58fa9696c64d1201979cf88733e40be02d262739bb7d417cf22bf506fdb7666
d168458 scripted-diff: Remove unused chainparamsbase includes (TheCharlatan) e9ee8aa Add missing definitions in prep for scripted diff (TheCharlatan) ba8fc7d refactor: Replace string chain name constants with ChainTypes (TheCharlatan) 401453d refactor: Introduce ChainType getters for ArgsManager (TheCharlatan) bfc21c3 refactor: Create chaintype files (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project #24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is also a follow up to #26177. It replaces pull request #27294, which just moved the constants to a new file, but did not re-declare them as enums. The code move of the chain name constants out of the `chainparamsbase` to their own separate header allows the kernel `chainparams` to no longer include `chainparamsbase`. The `chainparamsbase` contain references to the `ArgsManager` and networking related options that should not belong to the kernel library. Besides this move, the constants are re-declared as enums with helper functions facilitating string conversions. ACKs for top commit: ryanofsky: Code review ACK d168458. Just suggested changes since last review. Tree-SHA512: ac2fbe5cbbab4f52eae1e30af1f16700b6589eb4764c328a151a712adfc37f326cc94a65c385534c57d4bc92cc1a13bf1777d92bc924a20dbb30440e7380b316
…base from kernel library db77f87 scripted-diff: move settings to common namespace (TheCharlatan) c27e4bd move-only: Move settings to the common library (TheCharlatan) c2dae5d kernel: Remove chainparams, chainparamsbase, args, settings from kernel library (TheCharlatan) 05870b1 refactor: Remove gArgs access from validation.cpp (TheCharlatan) 8789b11 refactor: Add path argument to FindSnapshotChainstateDir (TheCharlatan) ef95be3 refactor: Add stop_at_height option in ChainstateManager (TheCharlatan) Pull request description: 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 ACKs for top commit: MarcoFalke: lgtm ACK db77f87 🍄 hebasto: ACK db77f87, I have reviewed the code and it looks OK. ryanofsky: Code review ACK db77f87. Looks great! Tree-SHA512: cbfbd705d056f2f10f16810d4f869eb152362fff2c5ddae5e1ac6785deae095588e52ad48b29d921962b085e51de1e0ecab6e50f46149ffe3c16250608a2c93a
This pull request is part of the
libbitcoinkernelproject #24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.Context
The bitcoin kernel library currently relies on code containing user configurations through the
ArgsManager. This is not optimal, since as a stand-alone library it should not rely on bitcoind's argument parsing logic. Instead, its interfaces should accept control and options structs that control the kernel library's desired configuration.Similar work towards decoupling the
ArgsManagerfrom the kernel has been done in#25290, #25487, #25527 and #25862.
Changes
By moving the
CChainParamsclass definition into the kernel and giving it new factory functionsCChainParams::{RegTest,SigNet,Main,TestNet}it can be constructed without anArgsManagerreference, unlike the current factory functionCreateChainParams.The first few commits remove uses of
ArgsManagerwithinCChainParams. Then theCChainParamsdefinition is moved to a new file in thekernel/subdirectory.