kernel: Remove dependency on clientversion

sedited commented at 3:46 pm on May 17, 2025: contributor

Including a Bitcoin-Core specific version does not make sense if used by external applications.

DrahtBot commented at 3:46 pm on May 17, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32543.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	fjahr, fanquake
Stale ACK	Sjors

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:

#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#28690 (build: Introduce internal kernel library by TheCharlatan)

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.

DrahtBot added the label Validation on May 17, 2025

Sjors commented at 9:16 am on May 19, 2025: member

utACK d316a75f1fb807f5384ec9d4ed0b8aad6df7ed5f

This causes the version to be missing from some of the “Please report this issue here” messages. But if someone is running a node, rather than some kernel based application, they can easily find its version when reporting the bug.

in src/validation.cpp:3919 in d316a75f1f outdated

3897@@ -3898,8 +3898,8 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
3898             // assumeutxo snapshot block if assumeutxo snapshot metadata has an
3899             // incorrect hardcoded AssumeutxoData::m_chain_tx_count value.
3900             if (!Assume(pindex->m_chain_tx_count == 0 || pindex->m_chain_tx_count == prev_tx_sum(*pindex))) {
3901-                LogWarning("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i (%s %s). Please report this issue here: %s\n",
3902-                   pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex), CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT);
3903+                LogWarning("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i. Please report this issue here: %s\n",
3904+                   pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex), CLIENT_BUGREPORT);

maflcko commented at 9:48 am on May 19, 2025:

0                LogWarning(STR_INTERNAL_BUG(strprintf("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i",
1                   pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex))));

nit: Could use the STR_INTERNAL_BUG helper, while touching this?

in src/validation.cpp:3890 in d316a75f1f outdated

3869@@ -3870,8 +3870,8 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
3870     auto prev_tx_sum = [](CBlockIndex& block) { return block.nTx + (block.pprev ? block.pprev->m_chain_tx_count : 0); };
3871     if (!Assume(pindexNew->m_chain_tx_count == 0 || pindexNew->m_chain_tx_count == prev_tx_sum(*pindexNew) ||
3872                 pindexNew == GetSnapshotBaseBlock())) {
3873-        LogWarning("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i (%s %s). Please report this issue here: %s\n",
3874-            pindexNew->nHeight, pindexNew->m_chain_tx_count, prev_tx_sum(*pindexNew), CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT);
3875+        LogWarning("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i. Please report this issue here: %s\n",

maflcko commented at 9:48 am on May 19, 2025:

(same)

in src/util/check.cpp:16 in d316a75f1f outdated

16@@ -17,9 +17,8 @@
17 std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func)

maflcko commented at 11:49 am on May 19, 2025:

forgot to remove the include?

0[12:05:51.093] /ci_container_base/src/util/check.cpp should remove these lines:
1[12:05:51.093] - #include <clientversion.h>  // lines 9-9
2[12:05:51.093]

However, the CLIENT_NAME symbol is still used in the kernel, so I am not sure removing the link dependency is meaningful. I’d say it should either be removed fully, or it can be kept. If you just want to turn the link-time dependency into a compile-time dependency, this is also possible, because everything can be done at compile-time. Proof:

 0diff --git a/src/clientversion.cpp b/src/clientversion.cpp
 1index 10e9472b84..acf01ff1bb 100644
 2--- a/src/clientversion.cpp
 3+++ b/src/clientversion.cpp
 4@@ -55,8 +55,8 @@ static std::string FormatVersion(int nVersion)
 5 
 6 std::string FormatFullVersion()
 7 {
 8-    static const std::string CLIENT_BUILD(BUILD_DESC BUILD_SUFFIX);
 9-    return CLIENT_BUILD;
10+    constexpr std::string_view CLIENT_BUILD{BUILD_DESC BUILD_SUFFIX};
11+    return std::string{CLIENT_BUILD};
12 }
13 
14 /**

(Happy to submit a proper pull making it constexpr, if you want)

sedited commented at 1:57 pm on May 19, 2025:

forgot to remove the include?

yes -_-

However, the CLIENT_NAME symbol is still used in the kernel,

Mmh, that is done though the config header, and I am not sure how we should handle that yet. Seems like a separate problem to me? Then again the clientversion header is little more than some formatting helpers for some of these variables. Could also remove CLIENT_NAME in the few cases where it is relevant, but CLIENT_BUGREPORT is probably useful to keep.

maflcko commented at 2:08 pm on May 19, 2025:

I’d say it would be best to keep all of them. Maybe I am missing the context, but I fail to see how the client version is irrelevant. “they can easily find its version when reporting the bug” doesn’t sound great, because some people forget it, and it will just be another round-trip.

Also, I’d say it is important to know if the user ran some local (or remote) modifications. Lately it seems common to fork random commits of bitcoin core and apply random (policy or non-policy) patches. We’d want to quickly reject those, after assessing the bug has nothing to do with any commit in a release or master branch of Bitcoin Core.

sedited commented at 2:17 pm on May 19, 2025:

Also, I’d say it is important to know if the user ran some local (or remote) modifications. Lately it seems common to fork random commits of bitcoin core and apply random (policy or non-policy) patches. We’d want to quickly reject those, after assessing the bug has nothing to do with any commit in a release or master branch of Bitcoin Core.

I did not think about this yet. I guess that could save a bug report round-trip, or at least help with reducing confusion a bit. Then again, some people are just forking the project on master and tag some past version on it. That could be equally confusing, no?

maflcko commented at 3:38 pm on May 19, 2025:

Then again, some people are just forking the project on master and tag some past version on it. That could be equally confusing, no?

Hopefully that is rare. Though, just including the full commit hash in bug reports shouldn’t hurt.

fjahr commented at 12:53 pm on May 19, 2025: contributor

Concept ACK

sedited force-pushed on May 19, 2025

fanquake commented at 1:43 pm on May 19, 2025: member

Concept ACK

?

added_to_project_v2 sedited

?

project_v2_item_status_changed sedited

DrahtBot added the label Needs rebase on Aug 6, 2025

sedited force-pushed on Aug 7, 2025

sedited commented at 8:12 pm on August 7, 2025: contributor

Rebased 946d794919b640a3e5f78e853b3291f6711aa98b -> 88d8bc9a1cc0aa8177424d411fe876aaca47e466 (kernelRmClientversion_0 -> kernelRmClientversion_1, compare)

Fixed conflict with #33077

sedited commented at 8:20 pm on August 7, 2025: contributor

Updated 88d8bc9a1cc0aa8177424d411fe876aaca47e466 -> 04bc2bb530f9e8cc4dd8321be5811a9760624e53 (kernelRmClientversion_1 -> kernelRmClientversion_2, compare)

Addressed @maflcko’s comment, consistently applying STR_INTERNAL_BUG.

Moving this to draft, since I think @maflcko’s comment should be addressed properly.

sedited force-pushed on Aug 7, 2025

sedited marked this as a draft on Aug 7, 2025

DrahtBot removed the label Needs rebase on Aug 7, 2025

hodlinator commented at 9:51 am on November 6, 2025: contributor

How about having an externally defined software/package name+version string as an input to kernel that can still be defined by Bitcoin Core to get the same behavior as on master? It could default to empty or be a requirement for users of kernel.

Then again, I agree with #32543 (review) that having at least the commit hash would be okay.

sedited commented at 10:05 am on November 6, 2025: contributor

How about having an externally defined software/package name+version string as an input to kernel that can still be defined by Bitcoin Core to get the same behavior as on master

I recently discussed retaining the version information as a “product name” and then having separate versioning for the bitcoin kernel header. Not sure yet about that approach though. Including just the commit hash sounds like a reasonable trade off.

?

project_v2_item_status_changed sedited

DrahtBot added the label Needs rebase on Nov 10, 2025

kernel: Remove dependency on clientversion

Including a Bitcoin-Core specific version does not make sense if used by external applications.

29385e1e84

sedited force-pushed on Nov 12, 2025

DrahtBot removed the label Needs rebase on Nov 12, 2025

sedited commented at 2:31 pm on November 13, 2025: contributor

Closing this. Will pick it up again as part of a larger directional push to handle versioning in the kernel.

sedited closed this on Nov 13, 2025

?

project_v2_item_status_changed github-project-automation[bot]

Sakzz1994 commented at 1:08 am on November 24, 2025: none

👎

kernel: Remove dependency on clientversion #32543

Code Coverage & Benchmarks

Reviews

Conflicts