fix(blockchain): count lean_attestations_valid_total on gossip only

[MegaRedHand]

Problem

lean_attestations_valid_total is defined by leanSpec as the running count of attestations that pass all validation checks (paired with lean_attestations_invalid_total, "rejected during validation"). It is a validation-pipeline counter; leanSpec increments it once per validated gossip attestation (source="gossip").

ethlambda additionally incremented it once per participating validator for every block-included attestation (on_block_core), which:

• broke valid/invalid symmetry — inc_attestations_invalid only fires on the gossip validation path (store.rs:391, :479), so valid was inflated with no matching invalid counterpart;
• double-counted votes already counted when first seen on gossip;
• mixed units — a gossip aggregate counts as 1, while the same voters count as N when packed into a block.

Change

Drop the block-inclusion counting so the counter measures exactly the gossip validation pipeline, matching leanSpec. Votes arriving inside a block remain counted by lean_state_transition_attestations_processed_total. Every remaining call site increments by one, so the helper is simplified to a no-argument inc().

Verification

make fmt, make lint (clean), cargo test -p ethlambda-blockchain (46 unit tests pass), full workspace build. Fork-choice/signature spec tests require downloaded fixtures (absent in this env) and are unaffected by a metrics-only change.

[github-actions]

🤖 Kimi Code Review

The PR correctly fixes a metric semantic issue where block-included attestations were being counted in the gossip validation pipeline counter. The change aligns lean_attestations_valid_total with its intended purpose (gossip validation only) and maintains symmetry with inc_attestations_invalid.

crates/blockchain/src/metrics.rs:709-716

• The signature change from inc_attestations_valid(count: u64) to inc_attestations_valid() is appropriate given the new semantics (one increment per gossip message).
• Documentation clearly explains the rationale and references the complementary metric.

crates/blockchain/src/store.rs:685-692

• Removal of the block attestation counting loop is correct. The previous code was conflating two distinct concepts:
  1. Individual attestations validated through gossip (per-message)
  2. Aggregated votes processed inside state transition (per-validator-bit)
• The comment correctly notes that block attestations should be tracked by lean_state_transition_attestations_processed_total instead.

Verification needed:
Ensure that lean_state_transition_attestations_processed_total is actually being incremented elsewhere when processing block attestations during state transition. If not, this PR would result in loss of visibility for block-attestation counts.

Minor suggestion:
Consider adding a debug_assert or metric consistency check to ensure the gossip invalid counter and valid counter are mutually exclusive (i.e., an attestation that fails validation doesn't increment both), though this is likely handled by the control flow in on_gossip_attestation.

Overall: Correct and safe change.

---

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

[github-actions]

🤖 Claude Code Review

This is a small, well-scoped metrics fix. The change is correct and matches its stated intent.

Review

Correctness: The fix is sound. Removing the block-inclusion increment in on_block_core (store.rs:685) correctly stops double-counting votes that were already counted once via inc_attestations_valid() on the gossip path (store.rs:430, 539), and restores symmetry with inc_attestations_invalid, which only ever fires on the gossip validation path. The remaining call sites all increment by exactly one attestation event, so collapsing inc_attestations_valid(count: u64) to a no-arg inc_attestations_valid() (metrics.rs:709-716) is a correct simplification, not just cosmetic — it removes the possibility of a future caller reintroducing count-based double-counting.

Verification of no leftover dependents: validator_indices is still imported and used elsewhere in store.rs (fork choice weight calculations at lines 1099/1123) and in aggregation.rs/reaggregate.rs/coverage.rs, so no dead-code/unused-import fallout from removing the loop in on_block_core.

Documentation: The doc comments added (metrics.rs:709-713, store.rs:685-689) clearly explain the invariant and point to the companion counter (lean_state_transition_attestations_processed_total) that now exclusively covers block-included votes — this will help avoid regressing the same bug later.

Minor observations (non-blocking):

• No test asserts the counter's new gossip-only semantics (e.g., a test building a block with attestations and checking lean_attestations_valid_total stays flat while lean_state_transition_attestations_processed_total increases). Given this is a metrics-correctness fix specifically about avoiding double counting, a regression test would have real value, but it's reasonable to skip for a small metrics-only patch.
• The removed loop's comment placeholder (store.rs:685-689) is a little verbose for what is essentially a "nothing happens here" note — could be trimmed to one line — but it's justified since it documents a non-obvious historical decision (why counting isn't happening) rather than restating obvious code.

Overall: a clean, well-justified metrics correctness fix with good commit/PR documentation. No functional or safety concerns.

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. The new comment in store.rs is inaccurate: block-included “votes” are not counted by lean_state_transition_attestations_processed_total. That counter increments once per aggregated attestation object, not once per participating validator, in state_transition/src/lib.rs and state_transition/src/lib.rs. This PR therefore removes participant-level counting from block import without a replacement, which will materially undercount busy slots and can break existing dashboards/alerts.

2. The new doc comment on metrics.rs says inc_attestations_valid is “symmetric with inc_attestations_invalid”, but the implementation is not symmetric. inc_attestations_valid() is called only after full verification in store.rs and store.rs, while inc_attestations_invalid() is only emitted for validate_attestation_data failures in store.rs and store.rs. Invalid validator indices, signature decode/verification failures, empty aggregation bits, and bad aggregate proofs are all excluded from the invalid counter, so the new comment is misleading.

No consensus, fork-choice, or memory-safety issues stood out in this diff beyond the metric-semantics problem above.

I couldn’t run cargo test -p ethlambda-blockchain --lib --quiet in this environment because rustup tried to write under /home/runner/.rustup, which is read-only here.

---

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

[greptile-apps]

Greptile Summary

This PR makes the valid-attestation metric count successful gossip validations only. The main changes are:

• Removes per-validator increments for block-included attestations.
• Changes the valid-attestation helper to increment by one.
• Updates both successful gossip-validation call sites.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.
• Both remaining increments occur after successful gossip validation.
• Block attestations remain tracked by the state-transition processed-attestations metric.

Important Files Changed

Filename	Overview
crates/blockchain/src/metrics.rs	Changes the valid-attestation metric helper from an arbitrary increment to a single increment.
crates/blockchain/src/store.rs	Updates both gossip call sites and removes valid-attestation increments from block processing.

_{Reviews (1): Last reviewed commit: "fix(blockchain): count lean_attestations..." | Re-trigger Greptile}