fix: request governance votes from more nodes on regtest #6712

UdjinM6 · 2025-06-06T12:17:33Z

Issue being fixed or feature implemented

We only ask nPeersPerHashMax (3) nodes for governance votes for the same governance object when syncing. However on regtest we also isolate nodes to create conflicting triggers and since we have 5 nodes to sync from asking 3 of them often results in asking "non-isolated" nodes only (24 votes) yet sometimes we do ask previously isolated node too (25 votes).

Should fix feature_governance.py flakiness. Alternative to #6710.

What was done?

Bump nPeersPerHashMax for regtest. Add more asserts in tests to fail earlier if smth isn't right, check votes on all nodes.

How Has This Been Tested?

run feature_governance.py

Breaking Changes

n/a

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

coderabbitai · 2025-06-06T12:29:20Z

Walkthrough

The core governance code was changed to make the per-hash vote-request limit chain-aware: real networks keep a peers-per-hash limit of 3, while mockable/regtest chains use an effectively unlimited limit (size_t max). The functional governance test was extended with many additional assertions that check total vote counts (gobject("count")["votes"]) and consistent YES/NO vote tallies across nodes at multiple points, including during isolation and after reconnection. No public APIs or exported signatures were modified.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

MCP integration is disabled by default for public repositories
Jira integration is disabled by default for public repositories
Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 89753f285de2383baea172e33fe157fc6fe5b153 and bd2aa80.

📒 Files selected for processing (1)

src/governance/governance.cpp (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/governance/governance.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: linux64_multiprocess-build / Build source
GitHub Check: mac-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: win64-build / Build source

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

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

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

Visit our Status Page to check the current availability of CodeRabbit.
Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

knst · 2025-06-06T18:32:34Z

src/governance/governance.cpp

Is it possible to choose "isolated" nodes somehow by more deterministic way instead bumping this variable?

can we increaese this value for both cases of mockable / non-mockable chain? What are possible downsides?

Mockable is basically regtest, non-mockable - everything else. We don't want to ask too many nodes for the same data on real networks, it would be a waste of bandwidth.

knst

ACK 89753f285de2383baea172e33fe157fc6fe5b153

I tested one more time by cherry-picking commits to top of develop - it works fine

knst · 2025-08-13T12:17:26Z

src/governance/governance.cpp

can we increaese this value for both cases of mockable / non-mockable chain? What are possible downsides?

PastaPastaPasta · 2025-08-16T05:12:45Z

Claude's comments, makes sense to me.

⚠️ Observations

Magic number: The value 10 appears arbitrary. Consider:
- Why 10 specifically? Would 5 or 7 suffice?
- Could this be a configurable parameter for tests?
Test assertions: The new assertions check for exact vote counts (15, 16, 19, 24, 25) which could be fragile if test setup changes
Performance consideration: Increasing from 3 to 10 nodes means more network requests during sync, but this is acceptable for test environments

Suggestions

Consider a constant: Define the regtest peer count as a named constant:
static constexpr size_t REGTEST_PEERS_PER_HASH = 10;
static constexpr size_t MAINNET_PEERS_PER_HASH = 3;
size_t nPeersPerHashMax = Params().IsMockableChain() ? REGTEST_PEERS_PER_HASH : MAINNET_PEERS_PER_HASH;
Test documentation: Consider adding a comment in the test explaining why specific vote counts are expected

github-actions · 2025-08-16T08:48:13Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

UdjinM6 · 2025-08-16T08:51:10Z

rebased on develop with no changes in previous commits and added bd2aa80 to address review suggestions

knst

LGTM a7c8116

UdjinM6 added this to the 23 milestone Jun 6, 2025

UdjinM6 mentioned this pull request Jun 6, 2025

fix: intermittent error in feature_governance.py due to exceed amount of votes #6710

Closed

5 tasks

knst reviewed Jun 6, 2025

View reviewed changes

knst previously approved these changes Aug 13, 2025

View reviewed changes

UdjinM6 dismissed knst’s stale review via a7c8116 August 16, 2025 08:47

UdjinM6 force-pushed the fix_gov_test3 branch from 89753f2 to a7c8116 Compare August 16, 2025 08:47

UdjinM6 added 3 commits August 16, 2025 11:48

fix: request governance votes from more nodes on regtest

4bcacc5

test: adjust feature_governance.py

c16a62f

refactor: use named constants, tweak and explain chosen numbers

bd2aa80

UdjinM6 force-pushed the fix_gov_test3 branch from a7c8116 to bd2aa80 Compare August 16, 2025 08:48

knst approved these changes Aug 16, 2025

View reviewed changes

knst requested a review from PastaPastaPasta August 21, 2025 19:30

PastaPastaPasta approved these changes Sep 2, 2025

View reviewed changes

PastaPastaPasta merged commit 9e083e7 into dashpay:develop Sep 2, 2025
33 of 35 checks passed

fix: request governance votes from more nodes on regtest #6712

fix: request governance votes from more nodes on regtest #6712

Uh oh!

Conversation

UdjinM6 commented Jun 6, 2025

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

coderabbitai bot commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Chat

Support

CodeRabbit Commands (Invoked using PR/Issue comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Status, Documentation and Community

Uh oh!

knst Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

knst Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

knst Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Aug 16, 2025

Uh oh!

github-actions bot commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

UdjinM6 commented Aug 16, 2025

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Jun 6, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

github-actions bot commented Aug 16, 2025 •

edited

Loading