Avoid lock: Call FlushStateToDisk(...) regardless of fCheckForPruning #11617

practicalswift · 2017-11-06T10:27:02Z

FlushStateToDisk(...) won't do anything besides check if we need to prune if
FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning
which is guarded by the mutex cs_LastBlockFile.

promag · 2017-11-06T14:52:03Z

From what I can tell it's missing only one lock (in PruneAndFlush) maybe fix it here too?

practicalswift · 2017-11-06T15:28:06Z

@promag Good point. Looks good now now?

promag · 2017-11-06T15:31:07Z

In that case reword PR and commit, like Add missing cs_LastBlockFile locks

practicalswift · 2017-11-06T15:44:21Z

@promag Done :-)

practicalswift · 2017-11-06T16:23:27Z

@promag Had to revert to initial version. Encountered timeouts when adding locking in PruneAndFlush as suggested and haven't investigated deeper yet. What is your suggested patch? :-)

TheBlueMatt · 2017-11-06T20:25:06Z

src/validation.cpp

@@ -3232,6 +3232,7 @@ static bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CValidation
        return AbortNode(state, std::string("System error: ") + e.what());
    }

+    LOCK(cs_LastBlockFile);


It seems like we could just always call FlushStateToDisk instead? It wont do anything besides check if we need to prune if FLUSH_STATE_NONE is given.

practicalswift · 2017-11-06T20:33:38Z

@TheBlueMatt Even better! PR updated. Please re-review :-)

promag · 2017-11-06T20:44:33Z

utACK 96ff97c. Please reword PR again.

TheBlueMatt · 2017-11-06T21:51:43Z

src/validation.cpp

@@ -3232,8 +3232,7 @@ static bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CValidation
        return AbortNode(state, std::string("System error: ") + e.what());
    }

-    if (fCheckForPruning)
-        FlushStateToDisk(chainparams, state, FLUSH_STATE_NONE); // we just allocated more disk space for block files
+    FlushStateToDisk(chainparams, state, FLUSH_STATE_NONE); // we just allocated more disk space for block files


Comment now needs updated.

TheBlueMatt · 2017-11-06T21:56:40Z

utACK 96ff97cc14b483643020df2497ab06511f5d2ae8 modulo now-incorrect comment.

practicalswift · 2017-11-06T22:16:15Z

@TheBlueMatt Comment removed :-)

TheBlueMatt · 2017-11-06T23:46:35Z

utACK 932dcd49486c6616ee5bfab9588d59fd75c4f0d4

promag · 2017-11-07T00:54:57Z

This PR is a bit messy now. Regarding the original intent, I think it should do:

@@ -2012,6 +2012,7 @@ void FlushStateToDisk() {
 }

 void PruneAndFlush() {
+    LOCK2(cs_main, cs_LastBlockFile);
     CValidationState state;
     fCheckForPruning = true;
     const CChainParams& chainparams = Params();

An alternative is to add an argument to FlushStateToDisk, like bool force_check_for_pruning to avoid touching fCheckForPruning.

TheBlueMatt · 2017-11-07T00:58:23Z

FlushStateToDisk already checks fCheckForPruning and doesn't do anything if it's not set and NONE is passed as the second argument. Despite the complication in FlushStateToDisk, I do not believe this PR has any effect except resolving the (probably not actually a) missing lock bug.

…

On November 6, 2017 7:55:04 PM EST, "João Barbosa" ***@***.***> wrote: This PR is a bit messy now. Regarding the original intent, I think it should do: ```diff @@ -2012,6 +2012,7 @@ void FlushStateToDisk() { } void PruneAndFlush() { + LOCK2(cs_main, cs_LastBlockFile); CValidationState state; fCheckForPruning = true; const CChainParams& chainparams = Params(); ``` An alternative is to add an argument to `FlushStateToDisk`, like `bool force_check_for_pruning` to avoid touching `fCheckForPruning`. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #11617 (comment)

promag · 2017-11-07T01:00:15Z

@TheBlueMatt IMO the best approach to avoid long/recursive/shared locks is to create a snapshot of the chain and use that downstream. The only lock required is when the chain snapshot is created. A slight more advanced solution is copy-on-write, which compared to shared lock, doesn't make the writer wait for readers to finish.

promag · 2017-11-07T01:04:15Z

@TheBlueMatt PruneAndFlush changes fCheckForPruning without the lock.

With:

@@ -2012,6 +2012,7 @@ void FlushStateToDisk() {
 }

 void PruneAndFlush() {
+    AssertLockHeld(cs_LastBlockFile);
     CValidationState state;
     fCheckForPruning = true;
     const CChainParams& chainparams = Params();

The following fails:

./test/functional/pruning.py
2017-11-07 01:02:25.034000 TestFramework (INFO): Initializing test directory /var/folders/1v/8_69hby54nj2k3n6fywt44x80000gn/T/testgwrg340t
Assertion failed: lock cs_LastBlockFile not held in validation.cpp:2015; locks held:

Note ./configure CPPFLAGS=-DDEBUG_LOCKORDER.

TheBlueMatt · 2017-11-07T01:58:51Z

PruneAndFlush() would change behavior in an otherwise-simple lock-fix PR, please, no. The other part of my context is #10279, in which I want to more clearly separate out the disk operations from validation logic - thus, I'm strongly averse to adding a disk-management lock in validation functions directly.

…

On November 6, 2017 8:04:20 PM EST, "João Barbosa" ***@***.***> wrote: @TheBlueMatt `PruneAndFlush` changes `fCheckForPruning` without the lock. With: ```diff @@ -2012,6 +2012,7 @@ void FlushStateToDisk() { } void PruneAndFlush() { + AssertLockHeld(cs_LastBlockFile); CValidationState state; fCheckForPruning = true; const CChainParams& chainparams = Params(); ``` The following fails: ``` ./test/functional/pruning.py 2017-11-07 01:02:25.034000 TestFramework (INFO): Initializing test directory /var/folders/1v/8_69hby54nj2k3n6fywt44x80000gn/T/testgwrg340t Assertion failed: lock cs_LastBlockFile not held in validation.cpp:2015; locks held: ``` Note `./configure CPPFLAGS=-DDEBUG_LOCKORDER`. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #11617 (comment)

practicalswift · 2017-11-08T10:11:02Z

We have one utACK from @TheBlueMatt. Anyone else willing to review? :-)

promag · 2017-11-08T13:26:50Z

Testing fCheckForPruning is not needed in AcceptBlock (which BTW asserts cs_main is held) since the check is done in FlushStateToDisk.

ACK 932dcd4.

practicalswift · 2017-11-21T19:11:42Z

Ready for merge? :-)

practicalswift · 2018-01-28T10:48:51Z

Any chance of this getting merged or should I close?

practicalswift · 2018-04-09T23:02:58Z

Any chance of getting this merged or re-reviewed?

sdaftuar · 2018-04-10T18:14:25Z

utACK. I'd prefer if we also added clearer documentation that FlushStateMode::NONE behaves in the way that we're relying on here (perhaps an extra comment in FlushStateToDisk?).

FlushStateToDisk(...) won't do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile.

practicalswift · 2018-04-11T10:47:13Z

@sdaftuar Good point! Documentation added. Please re-review :-)

sdaftuar · 2018-04-11T12:56:59Z

Thanks, re-utACK 0000d8f

…heckForPruning 0000d8f Document how FlushStateMode::NONE is handled (practicalswift) 2311c7c Call FlushStateToDisk(...) regardless of fCheckForPruning (practicalswift) Pull request description: FlushStateToDisk(...) won't do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile. Tree-SHA512: 89df06256f73503a74b9e26d580ce9ed09efaef347fae1ff6a5759a2993b0db52edd2fadb65694d27e579a5aed92127753bdf41b5bb1bd516e577fcf17f17999

…heckForPruning Summary: 0000d8f Document how FlushStateMode::NONE is handled (practicalswift) 2311c7c Call FlushStateToDisk(...) regardless of fCheckForPruning (practicalswift) Pull request description: FlushStateToDisk(...) won't do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile. Tree-SHA512: 89df06256f73503a74b9e26d580ce9ed09efaef347fae1ff6a5759a2993b0db52edd2fadb65694d27e579a5aed92127753bdf41b5bb1bd516e577fcf17f17999 Backport of Core PR11617 bitcoin/bitcoin#11617 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3844

…heckForPruning Summary: 0000d8f Document how FlushStateMode::NONE is handled (practicalswift) 2311c7c Call FlushStateToDisk(...) regardless of fCheckForPruning (practicalswift) Pull request description: FlushStateToDisk(...) won't do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile. Tree-SHA512: 89df06256f73503a74b9e26d580ce9ed09efaef347fae1ff6a5759a2993b0db52edd2fadb65694d27e579a5aed92127753bdf41b5bb1bd516e577fcf17f17999 Backport of Core PR11617 bitcoin/bitcoin#11617 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3844

…s of fCheckForPruning 0000d8f Document how FlushStateMode::NONE is handled (practicalswift) 2311c7c Call FlushStateToDisk(...) regardless of fCheckForPruning (practicalswift) Pull request description: FlushStateToDisk(...) won't do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile. Tree-SHA512: 89df06256f73503a74b9e26d580ce9ed09efaef347fae1ff6a5759a2993b0db52edd2fadb65694d27e579a5aed92127753bdf41b5bb1bd516e577fcf17f17999

FlushStateToDisk(...) won't do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile. zcash: cherry picked from commit 2311c7c zcash: bitcoin/bitcoin#11617

zcash: cherry picked from commit 0000d8f zcash: bitcoin/bitcoin#11617

FlushStateToDisk(...) won't do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile. zcash: cherry picked from commit 2311c7c zcash: bitcoin/bitcoin#11617

zcash: cherry picked from commit 0000d8f zcash: bitcoin/bitcoin#11617

…s of fCheckForPruning 0000d8f Document how FlushStateMode::NONE is handled (practicalswift) 2311c7c Call FlushStateToDisk(...) regardless of fCheckForPruning (practicalswift) Pull request description: FlushStateToDisk(...) won't do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile. Tree-SHA512: 89df06256f73503a74b9e26d580ce9ed09efaef347fae1ff6a5759a2993b0db52edd2fadb65694d27e579a5aed92127753bdf41b5bb1bd516e577fcf17f17999

fanquake added the Validation label Nov 6, 2017

practicalswift changed the title ~~Add missing cs_LastBlockFile lock in AcceptBlock~~ validation: Add missing cs_LastBlockFile lock in AcceptBlock Nov 6, 2017

practicalswift force-pushed the cs_LastBlockFile-fCheckForPruning branch from d982e13 to 60151f9 Compare November 6, 2017 15:27

practicalswift changed the title ~~validation: Add missing cs_LastBlockFile lock in AcceptBlock~~ validation: Add missing cs_LastBlockFile locks Nov 6, 2017

practicalswift force-pushed the cs_LastBlockFile-fCheckForPruning branch from 60151f9 to f151609 Compare November 6, 2017 15:44

practicalswift force-pushed the cs_LastBlockFile-fCheckForPruning branch from f151609 to cc0d685 Compare November 6, 2017 16:21

TheBlueMatt reviewed Nov 6, 2017

View reviewed changes

practicalswift force-pushed the cs_LastBlockFile-fCheckForPruning branch from cc0d685 to 96ff97c Compare November 6, 2017 20:33

practicalswift changed the title ~~validation: Add missing cs_LastBlockFile locks~~ Call FlushStateToDisk(...) regardless of fCheckForPruning Nov 6, 2017

TheBlueMatt reviewed Nov 6, 2017

View reviewed changes

practicalswift force-pushed the cs_LastBlockFile-fCheckForPruning branch from 96ff97c to 932dcd4 Compare November 6, 2017 22:16

practicalswift changed the title ~~Call FlushStateToDisk(...) regardless of fCheckForPruning~~ Avoid (previously missing) lock: Call FlushStateToDisk(...) regardless of fCheckForPruning Apr 9, 2018

practicalswift changed the title ~~Avoid (previously missing) lock: Call FlushStateToDisk(...) regardless of fCheckForPruning~~ Avoid lock: Call FlushStateToDisk(...) regardless of fCheckForPruning Apr 9, 2018

practicalswift added 2 commits April 11, 2018 12:40

Call FlushStateToDisk(...) regardless of fCheckForPruning

2311c7c

FlushStateToDisk(...) won't do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile.

Document how FlushStateMode::NONE is handled

0000d8f

practicalswift force-pushed the cs_LastBlockFile-fCheckForPruning branch from 18271a7 to 0000d8f Compare April 11, 2018 10:46

laanwj merged commit 0000d8f into bitcoin:master Apr 11, 2018

practicalswift mentioned this pull request Apr 30, 2018

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

Closed

30 tasks

LarryRuane mentioned this pull request Mar 23, 2021

Bitcoin 0.17 locking PRs zcash/zcash#5034

Open

practicalswift deleted the cs_LastBlockFile-fCheckForPruning branch April 10, 2021 19:34

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 26, 2021

Document how FlushStateMode::NONE is handled

b9b6303

zcash: cherry picked from commit 0000d8f zcash: bitcoin/bitcoin#11617

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request May 27, 2021

Document how FlushStateMode::NONE is handled

a78deb8

zcash: cherry picked from commit 0000d8f zcash: bitcoin/bitcoin#11617

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

Avoid lock: Call FlushStateToDisk(...) regardless of fCheckForPruning #11617

Avoid lock: Call FlushStateToDisk(...) regardless of fCheckForPruning #11617

Uh oh!

Conversation

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

Uh oh!

Uh oh!

promag commented Nov 6, 2017

Uh oh!

practicalswift commented Nov 6, 2017

Uh oh!

promag commented Nov 6, 2017

Uh oh!

practicalswift 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!

TheBlueMatt Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Nov 6, 2017

Uh oh!

promag commented Nov 6, 2017

Uh oh!

TheBlueMatt Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 6, 2017

Uh oh!

practicalswift commented Nov 6, 2017

Uh oh!

TheBlueMatt commented Nov 6, 2017

Uh oh!

promag commented Nov 7, 2017

Uh oh!

TheBlueMatt commented Nov 7, 2017 via email

Uh oh!

promag commented Nov 7, 2017

Uh oh!

promag commented Nov 7, 2017

Uh oh!

TheBlueMatt commented Nov 7, 2017 via email

Uh oh!

practicalswift commented Nov 8, 2017

Uh oh!

promag commented Nov 8, 2017

Uh oh!

practicalswift commented Nov 21, 2017

Uh oh!

practicalswift commented Jan 28, 2018

Uh oh!

practicalswift commented Apr 9, 2018

Uh oh!

sdaftuar commented Apr 10, 2018

Uh oh!

practicalswift commented Apr 11, 2018

Uh oh!

sdaftuar commented Apr 11, 2018

Uh oh!

Uh oh!

practicalswift commented Nov 6, 2017 •

edited by maflcko

Loading

practicalswift commented Nov 6, 2017 •

edited

Loading