refactor: reduce needs on cs_main in network processing by PastaPastaPasta · Pull Request #6990 · dashpay/dash

PastaPastaPasta · 2025-11-18T18:41:32Z

Issue being fixed or feature implemented

see: #6953 (comment) -- based on that collected data; the cs_main contention in network processing is ~17% of cs_main contention. If we exclude the islock cs_main (resolved (or mostly resolved don't recall) in that PR) the percentage grows to 30%; if we can minimize contention here, it will benefit our overall cs_main contention data.

What was done?

convert CConman to a structure that is more in line with peerman; namely, objects protect their own internal data, and the vector stores shared pointers, so that we take a shared_ptr from the vector, and then can use it as we desire without lifetime concerns.

Be aware, I would like to upstream these changes, so that we don't have to maintain them long term, however there's no guarantee that will be viable.

How Has This Been Tested?

Deployed testing was minimal; after 6953 is merged; I'd like to deploy this version to some testnet nodes and analyze contention.

Breaking Changes

None

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

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 (for repository code-owners and collaborators only)

coderabbitai · 2025-11-18T18:47:40Z

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

💤 Files selected but had no reviewable changes (1)

src/net_processing.cpp

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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

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

github-actions · 2025-11-18T18:48:22Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

github-actions · 2025-12-03T19:24:13Z

This pull request has conflicts, please rebase.

knst · 2025-12-09T12:20:54Z

    //! Length of current-streak of unconnecting headers announcements
-    int nUnconnectingHeaders{0};
+    int nUnconnectingHeaders GUARDED_BY(m_mutex){0};


can it be atomic?

also fSyncStarted; nBlocksInFlight, fPreferredDownload, fPreferHeaders', fPreferHeadersCmpressed?

Maybe, but need to be more careful that full operation is atomic. I figured just using a mutex currently would be simplier

github-actions · 2025-12-16T14:23:58Z

This pull request has conflicts, please rebase.

thepastaclaw · 2026-03-30T14:48:08Z

✅ Final review complete — no blockers (commit 96ff7be)

thepastaclaw

Code Review

I did not validate a concrete correctness regression in src/net_processing.cpp on this SHA, but this is still a large concurrency-sensitive refactor of peer-state and object-request locking with no added regression coverage. The main reviewable concern is merge risk from behavioral drift in lifecycle/accounting paths that now depend on the new per-node and object-request mutexes.

Reviewed commit: 96ff7be

🟡 1 suggestion(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 `src/net_processing.cpp`:
- [SUGGESTION] lines 439-589: This lock-splitting refactor still lacks targeted regression coverage
  This patch moves `CNodeState` off `cs_main`, introduces `shared_ptr`-managed node-state lifetime plus a per-node mutex, and splits object-request coordination onto `g_object_request_mutex`, but it adds no unit or functional tests. Because the touched code governs header sync state, in-flight block/object accounting, eviction, and disconnect cleanup, the main merge risk here is silent behavioral drift rather than an obvious compile failure. Adding at least one focused regression test around disconnect cleanup / request bookkeeping (or a deterministic unit test for the moved counters and request/erase flow) would make this refactor materially safer to merge.

thepastaclaw · 2026-04-07T17:12:36Z

@@ -568,17 +572,22 @@ struct CNodeState {
        std::chrono::microseconds m_check_expiry_timer{0};
    };

-    ObjectDownloadState m_object_download;
+    ObjectDownloadState m_object_download GUARDED_BY(m_mutex);

    //! Whether this peer is an inbound connection
    const bool m_is_inbound;

    //! A rolling bloom filter of all announced tx CInvs to this peer.
-    CRollingBloomFilter m_recently_announced_invs = CRollingBloomFilter{INVENTORY_MAX_RECENT_RELAY, 0.000001};
+    CRollingBloomFilter m_recently_announced_invs GUARDED_BY(m_mutex){INVENTORY_MAX_RECENT_RELAY, 0.000001};

    CNodeState(bool is_inbound) : m_is_inbound(is_inbound) {}
 };

+// Keeps track of the time (in microseconds) when transactions were requested last time
+unordered_limitedmap<uint256, std::chrono::microseconds, StaticSaltedHasher> g_already_asked_for(MAX_INV_SZ, MAX_INV_SZ * 2);
+unordered_limitedmap<uint256, std::chrono::microseconds, StaticSaltedHasher> g_erased_object_requests(MAX_INV_SZ, MAX_INV_SZ * 2);
+Mutex g_object_request_mutex;


🟡 Suggestion: This lock-splitting refactor still lacks targeted regression coverage

This patch moves CNodeState off cs_main, introduces shared_ptr-managed node-state lifetime plus a per-node mutex, and splits object-request coordination onto g_object_request_mutex, but it adds no unit or functional tests. Because the touched code governs header sync state, in-flight block/object accounting, eviction, and disconnect cleanup, the main merge risk here is silent behavioral drift rather than an obvious compile failure. Adding at least one focused regression test around disconnect cleanup / request bookkeeping (or a deterministic unit test for the moved counters and request/erase flow) would make this refactor materially safer to merge.

_{source: ['codex-general', 'coordinator-manual']}

🤖 Fix this 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 `src/net_processing.cpp`: - [SUGGESTION] lines 439-589: This lock-splitting refactor still lacks targeted regression coverage This patch moves `CNodeState` off `cs_main`, introduces `shared_ptr`-managed node-state lifetime plus a per-node mutex, and splits object-request coordination onto `g_object_request_mutex`, but it adds no unit or functional tests. Because the touched code governs header sync state, in-flight block/object accounting, eviction, and disconnect cleanup, the main merge risk here is silent behavioral drift rather than an obvious compile failure. Adding at least one focused regression test around disconnect cleanup / request bookkeeping (or a deterministic unit test for the moved counters and request/erase flow) would make this refactor materially safer to merge.

PastaPastaPasta · 2026-09-08T20:01:45Z

Closing at the author’s request. The lock-contention objective remains relevant, but #5943 replaced much of the object-request machinery this patch refactors. A replacement will be scoped from profiling current develop and will include comparative performance measurements and targeted safety validation.

🤖 Posted autonomously by Codex on behalf of pasta.

7882982 perf(net): isolate object request scheduling from cs_main (pasta) Pull request description: ## 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 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. In `SendMessages` the getdata loop takes `cs_main` once to partition the requestable set by `AlreadyHave`, then takes the tracker mutex once to apply `RequestedTx`/`ForgetTxHash` for the batch, rather than acquiring both locks per inventory. `GetRequestedObjectCount` only reads the tracker, so it no longer requires `cs_main`. `SyncManager::RequestGovernanceObjectVotes` reads it without pinning `cs_main` while waiting on the tracker mutex. Add native `InventoryBatch100` / `InventoryBatch50000` benchmarks, an accounting test, and a regression test proving NOTFOUND can complete while another thread holds `cs_main`. ## How Has This Been Tested? ### Performance Earlier revision (per-inventory locking): [isolated comparison run](https://github.com/PastaPastaPasta/dash/actions/runs/34279569867) on an Ubuntu 24.04 GitHub-hosted runner, Clang `-O2 -g -DDEBUG_LOCKCONTENTION`, baseline `c652c314a24c59599d987da3779fe1c610dc4851`. Three alternating baseline/candidate pairs per workload, eight rounds each. Medians: | 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 +16% governance completion regression in that revision came from taking `cs_main` and the tracker mutex once per inventory in the getdata loop. The current revision batches both locks. The Python driver used for those measurements is not included in this PR; it lived at `contrib/devtools/benchmark_inventory.py` in the earlier revision if anyone wants to reproduce. Native benchmark on the current revision, debug build, Apple M4 Max, per-inventory INV → scheduling → NOTFOUND: | Benchmark | Per-inventory locking | Batched locking | | --- | ---: | ---: | | InventoryBatch100 | 3,814 ns | 2,147 ns | | InventoryBatch50000 | 6,025 ns | 2,986 ns | Debug builds inflate lock cost, so the absolute numbers are not representative of release; the relative improvement is from removing per-inventory lock acquisition. ### Correctness Local validation on Apple M4 Max, macOS 15, debug build with prebuilt depends: - Build passes with no new thread-safety warnings. - `net_tests,txrequest_tests,governance_inv_tests,denialofservice_tests` pass. - `notfound_does_not_wait_for_chainstate` fails on the baseline at its completion assertion and passes on this implementation. - Functional: `p2p_tx_download`, `p2p_invalid_messages`, `p2p_blocksonly`, `feature_sporks`, `p2p_instantsend`, `feature_governance_objects`, `feature_governance`, `p2p_compactblocks`, `p2p_sendheaders`, `feature_llmq_chainlocks`, `p2p_net_deadlock` (both transports) pass. - Whitespace, file, assertion, include, and circular-dependency lints pass; `clang-format-diff` reports no changes. Safety review: `NodesSnapshot` keeps the current node alive during message processing, so `State(pto->GetId())` is valid across the released-and-reacquired `cs_main` in `SendMessages`. Selection returns copied inventories; no `CNodeState` or chain-index reference escapes its lock. `RequestedTx` already tolerates an announcement that was completed or forgotten by another thread between selection and request (the superfluous-call branch in `txrequest.cpp`), which is the only new interleaving the split introduces. The `linux64_tsan-test` failure on the previous push was `feature_protx_version.py` hitting a data race on the static `ep2_curve_get_s3()::s3` buffer inside vendored relic, reached from two DKG phase-handler threads calling `CActiveMasternodeManager::Sign`. That race is in `src/dashbls`, not touched here, and develop's TSAN job passes intermittently on the same code. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone This pull request was created by Codex. Top commit has no ACKs. Tree-SHA512: fe39eb5f3a89dc3d6e125a1a51562402ba0314b8c37fdc3280f73960d698c514e5a2a2093897017f82825d1b1c08a4b579b6024680d0047352731eeb6fda48ec

github-actions Bot mentioned this pull request Nov 19, 2025

refactor: avoid return by reference and prefer return by value #6955

Open

5 tasks

PastaPastaPasta added this to the 23.1 milestone Nov 19, 2025

PastaPastaPasta requested review from UdjinM6 and knst November 19, 2025 19:00

PastaPastaPasta force-pushed the feat/connman-csmain-free branch from 7030342 to 3a8b312 Compare November 21, 2025 17:40

This was referenced Nov 25, 2025

refactor: separate network and consensus logic 3/N [governance] #7008

Merged

refactor: separate network and consensus logic 2a/N [signing] #6992

Merged

github-actions Bot mentioned this pull request Dec 3, 2025

backport: Merge bitcoin#28787, 23083 #7031

Closed

github-actions Bot added the needs rebase label Dec 3, 2025

PastaPastaPasta added 2 commits December 8, 2025 21:01

refactor: reduce needs on cs_main in network processing

82c7bff

fix: resolve remaining thread safety issues

96ff7be

PastaPastaPasta force-pushed the feat/connman-csmain-free branch from 12f5253 to 96ff7be Compare December 9, 2025 03:01

github-actions Bot removed the needs rebase label Dec 9, 2025

knst reviewed Dec 9, 2025

View reviewed changes

This was referenced Dec 13, 2025

refactor: extract CActiveMasternodeManager from LLMQContext (3/n, DKG session isolation, ActiveContext consolidation) #7065

Merged

refactor: extract CActiveMasternodeManager from LLMQContext (4/n, followups) #7066

Merged

github-actions Bot added the needs rebase label Dec 16, 2025

UdjinM6 removed this from the 23.1 milestone Feb 15, 2026

thepastaclaw reviewed Apr 7, 2026

View reviewed changes

github-actions Bot mentioned this pull request May 12, 2026

refactor: break circular dependency over net_processing and dkgsessionhandler #7314

Merged

5 tasks

github-actions Bot mentioned this pull request Aug 1, 2026

feat: coinjoin promotion / demotion #7052

Merged

github-actions Bot mentioned this pull request Sep 8, 2026

fix: purge pending recovered sigs in BanNode #7563

Merged

5 tasks

PastaPastaPasta closed this Sep 8, 2026

PastaPastaPasta mentioned this pull request Sep 8, 2026

perf(net): isolate object request scheduling from cs_main #7673

Merged

5 tasks

Navigation Menu

Uh oh!

refactor: reduce needs on cs_main in network processing - #6990

refactor: reduce needs on cs_main in network processing#6990
PastaPastaPasta wants to merge 2 commits into
dashpay:developfrom
PastaPastaPasta:feat/connman-csmain-free

PastaPastaPasta commented Nov 18, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot commented Nov 18, 2025 •

edited

Loading

Review skipped

Uh oh!

github-actions Bot commented Nov 18, 2025 •

edited

Loading

Uh oh!

github-actions Bot commented Dec 3, 2025

Uh oh!

knst Dec 9, 2025

Uh oh!

PastaPastaPasta Dec 9, 2025

Uh oh!

github-actions Bot commented Dec 16, 2025

Uh oh!

thepastaclaw commented Mar 30, 2026 •

edited

Loading

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Apr 7, 2026

Uh oh!

PastaPastaPasta commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

PastaPastaPasta commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

coderabbitai Bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

github-actions Bot commented Dec 3, 2025

Uh oh!

knst Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Dec 16, 2025

Uh oh!

thepastaclaw commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PastaPastaPasta commented Nov 18, 2025 •

edited

Loading

coderabbitai Bot commented Nov 18, 2025 •

edited

Loading

github-actions Bot commented Nov 18, 2025 •

edited

Loading

thepastaclaw commented Mar 30, 2026 •

edited

Loading