refactor: replace RecursiveMutex m_most_recent_block_mutex with Mutex
ClosedPublic
Actions

Authored by PiRK on Nov 30 2023, 10:22.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC8d8395b09f77: refactor: replace RecursiveMutex m_most_recent_block_mutex with Mutex

Summary

refactor: reduce scope of lock m_most_recent_block_mutex

This avoids calling the non-trivial method
CConnman::PushMessage within the critical section.

refactor: replace RecursiveMutex m_most_recent_block_mutex with Mutex

In each of the critical sections, only the the guarded variables are
accessed, without any chance that within one section another one is
called. Hence, we can use an ordinary Mutex instead of RecursiveMutex.

This is a backport of core#24062

Depends on D14878

Test Plan

cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Nov 30 2023, 10:22

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 30 2023, 10:22

PiRK requested review of this revision.Nov 30 2023, 10:22

teamcity edited the summary of this revision. (Show Details)Nov 30 2023, 10:22

PiRK edited the summary of this revision. (Show Details)Nov 30 2023, 10:24

PiRK added a parent revision: D14878: use cached most recent compact block when available.

Harbormaster completed remote builds in B25779: Diff 43329.Nov 30 2023, 10:36

Fabien added a subscriber: Fabien.Nov 30 2023, 13:49

Fabien added inline comments.

src/net_processing.cpp
6994 ↗	(On Diff #43329)	This change is unrelated to the mutex being non recursive right ? AFAICT there is no lock removal in this change

PiRK added inline comments.Nov 30 2023, 17:16

src/net_processing.cpp
6994 ↗	(On Diff #43329)	The goal is to reduce the scope of the lock by moving `PushMessage` out of the scope of the lock, but I don't think it was strictly necessary, I don't see how `PushMessage` could take that lock. The rationale is just that PushMessage is "non-trivial"