fix(net): request object votes from nPeersPerHashMax peers

[PastaPastaPasta]

Issue being fixed or feature implemented

SyncManager::RequestGovernanceObjectVotes is meant to ask up to nPeersPerHashMax distinct peers (3 on real networks, unlimited on regtest) for the votes of each governance object, at most once per peer per hour. When the per-object filter was moved into CGovernanceManager::FetchGovernanceObjectVotes (77faacc, first released in v23.1.0), the caller started passing nMaxObjRequestsPerNode as the helper's nPeersPerHashMax argument. The two values are unrelated: one is how many objects to request from a single peer per call, the other is how many peers to ask about a single object.

On mainnet nMaxObjRequestsPerNode is hard-coded to 1. So after one peer has been asked for an object's votes, the object is dropped from the candidate list for every other peer until the 1 hour mapAskedRecently entry expires. During initial governance sync each object's votes are therefore requested from exactly one peer instead of up to three. On testnet and devnets the bound becomes 2000 / enabled masternodes, which can be larger than 3. Objects that already have 3 asked peers are then re-selected, and that only costs some extra requests.

Why this is a real problem

Code path at c14104b:

• src/node/sync_manager.cpp#L55 and #L66: nPeersPerHashMax is 3 on real networks.
• src/node/sync_manager.cpp#L74: nMaxObjRequestsPerNode is 1 on mainnet.
• src/node/sync_manager.cpp#L86: FetchGovernanceObjectVotes(nMaxObjRequestsPerNode, nNow, mapAskedRecently).
• src/governance/governance.cpp#L942 and #L954: the parameter is named nPeersPerHashMax, and an object is skipped when asked_peers.size() >= nPeersPerHashMax.
• src/node/sync_manager.cpp#L122: the inner loop still stops at nPeersPerHashMax, but it never gets the chance on mainnet, because the outer filter has already removed the object.
• src/node/sync_manager.cpp#L297-L298: initial sync calls RequestGovernanceObjectVotes with one peer at a time, so the per-object filter is what decides whether a second and third peer are asked.

Before 77faacc the inline loop compared against nPeersPerHashMax. The refactor passed the other variable.

To show the behaviour directly, I injected a temporary, uncommitted test case into src/test/governance_inv_tests.cpp. It uses the existing mainnet GovernanceInvSetup fixture, adds one governance object and four connected peers, and calls the real SyncManager::RequestGovernanceObjectVotes once per peer, the same way the initial-sync loop does. It then counts the MNGOVERNANCESYNC per-object vote requests queued to each peer. The expected result is one request each to peers 1 to 3 and none to peer 4.

Unfixed (c14104b): only the first peer is ever asked

$ src/test/test_dash --run_test=governance_inv_tests/object_votes_requested_from_up_to_three_peers --log_level=message
Running 1 test case...
peer=1 RequestGovernanceObjectVotes returned 0, MNGOVERNANCESYNC (per-object vote request) messages queued=1
peer=2 RequestGovernanceObjectVotes returned -2, MNGOVERNANCESYNC (per-object vote request) messages queued=0
test/governance_inv_tests.cpp:599: error: in "governance_inv_tests/object_votes_requested_from_up_to_three_peers": peer=2 was asked for the object's votes 0 time(s), expected 1
peer=3 RequestGovernanceObjectVotes returned -2, MNGOVERNANCESYNC (per-object vote request) messages queued=0
test/governance_inv_tests.cpp:599: error: in "governance_inv_tests/object_votes_requested_from_up_to_three_peers": peer=3 was asked for the object's votes 0 time(s), expected 1
peer=4 RequestGovernanceObjectVotes returned -2, MNGOVERNANCESYNC (per-object vote request) messages queued=0

*** 2 failures are detected in the test module "Dash Core Test Suite"
exit=201

With this PR: three peers are asked, the fourth is not

$ src/test/test_dash --run_test=governance_inv_tests/object_votes_requested_from_up_to_three_peers --log_level=message
Running 1 test case...
peer=1 RequestGovernanceObjectVotes returned 0, MNGOVERNANCESYNC (per-object vote request) messages queued=1
peer=2 RequestGovernanceObjectVotes returned 0, MNGOVERNANCESYNC (per-object vote request) messages queued=1
peer=3 RequestGovernanceObjectVotes returned 0, MNGOVERNANCESYNC (per-object vote request) messages queued=1
peer=4 RequestGovernanceObjectVotes returned -2, MNGOVERNANCESYNC (per-object vote request) messages queued=0

*** No errors detected
exit=0

Why it matters

On mainnet the vote sync for each governance object has no redundancy. If the single peer picked for an object is slow, lagging or does not answer, the node's vote tally for that object stays incomplete. It stays that way until the hourly re-request after sync, or until the missing votes arrive through normal inventory relay. This affects nodes doing initial governance sync, such as fresh nodes and nodes restarting after a long sleep. There is no consensus or funds impact. The effect is slower and less reliable convergence of governance vote tallies. The regression has shipped since v23.1.0.

What was done?

Pass nPeersPerHashMax instead of nMaxObjRequestsPerNode as the first argument of FetchGovernanceObjectVotes in SyncManager::RequestGovernanceObjectVotes.

Why this is the correct minimal fix

The callee's parameter is the per-object peer cap, and the caller already computes the correct value (nPeersPerHashMax) and uses it for the matching inner-loop break. Passing it restores the pre-77faacce semantics exactly: an object stays a candidate until 3 distinct peers (unlimited on regtest) have been asked within the last hour. nMaxObjRequestsPerNode keeps its own role as the bound of the per-call object loop, so the number of objects requested from each peer per call does not change. No other caller of FetchGovernanceObjectVotes exists. A strong type for the argument, or a rename, would stop this kind of mix-up in the future, but both are refactors beyond restoring the intended behaviour.

How Has This Been Tested?

• Reproduction above: a temporary test case in src/test/governance_inv_tests.cpp that calls SyncManager::RequestGovernanceObjectVotes per peer on the mainnet fixture. It fails on the unfixed tree (peers 2 and 3 never asked) and passes with this change (peers 1 to 3 asked once, peer 4 not asked). It is not part of this PR. RequestGovernanceObjectVotes is a private member, and the harness reached it with an explicit-instantiation access trick, which is not acceptable in a committed test. The regtest functional tests cannot observe the bug, because on mockable chains the cap is effectively unlimited both before and after the fix (2000 / MN count vs SIZE_MAX).
• Unit tests, all passing: src/test/test_dash --run_test=<suite> for governance_inv_tests, governance_vote_sync_tests, governance_vote_processing_tests, governance_vote_wire_tests, governance_superblock_tests, governance_validators_tests.
• Functional tests, all passing: test/functional/test_runner.py feature_governance.py feature_governance_cl.py feature_governance_objects.py p2p_govsync_bloom.py. In the first parallel run, feature_governance.py hit an RPC timeout while the host load average was about 100. It passed when rerun on its own.
• clang-format on the changed lines produces no changes; test/lint/lint-whitespace.py passes.

Environment: macOS arm64, debug build (--enable-debug --enable-werror).

Breaking Changes

None.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 25 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7538972e-5cda-4fd4-8033-7dc4f2b0431c

📥 Commits

Reviewing files that changed from the base of the PR and between 8819507 and aaa5301.

📒 Files selected for processing (1)

• src/node/sync_manager.cpp

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

[thepastaclaw]

⛔ Final review complete — 1 blocking finding(s) (commit aaa5301) · triage: low

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The one-line change at aaa5301 correctly restores the per-object peer cap while preserving the independent object-request budget and hourly expiration; no source-code defect was found. The PR title is rejected by the title-validation workflow and needs a metadata-only correction. This was a static review: current CI shows successful formatting, merge checks, and amd64/arm64 container builds, pending multi-architecture manifests, failed title validation, and an unsuccessful predict_conflicts check with no failed-job log returned.

🔴 1 blocking

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🔴 Blocking: PR title uses a scope rejected by title validation
.github/workflows/semantic-pull-request.yml:39-52

The current PR title, fix(governance): request object votes from nPeersPerHashMax peers, uses governance, which is absent from the workflow's allowed scopes and the matching list in CONTRIBUTING.md. The failed log for run 37182444330 explicitly reports Unknown scope "governance", and the current PR check still fails at this exact head. Rename the PR to fix(net): request object votes from nPeersPerHashMax peers to use the supported P2P-network scope. This requires only a PR-title edit, not changes to the workflow or source code.

_{source: glm-5.3-flash (phase1-reviewer: general)}

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — The diff corrects a single argument in SyncManager::RequestGovernanceObjectVotes, a small and well-contained behavioral fix whose correctness is straightforward to verify against the callee's per-object peer limit.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 99% left, weekly 69% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

🤖 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 `.github/workflows/semantic-pull-request.yml`:
- [BLOCKING] .github/workflows/semantic-pull-request.yml:39-52: PR title uses a scope rejected by title validation
  The current PR title, `fix(governance): request object votes from nPeersPerHashMax peers`, uses `governance`, which is absent from the workflow's allowed scopes and the matching list in CONTRIBUTING.md. The failed log for run 37182444330 explicitly reports `Unknown scope "governance"`, and the current PR check still fails at this exact head. Rename the PR to `fix(net): request object votes from nPeersPerHashMax peers` to use the supported P2P-network scope. This requires only a PR-title edit, not changes to the workflow or source code.

[PastaPastaPasta]

Re the PR-title finding: the title is now fix(net): request object votes from nPeersPerHashMax peers, which uses an allowed scope. Validate PR title passes at the same head, and no code changed.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[PastaPastaPasta]

approved