Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main #22932

jonatack · 2021-09-09T16:00:41Z

Issues:

CBlockIndex member functions GetBlockPos(), GetUndoPos(), IsAssumedValid(), RaiseValidity(), and IsValid() and block storage functions WriteUndoDataForBlock() and IsBlockPruned() are missing thread safety lock annotations to help ensure that they are called with mutex cs_main to avoid bugs like consensus: don't call GetBlockPos in ReadBlockFromDisk without cs_main lock #22895. Doing this also enables the next step:
CBlockIndex::nStatus may be racy, i.e. potentially accessed by multiple threads, see CBlockIndex::nStatus race when pruning #17161. A solution is to guard it by cs_main, along with fellow data members nFile, nDataPos and nUndoPos.

This pull:

adds thread safety lock annotations for the functions listed above
guards CBlockIndex::nStatus, nFile, nDataPos and nUndoPos by cs_main

How to review and test:

debug build with clang and verify there are no -Wthread-safety-analysis warnings
review the code to verify each annotation or lock is necessary and sensible, or if any are missing
look for whether taking a lock can be replaced by a lock annotation instead
for more information about Clang thread safety analysis, see

Mitigates/potentially closes #17161.

DrahtBot · 2021-09-12T16:00:46Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23927 (rpc: Pruning nodes can not fetch blocks before syncing past their height by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonatack · 2021-09-16T20:04:00Z

Rebased following the merge of #22895 that this was based on.

maflcko

Concept ACK. Feel free to ignore the meta comment.

maflcko · 2021-09-17T10:17:17Z

src/chain.h

@@ -212,7 +215,8 @@ class CBlockIndex
    {
    }

-    FlatFilePos GetBlockPos() const {
+    FlatFilePos GetBlockPos() const EXCLUSIVE_LOCKS_REQUIRED(cs_main)


This is annotating the function, but not the underlying data member that needs the annotation.

Not sure how involved it will be to annotate everything properly here. Also, given that cs_main is the validation lock, we should (eventually/ideally) have a separate node_storage lock. Though, this again requires a major validation/node_storage rewrite.

However an argument against adding the lock annotation to the data member could be that chain is supposed to be a data-structure-only primitive, like transaction or block. In that case only annotating the getter and setter functions (and making them standalone) would seem appropriate.

Having a quick look at annotating nStatus to assess.

Appended a small commit to guard CBlockIndex::nStatus by cs_main as a starting point.

src/test/util/blockfilter.cpp

src/wallet/test/wallet_tests.cpp

jonatack · 2021-09-17T16:22:22Z

Thanks @MarcoFalke, updated with your suggestions per git diff f6c2fdb e950f0e

diff

diff --git a/src/test/util/blockfilter.cpp b/src/test/util/blockfilter.cpp
index 311913b5db..1a663bd0b3 100644
--- a/src/test/util/blockfilter.cpp
+++ b/src/test/util/blockfilter.cpp
@@ -11,8 +11,10 @@
 
 bool ComputeFilter(BlockFilterType filter_type, const CBlockIndex* block_index, BlockFilter& filter)
 {
+    LOCK(cs_main);
+
     CBlock block;
-    if (!ReadBlockFromDisk(block, WITH_LOCK(cs_main, return block_index->GetBlockPos()), Params().GetConsensus())) {
+    if (!ReadBlockFromDisk(block, block_index->GetBlockPos(), Params().GetConsensus())) {
         return false;
     }
 
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 37fd52a5e7..4ac8936ee7 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -86,8 +86,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
 {
     // Cap last block file size, and mine new block in a new block file.
     CBlockIndex* oldTip = m_node.chainman->ActiveChain().Tip();
-    const auto pos{WITH_LOCK(cs_main, return oldTip->GetBlockPos())};
-    GetBlockFileInfo(pos.nFile)->nSize = MAX_BLOCKFILE_SIZE;
+    {
+        LOCK(cs_main);
+        GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE;
+    }
     CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
     CBlockIndex* newTip = m_node.chainman->ActiveChain().Tip();

jonatack · 2021-09-23T21:29:53Z

Rebased after the merge of #21526.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

jonatack · 2022-01-25T20:06:33Z

Rebased.

This has concept ACKs by @MarcoFalke and @promag and ACKs by @hebasto, @achow101 and @vasild.

Review/re-ACKs welcome.

jamesob · 2022-01-26T01:50:39Z

Benched 6ea5682; no performance impact 👍.

bench name	command
ibd.local.range.500000.540000	`bitcoind -dbcache=10000 -debug=coindb -debug=bench -listen=0 -connect=0 -addnode=127.0.0.1:8888 -prune=9999999 -printtoconsole=0 -assumevalid=000000000000000000176c192f42ad13ab159fdb20198b87e7ba3c001e47b876`

#22932 vs. $mergebase (absolute)

bench name	x	#22932	$mergebase
ibd.local.range.500000.540000.total_secs	2	3093.6633 (± 17.1804)	3102.2931 (± 0.4422)
ibd.local.range.500000.540000.peak_rss_KiB	2	`6355854`.0000 (± 1646.0000)	`6355764`.0000 (± 1492.0000)
ibd.local.range.500000.540000.cpu_kernel_secs	2	263.2300 (± 3.3200)	264.0900 (± 0.1700)
ibd.local.range.500000.540000.cpu_user_secs	2	13020.2400 (± 4.7400)	13015.7250 (± 8.2850)

#22932 vs. $mergebase (relative)

bench name	x	#22932	$mergebase
ibd.local.range.500000.540000.total_secs	2	1.000	1.003
ibd.local.range.500000.540000.peak_rss_KiB	2	1.000	1.000
ibd.local.range.500000.540000.cpu_kernel_secs	2	1.000	1.003
ibd.local.range.500000.540000.cpu_user_secs	2	1.000	1.000

laanwj · 2022-01-27T09:53:01Z

Code review ACK 6ea5682

vasild

ACK 6ea5682

maflcko · 2022-01-28T13:14:58Z

src/chain.h

@@ -382,6 +382,7 @@ class CDiskBlockIndex : public CBlockIndex

    SERIALIZE_METHODS(CDiskBlockIndex, obj)
    {
+        LOCK(::cs_main);


can you explain why this is needed?

CDiskBlockIndex creates a copy of the data and it isn't used in validation or blockstorage code. It is only used to serialize a struct and then discarded.

can you explain why this is needed?

CDiskBlockIndex creates a copy of the data and it isn't used in validation or blockstorage code. It is only used to serialize a struct and then discarded.

Good point. Opened #24199 as an approach to address this.

maflcko · 2022-01-28T13:18:30Z

src/txdb.cpp

                pindexNew->nTx            = diskindex.nTx;
+                {
+                    LOCK(::cs_main);


Can you explain why this is needed? The calling function should already hold cs_main?

Good catch--we can drop the lock in favor of an annotation; done in #24197.

…nFile/n…

…eDB::LoadBlockIndexGuts() 20276ca Replace lock with thread safety annotation in CBlockTreeDB::LoadBlockIndexGuts() (Jon Atack) Pull request description: Following up on #22932 (comment) by Marco Falke (good observation, thank you), we can replace a cs_main lock in `CBlockTreeDB::LoadBlockIndexGuts()` with a Clang thread safety annotation/assertion instead. The unlocked code is reverted to its original state before #22932. ACKs for top commit: hebasto: ACK 20276ca, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 2d91d1c962af0286d835d92a56396a27ea00e9d061b869eaff9421b448a083b0513828e1d4df7504396896057bf1e344e180a50271a5cfd1e1c7b6527155b2bb

…lockTreeDB::LoadBlockIndexGuts() 20276ca Replace lock with thread safety annotation in CBlockTreeDB::LoadBlockIndexGuts() (Jon Atack) Pull request description: Following up on bitcoin#22932 (comment) by Marco Falke (good observation, thank you), we can replace a cs_main lock in `CBlockTreeDB::LoadBlockIndexGuts()` with a Clang thread safety annotation/assertion instead. The unlocked code is reverted to its original state before bitcoin#22932. ACKs for top commit: hebasto: ACK 20276ca, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 2d91d1c962af0286d835d92a56396a27ea00e9d061b869eaff9421b448a083b0513828e1d4df7504396896057bf1e344e180a50271a5cfd1e1c7b6527155b2bb

Summary: > consensus: don't call GetBlockPos in ReadBlockFromDisk without lock The `cs_main` lock only covered the first invocation of `CBlockIndex::GetBlockPos()`. Use the `blockPos` local variable instead of calling the function twice, rename it to `block_pos`, and make it const This is a backport of [[bitcoin/bitcoin#22895 | core#22895]] ---- > Require CBlockIndex::GetBlockPos() to hold mutex cs_main This is a partial backport of [[bitcoin/bitcoin#22932 | core#22932]] bitcoin/bitcoin@6fd4341 Test Plan: With clang and DEBUG: `ninja all check-all bitcoin-fuzzers` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13031

Summary: Mutex cs_main is already held by the caller of WriteUndoDataForBlock(). This change is needed to require CBlockIndex::GetUndoPos() to hold cs_main and CBlockIndex::nStatus to be guarded by cs_main in the following commits without adding 2 unnecessary cs_main locks to WriteUndoDataForBlock(). This is a partial backport of [[bitcoin/bitcoin#22932 | core#22932]] bitcoin/bitcoin@2e557ce Depends on D13031 Test Plan: With clang and DEBUG: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13032

Summary: This is a partial backport of [[bitcoin/bitcoin#22932 | core#22932]] bitcoin/bitcoin@5723934 Depends on D13032 Test Plan: With clang and DEBUG: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13033

Summary: This is a partial backport of [[bitcoin/bitcoin#22932 | core#22932]] bitcoin/bitcoin@e9f3aa5 Depends on D13033 Test Plan: With clang and DEBUG: `ninja all check-all bitcoin-fuzzers` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13034

Summary: This is a partial backport of [[bitcoin/bitcoin#22932 | core#22932]] bitcoin/bitcoin@8ef457c Depends on D13034 Test Plan: With clang and DEBUG: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13035

Summary: This is a partial backport of [[bitcoin/bitcoin#22932 | core#22932]] bitcoin/bitcoin@ca47b00 Depends on D13035 Test Plan: With clang and DEBUG: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13036

Summary: Co-authored-by: Vasil Dimov <vd@FreeBSD.org> This is a partial backport of [[bitcoin/bitcoin#22932 | core#22932]] bitcoin/bitcoin@eaeeb88 Depends on D13036 Test Plan: With clang and DEBUG: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D13037

Summary: > CBlockIndex::nStatus may be racy, i.e. potentially accessed by multiple threads, see [[bitcoin/bitcoin#17161 | core#17161]]. A solution is to guard it by cs_main, along with fellow data members nFile, nDataPos and nUndoPos. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> This concludes backport of [[bitcoin/bitcoin#22932 | core#22932]] and [[bitcoin/bitcoin#24197 | core#24197]] bitcoin/bitcoin@6ea5682 bitcoin/bitcoin@20276ca Depends on D13037 Test Plan: With clang and DEBUG: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D13038

jonatack mentioned this pull request Sep 9, 2021

consensus: don't call GetBlockPos in ReadBlockFromDisk without cs_main lock #22895

Merged

jonatack force-pushed the require-GetBlockPos-to-hold-cs_main branch from c97282d to a2571de Compare September 9, 2021 16:20

DrahtBot added Block storage Consensus UTXO Db and Indexes Validation Wallet labels Sep 9, 2021

jonatack force-pushed the require-GetBlockPos-to-hold-cs_main branch from a2571de to 845de73 Compare September 9, 2021 18:01

fanquake removed Wallet UTXO Db and Indexes Validation Consensus labels Sep 10, 2021

This was referenced Sep 13, 2021

Drop IO priority to idle while reading blocks for peer requests and startup verification #9245

Closed

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

jonatack force-pushed the require-GetBlockPos-to-hold-cs_main branch from 845de73 to f6c2fdb Compare September 16, 2021 15:27

maflcko reviewed Sep 17, 2021

View reviewed changes

src/test/util/blockfilter.cpp Outdated Show resolved Hide resolved

maflcko reviewed Sep 17, 2021

View reviewed changes

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the require-GetBlockPos-to-hold-cs_main branch from f6c2fdb to e950f0e Compare September 17, 2021 16:20

jonatack force-pushed the require-GetBlockPos-to-hold-cs_main branch from 285ecb2 to 0104df0 Compare September 17, 2021 18:10

DrahtBot mentioned this pull request Sep 18, 2021

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

Merged

DrahtBot added the Needs rebase label Sep 23, 2021

jonatack force-pushed the require-GetBlockPos-to-hold-cs_main branch from 0104df0 to a716569 Compare September 23, 2021 21:28

DrahtBot removed the Needs rebase label Sep 23, 2021

jonatack force-pushed the require-GetBlockPos-to-hold-cs_main branch from a716569 to 9cacc50 Compare September 24, 2021 11:29

jonatack changed the title ~~consensus: require CBlockIndex::GetBlockPos() to hold mutex cs_main~~ Guard CBlockIndex::nStatus by cs_main, require GetBlockPos/GetUndoPos to hold cs_main Oct 6, 2021

hebasto and others added 2 commits January 25, 2022 20:43

Remove/inline ReadRawBlockFromDisk(block_data, pindex, message_start)

5d59ae0

Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main

6ea5682

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

jonatack force-pushed the require-GetBlockPos-to-hold-cs_main branch from cf2145b to 6ea5682 Compare January 25, 2022 20:06

DrahtBot removed the Needs rebase label Jan 25, 2022

DrahtBot mentioned this pull request Jan 26, 2022

rpc: Pruning nodes can not fetch blocks before syncing past their height #23927

Merged

laanwj merged commit cf5bb04 into bitcoin:master Jan 27, 2022

jonatack deleted the require-GetBlockPos-to-hold-cs_main branch January 27, 2022 10:11

vasild reviewed Jan 27, 2022

View reviewed changes

maflcko reviewed Jan 28, 2022

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2022

Merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/…

0ac526b

…nFile/n…

This was referenced Jan 28, 2022

Replace lock with thread safety annotation in CBlockTreeDB::LoadBlockIndexGuts() #24197

Merged

Add cs_main annotation to WriteBatchSync(), drop lock in CDiskBlockIndex #24199

Closed

jonatack mentioned this pull request Jan 31, 2022

validation, refactor: add missing thread safety lock assertions #24177

Merged

ryanofsky mentioned this pull request Feb 1, 2022

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

LarryRuane mentioned this pull request Feb 8, 2022

Improve runtime performance of --reindex #16981

Merged

bitcoin locked and limited conversation to collaborators Jan 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main #22932

Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main #22932

jonatack commented Sep 9, 2021 •

edited

Loading

DrahtBot commented Sep 12, 2021 •

edited

Loading

jonatack commented Sep 16, 2021

maflcko left a comment

maflcko Sep 17, 2021

jonatack Sep 17, 2021

jonatack Sep 17, 2021 •

edited

Loading

jonatack commented Sep 17, 2021

jonatack commented Sep 23, 2021

jonatack commented Jan 25, 2022

jamesob commented Jan 26, 2022

laanwj commented Jan 27, 2022

vasild left a comment

maflcko Jan 28, 2022

jonatack Jan 29, 2022

maflcko Jan 28, 2022

jonatack Jan 28, 2022

Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main #22932

Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main #22932

Conversation

jonatack commented Sep 9, 2021 • edited Loading

DrahtBot commented Sep 12, 2021 • edited Loading

Conflicts

jonatack commented Sep 16, 2021

maflcko left a comment

Choose a reason for hiding this comment

maflcko Sep 17, 2021

Choose a reason for hiding this comment

jonatack Sep 17, 2021

Choose a reason for hiding this comment

jonatack Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

jonatack commented Sep 17, 2021

jonatack commented Sep 23, 2021

jonatack commented Jan 25, 2022

jamesob commented Jan 26, 2022

#22932 vs. $mergebase (absolute)

#22932 vs. $mergebase (relative)

laanwj commented Jan 27, 2022

vasild left a comment

Choose a reason for hiding this comment

maflcko Jan 28, 2022

Choose a reason for hiding this comment

jonatack Jan 29, 2022

Choose a reason for hiding this comment

maflcko Jan 28, 2022

Choose a reason for hiding this comment

jonatack Jan 28, 2022

Choose a reason for hiding this comment

jonatack commented Sep 9, 2021 •

edited

Loading

DrahtBot commented Sep 12, 2021 •

edited

Loading

jonatack Sep 17, 2021 •

edited

Loading