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
Encapsulate warnings in generalized node::Warnings and remove globals #30058
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 CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
cd8b420
to
4f06da2
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
Concept ACK on removing the warnings globals. |
3b2b72f
to
445e6de
Compare
Force-pushed to address compilation failure on non-macOS systems:
|
src/node/warnings.h
Outdated
class Warnings | ||
{ | ||
mutable Mutex m_mutex; | ||
std::map<std::string, bilingual_str> m_warnings GUARDED_BY(m_mutex); |
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.
As an alternative: How about making this a set of enums whose variants encode the various errors? Then we can add a function to convert the enums to strings (the enums and the conversion function could live in the kernel namespace) and use that to populate the vector for GetMessages()
.
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 have considered that approach, but see a couple of difficulties with it:
- We have kernel warnings ("unknown-new-rules-activated", "large-work-invalid-chain") and node warnings ("clock-out-of-sync", "pre-release-test-build", "fatal-internal-error"). I would rather not define the node warnings in the kernel namespace. We could have have
kernel::Warning
andnode::Warning
enums (and potentially more in the future), and have astd::variant<kernel::Warning, node::Warning> GetAllWarnings()
to address that. - not all warnings are compile-time constants, see e.g. the "unknown-new-rules-activated" warning:
const bilingual_str warning = strprintf(_("Unknown new rules activated (versionbit %i)"), bit);
, so I'm no sure a conversion function is desirable?
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 gave the kernel::Warning
and node::Warning
enums approach a go because it would be nice to have the possible warnings enumerated, and it ended up being less overhead than I expected. I'm leaning towards updating this PR with that approach. What do you think?
stickies-v@e4ea7ab - PoC but compiles and tests pass, needs rebase, cleanup and doc updates etc
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.
Thanks for testing this out! I forgot that the we'd need to reconstruct the versionbit before, sorry about that. I think I like this better. The reason I am a bit hesitant about keying by strings is that it makes them harder to discover for outside users and forces them to use a variable-size data type as a key for mapping them.
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.
EDIT: Looking at the PR more, I now see the previous discussion is about id strings, not message strings. In that case I agree with TheCharlatan it is a littler better to use enum ids than string ids.
I think in the longer run the string/enum distinction should not matter too much here.
In general, requiring kernel applications to get warning information from warning
, or warningSet
/warningUnset
callbacks is not a nice API. It would make more sense for the kernel to treat errors and warnings similarly, and return this information directly in function return values or output arguments, instead of indirect callbacks. We have PR #29642 and #29700 which change kernel code to bubble up errors, and another PR could be made to bubble up warnings.
With these changes, you should be able to look at any kernel API function, and know exactly what error and warning conditions it can trigger, and have that information returned to the caller. We shouldn't remain in the current situation where when you call a kernel function, there is no way to know what fatal errors, or flush errors, or warnings it might trigger, and the only way to get that information by handling indirect callbacks.
I also think when designing these API's, it would be good to distinguish between errors and warning conditions that are directly actionable by callers, and those that aren't. Conditions that are directly actionable are conditions where callers might add special cases to do things like scheduling a retry, or dropping a cache to free up resources, or starting an interaction with the user that goes beyond displaying a message string. If the only way an API caller can realistically handle a condition is by showing or logging a message string, I think returning a string is much better than returning an enum, because a string, unlike an enum, can provide context needed to understand and solve the problem. But if the condition is more directly actionable and could be handled with special case code, returning structured information instead of (or along with) a string could be better.
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've incorporated the id-as-enum approach in the latest force-push, thank you very much for both of your extensive feedback.
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.
Code review ACK 445e6de. It was a little unclear in some places what behavior was supposed to be changing, and I think that could be documented better. But overall the changes look good and seem very positive.
@@ -235,6 +235,7 @@ BITCOIN_CORE_H = \ | |||
node/txreconciliation.h \ | |||
node/utxo_snapshot.h \ | |||
node/validation_cache_args.h \ | |||
node/warnings.h \ |
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.
In commit "move-only: move warnings from common to node" (6681395)
s/RPc/RPC/ in commit message
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.
Thanks, fixed.
src/node/kernel_notifications.cpp
Outdated
static bool fWarned = false; | ||
node::SetMiscWarning(warning); | ||
if (!fWarned) { | ||
if (node::g_warnings.Set(id, warning)) { |
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.
In commit "introduce and use the generalized node::Warnings
interface" (6152c20)
Would be good to have release notes for this change mentioning new -alertnotify behavior, since it now can be trigged multiple times and in new cases
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.
Sounds good, I've added release notes to cover that. The way I read the startup option docstring ("Execute command when an alert is raised (%s in cmd is replaced by message)"), I think the new behaviour aligns better with how I'd expect this option to behave.
LOCK(m_mutex); | ||
std::vector<bilingual_str> messages; | ||
messages.reserve(m_warnings.size()); | ||
for (const auto& [id, msg] : m_warnings) { |
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.
In commit "introduce and use the generalized node::Warnings
interface" (6152c20)
I guess this change also affects the order of warnings. It seems like previous code was written to consistently put the pre-release warning first, followed by the miscellaneous warnings, the large-work chain warning, and the time offset warning last.
Changing this is probably fine, but it would be good to note the change in the commit message. Could also note in setWarning documentation that ID affects order warnings are shown in.
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 guess this change also affects the order of warnings.
It does indeed. I hadn't really considered this, and I think I agree that the current behaviour is fine - but I'll think about it more and open to suggestions.
Changing this is probably fine, but it would be good to note the change in the commit message.
I've updated the commit message section on behaviour change to:
Introduces behaviour change:
- the `-alertnotify` command is executed for all
`KernelNotifications::warningSet` calls, which now also covers the
`LARGE_WORK_INVALID_CHAIN` warning.
- previously, warnings were returned based on a predetermined order,
e.g. with the "pre-release test build" warning always first. This
is no longer the case, and Warnings::GetMessages() will return
messages sorted by the id of the warning.
Could also note in setWarning documentation that ID affects order warnings are shown in.
I don't think this belongs in setWarning
, kernel users could implement this how they want, so I've updated the Warnings::GetMessages()
docstring to:
/** Return potential problems detected by the node, sorted by the
* warning_type id */
src/node/warnings.cpp
Outdated
} | ||
|
||
void SetfLargeWorkInvalidChainFound(bool flag) | ||
bool Warnings::Set(const std::string& id, const bilingual_str& message) |
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.
In commit "introduce and use the generalized node::Warnings
interface" (6152c20)
Not important, but since these arguments are being inserted in a map, it would probably be a little better to pass them by value instead of reference, and use std::move so they can be moved instead of copied.
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.
Ah makes sense, I've updated that, thanks.
@@ -31,12 +32,15 @@ bool Warnings::Set(const std::string& id, const bilingual_str& message) | |||
{ | |||
LOCK(m_mutex); | |||
const auto& [_, inserted]{m_warnings.insert({id, message})}; | |||
if (inserted) uiInterface.NotifyAlertChanged(); |
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.
In commit "node: update uiInterface whenever warnings updated" (396b261)
Can commit message be updated to say what this change in behavior here is? It looks like now this UI notification will be triggered if a fatal error happens, so not sure if that is good or bad. It might be possible to avoid this by moving the uiInterface call to node/kernel_notifications.cpp instead. That would be good for consistency since uiInterface is already accessed there, and it would be nice for this new class not to rely on a global variable.
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.
Can commit message be updated to say what this change in behavior here is?
I've updated the commit message:
node: update uiInterface whenever warnings updated
This commit introduces slight behaviour change. Previously, the
GUI status bar would be updated for most warnings, namely
UNKNOWN_NEW_RULES_ACTIVATED, CLOCK_OUT_OF_SYNC and
PRE_RELEASE_TEST_BUILD, but not for LARGE_WORK_INVALID_CHAIN
(and not for FATAL_INTERNAL_ERROR, but that is not really
meaningful).
Fix this by always updating the status bar when the warnings are
changed.
It looks like now this UI notification will be triggered if a fatal error happens, so not sure if that is good or bad
I'm not super familiar with the GUI, but I think it's mostly irrelevant for the internal fatal error case, or any case where we shutdown the node? We're not creating any messageboxes etc, the uiInterface.NotifyAlertChanged()
just updates the status bar, so it's not blocking.
It might be possible to avoid this by moving the uiInterface call to node/kernel_notifications.cpp instead. That would be good for consistency since uiInterface is already accessed there, and it would be nice for this new class not to rely on a global variable.
I agree that not having a global variable in Warnings
would be better, and that's how I originally implemented it at first too. But I think the current approach is the most consistent? It seems undesirable that we would ever want to create a warning, and then not show it in the GUI until another - unrelated - warning is created that does trigger the GUI update. So, if we agree that we always want to update the GUI when the warnings are modified, then I think just doing it inside the Set()
and Unset()
methods is the most sensible approach?
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.
^ cc @hebasto
Since rpc/util.cpp is in common, also move GetNodeWarnings() to node::GetWarningsForRPC()
445e6de
to
674da14
Compare
674da14
to
5dfbc35
Compare
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.
Thank you for the review, @TheCharlatan and @ryanofsky .
In this force push, I've:
- rebased to address merge conflict from rpc: avoid copying into UniValue #30115
- incorporated @TheCharlatan's suggestion to use
enum class
instead ofstd::string
for the warning identifiers - addressed @ryanofsky's comments:
- improved commit messages to describe the introduced behaviour change and improved a couple of docstrings
- added release notes to describe the new
-alertnotify
behaviour - updated the
Warnings::Set()
signature to pass by value and use move semantics
- removed an unnecessary include in
kernel/warning.h
@@ -235,6 +235,7 @@ BITCOIN_CORE_H = \ | |||
node/txreconciliation.h \ | |||
node/utxo_snapshot.h \ | |||
node/validation_cache_args.h \ | |||
node/warnings.h \ |
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.
Thanks, fixed.
src/node/kernel_notifications.cpp
Outdated
static bool fWarned = false; | ||
node::SetMiscWarning(warning); | ||
if (!fWarned) { | ||
if (node::g_warnings.Set(id, warning)) { |
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.
Sounds good, I've added release notes to cover that. The way I read the startup option docstring ("Execute command when an alert is raised (%s in cmd is replaced by message)"), I think the new behaviour aligns better with how I'd expect this option to behave.
src/node/warnings.cpp
Outdated
} | ||
|
||
void SetfLargeWorkInvalidChainFound(bool flag) | ||
bool Warnings::Set(const std::string& id, const bilingual_str& message) |
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.
Ah makes sense, I've updated that, thanks.
@@ -31,12 +32,15 @@ bool Warnings::Set(const std::string& id, const bilingual_str& message) | |||
{ | |||
LOCK(m_mutex); | |||
const auto& [_, inserted]{m_warnings.insert({id, message})}; | |||
if (inserted) uiInterface.NotifyAlertChanged(); |
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.
Can commit message be updated to say what this change in behavior here is?
I've updated the commit message:
node: update uiInterface whenever warnings updated
This commit introduces slight behaviour change. Previously, the
GUI status bar would be updated for most warnings, namely
UNKNOWN_NEW_RULES_ACTIVATED, CLOCK_OUT_OF_SYNC and
PRE_RELEASE_TEST_BUILD, but not for LARGE_WORK_INVALID_CHAIN
(and not for FATAL_INTERNAL_ERROR, but that is not really
meaningful).
Fix this by always updating the status bar when the warnings are
changed.
It looks like now this UI notification will be triggered if a fatal error happens, so not sure if that is good or bad
I'm not super familiar with the GUI, but I think it's mostly irrelevant for the internal fatal error case, or any case where we shutdown the node? We're not creating any messageboxes etc, the uiInterface.NotifyAlertChanged()
just updates the status bar, so it's not blocking.
It might be possible to avoid this by moving the uiInterface call to node/kernel_notifications.cpp instead. That would be good for consistency since uiInterface is already accessed there, and it would be nice for this new class not to rely on a global variable.
I agree that not having a global variable in Warnings
would be better, and that's how I originally implemented it at first too. But I think the current approach is the most consistent? It seems undesirable that we would ever want to create a warning, and then not show it in the GUI until another - unrelated - warning is created that does trigger the GUI update. So, if we agree that we always want to update the GUI when the warnings are modified, then I think just doing it inside the Set()
and Unset()
methods is the most sensible approach?
LOCK(m_mutex); | ||
std::vector<bilingual_str> messages; | ||
messages.reserve(m_warnings.size()); | ||
for (const auto& [id, msg] : m_warnings) { |
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 guess this change also affects the order of warnings.
It does indeed. I hadn't really considered this, and I think I agree that the current behaviour is fine - but I'll think about it more and open to suggestions.
Changing this is probably fine, but it would be good to note the change in the commit message.
I've updated the commit message section on behaviour change to:
Introduces behaviour change:
- the `-alertnotify` command is executed for all
`KernelNotifications::warningSet` calls, which now also covers the
`LARGE_WORK_INVALID_CHAIN` warning.
- previously, warnings were returned based on a predetermined order,
e.g. with the "pre-release test build" warning always first. This
is no longer the case, and Warnings::GetMessages() will return
messages sorted by the id of the warning.
Could also note in setWarning documentation that ID affects order warnings are shown in.
I don't think this belongs in setWarning
, kernel users could implement this how they want, so I've updated the Warnings::GetMessages()
docstring to:
/** Return potential problems detected by the node, sorted by the
* warning_type id */
src/node/warnings.h
Outdated
class Warnings | ||
{ | ||
mutable Mutex m_mutex; | ||
std::map<std::string, bilingual_str> m_warnings GUARDED_BY(m_mutex); |
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've incorporated the id-as-enum approach in the latest force-push, thank you very much for both of your extensive feedback.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
5dfbc35
to
66d4f31
Compare
Instead of having separate warning functions (and globals) for each different warning that can be raised, encapsulate this logic into a single class and allow to (un)set any number of warnings. Introduces behaviour change: - the `-alertnotify` command is executed for all `KernelNotifications::warningSet` calls, which now also covers the `LARGE_WORK_INVALID_CHAIN` warning. - previously, warnings were returned based on a predetermined order, e.g. with the "pre-release test build" warning always first. This is no longer the case, and Warnings::GetMessages() will return messages sorted by the id of the warning. Removes warnings.cpp from kernel.
This commit introduces slight behaviour change. Previously, the GUI status bar would be updated for most warnings, namely UNKNOWN_NEW_RULES_ACTIVATED, CLOCK_OUT_OF_SYNC and PRE_RELEASE_TEST_BUILD, but not for LARGE_WORK_INVALID_CHAIN (and not for FATAL_INTERNAL_ERROR, but that is not really meaningful). Fix this by always updating the status bar when the warnings are changed.
66d4f31
to
51cb837
Compare
Oops, forgot to update bitcoin-chainstate.cpp again. Force pushed to fix that, and also slightly improved function signatures to use |
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.
Nice change, just left some cosmetic nits.
ACK 51cb837
src/node/warnings.h
Outdated
void SetMiscWarning(const bilingual_str& warning); | ||
void SetfLargeWorkInvalidChainFound(bool flag); | ||
/** Pass std::nullopt to disable the warning */ | ||
void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning); | ||
/** Return potential problems detected by the node. */ | ||
std::vector<bilingual_str> GetWarnings(); | ||
/** | ||
* RPC helper function that wraps GetWarnings. Returns a UniValue::VSTR | ||
* with the latest warning if use_deprecated is set to true, or a |
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.
Nitty nit: Trailing whitespace. There are some other small formatting issues elsewhere too, maybe run clang-format-diff
once over the commits?
@@ -6,6 +6,7 @@ | |||
#define BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H | |||
|
|||
#include <cstdint> | |||
#include <string> |
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.
Nit: This include could be dropped again.
@@ -9,12 +9,17 @@ | |||
|
|||
#include <atomic> | |||
#include <cstdint> | |||
#include <string> |
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.
Nit: This include could be dropped again.
@@ -31,12 +32,15 @@ bool Warnings::Set(const std::string& id, const bilingual_str& message) | |||
{ | |||
LOCK(m_mutex); | |||
const auto& [_, inserted]{m_warnings.insert({id, message})}; | |||
if (inserted) uiInterface.NotifyAlertChanged(); |
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.
^ cc @hebasto
@@ -7,14 +7,14 @@ | |||
#define BITCOIN_NODE_WARNINGS_H | |||
|
|||
#include <sync.h> | |||
#include <util/translation.h> |
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 don't think this is needed? Maybe these patches would be a good opportunity to cleanup the includes of this module.
namespace node | ||
{ | ||
class Warnings; | ||
} // namespace node |
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 don't think this is needed.
This PR:
Warnings::Set()
andWarnings::Unset()
methods, instead of having a separate function and globals for each warning. As a result, this simplifies thekernel::Notifications
interface.Behaviour change introduced:
-alertnotify
command is executed for allKernelNotifications::warningSet
calls, which now also covers thelarge-work-invalid-chain
warningnode::AbortNode()
is called, or for thelarge-work-invalid-chain
warningSome discussion points:
const std::string& id
the best way to refer to warnings? Enums are an obvious alternative, but since we need to define warnings across libraries, strings seem like a straightforward solution.