[thread safety] prevent double lock of cs_main in calls to CChainState::UnwindBlock
ClosedPublic
Actions

Authored by majcosta on Jul 16 2020, 20:40.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rABC5d825d4abb76: [thread safety] prevent double lock of cs_main in calls to CChainState…

Summary

In upcoming backport D6940 InvalidateBlock(…) requires mutex cs_main not to be held. Currently we are requiring FinalizeBlock
to be called while cs_main is locked, leading to duplicate calls to LOCK(cs_main), this diff changes that.

Depends on D7203

Note to reviewer: feedback on this diff has been addressed by upcoming D7204. if preferable, they could be squashed together as to avoid any revision from having potential funny locking behavior in FinalizeBlock

Test Plan

cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check && ninja check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

prevent_double_locking

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 12034
Build 23936: Default Diff Build & Tests	build-diff
Build 23935: Build without Wallet	build-without-wallet
Build 23934: Build with clang-10	build-clang-10
Build 23933: arc lint + arc unit

Event Timeline

majcosta created this revision.Jul 16 2020, 20:40

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 16 2020, 20:40

majcosta requested review of this revision.Jul 16 2020, 20:40

Harbormaster completed remote builds in B12034: Diff 22315.Jul 16 2020, 20:54

majcosta mentioned this in D6940: [backport#15971] validation: Add compile-time checking for negative locking requirement in LimitValidationInterfaceQueue.Jul 16 2020, 21:00

deadalnix requested changes to this revision.Jul 16 2020, 23:15

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/validation.cpp
2911	Use assignments for these. these curly bracket are ugly and don't serve any purpose here.
2939	It seems like changes here in between the finalization and the invalidation could lead to very nasty results. IIRC, core intrduced a new mutex for the chainstate. You need to be using it n lieu of cs_main.