Add missing cs_main locks when accessing chainActive #11596

practicalswift · 2017-11-02T17:50:24Z

Add missing cs_main locks when accessing chainActive. (The variable chainActive is guarded by the mutex cs_main.)
Add corresponding annotations (GUARDED_BY + EXCLUSIVE_LOCKS_REQUIRED).

sdaftuar · 2017-11-02T17:53:17Z

src/validation.cpp

I think we can just add an AssertLockHeld(cs_main) to the top of this function -- this function only has two callers, both of which lock cs_main before calling.

EDIT: actually one of the callers does AssertLockHeld(cs_main).

morcos · 2017-11-02T18:02:38Z

src/wallet/wallet.cpp

I think we should just get rid of this assert. We can't guarantee chainActive height will not go down.

practicalswift · 2017-11-02T21:10:40Z

@sdaftuar @morcos Thanks for reviewing. Suggested changes incorporated! :-)

promag

utACK.

promag · 2017-11-03T22:02:56Z

src/wallet/wallet.cpp

Remove, all callers have cs_main locked, assert lock is held instead. Ping @ryanofsky as he says the goal is to removed recursive locks.

@promag Thanks! Now fixed.

@promag I'm getting an assertion failure when running test/functional/test_runner.py with your suggested patch applied. Does it pass for you? Reverting to previous version for now.

I'll check.

@promag See @TheBlueMatt's comment about cs_main locking in CWallet::CreateTransaction(…): #11596 (comment)

promag · 2017-11-06T16:19:32Z

@practicalswift following #11596 (comment), I believe a7eb21a is the fix.

practicalswift · 2017-11-06T16:38:35Z

@promag What is the reasoning behind the patch (a7eb21a) – what are the underlying locking requirements (exact variables being guarded)? Your patch is likely correct, but I'm trying to understand exactly why :-)

TheBlueMatt · 2017-11-06T19:46:00Z

src/rest.cpp

nit: care to do the serialization of bitmap/outs outside of cs_main (same below)?

TheBlueMatt · 2017-11-06T20:09:35Z

src/wallet/wallet.cpp

Ugh, can we not add more cs_main requirements - if the goal is to resolve the chainActive.Height() issue only then please add the cs_main around just that, its not gonna hurt anything to not hold cs_main for the duration of this function, and we really should be trying to remove cs_main from this function, not go the other way.

practicalswift · 2017-11-06T20:48:40Z

@TheBlueMatt Thanks for reviewing. Feedback addressed.

TheBlueMatt · 2017-11-07T16:51:47Z

src/wallet/rpcwallet.cpp

Please no. Can we instead add the locking in CreateTransaction itself? We need to be pushing locks down, especially when it comes to cs_main, not putting more locks in RPC functions.

practicalswift · 2017-11-07T18:09:31Z

Reverting the patch suggested by @promag. The locking is now made down in CreateTransaction as suggested by @TheBlueMatt.

TheBlueMatt · 2017-11-07T18:25:20Z

utACK 2e441c91e7806e6ae781ae0b0aefea958079d15b
Thanks!

luke-jr · 2017-11-10T16:22:33Z

src/rest.cpp

This isn't sufficient I think. We don't want to allow chainActive to change from when we get the results, to when we identify the chain they're for. So extract this info above in the first lock...

Which first lock?

@luke-jr Thanks for reviewing. Do you mean like this?

diff --git a/src/rest.cpp b/src/rest.cpp index 0ed11e6..649059e 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -479,6 +479,8 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart) std::string bitmapStringRepresentation; std::vector<bool> hits; bitmap.resize((vOutPoints.size() + 7) / 8); + int chainActiveHeight; + uint256 chainActiveTipBlockHash; { LOCK2(cs_main, mempool.cs); @@ -503,6 +505,9 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart) bitmapStringRepresentation.append(hit ? "1" : "0"); // form a binary string representation (human-readable for json output) bitmap[i / 8] |= ((uint8_t)hit) << (i % 8); } + + chainActiveHeight = chainActive.Height(); + chainActiveTipBlockHash = chainActive.Tip()->GetBlockHash(); } switch (rf) { @@ -510,11 +515,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart) // serialize data // use exact same output as mentioned in Bip64 CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION); - { - LOCK(cs_main); - ssGetUTXOResponse << chainActive.Height() << chainActive.Tip()->GetBlockHash(); - } - ssGetUTXOResponse << bitmap << outs; + ssGetUTXOResponse << chainActiveHeight << chainActiveTipBlockHash << bitmap << outs; std::string ssGetUTXOResponseString = ssGetUTXOResponse.str(); req->WriteHeader("Content-Type", "application/octet-stream"); @@ -524,11 +525,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart) case RF_HEX: { CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION); - { - LOCK(cs_main); - ssGetUTXOResponse << chainActive.Height() << chainActive.Tip()->GetBlockHash(); - } - ssGetUTXOResponse << bitmap << outs; + ssGetUTXOResponse << chainActiveHeight << chainActiveTipBlockHash << bitmap << outs; std::string strHex = HexStr(ssGetUTXOResponse.begin(), ssGetUTXOResponse.end()) + "\n"; req->WriteHeader("Content-Type", "text/plain"); @@ -541,11 +538,8 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart) // pack in some essentials // use more or less the same output as mentioned in Bip64 - { - LOCK(cs_main); - objGetUTXOResponse.push_back(Pair("chainHeight", chainActive.Height())); - objGetUTXOResponse.push_back(Pair("chaintipHash", chainActive.Tip()->GetBlockHash().GetHex())); - } + objGetUTXOResponse.push_back(Pair("chainHeight", chainActiveHeight)); + objGetUTXOResponse.push_back(Pair("chaintipHash", chainActiveTipBlockHash.GetHex())); objGetUTXOResponse.push_back(Pair("bitmap", bitmapStringRepresentation)); UniValue utxos(UniValue::VARR);

luke-jr · 2017-11-10T16:22:48Z

src/rest.cpp

luke-jr · 2017-11-10T16:23:02Z

src/rest.cpp

practicalswift · 2017-11-10T19:05:55Z

Added commit 9b3d094894350933342b5e352785cfdd9f2d03fe addressing @luke-jr:s feedback.

Please re-review :-)

TheBlueMatt · 2017-11-10T21:10:40Z

@practicalswift why did you rebase this? It makes reviewing something that was previously reviewed much more difficult. Mind squashing the two commits? There seems to be no reason to have them separate.

luke-jr

utACK

The variable chainActive is guarded by the mutex cs_main. Github-Pull: bitcoin#11596 Rebased-From: 4786fe1a50c16381d723b5291938a615cc13a490

…n we identify the chain they're for Github-Pull: bitcoin#11596 Rebased-From: 9b3d094894350933342b5e352785cfdd9f2d03fe

practicalswift · 2017-11-12T15:44:22Z

Now squashed into one commit. Please re-review! :-)

@TheBlueMatt The reason for the rebase was that I wanted to get rebased on top of 76ea17c (IIRC) in order to get the build to pass – I always compile with -Werror=thread-safety-analysis :-)

The reason for the two commits was to allow separate review for the latter commit in the case that I had misunderstood @luke-jr:s suggestion and needed to revert.

practicalswift · 2018-03-19T20:37:10Z

I've now reworked this PR and split it up in three commits:

Add missing LOCK(cs_main):s where required for chainActive access
Annotation: Add chainActive GUARDED_BY(cs_main) – does not change run-time behaviour
Annotation: Add EXCLUSIVE_LOCKS_REQUIRED(...) as implied by the chainActive guard – does not change run-time behaviour

Please re-review :-)

/cc @sdaftuar @morcos @promag @TheBlueMatt @luke-jr @MarcoFalke

practicalswift · 2018-04-03T11:39:58Z

Rebased!

practicalswift · 2018-04-09T13:13:05Z

Rebased!

practicalswift · 2018-04-14T13:14:09Z

Rebased! Please re-review :-)

TheBlueMatt

I guess this is somewhat duplicative with #11652, no? My review comments there apply here too.

TheBlueMatt · 2018-04-17T18:12:52Z

src/wallet/test/wallet_tests.cpp

Can you just take both locks instead of adding a new scope and lots of diff lines? Its in a test, locks shouldn't matter much.

TheBlueMatt · 2018-04-17T18:14:37Z

src/interfaces/wallet.cpp

a) annotations need to go in the header file, not the cpp file. b) I dont think we can put locks required in the interfaces file, as it may become a process boundary. The callee will have to take the lock itself.

annotations need to go in the header file

This is ok, these are just internal functions (in an anonymous namespace) not shared or exposed in headers.

…ctive)

@Marcos

…not go down As noted by @Marcos in bitcoin#11596 (comment)

…at follow from "chainActive GUARDED_BY(cs_main)"

practicalswift · 2018-04-26T04:22:58Z

Rebased and updated. Please re-review :-)

practicalswift · 2018-04-30T16:37:36Z

Closing. Will add new PR based on #11652 and #13083.

sdaftuar reviewed Nov 2, 2017

View reviewed changes

morcos reviewed Nov 2, 2017

View reviewed changes

practicalswift force-pushed the chainActive branch from 52b2f34 to 1c93531 Compare November 2, 2017 21:09

fanquake added the Refactoring label Nov 3, 2017

promag reviewed Nov 3, 2017

View reviewed changes

practicalswift force-pushed the chainActive branch 2 times, most recently from 3371efe to 0ada52c Compare November 6, 2017 15:19

practicalswift force-pushed the chainActive branch from 0ada52c to fdea0e2 Compare November 6, 2017 16:37

TheBlueMatt reviewed Nov 6, 2017

View reviewed changes

practicalswift force-pushed the chainActive branch 2 times, most recently from 17b7775 to cd8ac3c Compare November 6, 2017 20:24

TheBlueMatt reviewed Nov 7, 2017

View reviewed changes

practicalswift force-pushed the chainActive branch from cd8ac3c to 2e441c9 Compare November 7, 2017 18:00

ryanofsky mentioned this pull request Nov 10, 2017

[WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) #11226

Closed

luke-jr reviewed Nov 10, 2017

View reviewed changes

practicalswift force-pushed the chainActive branch from 2e441c9 to 9b3d094 Compare November 10, 2017 19:05

luke-jr approved these changes Nov 11, 2017

View reviewed changes

practicalswift force-pushed the chainActive branch from 9b3d094 to 0b9219e Compare November 12, 2017 15:38

practicalswift force-pushed the chainActive branch from 0b9219e to 11256af Compare November 23, 2017 12:13

practicalswift force-pushed the chainActive branch from 2d77055 to cb21977 Compare March 19, 2018 19:16

practicalswift changed the title ~~Add missing cs_main locks when accessing chainActive [wip]~~ Add missing cs_main locks when accessing chainActive Mar 19, 2018

practicalswift force-pushed the chainActive branch from cb21977 to 1090fa1 Compare March 20, 2018 07:05

practicalswift force-pushed the chainActive branch from 1090fa1 to 7abc218 Compare March 23, 2018 15:02

practicalswift force-pushed the chainActive branch from 7abc218 to c04750f Compare April 3, 2018 11:39

practicalswift force-pushed the chainActive branch from c04750f to 7d5e26e Compare April 9, 2018 13:12

practicalswift force-pushed the chainActive branch 2 times, most recently from a40b60f to f882f91 Compare April 14, 2018 13:13

TheBlueMatt reviewed Apr 17, 2018

View reviewed changes

practicalswift added 3 commits April 26, 2018 05:53

Locking annotations: Add missing cs_main locks (when accessing chainA…

d440996

…ctive)

Remove invalid assertion: We can't guarantee chainActive height will …

77b7b06

…not go down As noted by @Marcos in bitcoin#11596 (comment)

Locking annotations: Add EXCLUSIVE_LOCKS_REQUIRED(...) annotations th…

5ff77fb

…at follow from "chainActive GUARDED_BY(cs_main)"

practicalswift force-pushed the chainActive branch from f882f91 to d8c00bb Compare April 26, 2018 03:55

Locking annotations: Variable chainActive is guarded by cs_main

b0279a0

practicalswift force-pushed the chainActive branch from d8c00bb to b0279a0 Compare April 26, 2018 04:19

practicalswift mentioned this pull request Apr 30, 2018

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

Closed

30 tasks

practicalswift closed this Apr 30, 2018

practicalswift deleted the chainActive branch April 10, 2021 19:34

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

Add missing cs_main locks when accessing chainActive #11596

Add missing cs_main locks when accessing chainActive #11596

Uh oh!

Conversation

practicalswift commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Nov 2, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 6, 2017

Uh oh!

practicalswift commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Nov 7, 2017

Uh oh!

TheBlueMatt commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Nov 10, 2017

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Nov 12, 2017

Uh oh!

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

Uh oh!

Uh oh!

practicalswift commented Apr 3, 2018

Uh oh!

practicalswift commented Apr 9, 2018

Uh oh!

practicalswift commented Apr 14, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Nov 2, 2017 •

edited

Loading

sdaftuar Nov 2, 2017 •

edited

Loading

practicalswift Nov 6, 2017 •

edited

Loading

practicalswift commented Nov 6, 2017 •

edited

Loading

TheBlueMatt commented Nov 7, 2017 •

edited

Loading

practicalswift commented Nov 10, 2017 •

edited

Loading

practicalswift commented Mar 19, 2018 •

edited

Loading