validation: Guard chainman chainstates with cs_main #21025

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. [#20749 (review)](/bitcoin-bitcoin/20749/#discussion_r559544027)
2. [#19806 (review)](/bitcoin-bitcoin/19806/#discussion_r561023961)
3. [#19806 (comment)](/bitcoin-bitcoin/19806/#issuecomment-768946522)
4. [#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)

ACK f92dc6557a153b390a1ae1d0808ff7ed5d02c66e

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

Code review ACK e6eb8d2bbbd32e5741c1dc1ef4c9cec7aac16b34

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

cr ACK e6eb8d2bbbd32e5741c1dc1ef4c9cec7aac16b34: patch looks correct

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

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.

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

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

Pushed 24fcc4cb00 -> 20677ffa22

• Rebased over master for #21051

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.

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.