Skip to content

Verify proposer attestation identity matches block proposer #220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pablodeymo merged 1 commit into from
Mar 13, 2026
Merged

Verify proposer attestation identity matches block proposer #220

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions crates/blockchain/src/store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,14 @@ pub enum StoreError {

#[error("Validator {validator_index} is not the proposer for slot {slot}")]
NotProposer { validator_index: u64, slot: u64 },

#[error(
"Proposer attestation validator_id {attestation_id} does not match block proposer_index {proposer_index}"
)]
ProposerAttestationMismatch {
attestation_id: u64,
proposer_index: u64,
},
Comment on lines +913 to +919
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field name attestation_id is ambiguous

The error message renders the field as validator_id (the description says "validator_id {attestation_id}"), but the struct field is named attestation_id. This slight mismatch between the struct field name and the error message text can make it harder to correlate a runtime error string back to the source struct when debugging.

A more self-documenting name would make the relationship explicit:

Suggested change
#[error(
"Proposer attestation validator_id {attestation_id} does not match block proposer_index {proposer_index}"
)]
ProposerAttestationMismatch {
attestation_id: u64,
proposer_index: u64,
},
#[error(
"Proposer attestation validator_id {attestation_validator_id} does not match block proposer_index {proposer_index}"
)]
ProposerAttestationMismatch {
attestation_validator_id: u64,
proposer_index: u64,
},

(The construction site at line 1224 would need to update the field name accordingly.)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 913-919

Comment:
**Field name `attestation_id` is ambiguous**

The error message renders the field as `validator_id` (the description says "validator_id {attestation_id}"), but the struct field is named `attestation_id`. This slight mismatch between the struct field name and the error message text can make it harder to correlate a runtime error string back to the source struct when debugging.

A more self-documenting name would make the relationship explicit:

```suggestion
    #[error(
        "Proposer attestation validator_id {attestation_validator_id} does not match block proposer_index {proposer_index}"
    )]
    ProposerAttestationMismatch {
        attestation_validator_id: u64,
        proposer_index: u64,
    },
```

(The construction site at line 1224 would need to update the field name accordingly.)

How can I resolve this? If you propose a fix, please make it concise.

}

/// Build an AggregationBits bitfield from a list of validator indices.
Expand Down Expand Up @@ -1211,6 +1219,13 @@ fn verify_signatures(

let proposer_attestation = &signed_block.message.proposer_attestation;

if proposer_attestation.validator_id != block.proposer_index {
return Err(StoreError::ProposerAttestationMismatch {
attestation_id: proposer_attestation.validator_id,
proposer_index: block.proposer_index,
});
}
Comment on lines +1222 to +1227
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No unit test for the new mismatch check

The new validation path (ProposerAttestationMismatch) is security-critical but has no dedicated unit test. The existing module already has verify_signatures_rejects_participants_mismatch as a pattern to follow. Without a test here, a future refactor could accidentally remove or bypass this guard and no regression would catch it.

Consider adding a unit test analogous to the existing one, e.g.:

#[test]
fn verify_signatures_rejects_proposer_attestation_mismatch() {
    let state = State::from_genesis(1000, vec![]);

    let attestation_data = AttestationData {
        slot: 0,
        head: Checkpoint::default(),
        target: Checkpoint::default(),
        source: Checkpoint::default(),
    };

    // block.proposer_index = 0, but proposer_attestation.validator_id = 1 — mismatch
    let signed_block = SignedBlockWithAttestation {
        message: BlockWithAttestation {
            block: Block {
                slot: 0,
                proposer_index: 0,  // actual proposer
                parent_root: H256::ZERO,
                state_root: H256::ZERO,
                body: BlockBody { attestations: AggregatedAttestations::new(vec![]).unwrap() },
            },
            proposer_attestation: Attestation {
                validator_id: 1, // different validator — triggers the new guard
                data: attestation_data,
            },
        },
        signature: BlockSignatures {
            attestation_signatures: ssz_types::VariableList::new(vec![]).unwrap(),
            proposer_signature: ssz_types::FixedVector::from_elem(0),
        },
    };

    let result = verify_signatures(&state, &signed_block);
    assert!(
        matches!(
            result,
            Err(StoreError::ProposerAttestationMismatch {
                attestation_id: 1,
                proposer_index: 0,
            })
        ),
        "Expected ProposerAttestationMismatch, got: {result:?}"
    );
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1222-1227

Comment:
**No unit test for the new mismatch check**

The new validation path (`ProposerAttestationMismatch`) is security-critical but has no dedicated unit test. The existing module already has `verify_signatures_rejects_participants_mismatch` as a pattern to follow. Without a test here, a future refactor could accidentally remove or bypass this guard and no regression would catch it.

Consider adding a unit test analogous to the existing one, e.g.:

```rust
#[test]
fn verify_signatures_rejects_proposer_attestation_mismatch() {
    let state = State::from_genesis(1000, vec![]);

    let attestation_data = AttestationData {
        slot: 0,
        head: Checkpoint::default(),
        target: Checkpoint::default(),
        source: Checkpoint::default(),
    };

    // block.proposer_index = 0, but proposer_attestation.validator_id = 1 — mismatch
    let signed_block = SignedBlockWithAttestation {
        message: BlockWithAttestation {
            block: Block {
                slot: 0,
                proposer_index: 0,  // actual proposer
                parent_root: H256::ZERO,
                state_root: H256::ZERO,
                body: BlockBody { attestations: AggregatedAttestations::new(vec![]).unwrap() },
            },
            proposer_attestation: Attestation {
                validator_id: 1, // different validator — triggers the new guard
                data: attestation_data,
            },
        },
        signature: BlockSignatures {
            attestation_signatures: ssz_types::VariableList::new(vec![]).unwrap(),
            proposer_signature: ssz_types::FixedVector::from_elem(0),
        },
    };

    let result = verify_signatures(&state, &signed_block);
    assert!(
        matches!(
            result,
            Err(StoreError::ProposerAttestationMismatch {
                attestation_id: 1,
                proposer_index: 0,
            })
        ),
        "Expected ProposerAttestationMismatch, got: {result:?}"
    );
}
```

How can I resolve this? If you propose a fix, please make it concise.


let proposer_signature =
ValidatorSignature::from_bytes(&signed_block.signature.proposer_signature)
.map_err(|_| StoreError::ProposerSignatureDecodingFailed)?;
Expand Down
Loading