foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-02 09:46:52 -05:00
Code Issues Activity

Merge #21025: validation: Guard chainman chainstates with cs_main

Browse source 
20677ffa22 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. 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 (#20158)
  - Is shot in the test framework in the au.activate PR (#19806)

ACKs for top commit:
  jnewbery:
    code review ACK 20677ffa22. I've verified by eye that neither of these members are accessed without cs_main.
  ryanofsky:
    Code review ACK 20677ffa22. 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 f92dc6557a 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.

Tree-SHA512: 68a3a46d79a407b774eab77e1d682a97e95f1672db0a5fcb877572e188bec09f3a7b47c5d0cc1f2769ea276896dcbe97cb35c861acf7d8e3e513e955dc773f89
This commit is contained in:
MarcoFalke 2021-02-04 10:21:38 +01:00
commit f1239b70d1
No known key found for this signature in database
GPG key ID: D2EA4850E7528B25
2 changed files with 10 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
src/validation.cpp
View file

@ -5159,7 +5159,7 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
}
Optional<uint256> ChainstateManager::SnapshotBlockhash() const {
LOCK(::cs_main); // for m_active_chainstate access
LOCK(::cs_main);
if (m_active_chainstate != nullptr) {
// If a snapshot chainstate exists, it will always be our active.
return m_active_chainstate->m_from_snapshot_blockhash;
@ -5169,6 +5169,7 @@ Optional<uint256> ChainstateManager::SnapshotBlockhash() const {
std::vector<CChainState*> ChainstateManager::GetAll()
{
LOCK(::cs_main);
std::vector<CChainState*> out;
if (!IsSnapshotValidated() && m_ibd_chainstate) {
@ -5213,11 +5214,13 @@ CChainState& ChainstateManager::ActiveChainstate() const
bool ChainstateManager::IsSnapshotActive() const
{
return m_snapshot_chainstate && WITH_LOCK(::cs_main, return m_active_chainstate) == m_snapshot_chainstate.get();
LOCK(::cs_main);
return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get();
}
CChainState& ChainstateManager::ValidatedChainstate() const
{
LOCK(::cs_main);
if (m_snapshot_chainstate && IsSnapshotValidated()) {
return *m_snapshot_chainstate.get();
}
@ -5227,6 +5230,7 @@ CChainState& ChainstateManager::ValidatedChainstate() const
bool ChainstateManager::IsBackgroundIBD(CChainState* chainstate) const
{
LOCK(::cs_main);
return (m_snapshot_chainstate && chainstate == m_ibd_chainstate.get());
}
@ -5242,12 +5246,10 @@ void ChainstateManager::Unload()
void ChainstateManager::Reset()
{
LOCK(::cs_main);
m_ibd_chainstate.reset();
m_snapshot_chainstate.reset();
{
LOCK(::cs_main);
m_active_chainstate = nullptr;
}
m_active_chainstate = nullptr;
m_snapshot_validated = false;
}

4
src/validation.h
View file

@ -802,7 +802,7 @@ private:
//! This is especially important when, e.g., calling ActivateBestChain()
//! on all chainstates because we are not able to hold ::cs_main going into
//! that call.
std::unique_ptr<CChainState> m_ibd_chainstate;
std::unique_ptr<CChainState> m_ibd_chainstate GUARDED_BY(::cs_main);
//! A chainstate initialized on the basis of a UTXO snapshot. If this is
//! non-null, it is always our active chainstate.
@ -815,7 +815,7 @@ private:
//! This is especially important when, e.g., calling ActivateBestChain()
//! on all chainstates because we are not able to hold ::cs_main going into
//! that call.
std::unique_ptr<CChainState> m_snapshot_chainstate;
std::unique_ptr<CChainState> m_snapshot_chainstate GUARDED_BY(::cs_main);
//! Points to either the ibd or snapshot chainstate; indicates our
//! most-work chain.