validation: Guard chainman chainstates with cs

dongcarl commented at 6:31 pm on January 28, 2021: member

 0This avoids a potential race-condition where a thread is reading the
 1ChainstateManager::m_active_chainstate pointer while another one is
 2writing to it. There is no portable guarantee that reading/writing the
 3pointer is thread-safe.
 4
 5This is also done in way that mimics ::ChainstateActive(), so the
 6transition from that function to this method is easy.
 7
 8More discussion:
 91. [#20749 (review)](/bitcoin-bitcoin/20749/#discussion_r559544027)
102. [#19806 (review)](/bitcoin-bitcoin/19806/#discussion_r561023961)
113. [#19806 (comment)](/bitcoin-bitcoin/19806/#issuecomment-768946522)
124. [#19806 (comment)](/bitcoin-bitcoin/19806/#issuecomment-768955695)

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

Is multiplied (but still unshot) in the chainman deglobalization PRs (#20158)
Is shot in the test framework in the au.activate PR (#19806)

dongcarl force-pushed on Jan 28, 2021

in src/validation.cpp:5217 in 5b8118e0fd outdated

5196 }
5197 
5198 bool ChainstateManager::IsSnapshotActive() const
5199 {
5200-    return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get();
5201+    return m_snapshot_chainstate && WITH_LOCK(::cs_main, return m_active_chainstate) == m_snapshot_chainstate.get();

jnewbery commented at 7:14 pm on January 28, 2021:

This seems pretty difficult to read. I think just locking cs_main at the top of the function, or asserting that it’s held and making the callers lock cs_main fine would be more readable:

0    LOCK(::cs_main);
1    return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get();

dongcarl commented at 8:49 pm on January 28, 2021:

Done!

dongcarl force-pushed on Jan 28, 2021

in src/validation.cpp:5162 in f92dc6557a outdated

5142@@ -5143,6 +5143,7 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
5143 }
5144 
5145 Optional<uint256> ChainstateManager::SnapshotBlockhash() const {
5146+    LOCK(::cs_main);  // for m_active_chainstate access

jnewbery commented at 7:15 pm on January 28, 2021:

I don’t find these // for m_active_chainstate access comments very useful. This function is only accessing m_active_chainstate so it’s pretty obvious what the lock is for.

dongcarl commented at 8:49 pm on January 28, 2021:

Removed!

in src/validation.cpp:5253 in f92dc6557a outdated

5227@@ -5226,7 +5228,10 @@ void ChainstateManager::Reset()
5228 {
5229     m_ibd_chainstate.reset();
5230     m_snapshot_chainstate.reset();
5231-    m_active_chainstate = nullptr;
5232+    {
5233+        LOCK(::cs_main);

jnewbery commented at 7:18 pm on January 28, 2021:

Again, I think it’s fine to hold this lock for the entirety of this function (in fact, the other ChainstateManager data members should be guarded by the same mutex).

dongcarl commented at 8:50 pm on January 28, 2021:

Done! Not 100% sure about the other members, we could do those in the future.

jnewbery commented at 7:19 pm on January 28, 2021: member

ACK f92dc6557a153b390a1ae1d0808ff7ed5d02c66e

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

DrahtBot added the label Validation on Jan 28, 2021

dongcarl force-pushed on Jan 28, 2021

laanwj commented at 10:17 pm on January 28, 2021: member

Code review ACK e6eb8d2bbbd32e5741c1dc1ef4c9cec7aac16b34

DrahtBot commented at 1:46 am on January 29, 2021: member

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

Conflicts

No conflicts as of last run.

practicalswift commented at 7:51 am on January 29, 2021: contributor

cr ACK e6eb8d2bbbd32e5741c1dc1ef4c9cec7aac16b34: patch looks correct

MarcoFalke commented at 8:10 am on January 29, 2021: member

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

jnewbery commented at 10:37 am on February 1, 2021: member

Code review ACK e6eb8d2bbb

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 commented at 12:20 pm on February 1, 2021: member

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

DrahtBot added the label Needs rebase on Feb 1, 2021

dongcarl force-pushed on Feb 1, 2021

dongcarl renamed this:
~~validation: Guard the active_chainstate with cs_main~~
validation: Guard chainman chainstates with cs_main
on Feb 1, 2021

dongcarl commented at 5:13 pm on February 1, 2021: member

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

DrahtBot removed the label Needs rebase on Feb 1, 2021

validation: Guard all chainstates with cs_main

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.

20677ffa22

dongcarl force-pushed on Feb 2, 2021

dongcarl commented at 3:09 am on February 2, 2021: member

Pushed 24fcc4cb00 -> 20677ffa22

Rebased over master for #21051

jnewbery commented at 9:09 am on February 2, 2021: member

code review ACK 20677ffa22e93e7408daadbd15d433f1e42faa86. 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 f92dc6557a153b390a1ae1d0808ff7ed5d02c66e 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 approved

ryanofsky commented at 8:17 pm on February 2, 2021: member

Code review ACK 20677ffa22e93e7408daadbd15d433f1e42faa86. 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 f92dc6557a153b390a1ae1d0808ff7ed5d02c66e 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 f92dc6557a153b390a1ae1d0808ff7ed5d02c66e and 20677ffa22e93e7408daadbd15d433f1e42faa86 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.

MarcoFalke merged this on Feb 4, 2021

MarcoFalke closed this on Feb 4, 2021

sidhujag referenced this in commit 203242fcfc on Feb 4, 2021

Fabcien referenced this in commit 0752566336 on Jul 21, 2022

DrahtBot locked this on Aug 16, 2022

validation: Guard chainman chainstates with cs_main #21025

Conflicts