test: skip leanSpec fixture tests while fixtures lag leanVM

[MegaRedHand]

Motivation

The latest leanSpec fixture release is still generated with the leanVM format from before #606. Its validator pubkeys no longer match ours, so every spectest fails to decode its fixture (ValidatorPubkey length != 32) and CI on main has been red since #606 merged.

Description

Temporarily skip every test that reads leanSpec fixtures, until leanSpec publishes fixtures in the current leanVM format.

• Mark the four datatest-stable harnesses test = false, so cargo test / make test no longer run them:
  • forkchoice_spectests, signature_spectests (ethlambda-blockchain)
  • stf_spectests (ethlambda-state-transition)
  • ssz_spectests (ethlambda-types)
• --all-targets also leaves test = false targets out, so clippy would stop compiling them. Add a clippy invocation that names them, to CI's lint job and to make lint, so they don't rot while skipped.
• They can still be run on demand: cargo test --profile release-fast --test <name>.

The fixture download in CI and the leanSpec/fixtures prerequisite of make test are left as they are, so re-enabling the tests is a plain revert of this PR.

[github-actions]

🤖 Kimi Code Review

I'll review this PR which disables certain spectests from running by default while ensuring they still compile via explicit clippy invocations.

Overall Assessment

This is a reasonable temporary workaround for stale test fixtures, but there are several issues with the approach that could lead to silent breakage and maintenance problems.

---

Issues Found

1. CI and Makefile Divergence Risk (.github/workflows/ci.yml:52-56, Makefile:11-13)

The list of test names is duplicated in three places (CI, Makefile, and individual Cargo.toml files). This is prone to drift. If someone adds a new spectest, they must remember to update both the CI and Makefile.

Suggestion: Consider a single source of truth. A simple shell loop or a dedicated script would reduce this risk:

# Makefile
SPECTESTS := forkchoice_spectests signature_spectests stf_spectests ssz_spectests

lint: ## 🔍 Run clippy on all workspace crates
	cargo clippy --locked --workspace --all-targets -- -D warnings
	cargo clippy --locked --workspace $(foreach t,$(SPECTESTS),--test $(t)) -- -D warnings

---

2. Missing test = false on harness = false Tests — Potential Unintended Side Effect

In crates/blockchain/Cargo.toml:46-50 and other Cargo.toml files, harness = false already prevents these from running as standard tests. Adding test = false makes them binary targets rather than test targets. This changes how cargo treats them:

• They won't be built by cargo test at all (intended)
• They will be built by cargo build --tests? No — test = false excludes them from --tests
• They are only built when explicitly named

Verify: Does cargo clippy --test <name> actually work for test = false targets? According to Cargo docs, --test <name> selects a test target by name. If test = false, the target may not exist as a test target at all.

This may be a bug. If test = false, the target becomes a [[bin]]-like target, and cargo clippy --test <name> might fail with "no test target named <name>".

Please verify this works as expected. If clippy fails, you may need cargo clippy --bin <name> or to use a different approach.

---

3. Comment Inconsistency / Copy-Paste Error

All four Cargo.toml files contain identical comments:

"the released leanSpec fixtures still carry the old leanVM key format and fail to decode"

This is inaccurate for:

• forkchoice_spectests — fork choice tests don't involve key formats
• stf_spectests — state transition tests may not involve signatures/keys
• ssz_spectests — SSZ tests are pure serialization, no keys involved

Only signature_spectests plausibly involves key formats. The others likely fail for different reasons (general fixture staleness). Misleading comments waste debugging time.

Suggestion: Use accurate per-test comments or a generic comment like "fixtures are stale and need updating for current leanVM/leanSpec."

---

4. No Tracking Issue or TODO for Re-enabling

There's no TODO(#issue) or link to track re-enabling these tests. In a blockchain client, disabled consensus tests are a significant risk. Future developers have no visibility into when this workaround should be removed.

Suggestion: Add a comment with a tracking issue:

# TODO(#624): Re-enable when leanSpec fixtures are updated for new key format
test = false

---

5. CI Job Naming (.github/workflows/ci.yml:52)

The comment says "Name them so they keep compiling until they run again" but the job name is "Clippy (skipped spectests)". The intent is clear but the naming could be more precise: "Clippy (stale spectests — compile-only)".

Minor issue.

---

6. datatest-stable Dependency Still Present

The datatest-stable dependency remains in all four crates. If these tests are permanently disabled, consider whether the dependency should be dev-dependencies only (it already is) and whether it can be removed entirely if the tests won't run in CI.

However, since the intent is temporary disablement, keeping the dependency is correct.

---

Security Considerations

Concern	Assessment
Consensus test coverage gap	Medium risk. These tests validate fork choice, signatures, state transition, and SSZ — core consensus logic. Disabling them reduces confidence in correctness.
Silent fixture staleness	The root cause (fixture format mismatch) should be fixed upstream or with a compatibility layer, not worked around indefinitely.
Compilation-only checks	Better than nothing, but doesn't validate behavior against spec.

---

Recommendations Summary

1. Verify cargo clippy --test <name> works with test = false — this is the most critical potential bug.
2. Fix misleading comments in Cargo.toml files to accurately describe why each test is disabled.
3. Add tracking issue references for re-enabling tests.
4. Deduplicate test name lists between CI and Makefile to prevent drift.
5. Set a timeline for fixing or updating the stale fixtures rather than leaving tests permanently disabled.

The mechanical approach is sound if clippy invocation works, but the documentation and maintainability issues are worth addressing before merge.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

🤖 Codex Code Review

Looks good overall. This PR is config-only and does not change consensus logic, state transition code, signature handling, or SSZ implementations, so I don’t see any direct fork-choice / STF / attestation correctness risk in the diff.

• crates/blockchain/Cargo.toml:49, crates/blockchain/Cargo.toml:58, crates/blockchain/state_transition/Cargo.toml:36, crates/common/types/Cargo.toml:37

  • Setting test = false is a reasonable way to keep stale spectests out of cargo test while preserving explicit invocation by name.
  • I don’t see a correctness or security issue here, assuming the team is comfortable with these tests no longer running in the default test target set.

• .github/workflows/ci.yml:56 and Makefile:13

  • Good catch to lint these targets explicitly, since --all-targets skips entries marked test = false.
  • This helps prevent silent bitrot in consensus-adjacent test code.

One thing to watch:

• .github/workflows/ci.yml:56
  • The separate cargo clippy --test ... invocation only covers the four named spectests. If more test = false targets are added later, CI will miss them unless this list is kept in sync.
  • Suggestion: add a short maintainer note near these [[test]] entries or in contributor docs saying any future test = false target must also be added to CI/make lint.

Minor maintainability note:

• Makefile:11 and .github/workflows/ci.yml:52
  • The comments mention “old leanVM key format,” while the practical effect is “skip default test execution, but keep compile/lint coverage.” That intent is clear enough, but a brief note about why explicit --test <name> still works despite test = false could help future readers.

No blocking issues from me. The change is small, targeted, and improves CI coverage for otherwise-skipped spectest binaries.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Claude Code Review

Review of PR 623: skip leanSpec fixture tests while fixtures lag leanVM

The change is small and looks correct. I read the diff but didn't run any of the cargo or make commands.

• test = false on the four datatest-stable targets (forkchoice_spectests, signature_spectests, stf_spectests, ssz_spectests): this is the right way to skip them. cargo test no longer runs them. An explicit --test <name> still builds and runs a test = false target, so the on-demand path in the PR description holds.
• Extra clippy step in CI and make lint: this is a sensible guard against the skipped harnesses no longer compiling. --workspace --test a --test b … resolves each name in whichever package owns it, and all four names exist. The duplicated command line in ci.yml and the Makefile is a small maintenance cost. Adding a fifth spectest would mean editing both, and forgetting one would only surface when the tests are re-enabled.
• Fixture download and leanSpec/fixtures prerequisite left in place: this keeps the revert trivial, as the PR says. The cost is a CI fixture download that nothing uses for now.
• Comment lines inside the Makefile recipe: make echoes them when make lint runs. This is harmless and matches the existing style in the test recipe.

Nits, not blocking

• Add a tracking issue or TODO that names the condition for reverting, for example "leanSpec publishes fixtures on the post-feat(crypto)!: bump to new leanVM version #606 leanVM format". Then the skip can't be forgotten. The four identical Cargo.toml comments could each point to it.
• This turns off consensus coverage: fork choice, state transition, signature and SSZ spectests. Until the revert lands, CI has no spec-conformance check, so avoid merging consensus-logic changes without running the fixtures locally where possible.

Otherwise LGTM.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt