fix: memoize governance vote signature checks to bound govsync cost - #7518
fix: memoize governance vote signature checks to bound govsync cost#7518PastaPastaPasta wants to merge 8 commits into
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughGovernance 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
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
🔍 Review in progress — actively reviewing now (commit 23e8e5f) |
| // Iterate the stored votes in place: CheckSignature memoises its verdict on | ||
| // the vote instance, and a GetVotes() copy would discard that memo, so every | ||
| // walk would pay a fresh ECDSA recovery or BLS pairing per vote. | ||
| for (const auto& vote : fileVotes.GetVoteList()) { |
There was a problem hiding this comment.
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/governance/vote.cpp (2)
186-206: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid recomputing
GetSignatureHash()twice on a cache miss.
GetSignatureHash()re-serializes and hashes the whole vote object.CheckSignature(const CBLSPublicKey&)calls it once to buildcache_keyand a second time to pass intoVerifyInsecure. 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 winSame redundant
GetSignatureHash()call in the TESTNET branch.For consistency with the fix suggested for
CheckSignature(const CBLSPublicKey&), apply the same hash reuse here forCHashSigner::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
📒 Files selected for processing (7)
src/Makefile.test.includesrc/governance/governance.cppsrc/governance/net_governance.cppsrc/governance/vote.cppsrc/governance/vote.hsrc/governance/votedb.hsrc/test/governance_vote_sync_tests.cpp
3414191 to
7f40ff7
Compare
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If these PRs merge firstThis PR will likely need a rebase:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| HashWriter ss{}; | ||
| ss << key << sigHash << vchSig; |
There was a problem hiding this comment.
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
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
A single MNGOVERNANCESYNC vote-sync request is rate-limited via CNetFulfilledRequestManager keyed on peer.addr. For inbound peers that address includes the ephemeral TCP source port, so reconnecting or opening parallel connections used to mint a fresh throttle key and re-run the expensive per-vote signature walk. Add a unit test that records a fulfilled request for one source port and asserts the same request from the same IP with a different port is still throttled. Pre-fix this fails; the following fix makes it pass.
MNGOVERNANCESYNC vote-sync walked every stored vote under cs_store and re-ran ECDSA/BLS verification on each request. The only throttle was CNetFulfilledRequestManager keyed on peer.addr including the ephemeral source port, so reconnecting reset the quota and the first request per (connection, object) still paid full verification cost. Key fulfilled requests on the peer network address with port zero so reconnects from the same host share a quota, memoize CheckSignature results on CGovernanceVote (invalidated on key or signature change), and iterate the live vote list in GetSyncableVoteInvs so that memo survives the sync path. Narrowing cs_store is deferred: the expensive work is now memoized and throttled; moving the loop off the lock needs a careful snapshot design.
The memo introduced for the govsync re-verification DoS was keyed on the verification key alone. nTime is part of the signed payload but is mutable via SetTime(), which governance.cpp and object.cpp call on reconstructed votes before verification, so a mutated vote could reuse a verdict computed for content it never carried -- a signature-check bypass. Key the memo on the verification key, the signature hash and the signature bytes, so any change to signed content misses the cache and re-verifies. Add a regression test asserting a mutated vote is rejected.
Keying mapFulfilledRequests on the network address without the port traded a CPU DoS for a remotely triggerable ban of honest peers. The map is shared with SyncManager's per-peer outbound bookkeeping (full-sync, governance-sync, spork-sync, mempool-sync), and the govsync quota is coupled to PeerMisbehaving(20). Several honest nodes legitimately share one address (NAT, colocation, CI), so collapsing them onto one key makes the second honest peer that asks about an object look like a repeat offender; the ban score escalates to DISCOURAGE THRESHOLD EXCEEDED. This was measured, not theorised: with the port stripped, feature_governance.py fails in both wallet variants (every node is on 127.0.0.1) with honest peers scored to 100; with netfulfilledman.cpp restored to upstream it passes. Restore upstream keying and document at the govsync call site why the port must stay in the key. The expensive half of U007/V058/U011 is addressed where the cost lives: the CGovernanceVote signature memo, which is retained. Replace the throttle unit test with one pinning port-sensitivity as intended behaviour. Also document the memo's thread-safety and content-keying invariants, and add a regression test proving a BLS verdict cannot be reused by the CKeyID overload (key-type confusion).
The comments were written as review answers rather than as documentation: they described what the code used to do, what this change fixes, and what was measured while making it. None of that survives the merge, and "re-running BLS/ECDSA on every govsync is the DoS" is meaningless once there is no diff to read it against. State the standing constraints instead, and keep in each place only the rule a future reader could otherwise violate: do not narrow the signature-memo key. Drop the vote.cpp duplicate of the vote.h memo documentation. Drop the fulfilled-request keying note from net_governance.cpp entirely - it explained a reverted approach, the same reasoning already heads the port-sensitivity unit test, and that copy is the one enforced by assertions. net_governance.cpp is no longer touched by this branch.
The memo was three loose members (m_sig_checked, m_sig_valid, m_sig_check_key) with the rule "compare the fingerprint before trusting the verdict" living only in a comment. Wrap them in a SignatureMemo whose Lookup() takes the fingerprint and returns std::optional<bool>, so a caller cannot reach the verdict without supplying the fingerprint it belongs to. This also drops the invalidation in SetSignature. Since the fingerprint covers vchSig, a signature swap changes the fingerprint and misses on its own; the explicit reset was belt-and-braces for an invariant the fingerprint already provides. signature_cache_invalidated_by_signed_field_mutation covers this.
GetVoteList() returned a reference into state guarded by CGovernanceObject::cs. Holding cs is enforced on GetVoteFile(), but no annotation carries that requirement onto a returned reference, so the idiom used elsewhere in this file -- WITH_LOCK(obj->cs, return obj->GetVoteFile().GetVotes()) -- would silently produce an unguarded read if applied to the list accessor. It is safe with GetVotes() only because that returns a copy. ForEachVote runs the callback inside the traversal, so the caller's lock necessarily covers every vote it sees, and there is no reference for a caller to hold past the lock.
Reserve invs against the known vote count instead of letting it grow geometrically under cs_store and the object's cs, and compute GetSignatureHash() once per CheckSignature rather than once for the memo fingerprint and again for the verifier on the miss path. In the tests, collapse the four copies of the build-sign-attach setup into one MakeSignedVote helper so each case opens with what it is actually about.
ef60267 to
23e8e5f
Compare
2 participants
Issue being fixed or feature implemented
CGovernanceManager::GetSyncableVoteInvs()walks every stored vote for a requested object and callsCGovernanceVote::IsValid(), which unconditionally re-runs full cryptographic verification viaCheckSignature()- ECDSA recovery for proposal funding votes, or a BLS pairing (~1 ms) for operator-key votes. Those signatures were already verified at acceptance inCGovernanceObject::ProcessVote, so the work is pure waste.The walk is driven by the P2P
MNGOVERNANCESYNChandler and runs on the single message-handler thread while holdingcs_storeandgovobj.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
CNetFulfilledRequestManagerquota does not help: it is keyed onpeer.addr, aCServicethat 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?
CGovernanceVote, keyed on the verification key,GetSignatureHash()and the signature bytes.GetSignatureHash()is aSerializeHashover exactly the fields thatGetSignatureString()signs, so the memo cannot be bypassed by mutating a covered field, and key-type confusion is not possible.CGovernanceObjectVoteFile::GetVoteList()soGetSyncableVoteInvsiterates live votes instead of copies.On the reverted commit: an intermediate commit in this branch throttled govsync by IP and is deliberately reverted by the last commit. Keying the throttle on IP alone, combined with a misbehaviour score, would let one attacker get NAT-colocated honest peers discouraged. That trade was not worth it, and the history is kept so the reasoning is visible rather than looking like an oversight.
Known remaining gap: the residual per-request cost is still O(votes) under
cs_store- aSerializeHash, a bloomcontainsand 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 onMNGOVERNANCESYNC, keyed perCNoderather than perCService, is still needed and is not attempted here.How Has This Been Tested?
The first commit adds a regression test proving the govsync fulfilled-request throttle is port-keyed, ordered before the fix.
Full build and test validation is delegated to CI on this PR; the changes were not built locally.
Breaking Changes
None.
Checklist: