refactor: Move chain names to the util library #27294

pull TheCharlatan wants to merge 6 commits into bitcoin:master from TheCharlatan:kernelChainName changing 63 files +202 −144
  1. TheCharlatan commented at 10:10 pm on March 21, 2023: contributor

    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.

    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.

    The commits are split up to accommodate scripted diffs.

  2. DrahtBot commented at 10:10 pm on March 21, 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 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:

    • #27419 (refactor: Extract common/args from util/system by TheCharlatan)
    • #27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
    • #27039 (blockstorage: do not flush block to disk if it is already there by pinheadmz)
    • #26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)
    • #26403 ([POLICY] Ephemeral anchors by instagibbs)
    • #25908 (p2p: remove adjusted time by fanquake)
    • #25038 (policy: nVersion=3 and Package RBF by glozow)

    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 Refactoring on Mar 21, 2023
  4. in src/kernel/chainname.h:12 in 571c7bd983 outdated 
     7+
 8+#include <string>
 9+
10+namespace kernel {
11+namespace chainname {
12+static const std::string MAIN{"main"};
    ajtowns commented at 10:30 pm on March 21, 2023:
    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…
    TheCharlatan commented at 8:53 am on March 22, 2023:
    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 https://github.com/TheCharlatan/bitcoin/commit/1e4ba77db8121f4bc58653c786595325b308ac31 as a proof of concept. Looking at the diff, I’m not sure if it is worth it though.
    ajtowns commented at 11:36 am on March 22, 2023:
    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"};)
  5. in src/kernel/chainparams.cpp:73 in 571c7bd983 outdated 
    69@@ -70,7 +70,7 @@ static CBlock CreateGenesisBlock(uint32_t nTime, uint32_t nNonce, uint32_t nBits
70 class CMainParams : public CChainParams {
71 public:
72     CMainParams() {
73-        strNetworkID = CBaseChainParams::MAIN;
74+        strNetworkID = kernel::chainname::MAIN;
    ajtowns commented at 11:23 pm on March 21, 2023:
    Might be easier to review if these were scripted-diffs? Something like https://github.com/ajtowns/bitcoin/commits/202303-pr27294split perhaps
    TheCharlatan commented at 8:53 am on March 22, 2023:
    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 :)
    ajtowns commented at 11:42 am on March 22, 2023:
    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.
    maflcko commented at 12:10 pm on March 22, 2023:

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

  6. ryanofsky commented at 1:09 pm on March 22, 2023: contributor
    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.
  7. TheCharlatan force-pushed on Mar 22, 2023
  8. TheCharlatan commented at 10:28 pm on March 22, 2023: contributor

    Thank you for the reviews and the helpful and patient comments!

    Updated 571c7bd983035e4b684f289d97f00542ac9dc117 -> 5d166cae8cd16959bac7527779540daf81c328e4 (kernelChainName_0 -> kernelChainName_1, compare):

    • Addressed @ajtowns comment for separating declaration and instantiation of the chain name constants.

    Updated 5d166cae8cd16959bac7527779540daf81c328e4 -> 603f739947ce29d50affe5451cdae92c1744fab2 (kernelChainName_1 -> kernelChainName_2, compare):

    • Addressed @ajtowns comment for separating the commits to accomodate a scripted diff. Also added an additional scripted diff for removing all dangling chainparamsbase includes.
    • Addressed @ryanofsky comment for moving the new chainname files into the util subdirectory and library. Indeed everything that the kernel depends on in the common library should rather be moved into util as discussed in #24352 (comment)
  9. TheCharlatan renamed this:
    refactor: Move chain names to the kernel namespace
    refactor: Move chain names to the common library
    on Mar 24, 2023
  10. TheCharlatan renamed this:
    refactor: Move chain names to the common library
    refactor: Move chain names to the util library
    on Mar 24, 2023
  11. DrahtBot added the label Needs rebase on Apr 3, 2023
  12. refactor: Create chainname files 
    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.
    081863ae0c
  13. refactor: Use chainname includes for CChainParamsBase::{CHAIN} 
    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.
    615ba4c35a
  14. scripted-diff: Switch from CBaseChainParams::CHAIN to chainname::CHAIN 
    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>
    79c462b501
  15. refactor: Move chain definition to chainame.cpp 
    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.
    0c57589359
  16. Add missing definitions in prep for scripted diff 
    The missing include and ArgsManager were found after applying the
scripted diff in the following commit.
    0cd03c540e
  17. scripted-diff: Remove unused chainparamsbase includes 
    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-
    f3c57bf465
  18. TheCharlatan force-pushed on Apr 3, 2023
  19. TheCharlatan commented at 4:23 pm on April 3, 2023: contributor
    Rebased 603f739947ce29d50affe5451cdae92c1744fab2 -> f3c57bf4654599a4343cee3a1d2ffa7026a8ade7 (kernelChainName_2 -> kernelChainName_3, compare).
  20. DrahtBot removed the label Needs rebase on Apr 3, 2023
  21. ryanofsky approved
  22. ryanofsky commented at 6:45 pm on April 7, 2023: contributor

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

    0enum class ChainType { MAIN, TESTNET, SIGNET, REGTEST };
1
2std::string ChainTypeToString(ChainType);
3
4std::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.

  23. willienh approved
  24. TheCharlatan commented at 10:20 am on April 19, 2023: contributor

    Re #27294#pullrequestreview-1376476610

    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:

    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 https://github.com/bitcoin/bitcoin/commit/f3525e24e3a156eaa004ca28a59fe2d449dc1f9e. 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.

    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.

    Yes, am closing this pull request now and opened #27491 instead.

  25. TheCharlatan closed this on Apr 19, 2023
  26. fanquake referenced this in commit fc06881f13 on May 9, 2023
  27. bitcoin locked this on Apr 18, 2024


TheCharlatan DrahtBot ajtowns maflcko ryanofsky willienh

Labels
Refactoring

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-11-27 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me