diff --git a/doc/release-notes-30058.md b/doc/release-notes-30058.md new file mode 100644 index 00000000000..47e7ae704e2 --- /dev/null +++ b/doc/release-notes-30058.md @@ -0,0 +1,7 @@ +- When running with -alertnotify, an alert can now be raised multiple +times instead of just once. Previously, it was only raised when unknown +new consensus rules were activated, whereas the scope has now been +increased to include all kernel warnings. Specifically, alerts will now +also be raised when an invalid chain with a large amount of work has +been detected. Additional warnings may be added in the future. +(#30058) diff --git a/src/Makefile.am b/src/Makefile.am index 77e0c3a6daf..4a1973aa876 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -196,6 +196,7 @@ BITCOIN_CORE_H = \ kernel/messagestartchars.h \ kernel/notifications_interface.h \ kernel/validation_cache_sizes.h \ + kernel/warning.h \ key.h \ key_io.h \ logging.h \ @@ -996,8 +997,7 @@ libbitcoinkernel_la_SOURCES = \ util/tokenpipe.cpp \ validation.cpp \ validationinterface.cpp \ - versionbits.cpp \ - warnings.cpp + versionbits.cpp # Required for obj/build.h to be generated first. # More details: https://www.gnu.org/software/automake/manual/html_node/Built-Sources-Example.html diff --git a/src/Makefile.test.include b/src/Makefile.test.include index fa7cadb726a..633d0776f56 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -118,6 +118,7 @@ BITCOIN_TESTS =\ test/net_peer_eviction_tests.cpp \ test/net_tests.cpp \ test/netbase_tests.cpp \ + test/node_warnings_tests.cpp \ test/orphanage_tests.cpp \ test/peerman_tests.cpp \ test/pmt_tests.cpp \ diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index ca4434a882c..ecbdcd48bb3 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -28,6 +29,7 @@ #include #include #include +#include #include #include @@ -86,9 +88,13 @@ int main(int argc, char* argv[]) { std::cout << "Progress: " << title.original << ", " << progress_percent << ", " << resume_possible << std::endl; } - void warning(const bilingual_str& warning) override + void warningSet(kernel::Warning id, const bilingual_str& message) override { - std::cout << "Warning: " << warning.original << std::endl; + std::cout << "Warning " << static_cast(id) << " set: " << message.original << std::endl; + } + void warningUnset(kernel::Warning id) override + { + std::cout << "Warning " << static_cast(id) << " unset" << std::endl; } void flushError(const bilingual_str& message) override { diff --git a/src/kernel/notifications_interface.h b/src/kernel/notifications_interface.h index 7283a88e862..8e090dd7db3 100644 --- a/src/kernel/notifications_interface.h +++ b/src/kernel/notifications_interface.h @@ -16,6 +16,8 @@ namespace kernel { //! Result type for use with std::variant to indicate that an operation should be interrupted. struct Interrupted{}; +enum class Warning; + //! Simple result type for functions that need to propagate an interrupt status and don't have other return values. using InterruptResult = std::variant; @@ -38,7 +40,8 @@ public: [[nodiscard]] virtual InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) { return {}; } virtual void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {} virtual void progress(const bilingual_str& title, int progress_percent, bool resume_possible) {} - virtual void warning(const bilingual_str& warning) {} + virtual void warningSet(Warning id, const bilingual_str& message) {} + virtual void warningUnset(Warning id) {} //! The flush error notification is sent to notify the user that an error //! occurred while flushing block data to disk. Kernel code may ignore flush diff --git a/src/kernel/warning.h b/src/kernel/warning.h new file mode 100644 index 00000000000..453f36c5523 --- /dev/null +++ b/src/kernel/warning.h @@ -0,0 +1,14 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_KERNEL_WARNING_H +#define BITCOIN_KERNEL_WARNING_H + +namespace kernel { +enum class Warning { + UNKNOWN_NEW_RULES_ACTIVATED, + LARGE_WORK_INVALID_CHAIN, +}; +} // namespace kernel +#endif // BITCOIN_KERNEL_WARNING_H diff --git a/src/node/abort.cpp b/src/node/abort.cpp index 6f836824b2d..68c09f0ef75 100644 --- a/src/node/abort.cpp +++ b/src/node/abort.cpp @@ -12,13 +12,12 @@ #include #include -#include namespace node { void AbortNode(util::SignalInterrupt* shutdown, std::atomic& exit_status, const bilingual_str& message) { - SetMiscWarning(message); + g_warnings.Set(Warning::FATAL_INTERNAL_ERROR, message); InitError(_("A fatal internal error occurred, see debug.log for details: ") + message); exit_status.store(EXIT_FAILURE); if (shutdown && !(*shutdown)()) { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 88af9dadbc5..9ea3f63fdf6 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -93,7 +93,7 @@ public: explicit NodeImpl(NodeContext& context) { setContext(&context); } void initLogging() override { InitLogging(args()); } void initParameterInteraction() override { InitParameterInteraction(args()); } - bilingual_str getWarnings() override { return Join(GetWarnings(), Untranslated("
")); } + bilingual_str getWarnings() override { return Join(node::g_warnings.GetMessages(), Untranslated("
")); } int getExitStatus() override { return Assert(m_context)->exit_status.load(); } uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); } bool baseInitialize() override diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index 1900ac31179..e18919f503f 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -46,16 +47,6 @@ static void AlertNotify(const std::string& strMessage) #endif } -static void DoWarning(const bilingual_str& warning) -{ - static bool fWarned = false; - node::SetMiscWarning(warning); - if (!fWarned) { - AlertNotify(warning.original); - fWarned = true; - } -} - namespace node { kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index) @@ -80,9 +71,16 @@ void KernelNotifications::progress(const bilingual_str& title, int progress_perc uiInterface.ShowProgress(title.translated, progress_percent, resume_possible); } -void KernelNotifications::warning(const bilingual_str& warning) +void KernelNotifications::warningSet(kernel::Warning id, const bilingual_str& message) { - DoWarning(warning); + if (node::g_warnings.Set(id, message)) { + AlertNotify(message.original); + } +} + +void KernelNotifications::warningUnset(kernel::Warning id) +{ + g_warnings.Unset(id); } void KernelNotifications::flushError(const bilingual_str& message) diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index f4d97a0fff2..96f09be4538 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -15,6 +15,10 @@ class CBlockIndex; enum class SynchronizationState; struct bilingual_str; +namespace kernel { +enum class Warning; +} // namespace kernel + namespace util { class SignalInterrupt; } // namespace util @@ -34,7 +38,9 @@ public: void progress(const bilingual_str& title, int progress_percent, bool resume_possible) override; - void warning(const bilingual_str& warning) override; + void warningSet(kernel::Warning id, const bilingual_str& message) override; + + void warningUnset(kernel::Warning id) override; void flushError(const bilingual_str& message) override; diff --git a/src/node/timeoffsets.cpp b/src/node/timeoffsets.cpp index 17ee44a92cc..b839b60565b 100644 --- a/src/node/timeoffsets.cpp +++ b/src/node/timeoffsets.cpp @@ -49,7 +49,7 @@ bool TimeOffsets::WarnIfOutOfSync() const // when median == std::numeric_limits::min(), calling std::chrono::abs is UB auto median{std::max(Median(), std::chrono::seconds(std::numeric_limits::min() + 1))}; if (std::chrono::abs(median) <= WARN_THRESHOLD) { - node::SetMedianTimeOffsetWarning(std::nullopt); + node::g_warnings.Unset(node::Warning::CLOCK_OUT_OF_SYNC); uiInterface.NotifyAlertChanged(); return false; } @@ -63,7 +63,7 @@ bool TimeOffsets::WarnIfOutOfSync() const "RPC methods to get more info." ), Ticks(WARN_THRESHOLD))}; LogWarning("%s\n", msg.original); - node::SetMedianTimeOffsetWarning(msg); + node::g_warnings.Set(node::Warning::CLOCK_OUT_OF_SYNC, msg); uiInterface.NotifyAlertChanged(); return true; } diff --git a/src/node/warnings.cpp b/src/node/warnings.cpp index 9d2239e64aa..8a94eb0bd2d 100644 --- a/src/node/warnings.cpp +++ b/src/node/warnings.cpp @@ -12,69 +12,53 @@ #include #include -#include +#include #include -static GlobalMutex g_warnings_mutex; -static bilingual_str g_misc_warnings GUARDED_BY(g_warnings_mutex); -static bool fLargeWorkInvalidChainFound GUARDED_BY(g_warnings_mutex) = false; -static std::optional g_timeoffset_warning GUARDED_BY(g_warnings_mutex){}; - namespace node { -void SetMiscWarning(const bilingual_str& warning) +Warnings g_warnings; + +Warnings::Warnings() { - LOCK(g_warnings_mutex); - g_misc_warnings = warning; -} - -void SetfLargeWorkInvalidChainFound(bool flag) -{ - LOCK(g_warnings_mutex); - fLargeWorkInvalidChainFound = flag; -} - -void SetMedianTimeOffsetWarning(std::optional warning) -{ - LOCK(g_warnings_mutex); - g_timeoffset_warning = warning; -} - -std::vector GetWarnings() -{ - std::vector warnings; - - LOCK(g_warnings_mutex); - // Pre-release build warning if (!CLIENT_VERSION_IS_RELEASE) { - warnings.emplace_back(_("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications")); + m_warnings.insert( + {Warning::PRE_RELEASE_TEST_BUILD, + _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications")}); } +} +bool Warnings::Set(warning_type id, bilingual_str message) +{ + LOCK(m_mutex); + const auto& [_, inserted]{m_warnings.insert({id, std::move(message)})}; + return inserted; +} - // Misc warnings like out of disk space and clock is wrong - if (!g_misc_warnings.empty()) { - warnings.emplace_back(g_misc_warnings); +bool Warnings::Unset(warning_type id) +{ + return WITH_LOCK(m_mutex, return m_warnings.erase(id)); +} + +std::vector Warnings::GetMessages() const +{ + LOCK(m_mutex); + std::vector messages; + messages.reserve(m_warnings.size()); + for (const auto& [id, msg] : m_warnings) { + messages.push_back(msg); } - - if (fLargeWorkInvalidChainFound) { - 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.")); - } - - if (g_timeoffset_warning) { - warnings.emplace_back(g_timeoffset_warning.value()); - } - - return warnings; + return messages; } UniValue GetWarningsForRpc(bool use_deprecated) { if (use_deprecated) { - const auto all_warnings{GetWarnings()}; + const auto all_warnings{g_warnings.GetMessages()}; return all_warnings.empty() ? "" : all_warnings.back().original; } UniValue warnings{UniValue::VARR}; - for (auto&& warning : GetWarnings()) { + for (auto&& warning : g_warnings.GetMessages()) { warnings.push_back(std::move(warning.original)); } return warnings; diff --git a/src/node/warnings.h b/src/node/warnings.h index 7766f1dbc91..c85e8a16a90 100644 --- a/src/node/warnings.h +++ b/src/node/warnings.h @@ -6,26 +6,86 @@ #ifndef BITCOIN_NODE_WARNINGS_H #define BITCOIN_NODE_WARNINGS_H -#include -#include +#include + +#include +#include #include class UniValue; struct bilingual_str; +namespace kernel { +enum class Warning; +} // namespace kernel + namespace node { -void SetMiscWarning(const bilingual_str& warning); -void SetfLargeWorkInvalidChainFound(bool flag); -/** Pass std::nullopt to disable the warning */ -void SetMedianTimeOffsetWarning(std::optional warning); -/** Return potential problems detected by the node. */ -std::vector GetWarnings(); +enum class Warning { + CLOCK_OUT_OF_SYNC, + PRE_RELEASE_TEST_BUILD, + FATAL_INTERNAL_ERROR, +}; + /** - * RPC helper function that wraps GetWarnings. Returns a UniValue::VSTR - * with the latest warning if use_deprecated is set to true, or a - * UniValue::VARR with all warnings otherwise. + * @class Warnings + * @brief Manages warning messages within a node. + * + * The Warnings class provides mechanisms to set, unset, and retrieve + * warning messages. + * + * This class is designed to be non-copyable to ensure warnings + * are managed centrally. + */ +class Warnings +{ + typedef std::variant warning_type; + + mutable Mutex m_mutex; + std::map m_warnings GUARDED_BY(m_mutex); + +public: + Warnings(); + //! A warnings instance should always be passed by reference, never copied. + Warnings(const Warnings&) = delete; + Warnings& operator=(const Warnings&) = delete; + /** + * @brief Set a warning message. If a warning with the specified + * `id` is already active, false is returned and the new + * warning is ignored. If `id` does not yet exist, the + * warning is set, and true is returned. + * + * @param[in] id Unique identifier of the warning. + * @param[in] message Warning message to be shown. + * + * @returns true if the warning was indeed set (i.e. there is no + * active warning with this `id`), otherwise false. + */ + bool Set(warning_type id, bilingual_str message) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** + * @brief Unset a warning message. If a warning with the specified + * `id` is active, it is unset, and true is returned. + * Otherwise, no warning is unset and false is returned. + * + * @param[in] id Unique identifier of the warning. + * + * @returns true if the warning was indeed unset (i.e. there is an + * active warning with this `id`), otherwise false. + */ + bool Unset(warning_type id) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Return potential problems detected by the node, sorted by the + * warning_type id */ + std::vector GetMessages() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); +}; + +/** + * RPC helper function that wraps g_warnings.GetMessages(). + * + * Returns a UniValue::VSTR with the latest warning if use_deprecated is + * set to true, or a UniValue::VARR with all warnings otherwise. */ UniValue GetWarningsForRpc(bool use_deprecated); + +extern Warnings g_warnings; } // namespace node #endif // BITCOIN_NODE_WARNINGS_H diff --git a/src/test/node_warnings_tests.cpp b/src/test/node_warnings_tests.cpp new file mode 100644 index 00000000000..2bcc2c95ed5 --- /dev/null +++ b/src/test/node_warnings_tests.cpp @@ -0,0 +1,52 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. +// + +#include +#include + +#include + +#include + +BOOST_FIXTURE_TEST_SUITE(node_warnings_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(warnings) +{ + node::Warnings warnings; + // On pre-release builds, a warning is generated automatically + warnings.Unset(node::Warning::PRE_RELEASE_TEST_BUILD); + + // For these tests, we don't care what the exact warnings are, so + // just refer to them as warning_1 and warning_2 + const auto warning_1{node::Warning::CLOCK_OUT_OF_SYNC}; + const auto warning_2{node::Warning::FATAL_INTERNAL_ERROR}; + + // Ensure we start without any warnings + BOOST_CHECK(warnings.GetMessages().size() == 0); + // Add two warnings + BOOST_CHECK(warnings.Set(warning_1, _("warning 1"))); + BOOST_CHECK(warnings.Set(warning_2, _("warning 2"))); + // Unset the second one + BOOST_CHECK(warnings.Unset(warning_2)); + // Since it's already been unset, this should return false + BOOST_CHECK(!warnings.Unset(warning_2)); + // We should now be able to set w2 again + BOOST_CHECK(warnings.Set(warning_2, _("warning 2 - revision 1"))); + // Setting w2 again should return false since it's already set + BOOST_CHECK(!warnings.Set(warning_2, _("warning 2 - revision 2"))); + + // Verify messages are correct + const auto messages{warnings.GetMessages()}; + BOOST_CHECK(messages.size() == 2); + BOOST_CHECK(messages[0].original == "warning 1"); + BOOST_CHECK(messages[1].original == "warning 2 - revision 1"); + + // Clearing all warnings should also clear all messages + BOOST_CHECK(warnings.Unset(warning_1)); + BOOST_CHECK(warnings.Unset(warning_2)); + BOOST_CHECK(warnings.GetMessages().size() == 0); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index c14c8f872ae..b3b1acbc319 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -27,11 +27,11 @@ #include #include #include +#include #include #include #include #include -#include #include #include #include @@ -1922,9 +1922,11 @@ void Chainstate::CheckForkWarningConditions() if (m_chainman.m_best_invalid && m_chainman.m_best_invalid->nChainWork > m_chain.Tip()->nChainWork + (GetBlockProof(*m_chain.Tip()) * 6)) { LogPrintf("%s: Warning: Found invalid chain at least ~6 blocks longer than our best chain.\nChain state database corruption likely.\n", __func__); - node::SetfLargeWorkInvalidChainFound(true); + m_chainman.GetNotifications().warningSet( + kernel::Warning::LARGE_WORK_INVALID_CHAIN, + _("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade.")); } else { - node::SetfLargeWorkInvalidChainFound(false); + m_chainman.GetNotifications().warningUnset(kernel::Warning::LARGE_WORK_INVALID_CHAIN); } } @@ -2907,7 +2909,7 @@ void Chainstate::UpdateTip(const CBlockIndex* pindexNew) if (state == ThresholdState::ACTIVE || state == ThresholdState::LOCKED_IN) { const bilingual_str warning = strprintf(_("Unknown new rules activated (versionbit %i)"), bit); if (state == ThresholdState::ACTIVE) { - m_chainman.GetNotifications().warning(warning); + m_chainman.GetNotifications().warningSet(kernel::Warning::UNKNOWN_NEW_RULES_ACTIVATED, warning); } else { warning_messages.push_back(warning); }