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

stickies-v · 2024-05-07T22:32:44Z

This PR:

moves warnings from common to the node library and into the node namespace (as suggested in rpc: return warnings as an array instead of just a single one #29845 (comment))
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.

DrahtBot · 2024-05-07T22:32:46Z

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.

DrahtBot · 2024-05-07T22:39:36Z

🚧 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}

TheCharlatan · 2024-05-08T08:24:06Z

Concept ACK on removing the warnings globals.

stickies-v · 2024-05-08T20:08:42Z

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

src/node/warnings.h

ryanofsky

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.

src/Makefile.am

src/node/kernel_notifications.cpp

src/node/warnings.cpp

stickies-v

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

src/Makefile.am

src/node/kernel_notifications.cpp

src/node/warnings.cpp

src/node/warnings.h

DrahtBot · 2024-05-24T05:39:56Z

🚧 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}

stickies-v · 2024-06-12T10:34:16Z

Force pushed to:

address merged conflict from [refactor] Check CTxMemPool options in ctor #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.

ryanofsky

Code review ACK 4ca48b3. Just rebase and a change to internal commits (removing DoWarning earlier) since last review.

TheCharlatan

Re-ACK 4ca48b3

Since rpc/util.cpp is in common, also move GetNodeWarnings() to node::GetWarningsForRPC()

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.

stickies-v · 2024-06-13T10:23:47Z

Force pushed to address merge conflict from #29015, no changes otherwise.

ryanofsky

Code review ACK 260f8da. Only change since last review was rebasing

TheCharlatan

Re-ACK 260f8da

achow101 · 2024-06-17T20:32:11Z

ACK 260f8da

stickies-v · 2024-07-08T13:43:07Z

Note: 9c4b0b7 introduced a double lock bug in bitcoin-qt as described in #30400.

hebasto · 2024-07-14T06:53:06Z

Ported to the CMake-based build system in hebasto#264.

stickies-v mentioned this pull request May 7, 2024

rpc: return warnings as an array instead of just a single one #29845

Merged

stickies-v force-pushed the 2024-04/move-warnings-node branch from cd8b420 to 4f06da2 Compare May 7, 2024 22:39

DrahtBot added the CI failed label May 7, 2024

This was referenced May 8, 2024

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

Draft

Stratum v2 Template Provider (take 3) #29432

Closed

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Open

This was referenced May 8, 2024

versionbits refactoring #29039

Merged

[refactor] Check CTxMemPool options in ctor #28830

Merged

stickies-v force-pushed the 2024-04/move-warnings-node branch 2 times, most recently from 3b2b72f to 445e6de Compare May 8, 2024 20:05

DrahtBot removed the CI failed label May 8, 2024

TheCharlatan reviewed May 22, 2024

View reviewed changes

src/node/warnings.h Outdated Show resolved Hide resolved

This was referenced May 23, 2024

Fee Estimation via Fee rate Forecasters #30157

Draft

kernel: De-globalize validation caches #30141

Merged

rpc: avoid copying into UniValue #30115

Merged

refactor: TxDownloadManager + fuzzing #30110

Merged

DrahtBot added the Needs rebase label May 23, 2024

ryanofsky reviewed May 23, 2024

View reviewed changes

src/Makefile.am Show resolved Hide resolved

src/node/kernel_notifications.cpp Outdated Show resolved Hide resolved

src/node/warnings.cpp Show resolved Hide resolved

src/node/warnings.cpp Outdated Show resolved Hide resolved

src/node/warnings.cpp Show resolved Hide resolved

DrahtBot requested a review from TheCharlatan May 23, 2024 17:10

stickies-v force-pushed the 2024-04/move-warnings-node branch from 445e6de to 674da14 Compare May 23, 2024 20:52

DrahtBot removed the Needs rebase label May 23, 2024

stickies-v force-pushed the 2024-04/move-warnings-node branch from 674da14 to 5dfbc35 Compare May 23, 2024 21:11

stickies-v commented May 23, 2024

View reviewed changes

DrahtBot added the CI failed label May 24, 2024

stickies-v force-pushed the 2024-04/move-warnings-node branch from 5dfbc35 to 66d4f31 Compare May 24, 2024 08:28

DrahtBot added the Needs rebase label Jun 11, 2024

stickies-v force-pushed the 2024-04/move-warnings-node branch from 468cf53 to 4ca48b3 Compare June 12, 2024 10:30

DrahtBot removed the Needs rebase label Jun 12, 2024

ryanofsky approved these changes Jun 12, 2024

View reviewed changes

DrahtBot requested a review from TheCharlatan June 12, 2024 14:20

TheCharlatan approved these changes Jun 12, 2024

View reviewed changes

DrahtBot mentioned this pull request Jun 12, 2024

doc: use TRUC instead of v3 and add release note #30272

Merged

DrahtBot added the Needs rebase label Jun 12, 2024

stickies-v added 5 commits June 13, 2024 11:20

refactor: remove unnecessary AppendWarning helper function

bed29c4

move-only: move warnings from common to node

20e616f

Since rpc/util.cpp is in common, also move GetNodeWarnings() to node::GetWarningsForRPC()

refactor: remove warnings globals

260f8da

stickies-v force-pushed the 2024-04/move-warnings-node branch from 4ca48b3 to 260f8da Compare June 13, 2024 10:22

DrahtBot removed the Needs rebase label Jun 13, 2024

ryanofsky approved these changes Jun 13, 2024

View reviewed changes

TheCharlatan approved these changes Jun 13, 2024

View reviewed changes

achow101 merged commit ddf2ebd into bitcoin:master Jun 17, 2024

stickies-v deleted the 2024-04/move-warnings-node branch June 18, 2024 11:43

hebasto added the Needs CMake port label Jun 29, 2024

hebasto mentioned this pull request Jul 14, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#264

Closed

hebasto removed the Needs CMake port label Jul 14, 2024

ryanofsky mentioned this pull request Jul 23, 2024

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

bitcoin locked and limited conversation to collaborators Jul 14, 2025

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

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

Uh oh!

Conversation

stickies-v commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented May 7, 2024

Uh oh!

TheCharlatan commented May 8, 2024

Uh oh!

stickies-v commented May 8, 2024

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented May 24, 2024

Uh oh!

stickies-v commented Jun 12, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v commented Jun 13, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 17, 2024

Uh oh!

stickies-v commented Jul 8, 2024

Uh oh!

hebasto commented Jul 14, 2024

Uh oh!

Uh oh!

stickies-v commented May 7, 2024 •

edited

Loading

DrahtBot commented May 7, 2024 •

edited

Loading