Repository navigation
Conversation
Reject fresh proposals whose header names a validator other than the scheduled round proposer before ProcessProposal. Preserve reproposals with their original header and existing POL vote checks. Cover predecessor capture, later rounds and valid or invalid POLs; correct existing fixtures to use the proposing validator's identity. Co-Authored-By: OpenAI Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: dashpay/tenderdash/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
✅ Final review complete — no blockers (commit 86887be) · triage: critical |
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
Approved, grudgingly impressed: the proposer check is wired correctly, fails safe, and three independent passes couldn't find a single thing worth blocking on — just a rolling-upgrade caveat worth a sentence in the description, a couple of Go nits (sentinel errors, DRY), a test-coverage gap or two, and a CI-workflow stowaway that probably wanted its own PR.
0 finding(s) posted: 0 inline, 0 in this body.
🤖 Co-authored by Claudius the Magnificent AI Agent
📊 View full HTML review report
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The consensus change correctly validates the scheduled proposer for fresh proposals before processing or voting, while preserving the existing reproposal/POL behavior. One non-blocking consistency issue remains: the new proposer-mismatch rejection does not use the sentinel-error pattern used by the other prevote validation failures; the supplied CI checks are green, with only PR Hygiene still pending.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff changes consensus proposer validation in internal/consensus/state_data.go and related prevoter logic, directly altering a peer-facing consensus rule despite its small size. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 84% left, 5h 6% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer
🤖 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 `internal/consensus/state_data.go`:
- [SUGGESTION] internal/consensus/state_data.go:695-697: Use a sentinel for fresh-proposal proposer mismatch
The new proposer-mismatch branch returns an anonymous formatted error, while the other prevote validation failures use package sentinel errors. This prevents callers and tests from matching the rejection with errors.Is and makes the new consensus-critical failure reason less consistent and observable than its neighboring checks. Define an errPrevoteInvalidProposer sentinel and wrap it in this message; the nil-prevote behavior remains unchanged.
| if !bytes.Equal(s.ProposalBlock.ProposerProTxHash, proposer.ProTxHash) { | ||
| return fmt.Errorf("proposal-block proposer %X does not match round proposer %X", | ||
| s.ProposalBlock.ProposerProTxHash, proposer.ProTxHash) |
There was a problem hiding this comment.
🟡 Suggestion: Use a sentinel for fresh-proposal proposer mismatch
The new proposer-mismatch branch returns an anonymous formatted error, while the other prevote validation failures use package sentinel errors. This prevents callers and tests from matching the rejection with errors.Is and makes the new consensus-critical failure reason less consistent and observable than its neighboring checks. Define an errPrevoteInvalidProposer sentinel and wrap it in this message; the nil-prevote behavior remains unchanged.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
|
Ready for review — |
TL;DR: Reject new block proposals that name a different validator as their author, preventing misuse of that identity to influence subsequent proposer selection.
Issue being fixed or feature implemented
A scheduled proposer can sign a block naming another validator. Header validation checks membership but does not bind the named validator to the scheduled proposer.
User story
As a validator operator, I want nodes to verify who authored a new block so a proposer cannot use another validator's identity to retain proposal control.
Scenario
A validator takes its scheduled turn and submits a new block. Currently, peers accept a header naming its predecessor, which can cause the same validator to be selected again. With this change, peers refuse to vote for that new proposal.
What was done?
Detailed discussion
POLRound == -1), compareProposerProTxHashwithGetProposer(height, round)in the pre-prevote checks. A mismatch produces a nil prevote beforeProcessProposal, including its cache path.How Has This Been Tested?
ProcessProposal.go test -race -tags=deadlock -p 1 ./internal/consensus -count=1passed (92.711s).--new-from-rev=HEADpassed before commit (0 issues); gofmt andgit diff --checkpassed.Breaking Changes
Tightens consensus voting rules immediately, without an activation height or version gate. Updated validators reject fresh proposals with a mismatched author that older validators may accept. No wire-format or public API changes; historical commit validation and block sync are unchanged.
Checklist:
No separate documentation changes are included; the consensus rule change is described above and in the validation comment.
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
PR Hygiene ·
86887begithub(.github/workflows/claude-code-review.yml) — ktechmidas or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.