Skip to content

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Sep 5, 2021

Commit ccd8ef6 "Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock" in #11281 moved the cs_main lock from caller to ReadBlockFromDisk() for calling CBlockIndex::GetBlockPos(), but the second invocation doesn't have the lock, and IIUC there is no guarantee the compiler can know if state has changed.

Use the blockPos local variable instead, rename it to block_pos, and make it const.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-review ACK 350e034

Good catch!

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 350e034.

@maflcko
Copy link
Member

maflcko commented Sep 7, 2021

Shouldn't there be a lock annotation to prevent this in the future?

@jonatack
Copy link
Member Author

jonatack commented Sep 9, 2021

Shouldn't there be a lock annotation to prevent this in the future?

Proposed in #22932.

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #9245 (Drop IO priority to idle while reading blocks for peer requests and startup verification by luke-jr)

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.

@laanwj
Copy link
Member

laanwj commented Sep 16, 2021

Code review ACK 350e034

@laanwj laanwj merged commit cdf12c7 into bitcoin:master Sep 16, 2021
@jonatack jonatack deleted the ReadBlockFromDisk-block_pos branch September 16, 2021 15:03
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 20, 2021
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 21, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021
laanwj added a commit that referenced this pull request Jan 27, 2022
…DataPos/nUndoPos by cs_main

6ea5682 Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main (Jon Atack)
5d59ae0 Remove/inline ReadRawBlockFromDisk(block_data, pindex, message_start) (Hennadii Stepanov)
eaeeb88 Require IsBlockPruned() to hold mutex cs_main (Jon Atack)
ca47b00 Require CBlockIndex::IsValid() to hold cs_main (Vasil Dimov)
e9f3aa5 Require CBlockIndex::RaiseValidity() to hold cs_main (Vasil Dimov)
8ef457c Require CBlockIndex::IsAssumedValid() to hold cs_main (Vasil Dimov)
5723934 Require CBlockIndex::GetUndoPos() to hold mutex cs_main (Jon Atack)
2e557ce Require WriteUndoDataForBlock() to hold mutex cs_main (Jon Atack)
6fd4341 Require CBlockIndex::GetBlockPos() to hold mutex cs_main (Jon Atack)

Pull request description:

  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 #22895. Doing this also enables the next step:

  - `CBlockIndex::nStatus` may be racy, i.e. potentially accessed by multiple threads, see #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
      - https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
      - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes
      - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

  Mitigates/potentially closes #17161.

ACKs for top commit:
  laanwj:
    Code review ACK 6ea5682

Tree-SHA512: 3ebf429c8623c51f944a7245a2e48d2aa088dec4c4914b40aa6049e89856c1ee8586f6e2e3b65195190566637a33004468b51a781e61a082248748015167569b
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 14, 2022
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 15, 2022
fanquake added a commit that referenced this pull request Mar 1, 2022
269553f test: Call ceildiv helper with integer (Martin Zumsande)
2f60fc6 ci: Replace soon EOL hirsute with jammy (MarcoFalke)
801b0f0 build: patch qt to explicitly define previously implicit header include (Kittywhiskers Van Gogh)
c768bfa tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
f66bc42 tests: Test for assertion when feerate is rounded down (Andrew Chow)
bd7e08e fees: Always round up fee calculated from a feerate (Andrew Chow)
227ae65 wallet: fix segfault by avoiding invalid default-ctored `external_spk_managers` entry (Sebastian Falbesoner)
282863a refactor: include a missing <limits> header in fs.cpp (Joan Karadimov)
7febe4f consensus: don't call GetBlockPos in ReadBlockFromDisk without lock (Jon Atack)
c671c6f the result of CWallet::IsHDEnabled() was initialized with true. (Saibato)
a5a1538 build, qt: Fix typo in QtInputSupport check (Hennadii Stepanov)
c95b188 system: skip trying to set the locale on NetBSD (fanquake)
c1cdedd guix: Fix powerpc64(le) dynamic linker name (Carl Dong)
92d44ff doc: Add 23061 release notes (MarcoFalke)
db76db7 Fix (inverse) meaning of -persistmempool (MarcoFalke)
85c78e0 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan)

Pull request description:

  Collecting backports for the 22.1 release. Currently:
  * #23045
  * #23061
  * #23148
  * #22390
  * #22820
  * #22781
  * #22895
  * #23335
  * #23333
  * #22949
  * #23580
  * #23504
  * #24239

ACKs for top commit:
  achow101:
    ACK 269553f

Tree-SHA512: b3a57ea241be7a83488eeb032276f4cf82a0987aada906a82f94a20c4acf9f2397708249dcecbe1c7575e70d09c60b835233d4718af4013c7bc58896c618274c
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants