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

src/net_processing.cpp

    //! 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

✅ Review complete (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

src/net_processing.cpp

@@ -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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 intodashpay: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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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!

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