perf(net): isolate object request scheduling from cs_main

[PastaPastaPasta]

Issue being fixed or feature implemented

Large object-inventory workloads hold cs_main while the request tracker selects candidates, advances its time state, and handles NOTFOUND responses. This stalls unrelated chainstate readers such as getblockcount. Release-build stack sampling identified TxRequestTracker::GetRequestable and request-state transitions in SendMessages as substantial work inside the critical section.

Supersedes #6990. The object downloader was replaced by #5943, so this change works with the current tracker.

What was done?

Give the existing object request tracker its own mutex. Selection and tracker-only completion/cleanup can run without cs_main; block scheduling, availability checks, and per-peer response authorization remain protected by cs_main. When both locks are needed, acquire cs_main first. All tracker access sites and compiler lock annotations are updated.

Add native inventory benchmarks, a reproducible regtest contention workload with an independent RPC observer process, an accounting test, and a regression test proving NOTFOUND can complete while another thread holds cs_main.

How Has This Been Tested?

Performance

Successful isolated comparison run (download inventory-profile-results for raw RPC samples, metrics, lock waits, build logs, and source/binary hashes). Ubuntu 24.04 GitHub-hosted runner; both binaries built with Clang, -O2 -g, and -DDEBUG_LOCKCONTENTION. The same benchmark code is built into both; only net_processing.cpp differs from baseline c652c314a24c59599d987da3779fe1c610dc4851. The candidate source SHA256 is a94050f00e4282442ce3964f114ef6c8ba465133fad172f203cdf7eca1462629, matching this PR and the local safety builds.

Three alternating baseline/candidate pairs per workload, eight rounds each. Every run requested every announcement (400,000 for large, 3,200 for small, and 160,000 for governance). Values below are medians of the three per-run measurements:

Workload	RPC p99, ms	Worst RPC, ms	Total RPC wait for cs_main, ms	Completion, s
50,000 entries, 1 peer	0.403 → 0.382	107.25 → 76.94	1375.58 → 567.00 (−59%)	21.67 → 21.63
100 entries, 4 peers	0.316 → 0.327	1.65 → 1.37	3.19 → 0.58	2.320 → 2.319
5,000 governance votes, 4 peers	8.83 → 5.82 (−34%)	21.85 → 11.33	355.84 → 204.55 (−43%)	3.09 → 3.58 (+16%)

The large-message worst RPC latency improved in every pair: baseline 96.83 / 107.25 / 108.17 ms, candidate 76.89 / 76.94 / 77.04 ms. Its long stalls are too rare for p99 alone to characterize them. Governance p99 was 8.94 / 8.83 / 7.54 ms versus 6.06 / 5.82 / 5.80 ms.

Native INV → scheduling → NOTFOUND processing cost, median ns/inventory across three runs:

Benchmark	Baseline	Candidate	Change
InventoryBatch100	1629.13	1634.72	+0.3%
InventoryBatch50000	3628.92	3608.33	−0.6%

The benefit is reduced contention for independent chainstate readers. Native processing cost is essentially unchanged, while the governance socket workload completed about 16% slower; that workload includes P2P serialization, polling, and scheduling. This is a synthetic unknown-object/fallback workload, not evidence of higher live-network throughput. INV processing still holds cs_main, and callers needing both locks can still wait on the tracker. The benchmark tool and reproduction instructions are included in this PR.

Local validation on an Apple M4 Max, macOS 15 / Darwin 24.6, using prebuilt depends:

• Optimized release, debug, and ThreadSanitizer builds pass. No new compiler thread-safety warnings.
• net_tests,txrequest_tests,governance_inv_tests: 33 cases and 287,437 assertions pass.
• notfound_does_not_wait_for_chainstate fails on baseline c652c314a24c59599d987da3779fe1c610dc4851 at its completion assertion and passes on this implementation. Its bounded timeout is for failure cleanup; the invariant is completion while the other thread still holds cs_main.
• All 11 selected functional variants pass: transaction download/privacy, invalid messages, block-only mode, deadlock tests with both transports, compact blocks, sporks, governance objects, InstantSend, and sendheaders.
• Unsuppressed ThreadSanitizer (halt_on_error=1): transaction download, both deadlock variants, and concurrent inventory/RPC workloads for sporks and governance votes (32,000 announcements each) pass. The sanitizer build omits wallet support, so the full governance functional test could not start there; it passed in the normal build.
• Whitespace, file checks, assertion lint, selected Python lint rules, and formatting checks pass.

Safety review: NodesSnapshot keeps the current node alive during message processing. Selection returns copied inventories; no CNodeState or chain-index reference escapes its lock. The send path rechecks availability under cs_main. Typed accounting, request limits, delays/expiry, fallback behavior, consume-once semantics, and late-response grace retain their existing rules. The tests cover duplicates, shared hashes across types, unsolicited responses, fallback, reannouncement, and disconnect cleanup. Sanitizers and tests establish evidence for covered paths, not exhaustive proof over all interleavings.

Breaking Changes

None.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone

This pull request was created by Codex.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-08T21:38:09.324145Z	66807a1	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[thepastaclaw]

🕓 Queued for automated review — 33rd in line, estimated start in ~6 h (commit 66807a1)
Estimated review time once started: ~20 min (two-phase automated review; median of recent runs).

• Request priority review — tick this box and the review moves to the front of the queue.

[github-actions]

Potential PR merge conflicts

This is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order.

If this PR merges first

These open PRs will likely need a rebase:

• #7292: backport: Merge bitcoin/bitcoin#29458, (partial) 29236, 29459, 29479, 29615, 29433 Changed files: src/Makefile.bench.include.
• #7526: fix: fetch orphan-vote parents via the request tracker instead of broadcasting Changed files: src/net_processing.cpp.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: ff7a7ffc-d373-47f9-8320-2bc9172901fe

📥 Commits

Reviewing files that changed from the base of the PR and between f30335d and 66807a1.

📒 Files selected for processing (6)

• contrib/devtools/benchmark_inventory.py
• doc/benchmarking.md
• src/Makefile.bench.include
• src/bench/net_processing.cpp
• src/net_processing.cpp
• src/test/net_tests.cpp

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

Walkthrough

The change protects TxRequestTracker with a dedicated mutex instead of cs_main. It moves non-block getdata processing outside the cs_main scope and updates request lifecycle handling. New tests cover request accounting and asynchronous NOTFOUND cleanup. Native and functional benchmarks measure inventory processing and RPC latency. Documentation describes benchmark execution, metrics, and comparison workloads.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 66807

This change moves inventory request tracking onto a dedicated lock so request scheduling no longer waits on the main chainstate lock, and adds benchmarks, tests, and documentation. Investigation of the flagged concerns found no correctness, concurrency, or build problems at the current head, so the change appears safe to merge with normal validation.

Sequence Diagram(s)

sequenceDiagram
  participant BenchmarkPeer
  participant PeerManagerImpl
  participant TxRequestTracker
  participant RPCObserver
  BenchmarkPeer->>PeerManagerImpl: Announce inventory
  PeerManagerImpl->>TxRequestTracker: Register request under m_object_request_mutex
  BenchmarkPeer->>PeerManagerImpl: Return NOTFOUND
  PeerManagerImpl->>TxRequestTracker: Complete request
  RPCObserver->>PeerManagerImpl: Call getblockcount
  PeerManagerImpl-->>RPCObserver: Return block count and latency sample

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 3.70% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 4 files. (2 skipped: 2…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the primary change: isolating network object request scheduling from cs_main.
Description check	✅ Passed	The description is directly related to the changeset and explains the mutex isolation, lock behavior, benchmarks, tests, performance results, and limitations.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 3.70% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 4 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}