validation: Guard chainman chainstates with cs_main #21025

dongcarl · 2021-01-28T18:31:05Z

This avoids a potential race-condition where a thread is reading the
ChainstateManager::m_active_chainstate pointer while another one is
writing to it. There is no portable guarantee that reading/writing the
pointer is thread-safe.

This is also done in way that mimics ::ChainstateActive(), so the
transition from that function to this method is easy.

More discussion:
1. https://github.com/bitcoin/bitcoin/pull/20749#discussion_r559544027
2. https://github.com/bitcoin/bitcoin/pull/19806#discussion_r561023961
3. https://github.com/bitcoin/bitcoin/pull/19806#issuecomment-768946522
4. https://github.com/bitcoin/bitcoin/pull/19806#issuecomment-768955695

Basically this PR removes the loaded-but-unfired footgun, which:

Is multiplied (but still unshot) in the chainman deglobalization PRs (tree-wide: De-globalize ChainstateManager #20158)
Is shot in the test framework in the au.activate PR (validation: UTXO snapshot activation #19806)

jnewbery

ACK f92dc65

I've left a few style comments, but this seems fine.

src/validation.cpp

laanwj · 2021-01-28T22:17:19Z

Code review ACK e6eb8d2

DrahtBot · 2021-01-29T01:46:01Z

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

Conflicts

No conflicts as of last run.

practicalswift · 2021-01-29T07:51:54Z

cr ACK e6eb8d2: patch looks correct

maflcko · 2021-01-29T08:10:58Z

Any reason to only add the annotation to (1/3) of the m_*_chainstate? The others have the same requirement

jnewbery · 2021-02-01T10:37:21Z

Code review ACK e6eb8d2

Any reason to only add the annotation to (1/3) of the m_*_chainstate? The others have the same requirement

This resolves the blocker for #20749, but I agree that the other m_*_chainstates should be guarded by cs_main.

jnewbery · 2021-02-01T12:20:31Z

The older version was merged as part of #20749. Perhaps we should repurpose this PR to address the review comments (particularly #21025 (comment))?

dongcarl · 2021-02-01T17:13:20Z

As suggested by @jnewbery, this PR has been repurposed to address #21025 (comment)

Since these chainstates are: 1. Also vulnerable to the race condition described in the previous commit 2. Documented as having similar semantics as m_active_chainstate we should also protect them with ::cs_main.

dongcarl · 2021-02-02T03:09:50Z

Pushed 24fcc4c -> 20677ff

Rebased over master for Fix -Wmismatched-tags warnings #21051

jnewbery · 2021-02-02T09:09:03Z

code review ACK 20677ff. I've verified by eye that neither of these members are accessed without cs_main.

In the commit message, "Also vulnerable to the race condition described in the previous commit" no longer makes sense since f92dc65 is not the previous commit.

A couple of potential follow-ups:

it may be possible to initialize these fields in the initializer list and mark them const. That'd be even better than guarding them with cs_main, since it wouldn't need to be locked every time they're accessed.
perhaps InitializeChainstate() and MaybeRebalanceCaches() should have AssertLockHeld(cs_main) added. Those are the only two functions where these members are accessed and it's not immediately obvious that cs_main is already being held.

ryanofsky

Code review ACK 20677ff. It is safer to have these new GUARDED_BY annotations and locks than not to have them, but in the longer run I think every LOCK(cs_main) added here and added earlier in f92dc65 from #20749 should be removed and replaced with EXCLUSIVE_LOCKS_REQUIRED(cs_main) on the accessor methods instead. cs_main is a high level lock that should be explicitly acquired at a high level to prevent the chain state from changing. It shouldn't be acquired recursively in low-level methods just to read pointer values atomically.

After #19806 and its followups, the active chainstate pointer will be able to change at runtime, and code calling these accessor methods should care if the results they return are still up-to-date after returning. That code should be explicitly locking cs_main and making sure state is locked the right amount of time, not relying on recursive locking and potentially using outdated or inconsistent state pointers.

IIUC changing this now would make #19806 more difficult to rebase so is reasonable to delay. But after #19806 is done with, it would be good for all the new locks added in f92dc65 and
20677ff to be removed and replaced with lock required annotations, and for calling code like ProcessNewBlock to either lock or save pointers correctly and not rely on ActiveChainstate() and related methods return values to not change.

20677ff validation: Guard all chainstates with cs_main (Carl Dong) Pull request description: ``` This avoids a potential race-condition where a thread is reading the ChainstateManager::m_active_chainstate pointer while another one is writing to it. There is no portable guarantee that reading/writing the pointer is thread-safe. This is also done in way that mimics ::ChainstateActive(), so the transition from that function to this method is easy. More discussion: 1. bitcoin#20749 (comment) 2. bitcoin#19806 (comment) 3. bitcoin#19806 (comment) 4. bitcoin#19806 (comment) ``` Basically this PR removes the loaded-but-unfired footgun, which: - Is multiplied (but still unshot) in the chainman deglobalization PRs (bitcoin#20158) - Is shot in the test framework in the au.activate PR (bitcoin#19806) ACKs for top commit: jnewbery: code review ACK 20677ff. I've verified by eye that neither of these members are accessed without cs_main. ryanofsky: Code review ACK 20677ff. It is safer to have these new `GUARDED_BY` annotations and locks than not to have them, but in the longer run I think every `LOCK(cs_main)` added here and added earlier in f92dc65 from bitcoin#20749 should be removed and replaced with `EXCLUSIVE_LOCKS_REQUIRED(cs_main)` on the accessor methods instead. `cs_main` is a high level lock that should be explicitly acquired at a high level to prevent the chain state from changing. It shouldn't be acquired recursively in low-level methods just to read pointer values atomically. Tree-SHA512: 68a3a46d79a407b774eab77e1d682a97e95f1672db0a5fcb877572e188bec09f3a7b47c5d0cc1f2769ea276896dcbe97cb35c861acf7d8e3e513e955dc773f89

Summary: PR description: > This avoids a potential race-condition where a thread is reading the > ChainstateManager::m_active_chainstate pointer while another one is > writing to it. There is no portable guarantee that reading/writing the > pointer is thread-safe. This is a backport of [[bitcoin/bitcoin#21025 | core#21025]] Test Plan: With debug and tsan: `ninja check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11773

dongcarl force-pushed the 2021-01-chainman-activechainstate-locking branch from c1d4af7 to 5b8118e Compare January 28, 2021 18:50

dongcarl mentioned this pull request Jan 28, 2021

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

Merged

dongcarl force-pushed the 2021-01-chainman-activechainstate-locking branch from 5b8118e to f92dc65 Compare January 28, 2021 19:14

jnewbery reviewed Jan 28, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot added the Validation label Jan 28, 2021

dongcarl force-pushed the 2021-01-chainman-activechainstate-locking branch from f92dc65 to e6eb8d2 Compare January 28, 2021 20:48

DrahtBot mentioned this pull request Jan 29, 2021

validation: UTXO snapshot activation #19806

Merged

DrahtBot added the Needs rebase label Feb 1, 2021

dongcarl force-pushed the 2021-01-chainman-activechainstate-locking branch 3 times, most recently from 3f24a72 to 24fcc4c Compare February 1, 2021 16:47

dongcarl changed the title ~~validation: Guard the active_chainstate with cs_main~~ validation: Guard chainman chainstates with cs_main Feb 1, 2021

DrahtBot removed the Needs rebase label Feb 1, 2021

validation: Guard all chainstates with cs_main

20677ff

Since these chainstates are: 1. Also vulnerable to the race condition described in the previous commit 2. Documented as having similar semantics as m_active_chainstate we should also protect them with ::cs_main.

dongcarl force-pushed the 2021-01-chainman-activechainstate-locking branch from 24fcc4c to 20677ff Compare February 2, 2021 03:09

ryanofsky approved these changes Feb 2, 2021

View reviewed changes

maflcko merged commit f1239b7 into bitcoin:master Feb 4, 2021

ryanofsky mentioned this pull request Apr 13, 2021

ChainstateManager locking improvements #21620

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

validation: Guard chainman chainstates with cs_main #21025

validation: Guard chainman chainstates with cs_main #21025

Uh oh!

dongcarl commented Jan 28, 2021 •

edited

Loading

Uh oh!

jnewbery left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laanwj commented Jan 28, 2021

Uh oh!

DrahtBot commented Jan 29, 2021 •

edited

Loading

Uh oh!

practicalswift commented Jan 29, 2021

Uh oh!

maflcko commented Jan 29, 2021

Uh oh!

jnewbery commented Feb 1, 2021

Uh oh!

jnewbery commented Feb 1, 2021

Uh oh!

dongcarl commented Feb 1, 2021

Uh oh!

dongcarl commented Feb 2, 2021

Uh oh!

jnewbery commented Feb 2, 2021

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

validation: Guard chainman chainstates with cs_main #21025

validation: Guard chainman chainstates with cs_main #21025

Uh oh!

Conversation

dongcarl commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laanwj commented Jan 28, 2021

Uh oh!

DrahtBot commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Jan 29, 2021

Uh oh!

maflcko commented Jan 29, 2021

Uh oh!

jnewbery commented Feb 1, 2021

Uh oh!

jnewbery commented Feb 1, 2021

Uh oh!

dongcarl commented Feb 1, 2021

Uh oh!

dongcarl commented Feb 2, 2021

Uh oh!

jnewbery commented Feb 2, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongcarl commented Jan 28, 2021 •

edited

Loading

DrahtBot commented Jan 29, 2021 •

edited

Loading