Summary
verify_candidate_msg currently checks:
p.consensus_header().prev_block_hash != p.candidate.header().prev_block_hash
and returns ConsensusError::InvalidBlockHash.
In practice this is not meaningful for Candidate: Candidate::header() is derived directly from candidate.header() fields, so this comparison is tautological and the branch is effectively unreachable.
Context
- Check location:
consensus/src/proposal/handler.rs (verify_candidate_msg)
- Candidate header derivation:
node-data/src/message.rs (impl StepMessage for Candidate)
InvalidBlockHash is currently only used in this check.
Why this matters
- We keep an error branch that cannot be hit with the current type shape.
- It can mislead maintainers into thinking we validate an external header/payload mismatch here.
- It reduces testability and clarity of intent.
Proposed alternatives
Option A (preferred): move consistency check to message envelope vs candidate payload
In ProposalHandler::verify, compare msg.header (wire envelope) with candidate block header fields:
msg.header.prev_block_hash vs candidate.header().prev_block_hash
msg.header.round vs candidate.header().height
msg.header.iteration vs candidate.header().iteration
Then:
- keep/repurpose
InvalidBlockHash (or introduce a clearer error like InvalidConsensusHeader), and
- remove the tautological check from
verify_candidate_msg.
Option B (minimal cleanup): remove dead check and dead error variant
- Remove the
InvalidBlockHash check from verify_candidate_msg.
- Remove
ConsensusError::InvalidBlockHash if no longer used.
This is the smallest change if we intentionally do not validate envelope/header consistency.
Option C (transitional)
- Keep existing behavior for compatibility,
- add explicit comment that this branch is structural/defensive,
- create follow-up to implement Option A.
Acceptance criteria
- Agree on one option (A/B/C).
- Implement selected option.
- Add/adjust unit tests:
- If Option A: mismatch between envelope header and candidate header is rejected.
- If Option B: dead branch removed and tests/docs updated accordingly.
Related test coverage (Point 2)
A positive unit test for a valid candidate with non-empty tx/fault payload has been added in PR #4042, alongside size precondition assertions in count-limit tests to ensure they fail for count checks rather than block-size checks.
Summary
verify_candidate_msgcurrently checks:p.consensus_header().prev_block_hash != p.candidate.header().prev_block_hashand returns
ConsensusError::InvalidBlockHash.In practice this is not meaningful for
Candidate:Candidate::header()is derived directly fromcandidate.header()fields, so this comparison is tautological and the branch is effectively unreachable.Context
consensus/src/proposal/handler.rs(verify_candidate_msg)node-data/src/message.rs(impl StepMessage for Candidate)InvalidBlockHashis currently only used in this check.Why this matters
Proposed alternatives
Option A (preferred): move consistency check to message envelope vs candidate payload
In
ProposalHandler::verify, comparemsg.header(wire envelope) with candidate block header fields:msg.header.prev_block_hashvscandidate.header().prev_block_hashmsg.header.roundvscandidate.header().heightmsg.header.iterationvscandidate.header().iterationThen:
InvalidBlockHash(or introduce a clearer error likeInvalidConsensusHeader), andverify_candidate_msg.Option B (minimal cleanup): remove dead check and dead error variant
InvalidBlockHashcheck fromverify_candidate_msg.ConsensusError::InvalidBlockHashif no longer used.This is the smallest change if we intentionally do not validate envelope/header consistency.
Option C (transitional)
Acceptance criteria
Related test coverage (Point 2)
A positive unit test for a valid candidate with non-empty tx/fault payload has been added in PR #4042, alongside size precondition assertions in count-limit tests to ensure they fail for count checks rather than block-size checks.