kernel: Remove dependency on clientversion

TheCharlatan 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:

#31807 (kernel: Avoid duplicating symbols in the kernel and downstream users by theuni)

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:3901 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:3872 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)

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

TheCharlatan 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

kernel: Remove dependency on clientversion

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

946d794919

TheCharlatan force-pushed on May 19, 2025

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

Concept ACK

kernel: Remove dependency on clientversion #32543

Code Coverage & Benchmarks

Reviews

Conflicts