-
Notifications
You must be signed in to change notification settings - Fork 37.7k
kernel: Remove dependency on clientversion #32543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32543. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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. |
src/validation.cpp
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
src/validation.cpp
Outdated
@@ -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", |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Concept ACK |
d316a75
to
946d794
Compare
Concept ACK |
946d794
to
88d8bc9
Compare
Rebased 946d794 -> 88d8bc9 (kernelRmClientversion_0 -> kernelRmClientversion_1, compare)
|
Including a Bitcoin-Core specific version does not make sense if used by external applications.
Updated 88d8bc9 -> 04bc2bb (kernelRmClientversion_1 -> kernelRmClientversion_2, compare) Moving this to draft, since I think @maflcko's comment should be addressed properly. |
88d8bc9
to
04bc2bb
Compare
Including a Bitcoin-Core specific version does not make sense if used by external applications.