fix: stop caching invalid governance votes - #7569
Conversation
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 this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
|
Warning Review limit reached
Next review available in: 33 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 (1)
WalkthroughGovernance vote processing no longer caches permanently invalid votes. Valid votes remain indexed by Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 6a06268) |
2cc4932 to
f57cb59
Compare
PastaPastaPasta
changed the title
f57cb59 to
6a06268
Compare
![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: f57cb5974c
ℹ️ 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".
| bool fOk = govobj.ProcessVote(m_mn_metaman, fRateChecksEnabled, tip_mn_list, vote, exception); | ||
| if (fOk) { | ||
| fOk = cmapVoteToObject.Insert(nHashVote, it->second); | ||
| } else if (exception.GetType() == GOVERNANCE_EXCEPTION_PERMANENT_ERROR && exception.GetNodePenalty() == 20) { | ||
| cmapInvalidVotes.Insert(nHashVote, vote); | ||
| } |
There was a problem hiding this comment.
When peers repeatedly advertise and serve the same invalid BLS or ECDSA vote for a known governance object, each wire message is deserialized into a fresh CGovernanceVote, so its per-instance signature memo starts empty. Because this branch now stores only successful votes, ProcessVote repeats the expensive signature verification while holding cs_store, and ConfirmInventoryRequest continues requesting the hash because cmapVoteToObject never contains it. The removed global cache bounded exact repeats to one verification; retain an invalid cache keyed by the semantic vote hash plus signature instead of eliminating it.
AGENTS.md reference: AGENTS.md:L162-L171
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
as discussed in the PR description; this was the first thought, but was decided against. An attacker can just twiddle with bits inside the vote to make more and more invalid votes. they are limited to 5 before their connection is dropped.
Avoiding the verify operation is fine, but isn't much (5 BLS verifies or whatever). And introduces bugs such as what this is fixing.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 6a06268, the change removes semantic-hash caching of invalid governance votes while preserving valid-vote deduplication and the version-16 governance.dat field layout; the ECDSA, BLS, and persistence tests directly cover the intended regressions. The earlier exact-replay caching suggestion is non-actionable because each invalid response incurs a 20-point peer penalty and an attacker can trivially bypass a signature-aware cache by varying the signature or other hashed fields.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
|
LGTM |
2 participants
Issue being fixed or feature implemented
Governance cached permanently rejected votes by their semantic vote hash, which excludes the signature. A forged signature cached first could therefore suppress a later legitimate signature over the same vote fields.
The cache did not provide meaningful protection against adaptive verification work: a peer can always produce a fresh cache miss by changing the signature, timestamp, or parent hash. Invalid votes already carry a misbehavior penalty of 20, discouraging a peer after five failures.
What was done?
governance.datversion and field layout by consuming legacy invalid-cache entries during deserialization and writing the historical field back empty.Valid-vote inventory and deduplication remain keyed by the existing semantic vote hash.
How Has This Been Tested?
make -j4./src/test/test_dash --run_test=governance_vote_processing_tests./src/test/test_dash --run_test=cachemap_testsmake check -j4test/lint/all-lint.pyTested on macOS arm64 using the repository's depends toolchain.
Breaking Changes
None. This does not change vote serialization, inventory hashes, protocol versions, database versions, consensus rules, or valid-vote deduplication.
Checklist:
This pull request was created by Codex.