Extract shared test types to common module

[pablodeymo]

Motivation

crates/blockchain/tests/types.rs (fork choice tests) and crates/blockchain/tests/signature_types.rs (signature tests) duplicate ~14 type definitions with identical fields, serde attributes, and From trait implementations. This creates maintenance burden and risks divergence — any change to shared types (e.g. adding a new field to TestState or Block) must be applied in two places.

Description

Extract the 14 shared types into a new common.rs module that both test files import from:

Type	Purpose
Container<T>	Generic serde wrapper for JSON arrays with data field
Config + From<Config> for ChainConfig	Genesis time config
Checkpoint + From<Checkpoint> for DomainCheckpoint	Root + slot pair
BlockHeader + From<BlockHeader> for block::BlockHeader	Block header fields
Validator + From<Validator> for DomainValidator	Validator index + pubkey
TestState + From<TestState> for State	Full beacon state deserialization
Block + From<Block> for DomainBlock	Block with body
BlockBody + From<BlockBody> for DomainBlockBody	Attestation container
AggregatedAttestation + From impl	Aggregation bits + data
AggregationBits + From impl	Boolean bitfield
AttestationData + From impl	Slot + head/target/source checkpoints
ProposerAttestation + From impl	Validator ID + attestation data
TestInfo	Test metadata (hash, comment, description)
deser_pubkey_hex()	Hex string → ValidatorPubkeyBytes deserializer

Files changed:

• tests/common.rs (new) — shared types with #![allow(dead_code)] at module level
• tests/types.rs — reduced to fork-choice-specific types only (ForkChoiceTestVector, ForkChoiceTest, ForkChoiceStep, BlockStepData, StoreChecks, AttestationCheck), imports shared types via super::common
• tests/signature_types.rs — reduced to signature-specific types only (SSZ types, VerifySignaturesTestVector, TestSignedBlockWithAttestation, ProposerSignature, etc.), imports shared types via super::common
• tests/forkchoice_spectests.rs / tests/signature_spectests.rs — added mod common;
• Cargo.toml — added autotests = false to prevent common.rs, types.rs, and signature_types.rs from being auto-discovered as standalone test binaries

Net result: -575 lines added, +330 lines removed = 245 fewer lines of duplicated code.

Minor normalization

The From<BlockBody> impl differed slightly between files:

• types.rs: VariableList::new(attestations).expect(...)
• signature_types.rs: collect::<Vec<_>>().try_into().expect(...)

Both are equivalent. The common module uses the VariableList::new() form.

How to Test

make fmt    # No formatting changes
make lint   # No warnings
make test   # All 26 forkchoice spec tests pass

Note: signature_spectests have pre-existing failures (fixture format mismatch with ProposerSignature — the JSON provides a hex string where the code expects a struct). These failures are identical on main and are unrelated to this change.

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR performs a significant refactoring by extracting common test types into a shared module (common.rs) and eliminating duplication across test files. The changes are well-structured and improve maintainability.

Issues Found

1. Potential panic in From<BlockBody> conversion (common.rs:185-192)

   Self {
       attestations: VariableList::new(attestations).expect("too many attestations"),
   }

   Using expect here could panic if attestations exceed the limit. Consider proper error handling.

2. Incomplete BitList initialization (common.rs:143-144)

   justified_slots: BitList::with_capacity(0).unwrap(),
   justifications_validators: BitList::with_capacity(0).unwrap(),

   These are initialized with capacity 0 but the actual data from TestState (justified_slots, justifications_validators) is ignored. This appears to be a bug - the data should be converted to BitList properly.

3. Missing validation in pubkey deserialization (common.rs:300-310)
   The deser_pubkey_hex function strips the "0x" prefix but doesn't validate the remaining string contains only hex characters before attempting decode.

4. Inconsistent naming - TestState in common.rs vs direct usage in other files. Consider renaming to avoid confusion with domain types.

Positive Aspects

• Excellent code deduplication - removes ~400 lines of duplicated types
• Proper module organization with clear separation of concerns
• Maintains backward compatibility through From trait implementations
• Good use of Rust's module system with super::common imports

Recommendations

1. Fix the BitList initialization issue by properly converting the input data
2. Replace expect calls with proper error handling
3. Add input validation in deser_pubkey_hex
4. Consider adding unit tests for the conversion functions

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Findings

1. Potentially disabling all integration tests in this crate
   crates/blockchain/Cargo.toml:9 adds autotests = false. That disables Cargo’s automatic discovery of crates/blockchain/tests/*.rs unless explicit [[test]] entries are added. I don’t see new [[test]] stanzas in this diff, so these spec tests may stop running in CI. If the intent is to control test discovery, add explicit [[test]] entries or drop autotests = false.

2. Fixture data for justification bits is silently ignored
   crates/blockchain/tests/common.rs:74-116 keeps the previous behavior of constructing justified_slots and justifications_validators as empty BitList and ignoring the fixture data (justifiedSlots, justificationsValidators). That’s likely OK for current tests, but it means the tests won’t catch regressions involving those fields. If the fixtures include meaningful data, consider populating those bitlists from the containers. This is especially relevant for justification/finalization logic.

Notes

• The refactor to tests/common.rs looks clean and improves maintainability. No security-sensitive logic is touched; changes are confined to test helpers and types.

Residual risks / test gaps

• If autotests = false is left as-is without explicit [[test]], these consensus/sig spec tests won’t run at all, reducing coverage around fork choice and signature verification.

If you want, I can propose the minimal [[test]] entries and/or a small helper to map fixture bitlists into BitList to increase coverage.

---

Automated review by OpenAI Codex · custom prompt

[greptile-apps]

Greptile Summary

Successfully extracted 14 duplicated test type definitions from types.rs and signature_types.rs into a new shared common.rs module, eliminating 245 lines of code duplication. The refactoring includes all shared types (Container, Config, Checkpoint, BlockHeader, Validator, TestState, Block, BlockBody, attestation types, TestInfo, and deser_pubkey_hex helper), with proper From trait implementations preserved.

Key Changes:

• Created tests/common.rs with #![allow(dead_code)] attribute containing all shared types
• Updated types.rs to retain only fork-choice-specific types (ForkChoiceTestVector, ForkChoiceTest, ForkChoiceStep, etc.)
• Updated signature_types.rs to retain only signature-specific types (SSZ types, VerifySignaturesTestVector, ProposerSignature, etc.)
• Added mod common; declarations to both test runner files
• Added autotests = false to Cargo.toml to prevent module files from being discovered as standalone tests

Minor Normalization:
The From<BlockBody> implementation was standardized to use VariableList::new() (previously types.rs used this form while signature_types.rs used .collect::<Vec<_>>().try_into()). Both approaches are functionally equivalent.

No logical changes to test behavior. All existing tests should continue to pass.

Confidence Score: 5/5

• This PR is safe to merge with no risk - it's a pure code deduplication refactoring with no logical changes
• This is a straightforward code deduplication refactoring that extracts identical type definitions into a shared module. The changes are mechanical, preserve all functionality, and introduce no new logic. The standardization of the From<BlockBody> implementation uses equivalent approaches. The autotests = false setting is the correct way to prevent test helper modules from being treated as standalone tests.
• No files require special attention

Important Files Changed

Filename	Overview
crates/blockchain/Cargo.toml	Added autotests = false to prevent test module files from being auto-discovered as standalone test binaries
crates/blockchain/tests/common.rs	New shared types module with 14 extracted types and From implementations, marked with #![allow(dead_code)]
crates/blockchain/tests/types.rs	Removed 282 lines of duplicated types, now imports shared types from super::common, only retains fork-choice-specific types
crates/blockchain/tests/signature_types.rs	Removed ~280 lines of duplicated types, now imports shared types from super::common, only retains signature-specific types

_{Last reviewed commit: 806831e}

[github-actions]

🤖 Claude Code Review

Now I have a complete picture of the PR. Let me write the review.

---

PR Review: Extract shared test types to common module

Summary: Clean refactoring with no logic changes. The 245-line reduction is accurate, and the module boundary design is correct. A few items worth noting:

---

autotests = false is safe

The existing bot reviews flagged missing [[test]] entries, but this is incorrect. The Cargo.toml already has explicit [[test]] stanzas for both forkchoice_spectests and signature_spectests (with harness = false and required-features correctly set). The autotests = false flag is exactly right here — without it, Cargo would try to compile common.rs, types.rs, and signature_types.rs as standalone test binaries, which would fail since they're helper modules, not entry points.

---

justified_slots / justifications_validators silently zeroed — pre-existing gap

common.rs:171-174:

justified_slots: BitList::with_capacity(0).unwrap(),
justifications_validators: BitList::with_capacity(0).unwrap(),

Both fields are initialized empty, ignoring the justifiedSlots: Container<u64> and justificationsValidators: Container<bool> values from the fixture's TestState. This is identical to the original behavior in both types.rs and signature_types.rs, so it's faithfully preserved — but it means fork choice spec tests never exercise initial justification state from fixtures. Worth a follow-up issue if fixtures ever encode meaningful justification bit data at the anchor state.

---

Validator field visibility narrowed

In the old signature_types.rs, Validator.index and Validator.pubkey were pub. In the new common.rs:116-120, they are private:

pub struct Validator {
    index: u64,          // was pub in signature_types.rs
    pubkey: ValidatorPubkeyBytes,  // was pub in signature_types.rs
}

This is fine in practice — nobody outside common.rs accesses these fields directly (the From impl lives in the same module), and the old types.rs also had them private. But if a future test needs to construct or inspect a Validator directly, the private fields will require touching common.rs. Minor, but worth being aware of.

---

deser_pubkey_hex bot review is a false alarm

common.rs:323:

hex::decode(value.strip_prefix("0x").unwrap_or(&value))
    .map_err(|_| D::Error::custom("ValidatorPubkey value is not valid hex"))?

hex::decode already validates that the input contains only valid hex characters before attempting decoding. No additional pre-validation is needed.

---

VariableList::new(attestations).expect("too many attestations") — pre-existing

common.rs:221. The panic message is appropriate for the failure mode. Pre-existing behavior from types.rs; no regression here.

---

Overall

The change is correct and achieves its goal cleanly. No behavior is altered — only duplication removed. The module structure (mod common; in each test binary, #![allow(dead_code)] at file scope) is the right approach for shared test helpers in Rust. Approve with no blocking concerns.

---

Automated review by Claude (Anthropic) · custom prompt

[MegaRedHand]

LGTM