mempool: Add compile time checking for ::mempool.cs runtime locking assertions #13080
Conversation
src/rpc/blockchain.cpp
Outdated
There was a problem hiding this comment.
All the locks are taken. I don't see why this is necessary.
There was a problem hiding this comment.
@MarcoFalke invalidateblock(…) calls InvalidateBlock(…) without first locking ::mempool.cs. Calling InvalidateBlock(…) requires holding mutex mempool.cs exclusively. Then it follows that calling invalidateblock(…) should require ::mempool.cs too which is what the annotation says? What am I missing? :-)
There was a problem hiding this comment.
Calling InvalidateBlock(…) requires holding mutex mempool.cs
Yeah, I looked and it seems all the locks are taken here, which one did I miss?
There was a problem hiding this comment.
Ah, sorry. The InvalidateBlock(…) annotation seems incorrect. Thanks!
practicalswift
force-pushed
the
mempool.cs
branch
from
65379d3 to
04ce4b6
Compare
|
@MarcoFalke Updated! Please re-review :-) |
practicalswift
force-pushed
the
mempool.cs
branch
from
04ce4b6 to
03774ef
Compare
|
utACK 03774eff0e6f5242f02125cecb4d61fa74c00300 |
src/rpc/blockchain.cpp
Outdated
There was a problem hiding this comment.
How'd this slip in here? I'm kinda surprised it even linked with this.
practicalswift
force-pushed
the
mempool.cs
branch
from
03774ef to
aa8341e
Compare
|
@TheBlueMatt Thanks! Updated. Please re-review :-) |
|
re-utACK aa8341e3ae934587149dd609b542b21f9cf6017f (Only change was to get rid of unrelated change) |
practicalswift
changed the title
|
Missing CheckSequenceLocks. |
|
@TheBlueMatt Given that @MarcoFalke has utACK:ed aa8341e and in light of #13081 (comment), what about adding Both |
practicalswift
changed the title
|
re-utACK cbba1d2 |
…time locking assertions cbba1d2 Add compile time checking for all ::mempool.cs runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `::mempool.cs` 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: 1b5ec1cfca6be67edd1298fea1a52b5572ce833dd4ad05c4583f753c2d3229402663373675df87e950151d5c41aeb3ee02f0ad935ed83fe2f45ca8e4d55d901e
….cs runtime locking assertions cbba1d2 Add compile time checking for all ::mempool.cs runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `::mempool.cs` runtime locking assertions. This PR is a subset of bitcoin#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: 1b5ec1cfca6be67edd1298fea1a52b5572ce833dd4ad05c4583f753c2d3229402663373675df87e950151d5c41aeb3ee02f0ad935ed83fe2f45ca8e4d55d901e
….cs runtime locking assertions cbba1d2 Add compile time checking for all ::mempool.cs runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `::mempool.cs` runtime locking assertions. This PR is a subset of bitcoin#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: 1b5ec1cfca6be67edd1298fea1a52b5572ce833dd4ad05c4583f753c2d3229402663373675df87e950151d5c41aeb3ee02f0ad935ed83fe2f45ca8e4d55d901e
4 participants
Add compile time checking for
::mempool.csruntime 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 fromAssertLockHeld(…)/AssertLockNotHeld(…):s already existing in the repo.Consider the case where function
A(…)containsAssertLockHeld(cs_foo)(withoutfirst locking
cs_fooinA), and thatB(…)callsA(…)(without first lockingcs_main):A(…)should have anEXCLUSIVE_LOCKS_REQUIRED(cs_foo)annotation.B(…)should have anEXCLUSIVE_LOCKS_REQUIRED(cs_foo)annotation.