wallet: Add compile time checking for cs_wallet runtime locking assertions #13081
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
utACK c51b9e36e98ed1fd2151d923ae233970c4c39713 |
Contributor
|
Looks like GetConflicts was missed, |
practicalswift
force-pushed
the
cs_wallet-compile-time-checking
branch
from
c51b9e3 to
775b74e
Compare
Contributor
Author
|
@TheBlueMatt Correct! Added! @MarcoFalke @TheBlueMatt Please re-review! :-) |
maflcko
reviewed
Apr 29, 2018
src/wallet/wallet.cpp
Outdated
Member
There was a problem hiding this comment.
Add this to the header file instead?
Contributor
Author
There was a problem hiding this comment.
Moving it to the header file would generate a member access into an at that stage incomplete type CWallet I'm afraid. Any suggestion?
Member
There was a problem hiding this comment.
You can solve it by moving stuff around in the header file (i.e. make a forward declaration of CWalletTx, move the CWalletTx to the line after CWallet, move methods that still depend on the type to the cpp file, then add all the locking annotations. Note that just adding the annotation here is not sufficient, since I get a warning in wallet/rpcwallet.cpp...)
Personally, I had the feeling that c51b9e3 was a clear and obvious step in the right direction. I don't like to hold back the pull request based on that a single method was "missed". That can be fixed up later in a separate pull ... I'd suggest resetting hard to c51b9e3 and merge it
Member
There was a problem hiding this comment.
@practicalswift @TheBlueMatt Any further thoughts here?
Contributor
Author
There was a problem hiding this comment.
@MarcoFalke Good point. I'll revert to c51b9e3.
practicalswift
changed the title
This was referenced Apr 30, 2018
practicalswift
force-pushed
the
cs_wallet-compile-time-checking
branch
2 times, most recently
from
dea4474 to
c51b9e3
Compare
Contributor
Author
|
Now reverted to c51b9e36e98ed1fd2151d923ae233970c4c39713 as suggested by @MarcoFalke. Will add Ready for merge? :-) |
Member
|
@ryanofsky or @jnewbery You are doing a lot with the wallet lately. Mind taking a look here? |
practicalswift
changed the title
practicalswift
force-pushed
the
cs_wallet-compile-time-checking
branch
from
c51b9e3 to
1b34bda
Compare
Member
|
re-utACK 1b34bda4c9beb993e2378a5c8cb22f92c421eba0 |
ryanofsky
reviewed
May 11, 2018
Contributor
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
@ryanofsky or @jnewbery You are doing a lot with the wallet lately. Mind taking a look here?
utACK 1b34bda4c9beb993e2378a5c8cb22f92c421eba0.
I'm not very familiar with these annotations, but spot checking a few of them, they looked correct, and I assume there is basically no harm if an annotation is added incorrectly (other than maybe having to remove the annotation in a future change?). I also didn't check for missing annotations, because that seemed equally harmless.
Member
|
Needs rebase |
practicalswift
force-pushed
the
cs_wallet-compile-time-checking
branch
from
1b34bda to
66b0b1b
Compare
Contributor
Author
|
Rebased! Please re-review :-) |
Member
|
re-utACK 66b0b1b |
maflcko
pushed a commit
that referenced
this pull request
May 14, 2018
… locking assertions 66b0b1b Add compile time checking for all cs_wallet runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_wallet` 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: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 13, 2019
… locking assertions Summary: 66b0b1b Add compile time checking for all cs_wallet runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_wallet` 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: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd Backport of Core PR13081 bitcoin/bitcoin#13081 Depends on D3998 Test Plan: ./configure CXX=clang++ CC=clang make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3981
jonspock
pushed a commit
to jonspock/devault
that referenced
this pull request
Dec 22, 2019
… locking assertions Summary: 66b0b1b2a6 Add compile time checking for all cs_wallet runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_wallet` 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: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd Backport of Core PR13081 bitcoin/bitcoin#13081 Depends on D3998 Test Plan: ./configure CXX=clang++ CC=clang make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3981
proteanx
pushed a commit
to devaultcrypto/devault
that referenced
this pull request
Dec 23, 2019
… locking assertions Summary: 66b0b1b2a6 Add compile time checking for all cs_wallet runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_wallet` 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: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd Backport of Core PR13081 bitcoin/bitcoin#13081 Depends on D3998 Test Plan: ./configure CXX=clang++ CC=clang make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3981
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 1, 2020
…runtime locking assertions 66b0b1b Add compile time checking for all cs_wallet runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_wallet` 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: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Mar 10, 2022
…runtime locking assertions 66b0b1b Add compile time checking for all cs_wallet runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_wallet` 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: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add compile time checking for
cs_walletruntime 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.