Page MenuHomePhabricator

Merge #13083: Add compile time checking for cs_main runtime locking assertions
AbandonedPublic

Authored by nakihito on Aug 31 2019, 00:38.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

9e0a514112 Add compile time checking for all cs_main runtime locking assertions (practicalswift)

Pull request description:

Add compile time checking for `cs_main` runtime locking assertions.

This PR is a subset of #12665. The PR was broken up to make reviewing easier.

The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo.

Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without
first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`):
* It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation.
* It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation.

Tree-SHA512: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6

Backport of Core PR13083
https://github.com/bitcoin/bitcoin/pull/13083/

Depends on D3981 and D4026

Test Plan
../configure CXX=clang++ CC=clang
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR13083
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7299
Build 12643: Bitcoin ABC Buildbot (legacy)
Build 12642: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Aug 31 2019, 00:38

Rebased and added missing EXCLUSIVE_LOCKS_REQUIRED() to validation.cpp

jasonbcox requested changes to this revision.Sep 9 2019, 22:05
jasonbcox added inline comments.
src/validation.cpp
203 ↗(On Diff #11076)

This is not part of the original PR. Probably a missed backport.

458 ↗(On Diff #11076)

Why is UpdateMempoolForReorg missing?

2209 ↗(On Diff #11076)

This doesn't appear to be part of the original PR.

src/validation.h
494 ↗(On Diff #11076)

CheckFinalTx is missing (see src/wallet/finaltx.h)

This revision now requires changes to proceed.Sep 9 2019, 22:05

Will rebase off D4026 soon.

src/validation.cpp
203 ↗(On Diff #11076)

Missed this one. Fixed here: D4026.

458 ↗(On Diff #11076)

It looks like it was moved here: https://reviews.bitcoinabc.org/D1667#change-8J68DPC7df8b. I have applied the change to the new location.

2209 ↗(On Diff #11076)

This was added to address compiler warnings. Turns out, its actually necessary for DisconnectedBlockTransactions::updateMempoolForReorg() in txmempool.cpp

src/validation.h
494 ↗(On Diff #11076)

Fixed. Not sure why the compiler didn't throw a warning, though.

nakihito planned changes to this revision.EditedSep 10 2019, 01:21
This comment has been deleted.