refactor: Move chain names to the util library #27294
Conversation
|
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. |
src/kernel/chainname.h
Outdated
|
|
||
| namespace kernel { | ||
| namespace chainname { | ||
| static const std::string MAIN{"main"}; |
There was a problem hiding this comment.
This makes a copy of the name in every module that includes this header, which seems backwards, even if they end up getting merged by the linker...
There was a problem hiding this comment.
I can make chainname a struct instead and use the same pattern as it was before in chainparamsbase, but I'm not sure if this is so much better. Isn't the default string constructor called anyway for each time this header is included? How about taking this a step further and making them a const std::string_view instead? I pushed TheCharlatan@1e4ba77 as a proof of concept. Looking at the diff, I'm not sure if it is worth it though.
There was a problem hiding this comment.
Put the declaration in the header (eg extern const std::string MAIN;) and the definition in the corresponding cpp file (const std::string MAIN{"main"};)
src/kernel/chainparams.cpp
Outdated
| @@ -70,7 +70,7 @@ static CBlock CreateGenesisBlock(uint32_t nTime, uint32_t nNonce, uint32_t nBits | |||
| class CMainParams : public CChainParams { | |||
| public: | |||
| CMainParams() { | |||
| strNetworkID = CBaseChainParams::MAIN; | |||
| strNetworkID = kernel::chainname::MAIN; | |||
There was a problem hiding this comment.
Might be easier to review if these were scripted-diffs? Something like https://github.com/ajtowns/bitcoin/commits/202303-pr27294split perhaps
There was a problem hiding this comment.
Thank you for this concrete split suggestion. I did not add a scripted diff, to show the change of header and definition namespacing in the same commit. This makes it easier to see that for every changed file the new header is included as well. To me this feels like it will make for a cleaner commit history. However, I'm new to contributing and reviewing large code moves to the project. I'll ponder over your suggested split, I usually come around to these suggestions after a bit :)
There was a problem hiding this comment.
Adding/moving headers is usually pretty benign -- if you have an extra header it just makes compilation slightly slower and can be caught by iwyu, if you miss a header, it just won't compile. But changing code is where security bugs can sneak in, and if you're touching 100 lines all over the place, having an automated check that nothing's going wrong seems like a big win.
There was a problem hiding this comment.
... in the same commit. This makes it easier to see that for every changed file the new header is included as well.
For reviewers it is possible to squash two commits for review, if they believe it makes it easier for them to review.
|
I believe these names belong in the util library, not the kernel library. They are used by bitcoin-cli, and bitcoin-cli should not depend on the kernel because is an RPC client that should not contain validation code. |
571c7bd to
603f739
Compare
|
Thank you for the reviews and the helpful and patient comments! Updated 571c7bd -> 5d166ca (kernelChainName_0 -> kernelChainName_1, compare):
Updated 5d166ca -> 603f739 (kernelChainName_1 -> kernelChainName_2, compare):
|
This is the first of a number of commits with the goal of moving the chain name definitions out of chainparamsbase to their own file. The goal is to allow the kernel chainparams to no longer include chainparamsbase. The commit is part of an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager and other functionality that should not be part of the kernel library.
This is in preparation for the scripted diff in the following commit. The commit is part of an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager and other functionality that should not be part of the kernel library.
Rename CBaseChainParams:: to chainname:: , but don't change the definitions in util/chainname.* and chainparamsbase.cpp. -BEGIN VERIFY SCRIPT- sed -i 's/CBaseChainParams::/chainname::/g' $( git grep -l 'CBaseChainParams::' | grep -v "util/chainname\|chainparamsbase.cpp" ) -END VERIFY SCRIPT- Co-authored-by: Anthony Towns <aj@erisian.com.au> Co-authored-by: TheCharlatan <seb.kung@gmail.com>
The commit is part of an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager and other functionality that should not be part of the kernel library.
The missing include and ArgsManager were found after applying the scripted diff in the following commit.
This is a follow-up to previous commits moving the chain names out of chainparamsbase. The script removes the chainparamsbase header in all files where it is included, but not used. This is done by filtering against all defined symbols of the header as well as its respective .cpp file. The kernel chainparams now no longer relies on chainparamsbase. -BEGIN VERIFY SCRIPT- sed -i '/#include <chainparamsbase.h>/d' $( git grep -l 'chainparamsbase.h' | xargs grep -L 'CBaseChainParams\|CreateBaseChainParams\|SetupChainParamsBaseOptions\|BaseParams\|SelectBaseParams\|chainparamsbase.cpp' ) -END VERIFY SCRIPT-
603f739 to
f3c57bf
Compare
|
Rebased 603f739 -> f3c57bf (kernelChainName_2 -> kernelChainName_3, compare). |
There was a problem hiding this comment.
Code review ACK f3c57bf. This breaks dependency between server parameters (CChainParams in libbitcoin_kernel) and client parameters (CBaseChainParams in libbitcoin_common), so client parameters aren't dragged into the kernel.
However I think as long as every usage of the constants is changing anyway, it would be better to turn this into to enum and not introduce an odd namespace with string symbols. Suggestion would be to add a new src/util/chain_types.h file with:
enum class ChainType { MAIN, TESTNET, SIGNET, REGTEST };
std::string ChainTypeToString(ChainType);
std::optional<ChainType> ChainTypeFromString(std::string_view);substituting enum names for string constant names and ChainType for std::string, and calling string functions for the few places where strings are actually needed.
Using enum instead of string would be more idiomatic c++, and get rid of some global symbols and std::string initialization, and be compatible with switch. If you like this idea, would suggest implementing it in a different PR to keep review discussion coherent. If this doesn't seem like a good idea or doesn't seem worth extra work, feel free to ignore it.
I initially wanted to implement the constants as an enum, but then saw in the git history, that they were originally an enum and converted to string constants in #6235 commit f3525e2. The reasoning behind that commit struck me as weird, since an internal data representation does not have to be directly translatable, but I lacked the confidence to essentially revert that change.
Yes, am closing this pull request now and opened #27491 instead. |
…il library 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 bitcoin/bitcoin#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 bitcoin/bitcoin#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
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". It is also a follow up to #26177.The code move of the chain name constants out of the
chainparamsbaseto their own separate header allows the kernelchainparamsto no longer includechainparamsbase. Thechainparamsbasecontain references to theArgsManagerand networking related options that should not belong to the kernel library.The commits are split up to accommodate scripted diffs.