Encapsulate warnings in generalized node::Warnings and remove globals #30058

pull stickies-v wants to merge 5 commits into bitcoin:master from stickies-v:2024-04/move-warnings-node changing 39 files +361 −186
  1. stickies-v commented at 10:32 pm on May 7, 2024: contributor

    This PR:

    • moves warnings from common to the node library and into the node namespace (as suggested in #29845 (review))
    • generalizes the warnings interface to Warnings::Set() and Warnings::Unset() methods, instead of having a separate function and globals for each warning. As a result, this simplifies the kernel::Notifications interface.
    • removes warnings.cpp from the kernel library
    • removes warning globals
    • adds testing for the warning logic

    Behaviour change introduced:

    • the -alertnotify command is executed for all KernelNotifications::warningSet calls, which now also covers the LARGE_WORK_INVALID_CHAIN warning
    • the GUI is updated automatically whenever a warning is (un)set, covering some code paths where it previously wouldn’t be, e.g. when node::AbortNode() is called, or for the LARGE_WORK_INVALID_CHAIN warning

    Some discussion points:

    • ~is 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.~ edit: updated approach to use node::Warning and kernel::Warning enums.
  2. DrahtBot commented at 10:32 pm on May 7, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ryanofsky, TheCharlatan, achow101

    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:

    • #30272 (doc: use TRUC instead of v3 and add release note by glozow)
    • #30141 (kernel: De-globalize validation caches by TheCharlatan)
    • #30110 (refactor: TxDownloadManager by glozow)
    • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

    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.

  3. stickies-v force-pushed on May 7, 2024
  4. DrahtBot added the label CI failed on May 7, 2024
  5. DrahtBot commented at 10:39 pm on May 7, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/24704508399

  6. TheCharlatan commented at 8:24 am on May 8, 2024: contributor
    Concept ACK on removing the warnings globals.
  7. stickies-v force-pushed on May 8, 2024
  8. stickies-v force-pushed on May 8, 2024
  9. stickies-v commented at 8:08 pm on May 8, 2024: contributor

    Force-pushed to address compilation failure on non-macOS systems:

    • moved GetNodeWarnings() (in rpc/util.cpp) to node::GetWarningsForRpc() (in node/warnings.cpp). Since rpc/util.cpp is in common, this causes issues after warnings are moved to node. I don’t love this approach, but it seemed like the least bad one - open to suggestions if anyone has any?
    • updated bitcoin-chainstate.cpp to the new kernel::Notifications interface
  10. DrahtBot removed the label CI failed on May 8, 2024
  11. in src/node/warnings.h:31 in 445e6de114 outdated 
    26+ * to be non-copyable to ensure warnings are managed centrally.
27+ */
28+class Warnings
29+{
30+    mutable Mutex m_mutex;
31+    std::map<std::string, bilingual_str> m_warnings GUARDED_BY(m_mutex);
    TheCharlatan commented at 8:21 pm on May 22, 2024:
    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().
    stickies-v commented at 10:36 am on May 23, 2024:

    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 and node::Warning enums (and potentially more in the future), and have a std::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?
    stickies-v commented at 11:21 am on May 23, 2024:

    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?

    https://github.com/stickies-v/bitcoin/commit/e4ea7aba9f1426dc510dbdbff19788919bf9d3fb - PoC but compiles and tests pass, needs rebase, cleanup and doc updates etc

    TheCharlatan commented at 12:28 pm on May 23, 2024:
    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.
    ryanofsky commented at 3:48 pm on May 23, 2024:

    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.

    stickies-v commented at 8:40 pm on May 23, 2024:
    I’ve incorporated the id-as-enum approach in the latest force-push, thank you very much for both of your extensive feedback.
  12. in src/Makefile.am:243 in 6681395632 outdated 
    234@@ -235,6 +235,7 @@ BITCOIN_CORE_H = \
235   node/txreconciliation.h \
236   node/utxo_snapshot.h \
237   node/validation_cache_args.h \
238+  node/warnings.h \
    ryanofsky commented at 4:01 pm on May 23, 2024:

    In commit “move-only: move warnings from common to node” (6681395632c0ab5e048b34d63d26636856d4202d)

    s/RPc/RPC/ in commit message

    stickies-v commented at 7:19 pm on May 23, 2024:
    Thanks, fixed.
  13. in src/node/kernel_notifications.cpp:48 in 6152c20970 outdated 
    47+static void DoWarning(const std::string& id, const bilingual_str& warning)
48 {
49-    static bool fWarned = false;
50-    node::SetMiscWarning(warning);
51-    if (!fWarned) {
52+    if (node::g_warnings.Set(id, warning)) {
    ryanofsky commented at 4:06 pm on May 23, 2024:

    In commit “introduce and use the generalized node::Warnings interface” (6152c20970c4c4db308879524d4d1c999c4580ae)

    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

    stickies-v commented at 7:37 pm on May 23, 2024:
    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.
  14. in src/node/warnings.cpp:49 in 6152c20970 outdated 
    75-    if (fLargeWorkInvalidChainFound) {
76-        warnings.emplace_back(_("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade."));
77+    LOCK(m_mutex);
78+    std::vector<bilingual_str> messages;
79+    messages.reserve(m_warnings.size());
80+    for (const auto& [id, msg] : m_warnings) {
    ryanofsky commented at 4:13 pm on May 23, 2024:

    In commit “introduce and use the generalized node::Warnings interface” (6152c20970c4c4db308879524d4d1c999c4580ae)

    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.

    stickies-v commented at 8:03 pm on May 23, 2024:

    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:

    0Introduces behaviour change:
1- the `-alertnotify` command is executed for all
2`KernelNotifications::warningSet` calls, which now also covers the
3`LARGE_WORK_INVALID_CHAIN` warning.
4- previously, warnings were returned based on a predetermined order,
5e.g. with the "pre-release test build" warning always first. This
6is no longer the case, and Warnings::GetMessages() will return
7messages 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:

    0    /** Return potential problems detected by the node, sorted by the
1     * warning_type id */
  15. in src/node/warnings.cpp:30 in 6152c20970 outdated 
    36+        );
37+    }
38 }
39-
40-void SetfLargeWorkInvalidChainFound(bool flag)
41+bool Warnings::Set(const std::string& id, const bilingual_str& message)
    ryanofsky commented at 4:38 pm on May 23, 2024:

    In commit “introduce and use the generalized node::Warnings interface” (6152c20970c4c4db308879524d4d1c999c4580ae)

    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.

    stickies-v commented at 7:39 pm on May 23, 2024:
    Ah makes sense, I’ve updated that, thanks.
  16. DrahtBot added the label Needs rebase on May 23, 2024
  17. in src/node/warnings.cpp:33 in 396b261fc7 outdated 
    31@@ -31,12 +32,15 @@ bool Warnings::Set(const std::string& id, const bilingual_str& message)
32 {
33     LOCK(m_mutex);
34     const auto& [_, inserted]{m_warnings.insert({id, message})};
35+    if (inserted) uiInterface.NotifyAlertChanged();
    ryanofsky commented at 4:54 pm on May 23, 2024:

    In commit “node: update uiInterface whenever warnings updated” (396b261fc72d47d3ac4420d2db9910197a21d5ab)

    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.

    stickies-v commented at 7:51 pm on May 23, 2024:

    Can commit message be updated to say what this change in behavior here is?

    I’ve updated the commit message:

     0node: update uiInterface whenever warnings updated
 1
 2This commit introduces slight behaviour change. Previously, the
 3GUI status bar would be updated for most warnings, namely
 4UNKNOWN_NEW_RULES_ACTIVATED, CLOCK_OUT_OF_SYNC and
 5PRE_RELEASE_TEST_BUILD, but not for LARGE_WORK_INVALID_CHAIN
 6(and not for FATAL_INTERNAL_ERROR, but that is not really
 7meaningful).
 8
 9Fix this by always updating the status bar when the warnings are
10changed.

    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?

    TheCharlatan commented at 8:58 pm on May 24, 2024:
    ^ cc @hebasto
    ryanofsky commented at 4:04 pm on June 3, 2024:

    re: #30058 (review)

    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?

    Good point, I didn’t realize that was what was happening before. New approach seems much better.

  18. ryanofsky commented at 5:10 pm on May 23, 2024: contributor
    Code review ACK 445e6de114c00311c506b9245adf7f63bbbbec1d. 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.
  19. DrahtBot requested review from TheCharlatan on May 23, 2024
  20. stickies-v force-pushed on May 23, 2024
  21. DrahtBot removed the label Needs rebase on May 23, 2024
  22. stickies-v force-pushed on May 23, 2024
  23. stickies-v commented at 10:23 pm on May 23, 2024: contributor

    Thank you for the review, @TheCharlatan and @ryanofsky .

    In this force push, I’ve:

    • rebased to address merge conflict from #30115
    • incorporated @TheCharlatan’s suggestion to use enum class instead of std::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
  24. DrahtBot added the label CI failed on May 24, 2024
  25. DrahtBot commented at 5:39 am on May 24, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/25352180163

  26. stickies-v force-pushed on May 24, 2024
  27. stickies-v force-pushed on May 24, 2024
  28. DrahtBot removed the label CI failed on May 24, 2024
  29. stickies-v commented at 10:31 am on May 24, 2024: contributor
    Oops, forgot to update bitcoin-chainstate.cpp again. Force pushed to fix that, and also slightly improved function signatures to use (kernel::Warning id, const bilingual_str& message) instead of (kernel::Warning id, const bilingual_str& warning).
  30. in src/node/warnings.h:25 in 1a2e5e7c59 outdated 
    22 void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning);
23 /** Return potential problems detected by the node. */
24 std::vector<bilingual_str> GetWarnings();
25+/**
26+ * RPC helper function that wraps GetWarnings. Returns a UniValue::VSTR
27+ * with the latest warning if use_deprecated is set to true, or a
    TheCharlatan commented at 7:50 pm on May 24, 2024:
    Nitty nit: Trailing whitespace. There are some other small formatting issues elsewhere too, maybe run clang-format-diff once over the commits?
    stickies-v commented at 11:24 am on May 28, 2024:
    Ah, good catch, I ran clang-format-diff over all commits and incorporated all suggestions, thanks.
  31. in src/kernel/notifications_interface.h:9 in 349441c627 outdated 
    5@@ -6,6 +6,7 @@
6 #define BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H
7 
8 #include <cstdint>
9+#include <string>
    TheCharlatan commented at 8:26 pm on May 24, 2024:
    Nit: This include could be dropped again.
  32. in src/node/kernel_notifications.h:12 in 349441c627 outdated 
     8@@ -9,12 +9,17 @@
 9 
10 #include <atomic>
11 #include <cstdint>
12+#include <string>
    TheCharlatan commented at 8:30 pm on May 24, 2024:
    Nit: This include could be dropped again.
  33. in src/node/warnings.h:10 in 51cb837e0a outdated 
     6@@ -7,14 +7,14 @@
 7 #define BITCOIN_NODE_WARNINGS_H
 8 
 9 #include <sync.h>
10+#include <util/translation.h>
    TheCharlatan commented at 9:14 pm on May 24, 2024:
    I don’t think this is needed? Maybe these patches would be a good opportunity to cleanup the includes of this module.
    stickies-v commented at 2:05 pm on May 28, 2024:

    Forward declaring this doesn’t compile for me:

     0In file included from test/timeoffsets_tests.cpp:6:
 1In file included from ./node/timeoffsets.h:8:
 2In file included from ./sync.h:14:
 3In file included from ./threadsafety.h:9:
 4In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/mutex:191:
 5In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__memory/shared_ptr.h:30:
 6In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__memory/uninitialized_algorithms.h:13:
 7In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__algorithm/copy.h:12:
 8In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__algorithm/copy_move_common.h:14:
 9In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__algorithm/unwrap_range.h:19:
10/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__utility/pair.h:72:9: error: field has incomplete type 'bilingual_str'
11    _T2 second;
12        ^
13/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/map:739:16: note: in instantiation of template class 'std::pair<const std::variant<kernel::Warning, node::Warning>, bilingual_str>' requested here
14    value_type __cc_;
15               ^
16/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__tree:774:23: note: in instantiation of template class 'std::__value_type<std::variant<kernel::Warning, node::Warning>, bilingual_str>' requested here
17    __node_value_type __value_;
18                      ^
19/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__tree:1810:49: note: in instantiation of template class 'std::__tree_node<std::__value_type<std::variant<kernel::Warning, node::Warning>, bilingual_str>, void *>' requested here
20        destroy(static_cast<__node_pointer>(__nd->__left_));
21                                                ^
22/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__tree:1801:3: note: in instantiation of member function 'std::__tree<std::__value_type<std::variant<kernel::Warning, node::Warning>, bilingual_str>, std::__map_value_compare<std::variant<kernel::Warning, node::Warning>, std::__value_type<std::variant<kernel::Warning, node::Warning>, bilingual_str>, std::less<std::variant<kernel::Warning, node::Warning>>, true>, std::allocator<std::__value_type<std::variant<kernel::Warning, node::Warning>, bilingual_str>>>::destroy' requested here
23  destroy(__root());
24  ^
25/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/map:1162:5: note: in instantiation of member function 'std::__tree<std::__value_type<std::variant<kernel::Warning, node::Warning>, bilingual_str>, std::__map_value_compare<std::variant<kernel::Warning, node::Warning>, std::__value_type<std::variant<kernel::Warning, node::Warning>, bilingual_str>, std::less<std::variant<kernel::Warning, node::Warning>>, true>, std::allocator<std::__value_type<std::variant<kernel::Warning, node::Warning>, bilingual_str>>>::~__tree' requested here
26    ~map() {
27    ^
28./node/warnings.h:39:7: note: in instantiation of member function 'std::map<std::variant<kernel::Warning, node::Warning>, bilingual_str>::~map' requested here
29class Warnings
30      ^
31./node/warnings.h:16:8: note: forward declaration of 'bilingual_str'
32struct bilingual_str;
33       ^
341 error generated.
    TheCharlatan commented at 7:20 pm on May 28, 2024:
    Mmh, not sure what iwyu was reporting here before, but it seems correct now. Please resolve :)
  34. in src/rpc/util.h:44 in 51cb837e0a outdated 
    37@@ -38,6 +38,11 @@ enum class TransactionError;
38 struct FlatSigningProvider;
39 struct bilingual_str;
40 
41+namespace node
42+{
43+class Warnings;
44+} // namespace node
    TheCharlatan commented at 9:15 pm on May 24, 2024:
    I don’t think this is needed.
    stickies-v commented at 11:27 am on May 28, 2024:
    Because there’s only one line in between? I agree it’s very obvious now, but I think it doesn’t hurt to have it and removes the arbitrariness of when it does become useful so I’d rather just be consistent and always have it?
    TheCharlatan commented at 7:21 pm on May 28, 2024:
    I meant the forward-declaration itself. Is it used for something?
    stickies-v commented at 8:23 pm on May 28, 2024:
    Oh damn it, I misread your comment. No, it isn’t. This is a leftover from an earlier approach. Removed now, thanks.
  35. TheCharlatan commented at 9:19 pm on May 24, 2024: contributor
    Nice change, just left some cosmetic nits. ACK 51cb837e0a392b96661e00f6e502fbe6458a4df5
  36. DrahtBot requested review from ryanofsky on May 24, 2024
  37. stickies-v force-pushed on May 28, 2024
  38. stickies-v commented at 5:50 pm on May 28, 2024: contributor
    Force-pushed to address @TheCharlatan’s (cosmetic) feedback: incorporated the clang-format-diff suggestions and removed a couple of unnecessary includes.
  39. TheCharlatan approved
  40. TheCharlatan commented at 7:24 pm on May 28, 2024: contributor
    Re-ACK 1a5ab1c27bfce12c46c1dcbb0922c8602798b20c
  41. stickies-v force-pushed on May 28, 2024
  42. stickies-v commented at 8:24 pm on May 28, 2024: contributor
    Force pushed to remove an unnecessary forward declaration, and also cleaned up some weird node::Warnings initializations.
  43. TheCharlatan approved
  44. TheCharlatan commented at 7:59 am on May 29, 2024: contributor
    Re-ACK 468cf536eb6473b0fbc1d0f9763c8d6caedeb849
  45. in src/node/kernel_notifications.cpp:48 in cb9a8f479b outdated 
    44@@ -44,13 +45,10 @@ static void AlertNotify(const std::string& strMessage)
45 #endif
46 }
47 
48-static void DoWarning(const bilingual_str& warning)
49+static void DoWarning(kernel::Warning id, const bilingual_str& message)
    ryanofsky commented at 3:53 pm on June 3, 2024:

    In commit “introduce and use the generalized node::Warnings interface” (cb9a8f479b278b83916841db8395c93b10107d6b)

    Instead of rewriting this function and in this commit and then inlining and deleting it in the last commit, would be nice to just inline and delete it here.

    stickies-v commented at 4:34 pm on June 3, 2024:
    Thanks, I’ll address this if I have to force push!
    stickies-v commented at 10:31 am on June 12, 2024:
    Addressed in latest force push.
  46. ryanofsky approved
  47. ryanofsky commented at 4:20 pm on June 3, 2024: contributor
    Code review ACK 468cf536eb6473b0fbc1d0f9763c8d6caedeb849. This PR is a nice change making behavior more consistent, cleaning up the kernel interface, and removing globals. Main changes since last review are switching from string to enum ids and documenting behavior changes.
  48. DrahtBot added the label Needs rebase on Jun 11, 2024
  49. stickies-v force-pushed on Jun 12, 2024
  50. stickies-v commented at 10:34 am on June 12, 2024: contributor

    Force pushed to:

    • address merged conflict from #28830
    • address @ryanofsky’s comment: inlined DoWarning() in commit “introduce and use the generalized node::Warnings interface” instead of in later commit “refactor: remove warnings globals” to avoid overhauling it twice.
  51. DrahtBot removed the label Needs rebase on Jun 12, 2024
  52. ryanofsky approved
  53. ryanofsky commented at 2:20 pm on June 12, 2024: contributor
    Code review ACK 4ca48b328ba601299fc5aa4efc9749420590a022. Just rebase and a change to internal commits (removing DoWarning earlier) since last review.
  54. DrahtBot requested review from TheCharlatan on Jun 12, 2024
  55. TheCharlatan approved
  56. TheCharlatan commented at 3:06 pm on June 12, 2024: contributor
    Re-ACK 4ca48b328ba601299fc5aa4efc9749420590a022
  57. DrahtBot added the label Needs rebase on Jun 12, 2024
  58. refactor: remove unnecessary AppendWarning helper function bed29c481a
  59. move-only: move warnings from common to node 
    Since rpc/util.cpp is in common, also move GetNodeWarnings() to
node::GetWarningsForRPC()
    20e616f864
  60. introduce and use the generalized `node::Warnings` interface 
    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.
    b071ad9770
  61. 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.
    9c4b0b7ce4
  62. refactor: remove warnings globals 260f8da71a
  63. stickies-v force-pushed on Jun 13, 2024
  64. stickies-v commented at 10:23 am on June 13, 2024: contributor
    Force pushed to address merge conflict from #29015, no changes otherwise.
  65. DrahtBot removed the label Needs rebase on Jun 13, 2024
  66. ryanofsky approved
  67. ryanofsky commented at 11:48 am on June 13, 2024: contributor
    Code review ACK 260f8da71a35232d859d7705861fc1a88bfbbe81. Only change since last review was rebasing
  68. TheCharlatan approved
  69. TheCharlatan commented at 2:20 pm on June 13, 2024: contributor
    Re-ACK 260f8da71a35232d859d7705861fc1a88bfbbe81
  70. achow101 commented at 8:32 pm on June 17, 2024: member
    ACK 260f8da71a35232d859d7705861fc1a88bfbbe81
  71. achow101 merged this on Jun 17, 2024
  72. achow101 closed this on Jun 17, 2024
  73. stickies-v deleted the branch on Jun 18, 2024
  74. hebasto added the label Needs CMake port on Jun 29, 2024


stickies-v DrahtBot TheCharlatan ryanofsky achow101

Labels
Needs CMake port

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me