perf(net): isolate object request scheduling from cs_main - #7673
perf(net): isolate object request scheduling from cs_main#7673PastaPastaPasta wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
⛔ Final review complete — 1 blocking finding(s) (commit 66807a1) · triage: critical · Phase 2 only (queue backlog) |
Potential PR merge conflictsThis 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 firstThese open PRs will likely need a rebase:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe change protects Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to 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
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Both supplied findings are confirmed against the exact head. The new profiling driver aborts on builds without optional lock-contention instrumentation, and the new Dash-specific benchmark is omitted from the file selection used by formatting and cppcheck checks. Verification was source-based; the reported build and runtime tests were not rerun.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — Changing networking request-tracker synchronization across all access sites and splitting critical sections can introduce deadlocks, data races, or request-authorization and scheduling regressions that compromise node availability and object propagation. - Phase 1 reviewers: not run (skipped for throughput: 24 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer
🔴 1 blocking | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `contrib/devtools/benchmark_inventory.py`:
- [BLOCKING] contrib/devtools/benchmark_inventory.py:71-72: Enable lock logging only when the binary supports it
The `lock` logging category exists only when `DEBUG_LOCKCONTENTION` is defined (`src/logging.cpp:177–179`). Without that instrumentation, this unconditional call reaches the unknown-category error in `src/rpc/node.cpp` and aborts the driver before peer setup or RPC sampling. The new documentation describes the compile flag as optional instrumentation to additionally record lock waits, so RPC-only profiling must work without it. Use the category map returned by the first `logging` call to enable `lock` only when supported.
In `src/Makefile.bench.include`:
- [NITPICK] src/Makefile.bench.include:50: Register the new benchmark with Dash-specific lint checks
The new benchmark is registered for compilation, but `src/bench/net_processing.cpp` is not matched by `test/util/data/non-backported.txt`. Both `.github/workflows/clang-diff-format.yml` and `test/lint/lint-cppcheck-dash.py` select files through that list; checking the resulting tracked-file selection confirms this benchmark is excluded. Add its path alongside the existing benchmark entries so the new Dash-specific code receives those checks.
| node.logging([], ["all"]) | ||
| node.logging(["lock"], []) |
There was a problem hiding this comment.
🔴 Blocking: Enable lock logging only when the binary supports it
The lock logging category exists only when DEBUG_LOCKCONTENTION is defined (src/logging.cpp:177–179). Without that instrumentation, this unconditional call reaches the unknown-category error in src/rpc/node.cpp and aborts the driver before peer setup or RPC sampling. The new documentation describes the compile flag as optional instrumentation to additionally record lock waits, so RPC-only profiling must work without it. Use the category map returned by the first logging call to enable lock only when supported.
| node.logging([], ["all"]) | |
| node.logging(["lock"], []) | |
| if "lock" in node.logging([], ["all"]): | |
| node.logging(["lock"], []) |
source: ['claude']
| bench/merkle_root.cpp \ | ||
| bench/nanobench.cpp \ | ||
| bench/nanobench.h \ | ||
| bench/net_processing.cpp \ |
There was a problem hiding this comment.
💬 Nitpick: Register the new benchmark with Dash-specific lint checks
The new benchmark is registered for compilation, but src/bench/net_processing.cpp is not matched by test/util/data/non-backported.txt. Both .github/workflows/clang-diff-format.yml and test/lint/lint-cppcheck-dash.py select files through that list; checking the resulting tracked-file selection confirms this benchmark is excluded. Add its path alongside the existing benchmark entries so the new Dash-specific code receives those checks.
source: ['claude']
2 participants
Issue being fixed or feature implemented
Large object-inventory workloads hold
cs_mainwhile the request tracker selects candidates, advances its time state, and handles NOTFOUND responses. This stalls unrelated chainstate readers such asgetblockcount. Release-build stack sampling identifiedTxRequestTracker::GetRequestableand request-state transitions inSendMessagesas 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 bycs_main. When both locks are needed, acquirecs_mainfirst. 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-resultsfor 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; onlynet_processing.cppdiffers from baselinec652c314a24c59599d987da3779fe1c610dc4851. The candidate source SHA256 isa94050f00e4282442ce3964f114ef6c8ba465133fad172f203cdf7eca1462629, 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:
cs_main, msThe 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:
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:
net_tests,txrequest_tests,governance_inv_tests: 33 cases and 287,437 assertions pass.notfound_does_not_wait_for_chainstatefails on baselinec652c314a24c59599d987da3779fe1c610dc4851at its completion assertion and passes on this implementation. Its bounded timeout is for failure cleanup; the invariant is completion while the other thread still holdscs_main.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.Safety review:
NodesSnapshotkeeps the current node alive during message processing. Selection returns copied inventories; noCNodeStateor chain-index reference escapes its lock. The send path rechecks availability undercs_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:
This pull request was created by Codex.