Add compile time checking for run time locking assertions

practicalswift commented at 5:17 pm on March 10, 2018: contributor

Add compile time checking for all run time locking assertions.

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.

fanquake added the label Refactoring on Mar 10, 2018

practicalswift renamed this:
~~Add compile time checking for all run time locking assertions~~
Add compile time checking for all run time locking assertions [wip]
on Mar 11, 2018

practicalswift force-pushed on Mar 11, 2018

in src/txmempool.cpp:508 in aa224a20d6 outdated

501@@ -502,10 +502,10 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso
502     }
503 }
504 
505-void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags)
506+void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs, mempool.cs)
507 {
508     // Remove transactions spending a coinbase which are now immature and no-longer-final transactions
509-    LOCK(cs);
510+    AssertLockHeld(cs);

MarcoFalke commented at 9:26 pm on March 11, 2018:

This 1 line diff should be a separate pull request

practicalswift commented at 10:31 pm on March 11, 2018:

That assertion does not seem to hold, so I think it should be removed. I’ll investigate.

MarcoFalke commented at 9:27 pm on March 11, 2018: member

Concept ACK

practicalswift force-pushed on Mar 11, 2018

practicalswift renamed this:
~~Add compile time checking for all run time locking assertions [wip]~~
Add compile time checking for all run time locking assertions
on Mar 11, 2018

practicalswift commented at 6:59 am on March 12, 2018: contributor

@MarcoFalke Updated! Please review :-)

laanwj commented at 11:49 am on March 13, 2018: member

Concept ACK - Unlike the title implies, not all the added run time checking requirements have an associated AssertLockHeld/AssertLockNotHeld (e.g. CoinSelection) is this on purpose?

practicalswift renamed this:
~~Add compile time checking for all run time locking assertions~~
Add compile time checking for run time locking assertions
on Mar 13, 2018

practicalswift force-pushed on Mar 13, 2018

practicalswift commented at 1:52 pm on March 13, 2018: contributor

@laanwj Thanks for reporting. The annotations for CoinSelection and two other functions were incorrect. That is now fixed.

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.

I’ll run through all annotations again to verify that they all follow directly or indirectly from AssertLockHeld(…)/AssertLockNotHeld(…):s.

These are the annotations added – let me know if something sticks out/looks weird:

0[Edit: List moved to PR description. See above!]

Sjors commented at 8:57 pm on March 13, 2018: member

Sadly that still didn’t catch my mistake here, but I assume this is a step in that direction.

Lightly tested on macOS (no change, as expected). @practicalswift maybe you can put that overview and additional explanation (“directly follows that…”) in the commit message or PR description?

I didn’t see a EXCLUSIVE_LOCKS_REQUIRED in LoadChainTip; I assume that’s done in a some dependent function? Maybe that can be made explicit in your overview if it’s not too tedious?

The EXLCUSIVE_LOCKS... stuff in validation.h is not repeated in validation.cpp, that’s intentional?

Should violations merely throw warnings?

meshcollider commented at 9:11 pm on March 13, 2018: contributor

Concept ACK

practicalswift commented at 9:19 am on March 14, 2018: contributor

@Sjors No intention to repeat annotations :-) Which specific annotations did you see repeated in validation.cpp? Did you check them in the files or just in the summary?

Violations are checked by Travis and cause a Travis build failure if found – see configure.ac.

practicalswift force-pushed on Mar 14, 2018

practicalswift commented at 9:22 am on March 14, 2018: contributor

@Sjors Did you mean that you didn’t see any AssertLockHeld(…) in LoadChainTip? :-)

practicalswift force-pushed on Mar 14, 2018

Sjors commented at 2:10 pm on March 14, 2018: member

Which specific annotations did you see repeated in validation.cpp

None, so that’s all good. I just found it slightly confusing that some annotations are in .h files and others in .cpp files.

Did you mean that you didn’t see any AssertLockHeld(…) in LoadChainTip?

Yes, sorry. To more specific, no AssertLockHeld(mempool.cs). But if you’re happy and the compiler is happy, I’m happy :-)

practicalswift force-pushed on Mar 14, 2018

practicalswift commented at 7:59 pm on March 14, 2018: contributor

@Sjors The annotations should be in the header files, but not all functions are exported so that’s why some annotations end up in the .h files.

LoadChainTip is annotated with EXCLUSIVE_LOCKS_REQUIRED(mempool.cs) since it (indirectly) calls CheckSequenceLocks (which contains AssertLockHeld(mempool.cs)) without first locking mempool.cs.

The call stack looks like this:

0LoadChainTip
1  ActivateBestChain
2    ActivateBestChainStep
3      UpdateMempoolForReorg
4        AcceptToMemoryPool
5          AcceptToMemoryPoolWithTime
6            AcceptToMemoryPoolWorker
7              CheckSequenceLocks

sipa commented at 1:07 am on March 15, 2018: member

Concept ACK

practicalswift force-pushed on Mar 23, 2018

practicalswift force-pushed on Apr 3, 2018

practicalswift commented at 11:35 am on April 3, 2018: contributor

Rebased!

practicalswift force-pushed on Apr 6, 2018

practicalswift force-pushed on Apr 9, 2018

practicalswift commented at 8:33 am on April 9, 2018: contributor

Rebased again!

practicalswift commented at 8:47 am on April 9, 2018: contributor

@laanwj This PR is a subset of #11226 (closed in favour of smaller locking PR:s). PR #11226 had a 0.17.0 milestone (originally a 0.16.0 milestone actually). Would it be possible to get a 0.17.0 milestone for this PR?

Of the locking PR:s this should be the best candidate for early merge. It contains only locking annotations for which we are already asserting using AssertLockHeld(…). Since this PR doesn’t change runtime behaviour (compile-time checks only) it should be low risk.

Having this PR merged would make the other locking assertions PR:s much smaller and easier to review since this PR contains the bulk of the assertions which the other PR:s contain too.

Keeping this PR rebased is getting a bit time consuming, so let me know if I can do anything to facilitate merging :-)

practicalswift force-pushed on Apr 9, 2018

practicalswift commented at 10:43 pm on April 9, 2018: contributor

Rebase number 3 performed :-)

practicalswift force-pushed on Apr 9, 2018

practicalswift force-pushed on Apr 10, 2018

Sjors commented at 11:18 am on April 11, 2018: member

Can confirm 9d3e5374e30cd2b1c04c0bb45da24236eb3b2b37 still compiles on macOS. @TheBlueMatt and @promag recently worked on concurrency related bugs in init.cpp, so they might be able to review.

practicalswift force-pushed on Apr 22, 2018

practicalswift commented at 9:44 am on April 22, 2018: contributor

Rebase number 4 performed :-) Please review :-)

practicalswift commented at 9:53 am on April 22, 2018: contributor

@laanwj This PR is getting a bit heavy to keep up-to-date. Do you think this PR has a chance of getting a 0.17.0 milestone (like #11226 had before getting closed in favour of this and other smaller PR:s)? :-)

in src/rpc/rawtransaction.cpp:1136 in 9a17c13c04 outdated

1132@@ -1133,7 +1133,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
1133     return hashTx.GetHex();
1134 }
1135 
1136-UniValue testmempoolaccept(const JSONRPCRequest& request)
1137+UniValue testmempoolaccept(const JSONRPCRequest& request) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs)

MarcoFalke commented at 12:57 pm on April 22, 2018:

I fail to see how this could compile, since we never LOCK(mempool.cs) before calling into this, no?

in src/test/miner_tests.cpp:205 in 9a17c13c04 outdated

201@@ -202,7 +202,7 @@ void TestPackageSelection(const CChainParams& chainparams, CScript scriptPubKey,
202 }
203 
204 // NOTE: These tests rely on CreateNewBlock doing its own self-validation!
205-BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
206+BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs)

MarcoFalke commented at 12:58 pm on April 22, 2018:

Same here

in src/txmempool.cpp:508 in 9a17c13c04 outdated

501@@ -502,7 +502,7 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso
502     }
503 }
504 
505-void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags)
506+void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main, mempool.cs)
507 {
508     // Remove transactions spending a coinbase which are now immature and no-longer-final transactions
509     LOCK(cs);

MarcoFalke commented at 12:59 pm on April 22, 2018:

It seem the lock is taken here, but you require EXCLUSIVE_LOCKS_REQUIRED(mempool.cs)

in src/validation.cpp:479 in 9a17c13c04 outdated

466@@ -468,7 +467,7 @@ static bool IsCurrentForFeeEstimation()
467  * and instead just erase from the mempool as needed.
468  */
469 
470-void UpdateMempoolForReorg(DisconnectedBlockTransactions &disconnectpool, bool fAddToMempool)
471+void UpdateMempoolForReorg(DisconnectedBlockTransactions &disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, mempool.cs)

MarcoFalke commented at 1:00 pm on April 22, 2018:

What about adding static to functions that have their lock annotations added in the cpp file and not in the header?

practicalswift force-pushed on Apr 24, 2018

Add compile time checking for all run time locking assertions

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.

0d3b75d59e

practicalswift force-pushed on Apr 24, 2018

practicalswift commented at 10:06 pm on April 24, 2018: contributor

@MarcoFalke Updated. I’ve now tried to address your feedback. Turns out that I was a bit too strict with the locking requirements for mempool.cs, so the updated diff is significantly smaller and hopefully easier to review. Please re-review :-)

MarcoFalke commented at 6:56 pm on April 25, 2018: member

utACK (could use ::mempool.cs instead of mempool.cs, since it is referring to the global, I guess)

Also, you could split up the cs_wallet and ::mempool.cs into two separate prs. I have the feeling it is easier to get stuff in that only touches a single “module”.

Use ::mempool.cs instead of mempool.cs to clarify that we are using the global 2e24b53266

practicalswift commented at 9:41 pm on April 25, 2018: contributor

@MarcoFalke Thanks for the utACK!

Added a commit which changed from mempool.cs to ::mempool.cs.

I’ve now split up this PR into four separate PRs – one per lock:

cs_KeyStore: #13077
::mempool.cs: #13080
cs_wallet: #13081
cs_main: #13083

Please review these four :-)

MarcoFalke referenced this in commit 3315007e03 on Apr 28, 2018

practicalswift closed this on May 2, 2018

MarcoFalke referenced this in commit 66cc47be98 on May 5, 2018

MarcoFalke referenced this in commit 0dec5b5af4 on May 14, 2018

MarcoFalke referenced this in commit 91186e5984 on Aug 25, 2018

PastaPastaPasta referenced this in commit 8784e4b82a on Jun 17, 2020

PastaPastaPasta referenced this in commit f671d8e2c0 on Jun 17, 2020

PastaPastaPasta referenced this in commit 1ca8c77592 on Jun 27, 2020

PastaPastaPasta referenced this in commit 5aa65d0e68 on Jun 27, 2020

PastaPastaPasta referenced this in commit 2ba0fb5ad8 on Jun 28, 2020

PastaPastaPasta referenced this in commit 3cfa14c5af on Jun 28, 2020

PastaPastaPasta referenced this in commit 4d49ad803c on Jun 29, 2020

PastaPastaPasta referenced this in commit 795076168b on Jul 1, 2020

practicalswift deleted the branch on Apr 10, 2021

Munkybooty referenced this in commit c69df3bc97 on Jun 30, 2021

Munkybooty referenced this in commit c09e919c67 on Jun 30, 2021

Munkybooty referenced this in commit 8a6e8d7a28 on Jul 1, 2021

Munkybooty referenced this in commit a898756320 on Jul 2, 2021

Munkybooty referenced this in commit 5c9d57b8b8 on Jul 4, 2021

UdjinM6 referenced this in commit 0f26a0df06 on Jul 7, 2021

Munkybooty referenced this in commit 23f332711e on Jul 7, 2021

Munkybooty referenced this in commit eae3b9abc4 on Jul 7, 2021

Munkybooty referenced this in commit 06e467d037 on Jul 8, 2021

gades referenced this in commit cb0f86cd7b on Mar 10, 2022

gades referenced this in commit e0e5cea096 on Mar 16, 2022

gades referenced this in commit 956217b1c7 on May 1, 2022

DrahtBot locked this on Aug 16, 2022

Add compile time checking for run time locking assertions #12665