Add compile time checking for cs_main runtime locking assertions #13083

practicalswift · 2018-04-25T21:37:52Z

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.

practicalswift · 2018-05-14T13:34:02Z

Rebased!

practicalswift · 2018-05-24T20:03:27Z

Rebased!

practicalswift · 2018-05-29T22:04:50Z

Rebased!

maflcko · 2018-05-29T23:59:12Z

src/qt/transactiondesc.cpp

@@ -48,7 +48,7 @@ QString TransactionDesc::FormatTxStatus(const interfaces::WalletTx& wtx, const i
    }
 }

-QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wallet, TransactionRecord *rec, int unit)
+QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wallet, TransactionRecord *rec, int unit) EXCLUSIVE_LOCKS_REQUIRED(cs_main)


I can't see why any of the changes in the qt and rpc folders are needed. Please make sure you don't add false positives.

practicalswift · 2018-06-15T20:15:32Z

Rebased! :-)

practicalswift · 2018-07-11T23:39:45Z

Rebased!

DrahtBot · 2018-08-25T16:40:55Z

Needs rebase

practicalswift · 2018-08-25T21:38:07Z

@MarcoFalke Rebased. Please re-review. Any outstanding issues? :-)

maflcko · 2018-08-25T21:44:53Z

re-utACK 5cc07926d82b4fecdc7ec6b68f113b709cfa1938

practicalswift · 2018-08-25T21:59:45Z

If this is merged then a lot of the other locking annotations PR:s would be drastically reduced since they repeat cs_main related annotations already contained in this PR. Review of this specific locking annotations PR would thus be extra appreciated :-)

Reviewers of previous locking annotations PR:s – @Empact, @laanwj, @TheBlueMatt or @promag – would you mind take a look at this PR? :-)

maflcko · 2018-08-25T22:04:45Z

@practicalswift Travis failed here

…ssertions 9e0a514 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

maflcko · 2018-08-25T22:39:34Z

src/txmempool.cpp

@@ -497,7 +497,7 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso
    }
 }

-void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags)
+void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main)


Looks like this was added in the cpp file. Would you mind moving it to the header file?

maflcko · 2018-08-25T22:39:49Z

src/wallet/wallet.cpp

@@ -4410,7 +4410,7 @@ bool CMerkleTx::IsImmatureCoinBase() const
    return GetBlocksToMaturity() > 0;
 }

-bool CWalletTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state)
+bool CWalletTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)


ryanofsky · 2018-08-27T15:20:53Z

src/interfaces/wallet.cpp

@@ -57,7 +57,7 @@ class PendingWalletTxImpl : public PendingWalletTx
 };

 //! Construct wallet tx struct.
-WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
+static WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_main)


These functions are in an anonymous namespace so adding static here has no effect.

I think it would be better to avoid using static in cases like this. Anonymous namespaces provide a reliable way of hiding all types of c++ linker symbols, while static keyword works on a non-member symbols (and was actually deprecated in C++03, though it is undeprecated now: https://stackoverflow.com/questions/8460327/why-are-anonymous-namespaces-not-a-sufficient-replacement-for-namespace-static).

@ryanofsky Yes, I know. I simply missed the fact that it was in an anonymous namespace :-)

Thanks for noticing though :-)

…cking assertions 9e0a514 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 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: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6 # Conflicts: # src/validation.cpp # src/validation.h # src/wallet/feebumper.cpp # src/wallet/rpcwallet.cpp # src/wallet/wallet.h

…cking assertions 9e0a514 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 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: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6

…cking assertions 9e0a514 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 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: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6 # Conflicts: # src/validation.cpp # src/validation.h # src/wallet/feebumper.cpp # src/wallet/rpcwallet.cpp # src/wallet/wallet.h # Conflicts: # src/wallet/wallet.h

…cking assertions 9e0a514 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 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: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6

practicalswift mentioned this pull request Apr 25, 2018

Add compile time checking for run time locking assertions #12665

Closed

fanquake added the Refactoring label Apr 25, 2018

This was referenced Apr 30, 2018

Meta-issue: Add Clang thread safety analysis annotations #13129

Closed

Add missing cs_main locks when accessing chainActive #11596

Closed

practicalswift changed the title ~~Add compile time checking for all cs_main runtime locking assertions~~ Add compile time checking for cs_main runtime locking assertions May 3, 2018

practicalswift force-pushed the cs_main branch from a92cbb3 to c06859a Compare May 5, 2018 06:09

practicalswift force-pushed the cs_main branch from c06859a to f4033d3 Compare May 14, 2018 13:30

practicalswift force-pushed the cs_main branch from f4033d3 to 15eddd4 Compare May 24, 2018 20:02

practicalswift force-pushed the cs_main branch from 15eddd4 to 8a3505c Compare May 29, 2018 22:04

maflcko mentioned this pull request May 29, 2018

Add missing locks: validation.cpp + related #11652

Closed

maflcko reviewed May 29, 2018

View reviewed changes

practicalswift force-pushed the cs_main branch from 8a3505c to e687193 Compare May 30, 2018 02:44

This was referenced Jun 7, 2018

Document validationinterace callback blocking deadlock potential. #13402

Merged

rpc: Add testblocktemplatevalidity #13415

Closed

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

[net] Tighten scope in net_processing #13417

Merged

DrahtBot added the Needs rebase label Jun 15, 2018

practicalswift force-pushed the cs_main branch from e687193 to fb80c8f Compare June 15, 2018 20:15

DrahtBot removed the Needs rebase label Jun 19, 2018

This was referenced Jun 19, 2018

doc: Rewrite some validation docs as lock annotations #13481

Merged

[net] Thread safety annotations in net_processing #13423

Merged

rpc: Add submitheader #13399

Merged

DrahtBot mentioned this pull request Jun 30, 2018

Fix get balance #13566

Merged

DrahtBot added the Needs rebase label Jul 9, 2018

practicalswift force-pushed the cs_main branch from fb80c8f to 4d692e8 Compare July 11, 2018 23:39

DrahtBot mentioned this pull request Aug 23, 2018

Utxoscriptindex #14035

Closed

DrahtBot added the Needs rebase label Aug 25, 2018

practicalswift force-pushed the cs_main branch from 2cb4b9a to 5cc0792 Compare August 25, 2018 21:33

Add compile time checking for all cs_main runtime locking assertions

9e0a514

practicalswift force-pushed the cs_main branch from fa81495 to 9e0a514 Compare August 25, 2018 22:26

maflcko merged commit 9e0a514 into bitcoin:master Aug 25, 2018

maflcko removed the Needs rebase label Aug 25, 2018

maflcko reviewed Aug 25, 2018

View reviewed changes

ryanofsky reviewed Aug 27, 2018

View reviewed changes

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

ryanofsky mentioned this pull request Sep 5, 2018

Refactor: separate wallet from node #10973

Merged

practicalswift deleted the cs_main branch April 10, 2021 19:35

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add compile time checking for cs_main runtime locking assertions #13083

Add compile time checking for cs_main runtime locking assertions #13083

Uh oh!

practicalswift commented Apr 25, 2018 •

edited

Loading

Uh oh!

practicalswift commented May 14, 2018

Uh oh!

practicalswift commented May 24, 2018

Uh oh!

practicalswift commented May 29, 2018

Uh oh!

maflcko May 29, 2018

Uh oh!

practicalswift commented Jun 15, 2018

Uh oh!

practicalswift commented Jul 11, 2018

Uh oh!

DrahtBot commented Aug 25, 2018

Uh oh!

practicalswift commented Aug 25, 2018

Uh oh!

maflcko commented Aug 25, 2018

Uh oh!

practicalswift commented Aug 25, 2018

Uh oh!

maflcko commented Aug 25, 2018

Uh oh!

maflcko Aug 25, 2018

Uh oh!

maflcko Aug 25, 2018

Uh oh!

ryanofsky Aug 27, 2018

Uh oh!

practicalswift Aug 27, 2018

Uh oh!

Uh oh!

Add compile time checking for cs_main runtime locking assertions #13083

Add compile time checking for cs_main runtime locking assertions #13083

Uh oh!

Conversation

practicalswift commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented May 14, 2018

Uh oh!

practicalswift commented May 24, 2018

Uh oh!

practicalswift commented May 29, 2018

Uh oh!

maflcko May 29, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jun 15, 2018

Uh oh!

practicalswift commented Jul 11, 2018

Uh oh!

DrahtBot commented Aug 25, 2018

Uh oh!

practicalswift commented Aug 25, 2018

Uh oh!

maflcko commented Aug 25, 2018

Uh oh!

practicalswift commented Aug 25, 2018

Uh oh!

maflcko commented Aug 25, 2018

Uh oh!

maflcko Aug 25, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 25, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Apr 25, 2018 •

edited

Loading