[PastaPastaPasta]

Issue being fixed or feature implemented

CGovernanceManager::GetSyncableVoteInvs() walks every stored vote for a requested object and calls CGovernanceVote::IsValid(), which unconditionally re-runs full cryptographic verification via CheckSignature() - ECDSA recovery for proposal funding votes, or a BLS pairing (~1 ms) for operator-key votes. Those signatures were already verified at acceptance in CGovernanceObject::ProcessVote, so the work is pure waste.

The walk is driven by the P2P MNGOVERNANCESYNC handler and runs on the single message-handler thread while holding cs_store and govobj.cs. On a mainnet trigger with thousands of masternode votes this is seconds of pairing work per request, stalling all P2P message processing.

Any unauthenticated peer can trigger it once the victim is synced. The existing CNetFulfilledRequestManager quota does not help: it is keyed on peer.addr, a CService that includes the ephemeral source port, and it is per-object, so it is bypassed by reconnecting or by cycling through other known objects.

What was done?

• Add a signature-verification memo to CGovernanceVote, keyed on the verification key, GetSignatureHash() and the signature bytes. GetSignatureHash() is a SerializeHash over exactly the fields that GetSignatureString() signs, so the memo cannot be bypassed by mutating a covered field, and key-type confusion is not possible.
• Add CGovernanceObjectVoteFile::ForEachVote() so GetSyncableVoteInvs visits the stored votes in place instead of GetVotes() copies, which would discard the memo on every walk.

On the fulfilled-request quota: an alternative fix - keying the govsync throttle on IP rather than the full CService - was tried and deliberately abandoned. The quota map is shared with SyncManager's per-peer outbound bookkeeping and is coupled to PeerMisbehaving(20); collapsing NAT-colocated honest peers onto one key would let one attacker get the others discouraged, and it breaks mnsync in tests where every node is on 127.0.0.1. A regression test (fulfilled_request_manager_is_port_sensitive) now pins port-sensitivity as the intended behaviour so this trade is not re-attempted by accident.

Known remaining gap: the residual per-request cost is still O(votes) under cs_store - a SerializeHash, a bloom contains and a masternode lookup per vote - with no working rate limit, and a cold cache after restart pays full price once. A proper request-rate or work budget on MNGOVERNANCESYNC, keyed per CNode rather than per CService, is still needed and is not attempted here.

How Has This Been Tested?

New unit-test file src/test/governance_vote_sync_tests.cpp:

• fulfilled_request_manager_is_port_sensitive - pins the govsync fulfilled-request quota as port-keyed (see above).
• vote_signature_verification_is_cached - a verdict is memoised per key and a different key is a different question.
• stored_vote_file_reuses_signature_cache - the AddVote copy carries the memo warmed by ProcessVote's pre-check, so sync walks answer from it.
• signature_cache_invalidated_by_signed_field_mutation - mutating a signed field (SetTime) leaves no usable memo; the next check re-verifies and rejects.
• signature_cache_does_not_confuse_key_types - a BLS verdict is never served to the CKeyID overload or vice versa.

Full build and test validation is delegated to CI on this PR; the changes were not built locally.

Breaking Changes

None.

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

[coderabbitai]

Walkthrough

Governance votes now memoize ECDSA and BLS signature-verification results using vote content and verification-key data. Vote synchronization iterates stored votes without copying them. New tests cover request quotas, peer-port isolation, repeated checks, cache invalidation, key changes, and separate ECDSA and BLS cache entries.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GetSyncableVoteInvs
  participant CGovernanceObjectVoteFile
  participant CGovernanceVote
  GetSyncableVoteInvs->>CGovernanceObjectVoteFile: iterate stored votes by const reference
  CGovernanceObjectVoteFile-->>GetSyncableVoteInvs: provide stored vote
  GetSyncableVoteInvs->>CGovernanceVote: check signature
  CGovernanceVote-->>GetSyncableVoteInvs: return cached or computed verdict

Loading

Suggested reviewers: knst

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: memoizing governance vote signature checks to reduce governance synchronization cost.
Description check	✅ Passed	The description directly explains the governance synchronization cost, memoization design, tests, limitations, and validation status.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[thepastaclaw]

✅ Final review complete — no blockers (commit 3b03544)

[PastaPastaPasta]

fileVotes.GetVotes copies; fileVotes.GetVoteList takes a reference; so when we later do vote.IsValid; when using GetVotes, nothing is cached, w/ GetVoteList it's cached (as my understanding)

[coderabbitai]

🧹 Nitpick comments (2)

src/governance/vote.cpp (2)

186-206: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Avoid recomputing GetSignatureHash() twice on a cache miss.

GetSignatureHash() re-serializes and hashes the whole vote object. CheckSignature(const CBLSPublicKey&) calls it once to build cache_key and a second time to pass into VerifyInsecure. This duplicate work runs on every fresh verification (new vote or new key), on the mainnet BLS path used for ordinary masternode voting. Since this PR's purpose is to remove redundant signature-related work, cache the hash once and reuse it.

♻️ Proposed fix

 bool CGovernanceVote::CheckSignature(const CBLSPublicKey& pubKey) const
 {
-    const uint256 cache_key{SignatureCacheKey(pubKey, GetSignatureHash(), vchSig)};
+    const uint256 sigHash{GetSignatureHash()};
+    const uint256 cache_key{SignatureCacheKey(pubKey, sigHash, vchSig)};
     if (m_sig_checked && m_sig_check_key == cache_key) {
         return m_sig_valid;
     }

     g_governance_vote_signature_checks.fetch_add(1, std::memory_order_relaxed);

     CBLSSignature sig;
     sig.SetBytes(vchSig, false);
-    const bool valid{sig.VerifyInsecure(pubKey, GetSignatureHash(), false)};
+    const bool valid{sig.VerifyInsecure(pubKey, sigHash, false)};

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/governance/vote.cpp` around lines 186 - 206, Update
CGovernanceVote::CheckSignature so GetSignatureHash() is evaluated once on a
cache miss, store that hash in a local variable, and reuse it both when
constructing cache_key and when calling VerifyInsecure. Preserve the existing
cache-hit behavior and signature validation flow.

---

155-184: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Same redundant GetSignatureHash() call in the TESTNET branch.

For consistency with the fix suggested for CheckSignature(const CBLSPublicKey&), apply the same hash reuse here for CHashSigner::VerifyHash. Impact is smaller since this branch only runs on testnet, but the fix is the same one-line change.

♻️ Proposed fix

 bool CGovernanceVote::CheckSignature(const CKeyID& keyID) const
 {
-    const uint256 cache_key{SignatureCacheKey(keyID, GetSignatureHash(), vchSig)};
+    const uint256 sigHash{GetSignatureHash()};
+    const uint256 cache_key{SignatureCacheKey(keyID, sigHash, vchSig)};
     if (m_sig_checked && m_sig_check_key == cache_key) {
         return m_sig_valid;
     }
@@
     if (Params().NetworkIDString() == CBaseChainParams::TESTNET) {
-        valid = CHashSigner::VerifyHash(GetSignatureHash(), keyID, vchSig, strError);
+        valid = CHashSigner::VerifyHash(sigHash, keyID, vchSig, strError);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/governance/vote.cpp` around lines 155 - 184, Update
CGovernanceVote::CheckSignature(const CKeyID&) so the TESTNET branch computes
GetSignatureHash() once and reuses that value when calling
CHashSigner::VerifyHash, matching the existing hash-reuse fix for the BLS
overload.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/governance/vote.cpp`:
- Around line 186-206: Update CGovernanceVote::CheckSignature so
GetSignatureHash() is evaluated once on a cache miss, store that hash in a local
variable, and reuse it both when constructing cache_key and when calling
VerifyInsecure. Preserve the existing cache-hit behavior and signature
validation flow.
- Around line 155-184: Update CGovernanceVote::CheckSignature(const CKeyID&) so
the TESTNET branch computes GetSignatureHash() once and reuses that value when
calling CHashSigner::VerifyHash, matching the existing hash-reuse fix for the
BLS overload.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a92fbd0d-4564-47ce-b5c6-f20d3f0d5ca9

📥 Commits

Reviewing files that changed from the base of the PR and between c751ae4 and dc3d45f.

📒 Files selected for processing (7)

• src/Makefile.test.include
• src/governance/governance.cpp
• src/governance/net_governance.cpp
• src/governance/vote.cpp
• src/governance/vote.h
• src/governance/votedb.h
• src/test/governance_vote_sync_tests.cpp

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef602678a9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Serialize BLS cache keys using the verification scheme

During a BLS activation or reorg where bls_legacy_scheme changes, ss << key serializes CBLSPublicKey using that mutable global, while the cached operation is always VerifyInsecure(..., false). Legacy serialization of a key can equal basic serialization of its negation, so if an operator key rotates from P to -P across such a boundary, a valid verdict cached for P can be returned for -P, allowing the old vote to survive revalidation and be advertised despite failing verification. Serialize the fingerprint key explicitly with the non-legacy scheme used by verification.

AGENTS.md reference: AGENTS.md:L169-L170

Useful? React with 👍 / 👎.

[thepastaclaw]

Preliminary review — Codex only

The final tree implements the governance vote signature memo safely and leaves fulfilled-request keying unchanged, with relevant regression coverage. However, the commit stack deliberately includes a failing test, an unsafe signature-cache implementation, and a network-throttle change later reverted after breaking functional tests, so the history must be rewritten before merge.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking | 🟡 1 suggestion(s)

2 additional finding(s) omitted (not in diff).

🤖 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 `<commit:7647f98>`:
- [BLOCKING] <commit:7647f98>:1: Rewrite the known-red throttle-and-revert sequence
  The stack is not bisectable. Commit 7647f983a51 explicitly adds a test that its message says fails until the next commit. Commit 00b3b2bcadb then makes fulfilled requests port-agnostic, which 747cbdc8bd6 later reverts because it causes honest peers sharing an address to accumulate misbehavior scores and makes feature_governance.py fail. The same intermediate commit caches signature verdicts using only the verification key, allowing signed-content mutation to reuse a stale verdict until cde8a4668a0 repairs it. Rewrite the stack so the signature memo is introduced with its final content-sensitive fingerprint and tests, remove the netfulfilledman.cpp change/revert, and introduce any retained port-sensitivity test directly in passing form. Preserve the rejected throttle rationale in the final commit message or test comments rather than permanent reverted history.

In `<commit:43eefde>`:
- [SUGGESTION] <commit:43eefde>:1: Fold cleanup of newly introduced comments and vote traversal API
  Commit 43eefdea957 only rewrites or removes comments introduced earlier in this stack, including deleting the net_governance.cpp explanation added by 747cbdc8bd6. Similarly, 7f40ff728c3 replaces GetVoteList(), which this stack had just introduced, with the final ForEachVote() API. Fold these review-time corrections into the commits that introduce the signature memo and stored-vote traversal so permanent history contains the final documentation and API directly.

[thepastaclaw]

Final validation — Codex + Sonnet

The rewritten one-commit range resolves both prior history findings and directly introduces the final content-sensitive signature memo and callback-based vote traversal. The implementation is sound, but the fixed-scheme BLS key encoding protects a subtle security invariant that is not covered by the new regression tests.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — final_verifier (fallback)
• Sonnet reviewers: claude-sonnet-5 — general (failed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (completed)

🟡 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/governance/vote.cpp`:
- [SUGGESTION] src/governance/vote.cpp:41-43: Add a regression for cross-scheme BLS fingerprint collisions
  The explicit non-legacy key encoding prevents a valid cached verdict from being reused for a different point after `bls_legacy_scheme` changes, but the new tests do not exercise this invariant. For a public key whose legacy sign bit is set, parsing its legacy bytes under the basic scheme produces the opposite point, and `basic(-P)` has the same bytes as `legacy(P)`. A future refactor from `ToBytes(false)` back to generic serialization would therefore let a verdict cached for P under the legacy global scheme be returned for -P under the basic scheme. Add a regression that selects such a key, warms a valid verdict for P while the global scheme is legacy, parses `P.ToBytes(true)` as a basic public key, switches the global scheme to basic, and verifies that the opposite key has no memoized verdict and fails verification. Restore the global scheme on test exit.

[thepastaclaw]

🟡 Suggestion: Add a regression for cross-scheme BLS fingerprint collisions

The explicit non-legacy key encoding prevents a valid cached verdict from being reused for a different point after bls_legacy_scheme changes, but the new tests do not exercise this invariant. For a public key whose legacy sign bit is set, parsing its legacy bytes under the basic scheme produces the opposite point, and basic(-P) has the same bytes as legacy(P). A future refactor from ToBytes(false) back to generic serialization would therefore let a verdict cached for P under the legacy global scheme be returned for -P under the basic scheme. Add a regression that selects such a key, warms a valid verdict for P while the global scheme is legacy, parses P.ToBytes(true) as a basic public key, switches the global scheme to basic, and verifies that the opposite key has no memoized verdict and fails verification. Restore the global scheme on test exit.

_{source: ['codex']}

[PastaPastaPasta]

Correct observation on the aliasing mechanism, but skipping the regression test as the guarded ordering is unreachable on mainnet.

The exploit requires a verdict to be cached while bls_legacy_scheme is legacy and then queried after it flips to basic. On mainnet the global tracks v19 activation (locked in mid-2023): once a node is synced it is permanently basic, and flipping back would take a reorg across the activation boundary, thousands of blocks deep. Governance sync — the only path that warms these memos — does not run until the node is synced, i.e. until the global is already parked at basic. So even with the hypothesized refactor regression, the bad ordering cannot occur on mainnet; the residual exposure is limited to regtest/devnet/testnet setups with nearby activation boundaries, where the consequence is test flakiness rather than a signature-check bypass.

Keeping the ToBytes(/*specificLegacyScheme=*/false) hardening plus the code comment as the guard: it removes the footgun outright at zero cost. If a future serialization migration reuses the mutable-global pattern, that is the point to add this tripwire.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[knst]

does it introduce regression? vote.IsValid() is not deterministic call and could be changed as far as tip_mn_list is changed.

If node is out of sync - masternode list can't have the vote's masternode
If new blocks are synced - masternode's collateral can be spend and vote is no more valid.

How does it work?