[2rdzy]

Summary

Backport of bitcoin#35498. FetchBlock (used by the
getblockfrompeer RPC) takes a PeerRef before locking cs_main. If the
peer is finalized on a net thread in between, BlockRequested hits
assert(state != nullptr) and the node aborts. This PR takes cs_main
before GetPeerRef.

Based on rpc_getblockfrompeer_wo_header (ce3641a), which already
includes fix_getblockfrompeer_rereq_err; both are needed for this to apply.

Problem

All line numbers are at ce3641a.

• FetchBlock calls GetPeerRef(peer_id) (src/net_processing.cpp:2055)
  and only then LOCK(cs_main) (:2061).
• FinalizeNode holds cs_main (:1776), removes the peer from the peer
  map (:1783) and erases its CNodeState (:1817).
• If FinalizeNode runs between those two points in FetchBlock, the
  CNodeState is gone when BlockRequested runs, and
  assert(state != nullptr) fires (:1365).
• Only reachable when the block header is known, because BlockRequested
  is called only if block_index != nullptr (:2066).
• The only caller is getblockfrompeer (src/rpc/blockchain.cpp:473),
  which runs on an HTTP worker thread. Peer disconnects run on net threads.

Reproduction

The window is very small, so the repro injects a delay. This is a test-only
change and is not part of the diff.

1. Build with a 2 s sleep just before LOCK(cs_main) in FetchBlock:
   if (block_index != nullptr) UninterruptibleSleep(2s);
2. Node 0 has only the header of a block that node 1 has.
3. Call getblockfrompeer <hash> <peer_id> on node 0 for a P2P peer.
4. 0.5 s later, run disconnectnode on that peer.

Functional test used

#!/usr/bin/env python3
import threading, time
from test_framework.test_framework import BitcoinTestFramework
from test_framework.p2p import P2PInterface

class Repro(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 2
    def setup_network(self):
        self.setup_nodes()
    def run_test(self):
        n0, n1 = self.nodes
        self.generate(n0, 4, sync_fun=self.no_op)
        self.generate(n1, 3, sync_fun=self.no_op)
        short_tip = n1.getbestblockhash()
        self.connect_nodes(0, 1)
        self.sync_blocks(self.nodes)
        n0.add_p2p_connection(P2PInterface())
        pid = [p["id"] for p in n0.getpeerinfo() if p["inbound"] and p["id"] != n0.getpeerinfo()[0]["id"]][-1]
        res = {}
        def call():
            try:
                res["r"] = n0.cli.getblockfrompeer(short_tip, pid)
            except Exception as e:
                res["e"] = repr(e)
        t = threading.Thread(target=call); t.start()
        time.sleep(0.5)
        n0.disconnectnode(nodeid=pid)
        t.join(30)
        time.sleep(1)
        self.log.info("RESULT: %s ; node0 returncode=%s", res, n0.process.poll())

if __name__ == '__main__':
    Repro(__file__).main()

Before

node 0 aborts (returncode=-6):

bitcoind: net_processing.cpp:1365: bool {anonymous}::PeerManagerImpl::BlockRequested(...): Assertion `state != nullptr' failed.

Timeline from node 0's debug.log:

20:02:34.293  getblockfrompeer enters
20:02:34.791  disconnect by id peer=1; disconnecting
20:02:34.840  FinalizeNode: Cleared nodestate for peer=1
              (delayed RPC thread resumes, assertion fires)

Change

@@ -2051,6 +2051,12 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(...)
 {
     if (m_chainman.m_blockman.LoadingBlocks()) return "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 "Peer does not exist";
@@ -2058,8 +2064,6 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(...)
     // Ignore pre-segwit peers
     if (!CanServeWitnesses(*peer)) return "Pre-SegWit peer";
 
-    LOCK(cs_main);
-
     if (IsBlockRequestedFromPeer(hash, peer_id)) return "Already requested from this peer";

The upstream patch does not apply as-is: FetchBlock here returns
std::optional<std::string> and takes (peer_id, hash, block_index). The
change is the same two hunks adapted to that signature. Lock order
(cs_main, then m_peer_mutex) matches FinalizeNode.

After

Same repro with the lock moved up: node 0 keeps running
(returncode=None) and the RPC returns a clean error:

JSONRPCException('Peer not fully connected (-1)')

FinalizeNode waits for the lock instead of racing: the RPC entered at
20:03:00.114 and FinalizeNode finished at 20:03:02.164.

Test results

Tested at ce3641a, with and without this change. Autotools build:
--without-bdb --with-gui=qt5 --enable-wallet --disable-bench.

• make check: 126 test_bitcoin suites, test_bitcoin-qt, univalue,
  minisketch, secp256k1 and util tests all passed, before and after.
• Functional with --extended (314 tests): 247 passed, 63 skipped,
  4 failed, before and after.
  • feature_dbcrash.py, feature_pruning.py, feature_index_prune.py and
    rpc_getblockfrompeer.py passed.
  • The same 4 tests failed in both runs:
    wallet_multiwallet.py --usecli, wallet_multiwallet.py --descriptors,
    wallet_listtransactions.py --descriptors and p2p_dns_seeds.py. Their
    errors are RPC connection failures ("Unable to connect to bitcoind",
    "Could not connect to the server ... EOF reached"). Run on their own,
    all 4 pass, with and without this change.

Skips:

• 53 legacy-wallet tests: BDB not compiled.
• 5 interface_usdt_*: no permissions to use BPF.
• 3 tests need previous releases.
• 2 feature_bind_port_*: need routable addresses 1.1.1.1 and 2.2.2.2.

Caveats

• The crash needs the injected delay. In normal use the window is tiny, so
  there is no deterministic regression test.
• The repro test and the sleep are not part of this PR.

[kwsantiago]

Please rebase onto the oldest commit this applies to rather than the 29.x-knots tip, which the spec assembler cannot merge. It needs rpc_getblockfrompeer_wo_header and fix_getblockfrompeer_rereq_err: merging both and picking this on top applies cleanly.

[2rdzy]

Rebased onto rpc_getblockfrompeer_wo_header (ce3641a), which already includes fix_getblockfrompeer_rereq_err. Re-tested on that base; description updated.

[kwsantiago]

Tested ACK 1a2066e merged into 29.x-knots. With a 2s delay, base aborts in BlockRequested; with this, the RPC returns "Peer not fully connected" and the block still fetches from another peer.

[kwsantiago]

Please keep the original Core author on the backported commit, with Github-Pull and Rebased-From trailers.

[2rdzy]

Done: the backported commits now carry the original upstream author, with Github-Pull and Rebased-From trailers (marked (partial) where part of the upstream commit is left out). The tree is unchanged.