fix(qbft): validate round change justification consistency with data_round #567

diegomrsantos · 2025-09-02T18:57:41Z

Issue Addressed

This PR improves QBFT round change message handling by implementing defensive validation that aligns with the EEA QBFT v1 specification's construction patterns.

Proposed Changes

Add defensive validation in validate_proposal_justifications() to ensure round change messages follow expected construction patterns
Add corresponding validation in received_round_change() for consistency
Add comprehensive tests verifying the validation logic
Complete documentation for the validate_proposal_justifications function

Implementation Improvement:
This validation implements defense-in-depth by ensuring:

Messages claiming no preparation (data_round == 0) have empty justifications (following spec construction intent)
Consistent message processing behavior across all validation paths
Prevention of unnecessary processing of irrelevant prepare messages

Specification Alignment

EEA QBFT v1 Construction Patterns

While the validRoundChange predicate only constrains payload fields (preparedRound/preparedValue), the specification's construction functions show the intended message shape:

createRoundChange sets roundChangeJustification to {} when there's no prepared block
getRoundChangeJustification returns {} for nodes with no preparation
This establishes the expected construction pattern for honest implementations

Proposal Validation Logic

The isProposalJustification predicate handles gating based on round change payload claims:

If all RC payloads have no preparedRound/preparedValue, enters unprepared branch and ignores prepares
Extra prepares in RCs with preparedRound=None are simply ignored during validation
This defensive check prevents unnecessary processing without affecting correctness

Implementation Rationale

This validation provides:

Construction consistency: Aligns implementation with specification's intended message construction
Resource efficiency: Prevents processing of irrelevant prepare messages
Implementation robustness: Removes potential confusion from inconsistent message patterns
Defense-in-depth: Additional validation layer without changing core protocol behavior

Additional Info

This change represents a defensive implementation improvement that:

Follows the specification's construction guidance more closely
Prevents potential footguns in message processing
Maintains full compatibility with the formal validRoundChange predicate requirements
Adds no breaking changes to existing functionality

The validation mirrors the specification's construction patterns while providing additional implementation robustness.

…round Fix critical consensus vulnerability where malicious nodes could include unvalidated prepare messages in round changes claiming no preparation (data_round=0). Changes: - Add validation in validate_proposal_justifications() to reject round changes with data_round=0 but non-empty prepare justifications - Add validation in received_round_change() for the same vulnerability - Add comprehensive test verifying both malicious and valid cases According to EEA QBFT v1 specification: - If data_round == 0: No prepare justifications should be present - If data_round > 0: Prepare justifications MUST be validated This prevents consensus safety violations where unvalidated prepare messages could bypass validation and corrupt justification logic.

- Clarify that prepare justifications are used to justify the highest prepared value from round changes - Fix incomplete comment that was cut off mid-sentence

…s://github.com/diegomrsantos/anchor into fix/qbft-round-change-justification-validation

Copilot

Pull Request Overview

This PR fixes a critical QBFT consensus vulnerability by implementing validation for round change message consistency with the EEA QBFT v1 specification. The fix ensures that nodes claiming no preparation (data_round == 0) cannot include prepare message justifications, preventing malicious nodes from bypassing consensus safety checks.

Adds validation in validate_proposal_justifications() to reject round change messages with inconsistent data_round and justification fields
Adds validation in received_round_change() with the same consistency checks
Includes comprehensive test coverage verifying the vulnerability fix and valid message acceptance

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
anchor/common/qbft/src/lib.rs	Implements security validation in two functions to ensure round change message consistency between `data_round` and justifications
anchor/common/qbft/src/tests.rs	Adds comprehensive test verifying the vulnerability fix by testing both malicious and valid round change scenarios

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

anchor/common/qbft/src/tests.rs

Copilot · 2025-09-02T21:18:40Z

anchor/common/qbft/src/tests.rs

+        round_change_justification: vec![signed_malicious_prepare], // BUT includes justifications!
+        prepare_justification: vec![],


The test is using round_change_justification field to include prepare messages, but based on the validation logic in lib.rs, this should be checking if prepare justifications are included in a round change message. The field name suggests this should contain round change messages, not prepare messages.

Suggested change

round_change_justification: vec![signed_malicious_prepare], // BUT includes justifications!

prepare_justification: vec![],

round_change_justification: vec![], // Should only include round change messages

prepare_justification: vec![signed_malicious_prepare], // Maliciously includes prepare justification!

@dknopik how about this?

Zacholme7

Makes sense

Zacholme7 · 2025-09-02T21:19:05Z

my bad, didnt see this was draft

diegomrsantos · 2025-09-02T21:21:56Z

my bad, didnt see this was draft

I changed to draft right now, I wanna give another review on this tomorrow with a fresh mind. But thanks anyway ;)

dknopik · 2025-09-03T07:37:28Z

before merging this, let me test this against some fuzzer inputs I saved 🙏

- Move doc comments above #[test] attributes for proper documentation - Convert // comments to /// doc comments for test functions - Ensure consistent documentation style across all tests

- Extract helper functions to reduce test duplication: - create_signed_ssv_message(): Creates SSVMessage boilerplate - create_wrapped_round_change(): Creates round change messages - test_round_change_acceptance(): Tests acceptance/rejection logic - Reduce test from ~200 lines to ~90 lines - Improve maintainability and readability - Same test coverage with cleaner code

- Added DRY principle guidelines to Code Style section - Added detailed Testing Guidelines with code duplication prevention - Emphasized immediate refactoring when duplication is detected - Added proper test documentation placement requirements - Included helper function extraction patterns and examples Prevents future code duplication issues by establishing clear guidelines for when and how to extract helper functions.

- Added guidance to replace/consolidate rather than always add new content - Emphasized concise, essential-only updates - Added periodic review process for file maintenance - Prioritized quality over quantity in guidelines Prevents instruction files from becoming unwieldy over time.

…erence - Added mandatory format & lint step before all commits - Removed 'commit without permission' to respect user preferences - Ensures code quality standards are maintained in learning updates

…ication - Streamline CLAUDE.md testing section to eliminate duplication with tester-subagent - Add comprehensive proactive duplication prevention methodology to tester-subagent - Enforce design-first approach: plan helper functions before writing tests - Add enforcement rule: extract helpers immediately at 3+ line duplication - Replace reactive cleanup approach with mandatory upfront pattern identification

…n-validation

dknopik · 2025-09-03T10:13:15Z

Related ssv spec issue: ssvlabs/ssv-spec#582

It sounds like this is to be added as message validation rule (i.e. in message_validator). Wdyt about that?

diegomrsantos · 2025-09-03T10:23:37Z

Related ssv spec issue: ssvlabs/ssv-spec#582

It sounds like this is to be added as message validation rule (i.e. in message_validator). Wdyt about that?

Isn't it different? data_round == 0 and no justifications is consistent

diegomrsantos · 2025-09-03T10:26:55Z

My initial idea was to implement this kind of validation in the QBFT crate, but I don't recall why I changed my mind. The advantage of having it in the validator is that we reject them faster and penalize the node, but not having this in the QBFT feels wrong.

dknopik · 2025-09-03T11:32:04Z

Isn't it different? data_round == 0 and no justifications is consistent

Oh, right, it is kinda different.

I guess either all three or none of those should be true:

a) has full data
b) data_round != 0
c) has justifications

so it is kinda related?

diegomrsantos · 2025-09-03T12:20:31Z

Isn't it different? data_round == 0 and no justifications is consistent

Oh, right, it is kinda different.

I guess either all three or none of those should be true:

a) has full data b) data_round != 0 c) has justifications

so it is kinda related?

Maybe. How does full data translate to the definition of validRoundChange on https://entethalliance.github.io/client-spec/qbft_spec.html?

mergify · 2025-09-03T13:15:22Z

Some required checks have failed. Could you please take a look @diegomrsantos? 🙏

dknopik · 2025-09-04T17:41:16Z

Maybe. How does full data translate to the definition of validRoundChange on https://entethalliance.github.io/client-spec/qbft_spec.html?

I believe to preparedValue: Optional<Hash>, within UnsignedRoundChange, but do not quote me on that.

mergify · 2025-10-03T16:07:44Z

Hi @diegomrsantos, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

diegomrsantos changed the base branch from stable to release-v0.3.0 September 2, 2025 18:58

diegomrsantos force-pushed the fix/qbft-round-change-justification-validation branch from 9e40627 to adcd9dd Compare September 2, 2025 19:03

diegomrsantos added 3 commits September 2, 2025 21:07

docs(qbft): complete documentation comment for prepare justifications

94a47f7

- Clarify that prepare justifications are used to justify the highest prepared value from round changes - Fix incomplete comment that was cut off mid-sentence

fmt + lint

ac1425a

Merge branch 'fix/qbft-round-change-justification-validation' of http…

9997054

…s://github.com/diegomrsantos/anchor into fix/qbft-round-change-justification-validation

diegomrsantos requested a review from Copilot September 2, 2025 21:17

diegomrsantos marked this pull request as draft September 2, 2025 21:18

Copilot AI reviewed Sep 2, 2025

View reviewed changes

Zacholme7 previously approved these changes Sep 2, 2025

View reviewed changes

diegomrsantos requested a review from dknopik September 2, 2025 21:22

docs(qbft): fix test documentation placement

be8f24a

- Move doc comments above #[test] attributes for proper documentation - Convert // comments to /// doc comments for test functions - Ensure consistent documentation style across all tests

diegomrsantos dismissed Zacholme7’s stale review via be8f24a September 3, 2025 09:02

diegomrsantos added 6 commits September 3, 2025 11:32

fmt

a2d67fa

docs: require format & lint before commits and remove auto-commit ref…

f1cab82

…erence - Added mandatory format & lint step before all commits - Removed 'commit without permission' to respect user preferences - Ensures code quality standards are maintained in learning updates

diegomrsantos self-assigned this Sep 3, 2025

diegomrsantos added the QBFT label Sep 3, 2025

diegomrsantos marked this pull request as ready for review September 3, 2025 09:58

diegomrsantos added 3 commits September 3, 2025 12:01

update wordlist.txt

0efa65f

Merge branch 'release-v0.3.0' into fix/qbft-round-change-justificatio…

3d63637

…n-validation

update wordlist.txt

e462f61

diegomrsantos requested a review from Zacholme7 September 3, 2025 10:06

diegomrsantos added the ready-for-review This PR is ready to be reviewed label Sep 3, 2025

mergify bot added waiting-on-author and removed ready-for-review This PR is ready to be reviewed labels Sep 3, 2025

refactor(qbft): tone down test language

038f9db

diegomrsantos force-pushed the fix/qbft-round-change-justification-validation branch from 8292880 to 038f9db Compare September 3, 2025 13:21

mergify bot closed this Oct 3, 2025

mergify bot added the stale label Oct 3, 2025

		round_change_justification: vec![signed_malicious_prepare], // BUT includes justifications!
		prepare_justification: vec![],

fix(qbft): validate round change justification consistency with data_round #567

fix(qbft): validate round change justification consistency with data_round #567

Uh oh!

Conversation

diegomrsantos commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Specification Alignment

EEA QBFT v1 Construction Patterns

Proposal Validation Logic

Implementation Rationale

Additional Info

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

diegomrsantos Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Zacholme7 left a comment

Choose a reason for hiding this comment

Uh oh!

Zacholme7 commented Sep 2, 2025

Uh oh!

diegomrsantos commented Sep 2, 2025

Uh oh!

dknopik commented Sep 3, 2025

Uh oh!

dknopik commented Sep 3, 2025

Uh oh!

diegomrsantos commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegomrsantos commented Sep 3, 2025

Uh oh!

dknopik commented Sep 3, 2025

Uh oh!

diegomrsantos commented Sep 3, 2025

Uh oh!

mergify bot commented Sep 3, 2025

Uh oh!

dknopik commented Sep 4, 2025

Uh oh!

mergify bot commented Oct 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

diegomrsantos commented Sep 2, 2025 •

edited

Loading

diegomrsantos commented Sep 3, 2025 •

edited

Loading