Use `WITH_LOCK` in `Warnings::Set` #30404

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:warnings-min-lock changing 1 files +1 −2

achow101 commented at 10:15 PM on July 6, 2024: member

The scope of the lock should be limited to just guarding m_warnings as anything listening on NotifyAlertChanged may execute code that requires the lock as well.

Fixes #30400

Use WITH_LOCK in Warnings::Set

The scope of the lock should be limited to just guarding m_warnings as
anything listening on `NotifyAlertChanged` may execute code that
requires the lock as well.

6af51e8198

DrahtBot commented at 10:15 PM on July 6, 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 TheCharlatan, maflcko, glozow, willcl-ark, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
achow101 added this to the milestone 28.0 on Jul 6, 2024
TheCharlatan approved
TheCharlatan commented at 9:01 AM on July 7, 2024: contributor

ACK 6af51e819847e737449609daa214e16f9453e85d
maflcko commented at 9:32 AM on July 8, 2024: member

lgtm ACK 6af51e819847e737449609daa214e16f9453e85d
glozow commented at 10:06 AM on July 8, 2024: member

ACK 6af51e819847e737449609daa214e16f9453e85d

Not familiar with this code at all, but it looks like the handler function handleNotifyAlertChanged calls getStatusBarWarnings https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L256-L260 which calls Node::getWarnings() https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L164-L167 which calls GetMessages() https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/node/interfaces.cpp#L101 and GetMessages takes the lock as well https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/node/warnings.cpp#L44-L46
glozow requested review from stickies-v on Jul 8, 2024
willcl-ark approved
willcl-ark commented at 1:18 PM on July 8, 2024: member

ACK 6af51e819847e737449609daa214e16f9453e85d

It's generally best-practice to hold locks for the shortest possible time to minimize contention.
stickies-v approved
stickies-v commented at 1:37 PM on July 8, 2024: contributor

ACK 6af51e819847e737449609daa214e16f9453e85d

This seems quite tricky to have test coverage for (since it only manifests in bitcoin-qt), and unfortunately the EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) annotations didn't catch this either, so I'm not sure if there's much more we can do to prevent regressions?

Review note: easy steps to reproduce are here. Thanks again for finding this @achow101 .
glozow merged this on Jul 8, 2024
glozow closed this on Jul 8, 2024
bitcoin locked this on Jul 8, 2025