highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-17 01:58:57 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#35498: net: move cs_main up in FetchBlock to fix rpc assert crash

Browse Source 
359680b74d net: move cs_main up in FetchBlock to fix rpc assert crash (Eugene Siegel)

Pull request description:

  A benign, racy assert can fail when calling FetchBlock and the peer is being cleaned up.

  1. FetchBlock runs in a http worker thread. It acquires a PeerRef, locks cs_main, then may later call BlockRequested which asserts that CNodeState exists for the peer.

  2. FinalizeNode may run in either the bitcoind or b-net threads. It locks cs_main, fetches a PeerRef from RemovePeer, fetches a CNodeState, and later removes it from m_node_states.

  Because of the lock placement in FetchBlock, the http worker thread in 1) can acquire a valid PeerRef and block while the b-net thread in 2) is cleaning up the peer in FinalizeNode. When the worker thread later acquires cs_main, it may crash in BlockRequested since no CNodeState exists. Fix this by acquiring the lock earlier in FetchBlock.

  I tested the assert can be hit and the fix works by adding sleeps. Was introduced in https://github.com/bitcoin/bitcoin/pull/25514 which moved the lock down.

ACKs for top commit:
  maflcko:
    lgtm  ACK 359680b74d
  dergoegge:
    utACK 359680b74d
  sedited:
    ACK 359680b74d

Tree-SHA512: dd29db28bc95c781b32b1cb6e782190fc8abd28bba36a5149fb35c86eaf56448e58a5c68a1c858a50348dd2c940e423942c67340a52e934818711932f538d708
This commit is contained in:
merge-script
2026-06-14 15:50:32 +02:00
parent debac5f2cd 359680b74d
commit 2cfb10b668
1 changed files with 6 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/net_processing.cpp
View File

@@ -1986,6 +1986,12 @@ util::Expected<void, std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, co
{
if (m_chainman.m_blockman.LoadingBlocks()) return util::Unexpected{"Loading blocks ..."};
// The lock must be taken here before fetching Peer so another thread does
// not delete the CNodeState from under the current thread, causing an
// assertion failure in BlockRequested. This lock can be replaced with a
// net-specific lock when more of CNodeState is moved into Peer.
LOCK(cs_main);
// Ensure this peer exists and hasn't been disconnected
PeerRef peer = GetPeerRef(peer_id);
if (peer == nullptr) return util::Unexpected{"Peer does not exist"};
@@ -1993,8 +1999,6 @@ util::Expected<void, std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, co
// Ignore pre-segwit peers
if (!CanServeWitnesses(*peer)) return util::Unexpected{"Pre-SegWit peer"};
LOCK(cs_main);
// Forget about all prior requests
RemoveBlockRequest(block_index.GetBlockHash(), std::nullopt);