refactor: Move chain constants to the util library #27491

TheCharlatan · 2023-04-19T10:19:09Z

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.

DrahtBot · 2023-04-19T10:19:12Z

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:

#27596 (assumeutxo (2) by jamesob)
#27534 (rpc: add 'getnetmsgstats', new rpc to view network message statistics by satsie)
#27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
#27039 (blockstorage: do not flush block to disk if it is already there by pinheadmz)
#26564 (test: allow BITCOIN_TEST_PATH to specify working dir by LarryRuane)
#26403 ([POLICY] Ephemeral anchors by instagibbs)
#25977 (refactor: Replace std::optional<bilingual_str> with util::Result by ryanofsky)
#25908 (p2p: remove adjusted time by fanquake)
#25038 (policy: nVersion=3 and Package RBF by glozow)
#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

fanquake · 2023-04-19T10:22:27Z

7ea09dd60  scripted-diff: Remove unused chainparamsbase includes
d99bd93e0 Add missing definitions in prep for scripted diff
5589a4b0d refactor: Replace string chain name constants with ChainTypes
bd6493bf3 refactor: Create chaintype files

Error: script block marker but no scripted-diff in title of commit 7ea09dd60cd43e943f9910c950b05dda89ef3725
Failed

TheCharlatan · 2023-04-19T10:31:59Z

Re #27491 (comment)

7ea09dd scripted-diff: Remove unused chainparamsbase includes

Seems like the trailing whitespace in the commit mesasge header was tripping it up?

src/chainparams.cpp

ryanofsky

Code review ACK bbe0591. Left a few suggestions, but this looks good in its current form

src/util/chaintype.cpp

src/qt/guiutil.cpp

src/util/system.cpp

ajtowns

Would be better if the second commit (renaming CBaseChainParam::MAIN to ChainType::MAIN etc) could be a scripted-diff. Perhaps structure it as:

introduce ChainType
add CreateBaseChainParams(ChainType) and replace CreateBaseChainParams(string) with { auto ct = ChainTypeFromString(chain); if (ct) { return CreateBaseChainParams(ct); } else { throw...; } (and so on)
scripted-diff to switch to ChainType::MAIN and args.GetChainType()
remove the now unused functions that accepted a string

ajtowns · 2023-04-20T04:47:34Z

src/util/chaintype.h

+enum class ChainType { MAIN,
+                       TESTNET,
+                       SIGNET,
+                       REGTEST };


Put MAIN on a newline and just use 4-space indentation? Newlines for { and } as well, per style guide? Trailing , after REGTEST in case we add any new ones later?

The style guide is vague on enums (though I guess you could follow the recommendation for classes)

- Braces on new lines for classes, functions, methods. - Braces on the same line for everything else.

and our code (even the most recent additions) has a mix between putting braces on the same line and a new line for enums.

ajtowns · 2023-04-20T04:58:55Z

src/util/chaintype.cpp

+#include <stdexcept>
+#include <string>
+
+std::string ChainTypeToString(ChainType chain)


Calling it ChainName(ChainType chain) might be a bit clearer?

I'm not sure on this one. Granted, std::string ChainName(ChainType chain) would make it clear for the caller that this is most likely a label associated with the ChainType. However, for consistency I would also have to rename the helper functions to GetChainName(), and that seems less clear, because it does not have a typed argument allowing the caller to infer the relationship with ChainType.

TheCharlatan · 2023-04-20T17:01:25Z

scripted-diff to switch to ChainType::MAIN and args.GetChainType()

I don't think this is feasible, because we have to decide on the type (either calling a helper function for a string or the enum) based on the context, no?

TheCharlatan · 2023-04-20T20:58:28Z

Thank you for the reviews!

Updated bbe0591 -> 32a7086 (kernelChainType_0 -> kernelChainType_1, compare)

Addressed @ryanofsky's comment, removing ChainType pass by reference.
Addressed @ryanofsky's comment, removing the switch statement default cases.
Addressed @ryanofsky's comment, updating the comment as suggested.
Addressed @ryanofsky's comment, cleaning up remaining mentions of the now renamed functions and variable names
Addressed @ryanofsky's comment, adding a commit with the suggested refactor for avoiding needless type conversions.
Addressed @AJTown's comment, improving enum declaration formatting.

TheCharlatan · 2023-04-21T14:06:13Z

Rebased 32a7086 -> 7d36108 (kernelChainType_1 -> kernelChainType_2, compare)

TheCharlatan · 2023-05-09T14:06:19Z

Thank you for your patience @ryanofsky

Updated 1ae4d9a -> d168458 (kernelChainType_6 -> kernelChainType_7, compare)

Addressed @ryanofsky's comment, changed commits as suggested. No net changes to the code, but should result in fewer touched lines in the history.

ryanofsky

Code review ACK d168458. Just suggested changes since last review.

If you can, it would also be helpful to mark previous review comments as resolved (github won't let me do this)

Thank you for your patience

Likewise! Thank you for all the improvements

fanquake · 2023-05-09T14:29:42Z

I'm going to merge this shortly. #27125 (which conflicts) will be rebased (some other conflicting changes may also be merged in the interim), along with its current set of review feedback addressed.

src/chainparams.cpp

src/chainparamsbase.cpp

maflcko · 2023-05-09T16:50:59Z

See also #14309 , where I added the ChainType enum class.

This is a follow up of bitcoin#27491, more concretely bitcoin#27491 (comment), for not using default cases (as per the style guide), and bitcoin#27491 (comment) and bitcoin#27491 (comment) for avoiding dead code.

This is a follow up of bitcoin#27491, more concretely bitcoin#27491 (comment), for not using default cases (as per the style guide), and bitcoin#27491 (comment) and bitcoin#27491 (comment) for avoiding dead code. Also change chain name to chain type in docstrings

maflcko · 2023-05-10T09:03:42Z

src/common/args.h

+     * @return ChainType::MAIN string by default; raises runtime error if an
+     * invalid combination is given.
+     */
+    std::string GetChainTypeString() const;


I wonder if we can make those two getters return Results, instead of throwing exceptions.

I tried that in the past because there was an unguarded call in the gui, which would then cause a segfault, but that seems now fixed. Still, low-prio and long-term this may be a thing to keep in mind.

e230887 refactor: Use ChainType enum exhaustively (TheCharlatan) Pull request description: This is a follow up of bitcoin/bitcoin#27491, more concretely bitcoin/bitcoin#27491 (comment), for not using default cases (as per the style guide), and bitcoin/bitcoin#27491 (comment) and bitcoin/bitcoin#27491 (comment) for avoiding dead code. ACKs for top commit: fanquake: ACK e230887 - deals with almost all follow up comments out of #27491. Tree-SHA512: 1794190b03b91d3ca349a4da08e9610dbb3432983eee7cb21ecc758d1d7d710560c97661de14cdf493c28c00ebe8977511b4696055c0940e7f815b622dbacd16

e230887 refactor: Use ChainType enum exhaustively (TheCharlatan) Pull request description: This is a follow up of bitcoin#27491, more concretely bitcoin#27491 (comment), for not using default cases (as per the style guide), and bitcoin#27491 (comment) and bitcoin#27491 (comment) for avoiding dead code. ACKs for top commit: fanquake: ACK e230887 - deals with almost all follow up comments out of bitcoin#27491. Tree-SHA512: 1794190b03b91d3ca349a4da08e9610dbb3432983eee7cb21ecc758d1d7d710560c97661de14cdf493c28c00ebe8977511b4696055c0940e7f815b622dbacd16

This is a follow up of bitcoin#27491, more concretely bitcoin#27491 (comment), for not using default cases (as per the style guide), and bitcoin#27491 (comment) and bitcoin#27491 (comment) for avoiding dead code. Also change chain name to chain type in docstrings

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

This is a follow up of bitcoin/bitcoin#27491, more concretely bitcoin/bitcoin#27491 (comment), for not using default cases (as per the style guide), and bitcoin/bitcoin#27491 (comment) and bitcoin/bitcoin#27491 (comment) for avoiding dead code. Also change chain name to chain type in docstrings

TheCharlatan changed the title ~~Kernel chain type~~ refactor: Move chain constants to the util library Apr 19, 2023

DrahtBot added the Refactoring label Apr 19, 2023

TheCharlatan mentioned this pull request Apr 19, 2023

refactor: Move chain names to the util library #27294

Closed

TheCharlatan force-pushed the kernelChainType branch from 7ea09dd to bbe0591 Compare April 19, 2023 10:25

ryanofsky reviewed Apr 19, 2023

View reviewed changes

src/chainparams.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Apr 19, 2023

View reviewed changes

src/util/chaintype.cpp Outdated Show resolved Hide resolved

src/qt/guiutil.cpp Outdated Show resolved Hide resolved

src/util/system.cpp Outdated Show resolved Hide resolved

src/util/system.cpp Outdated Show resolved Hide resolved

ajtowns reviewed Apr 20, 2023

View reviewed changes

TheCharlatan force-pushed the kernelChainType branch from bbe0591 to 32a7086 Compare April 20, 2023 20:57

DrahtBot mentioned this pull request Apr 21, 2023

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

DrahtBot added the Needs rebase label Apr 21, 2023

TheCharlatan force-pushed the kernelChainType branch from 32a7086 to 7d36108 Compare April 21, 2023 14:06

DrahtBot removed the Needs rebase label Apr 21, 2023

DrahtBot mentioned this pull request Apr 27, 2023

rpc: add 'getnetmsgstats', new rpc to view network message statistics #27534

Closed

TheCharlatan force-pushed the kernelChainType branch from 1ae4d9a to d168458 Compare May 9, 2023 14:06

fanquake requested review from maflcko and ryanofsky May 9, 2023 14:14

ryanofsky approved these changes May 9, 2023

View reviewed changes

fanquake merged commit fc06881 into bitcoin:master May 9, 2023

maflcko reviewed May 9, 2023

View reviewed changes

src/chainparams.cpp Show resolved Hide resolved

src/chainparamsbase.cpp Show resolved Hide resolved

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 9, 2023

Merge bitcoin#27491: refactor: Move chain constants to the util library

8148f0c

TheCharlatan mentioned this pull request May 10, 2023

refactor: Use ChainType enum exhaustively #27611

Merged

maflcko reviewed May 10, 2023

View reviewed changes

TheCharlatan mentioned this pull request May 10, 2023

Bitcoin Kernel Library Project Tracking #27587

Open

2 tasks

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

stickies-v mentioned this pull request Oct 5, 2023

[24.x] Further backports #28535

Merged

bitcoin locked and limited conversation to collaborators May 9, 2024

refactor: Move chain constants to the util library #27491

refactor: Move chain constants to the util library #27491

Uh oh!

Conversation

TheCharlatan commented Apr 19, 2023

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake commented Apr 19, 2023

Uh oh!

TheCharlatan commented Apr 19, 2023

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

ajtowns Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan commented Apr 20, 2023

Uh oh!

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

Uh oh!

Uh oh!

TheCharlatan commented Apr 21, 2023

Uh oh!

TheCharlatan commented May 9, 2023

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented May 9, 2023

Uh oh!

Uh oh!

Uh oh!

maflcko commented May 9, 2023

Uh oh!

maflcko May 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Apr 19, 2023 •

edited

Loading

TheCharlatan commented Apr 20, 2023 •

edited

Loading