Skip to content

Conversation

TheCharlatan
Copy link
Contributor

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

@DrahtBot
Copy link
Contributor

DrahtBot commented May 17, 2025

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:

  • #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.

@Sjors
Copy link
Member

Sjors commented May 19, 2025

utACK d316a75

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.

Comment on lines 3901 to 3919
LogWarning("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i. Please report this issue here: %s\n",
pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex), CLIENT_BUGREPORT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LogWarning("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i. Please report this issue here: %s\n",
pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex), CLIENT_BUGREPORT);
LogWarning(STR_INTERNAL_BUG(strprintf("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i",
pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex))));

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to remove the include?

[12:05:51.093] /ci_container_base/src/util/check.cpp should remove these lines:
[12:05:51.093] - #include <clientversion.h>  // lines 9-9
[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:

diff --git a/src/clientversion.cpp b/src/clientversion.cpp
index 10e9472b84..acf01ff1bb 100644
--- a/src/clientversion.cpp
+++ b/src/clientversion.cpp
@@ -55,8 +55,8 @@ static std::string FormatVersion(int nVersion)
 
 std::string FormatFullVersion()
 {
-    static const std::string CLIENT_BUILD(BUILD_DESC BUILD_SUFFIX);
-    return CLIENT_BUILD;
+    constexpr std::string_view CLIENT_BUILD{BUILD_DESC BUILD_SUFFIX};
+    return std::string{CLIENT_BUILD};
 }
 
 /**

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

Copy link
Contributor Author

@TheCharlatan TheCharlatan May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

fjahr commented May 19, 2025

Concept ACK

@TheCharlatan TheCharlatan force-pushed the kernelRmClientversion branch from d316a75 to 946d794 Compare May 19, 2025 13:40
@fanquake
Copy link
Member

Concept ACK

@TheCharlatan
Copy link
Contributor Author

Including a Bitcoin-Core specific version does not make sense if used by external applications.
@TheCharlatan
Copy link
Contributor Author

Updated 88d8bc9 -> 04bc2bb (kernelRmClientversion_1 -> kernelRmClientversion_2, compare)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for Review PRs
Development

Successfully merging this pull request may close these issues.

6 participants