feat: do not send any governance objects to old (pre-GOVSCRIPT_PROTO_VERSION) p2p clients

[knst]

Issue being fixed or feature implemented

Alternate solution to #6877 and unblock #6838

What was done?

Old pre-GOVSCRIPT_PROTO_VERSION p2p clients can not accept governance objects with p2sh payment_addresses anyway.
To avoid inconsistency, do not send any governance object to them.

How Has This Been Tested?

Run unit & functional tests.

Breaking Changes

P2p clients with version below GOVSCRIPT_PROTO_VERSION won't receive any governance object, not only governance objects with p2sh payment_address.
They are broken now anyway because receive only some governance proposals and it helps to avoid data inconsistency.

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

[github-actions]

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

• refactor: split off governance masternode-only logic to GovernanceSigner, drop Relay()s and use periodic relay instead, minor cleanup #6838 - refactor: split off governance masternode-only logic to GovernanceSigner, drop Relay()s and use periodic relay instead, minor cleanup

Please coordinate with the authors of these PRs to avoid merge conflicts.

[coderabbitai]

Walkthrough

An early-exit was added to CGovernanceManager::SyncObjects to skip governance syncing entirely when the peer's protocol version is older than GOVSCRIPT_PROTO_VERSION, replacing previous per-object gating. Separately, Relay in governance object handling now always broadcasts using GOVSCRIPT_PROTO_VERSION; the prior minProtoVersion gating and the special-case proposal/p2sh validation were removed. No other synchronization flow changes were made for up-to-date peers.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly and accurately describes the primary change of the pull request, which is preventing any governance objects from being sent to pre-GOVSCRIPT_PROTO_VERSION peers.
Description Check	✅ Passed	The description clearly outlines the issue addressed, the change implemented, and how it was tested, directly relating to the pull request’s objective of preventing governance objects from being sent to old protocol clients.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

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

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/governance/governance.cpp (1)

987-991: Early-exit gating is correct; consider log category and small UX tweak

Looks good. Optional improvements:

• Use BCLog::GOBJECT category and include peer context via GetLogString() for consistent logging.
• Optionally send SYNCSTATUSCOUNT with 0 objects to immediately signal completion.

-    if (peer.nVersion < GOVSCRIPT_PROTO_VERSION) {
-        LogPrintf("CGovernanceManager::%s -- node is not supporting p2sh govobj. Do not send anything to old node to avoid data inconsistency, peer=%d\n", __func__, peer.GetId());
-        return {};
-    }
+    if (peer.nVersion < GOVSCRIPT_PROTO_VERSION) {
+        LogPrint(BCLog::GOBJECT, "CGovernnanceManager::%s -- peer not supporting p2sh govobj, skipping sync: %s\n",
+                 __func__, peer.GetLogString());
+        // Optionally inform peer of zero results
+        CNetMsgMaker msgMaker(peer.GetCommonVersion());
+        connman.PushMessage(&peer, msgMaker.Make(NetMsgType::SYNCSTATUSCOUNT, MASTERNODE_SYNC_GOVOBJ, 0));
+        return {};
+    }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fb01c5a and f4ed16c.

📒 Files selected for processing (2)

• src/governance/governance.cpp (1 hunks)
• src/governance/object.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

• src/governance/object.cpp
• src/governance/governance.cpp

🔇 Additional comments (1)

src/governance/object.cpp (1)

606-610: Relay gating via GOVSCRIPT_PROTO_VERSION is appropriate

This aligns with the policy to avoid relaying governance objects to pre-GOVSCRIPT peers. LGTM.

To keep behavior consistent, please verify vote relaying also uses appropriate version gating:

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/governance/governance.cpp (1)

988-989: Consider using LogPrint instead of LogPrintf to reduce log noise.

The current implementation uses LogPrintf, which always logs regardless of debug categories. This could create log noise in production environments where many old clients might attempt to sync. Consider using LogPrint(BCLog::GOBJECT, ...) instead to allow operators to control this logging via the -debug flag.

Apply this diff:

-        LogPrintf("CGovernanceManager::%s -- peer=%d does not support p2sh govobj, skipping sync\n", __func__,
-                  peer.GetId());
+        LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- peer=%d does not support p2sh govobj, skipping sync\n", __func__,
+                 peer.GetId());

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4ed16c and 480c893.

📒 Files selected for processing (2)

• src/governance/governance.cpp (1 hunks)
• src/governance/object.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

• src/governance/object.cpp
• src/governance/governance.cpp

🔇 Additional comments (2)

src/governance/object.cpp (1)

606-609: LGTM! Clean implementation of protocol version gating.

The change correctly ensures that governance objects are only relayed to peers supporting GOVSCRIPT_PROTO_VERSION or higher. The inline comment clearly explains the rationale for this change, preventing potential bans from old clients that cannot handle p2sh payment addresses.

src/governance/governance.cpp (1)

987-991: Good placement of the early exit guard.

The early exit is correctly placed after the fulfilled request tracking (line 985), ensuring we still record that the peer requested governance sync even though we're skipping it due to their old protocol version. This prevents the peer from being able to spam sync requests.

[github-actions]

This pull request has conflicts, please rebase.

[kwvg]

Superseded by #6877