Skip unnecessary payload deserialization, batch attestation inserts, unify validator_indices, and clean up time constants

[pablodeymo]

Summary

Follow-up cleanup from reviewing the devnet 3 merge (#107). Four targeted optimizations and deduplication fixes — no behavior changes.

• Skip payload deserialization in update_safe_target (runs every slot)
• Batch insert_attestation_data_by_root commits in on_block_core (runs every block)
• Unify duplicated aggregation_bits_to_validator_indices into shared validator_indices()
• Derive MILLISECONDS_PER_SLOT from base constants, remove redundant SECONDS_PER_SLOT

Changes in detail

1. Key-only iterators for update_safe_target (crates/storage/src/store.rs, crates/blockchain/src/store.rs)

update_safe_target calls iter_known_aggregated_payloads() and iter_new_aggregated_payloads(), which deserialize Vec<StoredAggregatedPayload> per entry (each containing multi-KB AggregatedSignatureProof objects). It then immediately discards the values and passes only the keys to extract_latest_attestations.

Fix: Added iter_known_aggregated_payload_keys() and iter_new_aggregated_payload_keys() backed by a shared iter_aggregated_payload_keys(table) helper that decodes only the key bytes, skipping value deserialization entirely. update_safe_target now uses these through a HashSet for deduplication.

This runs every slot (every 4 seconds) and scales with validator count.

2. Batch attestation data inserts in on_block_core (crates/storage/src/store.rs, crates/blockchain/src/store.rs)

on_block_core called insert_attestation_data_by_root once per block body attestation plus once for the proposer attestation. Each call opened a separate write batch and committed — N+1 round-trips per block.

Fix: Added insert_attestation_data_by_root_batch that writes all entries in a single batch-commit. on_block_core now collects all attestation data entries (body + proposer) and inserts them in one go. The existing insert_attestation_data_by_root is kept for the single-entry callers (on_gossip_attestation, on_gossip_aggregated_attestation).

3. Unify aggregation_bits_to_validator_indices (crates/common/types/src/attestation.rs, crates/common/types/src/block.rs, crates/blockchain/src/store.rs)

store.rs had a free function aggregation_bits_to_validator_indices(&AggregationBits) -> Vec<u64> that was structurally identical to AggregatedSignatureProof::participant_indices() in block.rs. Both iterate a bitfield and collect set-bit indices.

Fix: Added validator_indices(&AggregationBits) -> impl Iterator<Item = u64> in ethlambda_types::attestation (where AggregationBits is defined). participant_indices() now delegates to it. The local function in store.rs is removed; all 5 call sites use the shared function.

4. Derive MILLISECONDS_PER_SLOT and remove SECONDS_PER_SLOT (crates/blockchain/src/lib.rs, test files)

Three independent time constants were defined:

pub const SECONDS_PER_SLOT: u64 = 4;
pub const MILLISECONDS_PER_SLOT: u64 = 4_000;
pub const MILLISECONDS_PER_INTERVAL: u64 = 800;
pub const INTERVALS_PER_SLOT: u64 = 5;

SECONDS_PER_SLOT was only used in two test files and was redundant. MILLISECONDS_PER_SLOT was independently defined rather than derived, creating a consistency risk if either base constant changes.

Fix: Removed SECONDS_PER_SLOT. Made MILLISECONDS_PER_SLOT = MILLISECONDS_PER_INTERVAL * INTERVALS_PER_SLOT. Updated test callers (forkchoice_spectests.rs, signature_spectests.rs) to use genesis_time * 1000 + slot * MILLISECONDS_PER_SLOT, matching the production arithmetic in get_proposal_head.

How to test

All existing tests cover these changes — no new behavior was introduced.

cargo test --workspace --release
cargo clippy --workspace --tests -- -D warnings
cargo fmt --all -- --check

All 102 tests pass.

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR introduces several improvements to the ethlambda consensus client, focusing on performance optimizations and code cleanup. The changes are generally positive, but there are a few areas that need attention.

Issues Found

1. Critical: Potential Overflow in MILLISECONDS_PER_SLOT

File: crates/blockchain/src/lib.rs:49

pub const MILLISECONDS_PER_SLOT: u64 = MILLISECONDS_PER_INTERVAL * INTERVALS_PER_SLOT;

• Issue: While the calculation is correct (800 * 5 = 4000), this creates a dependency between constants that could lead to overflow if INTERVALS_PER_SLOT is ever changed to a large value.
• Fix: Add a compile-time assertion or use const_assert! to ensure the multiplication doesn't overflow.

2. Performance: Unnecessary Collection in iter_aggregated_payload_keys

File: crates/storage/src/store.rs:824-833

let keys: Vec<_> = view
    .prefix_iterator(table, &[])
    .expect("iterator")
    .filter_map(|res| res.ok())
    .map(|(k, _)| decode_signature_key(&k))
    .collect();
keys.into_iter()

• Issue: The iterator is collected into a Vec and then immediately converted back into an iterator, which allocates memory unnecessarily.
• Fix: Return the iterator directly without collecting:

view.prefix_iterator(table, &[])
    .expect("iterator")
    .filter_map(|res| res.ok())
    .map(|(k, _)| decode_signature_key(&k))

3. Code Duplication: validator_indices Function

File: crates/common/types/src/attestation.rs:68-73

• Issue: While the new validator_indices function is good, there's still the old aggregation_bits_to_validator_indices function in blockchain/src/store.rs that's been removed, but the pattern might exist elsewhere.
• Fix: Ensure all similar patterns are updated to use the new validator_indices function.

4. Minor: Missing Error Handling in Batch Operations

File: crates/storage/src/store.rs:635-650

• Issue: The batch operations use .expect() which will panic on errors. While this might be acceptable for a consensus client, consider whether these should return Result instead.
• Fix: Consider propagating errors instead of panicking, especially for storage operations.

Positive Changes

1. Performance Optimization: The change from deserializing full payloads to only collecting keys in update_safe_target is a good performance improvement.

2. Code Cleanup: Removing the redundant SECONDS_PER_SLOT constant and deriving MILLISECONDS_PER_SLOT from interval constants is cleaner.

3. Batch Operations: Adding insert_attestation_data_by_root_batch reduces the number of database commits, improving performance.

4. DRY Principle: Moving validator_indices to a common location and reusing it across modules improves maintainability.

Security Considerations

• The changes don't appear to introduce new security vulnerabilities
• The signature verification logic remains unchanged
• Fork choice calculations still use the same algorithm, just with more efficient data access

Overall Assessment

The PR is well-structured and introduces meaningful performance improvements. The issues found are minor and easily addressable. The changes align with blockchain client best practices by reducing unnecessary allocations and improving batch operation efficiency.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

No blocking findings in this PR.

I checked the touched paths for fork-choice impact, attestation handling, signature/bitfield semantics, and storage consistency. The behavioral changes look correct:

• Safe-target computation now merges keys from both attestation pools without unnecessary payload deserialization.
• Proposer attestation is still excluded from the same-block fork-choice update.
• validator_indices centralization preserves prior semantics.

One non-blocking review note:

1. Public API compatibility: SECONDS_PER_SLOT was removed from the public ethlambda_blockchain surface (crates/blockchain/src/lib.rs:44). If this crate is consumed externally, consider keeping it as a derived/deprecated alias to avoid a breaking change.

Residual risk:

• I could not run the targeted tests due sandbox rustup permission errors (/home/runner/.rustup/tmp not writable), so this review is static-analysis only.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have all the context needed. Let me write the review.

---

PR #174 Review: Skip payload deserialization, batch attestation inserts, unify validator_indices, derive MILLISECONDS_PER_SLOT

Overall, this is a well-motivated cleanup PR. The four changes are independent, correctly scoped, and the PR description is accurate. Below are my findings, roughly in priority order.

---

Ordering change in on_block_core — safe, but worth a comment

crates/blockchain/src/store.rs, lines 597–606

The refactor moves insert_attestation_data_by_root_batch (which now includes the proposer attestation's data) to run before update_head. The comment at line 604 explains why update_head must precede adding the proposer's payload key, which is still correct — the proposer's key (LatestNewAggregatedPayloads or GossipSignatures) is added after update_head.

However, update_head calls extract_latest_known_attestations, which only iterates over LatestKnownAggregatedPayloads keys and then fetches the corresponding AttestationData from AttestationDataByRoot. Since the proposer's payload key is never in LatestKnownAggregatedPayloads at this point, storing its AttestationData beforehand has no effect on fork choice — the data is only reachable once a matching key exists in the payload tables.

The behavior is safe and semantically equivalent. A brief inline note like "data-only insert is safe before update_head — proposer payload key not yet in fork-choice tables" would help future readers distinguish this from the critical ordering constraint that follows.

---

Vec collection in iter_aggregated_payload_keys — necessary, not redundant

crates/storage/src/store.rs, lines 823–832

The pattern:

let keys: Vec<_> = view.prefix_iterator(...).filter_map(|r| r.ok()).map(...).collect();
keys.into_iter()

is deliberately the same pattern used in iter_aggregated_payloads (lines 808–821). The view borrow must not escape the function (lifetime), so eagerly collecting is required. This is correct. (The Kimi bot's suggestion to "return the iterator directly" would not compile without a significant API refactor.)

---

participant_indices delegation correctness

crates/common/types/src/block.rs, lines 108–111

Old implementation:

(0..self.participants.len()).filter(|&i| self.participants.get(i).unwrap_or(false)).map(|i| i as u64)

New implementation (via validator_indices):

bits.iter().enumerate().filter_map(|(i, bit)| if bit { Some(i as u64) } else { None })

BitList::iter() yields exactly len() elements in order, so these produce the same sequence. The old code used get(i).unwrap_or(false) to guard out-of-bounds, but since i < len(), the unwrap never triggered. The new code is strictly equivalent and cleaner.

---

MILLISECONDS_PER_SLOT test formula equivalence

crates/blockchain/tests/forkchoice_spectests.rs and signature_spectests.rs

Old:

(slot * SECONDS_PER_SLOT + genesis_time) * 1000
= slot * 4_000 + genesis_time * 1_000

New:

genesis_time * 1_000 + slot * MILLISECONDS_PER_SLOT
= genesis_time * 1_000 + slot * 4_000

Algebraically identical. The new form also directly matches the production formula in get_proposal_head (crates/blockchain/src/store.rs:728), which is a genuine improvement in consistency.

---

Silent error swallowing in iter_aggregated_payload_keys

crates/storage/src/store.rs, line 828

.filter_map(|res| res.ok())

This silently drops iterator errors (e.g., I/O failures reading from RocksDB). This is consistent with the pre-existing pattern in iter_aggregated_payloads (line 812), so it's not newly introduced. Since a storage read failure in update_safe_target would cause silent mis-weighting of attestations during fork choice — potentially affecting liveness — it would be worth a tracking issue to decide whether these should at least be logged or surfaced to the caller. Not a blocker for this PR.

---

const overflow concern from other review — not valid

The Kimi bot flagged MILLISECONDS_PER_INTERVAL * INTERVALS_PER_SLOT as a potential overflow risk. In Rust, const arithmetic that overflows is a compile-time error (in debug and release), not a runtime panic. No const_assert! is needed. The current 800 * 5 = 4000 is trivially safe.

---

Missing blank line between constants and impl

crates/blockchain/src/lib.rs, line 49–50

There is no blank line between pub const MILLISECONDS_PER_SLOT = ... and impl BlockChain. This is pre-existing (the previous last constant INTERVALS_PER_SLOT was already directly adjacent to impl), and cargo fmt accepts it. Not blocking.

---

Summary

The four changes are correct:

• Key-only iteration for update_safe_target: correct deduplication via HashSet, avoids deserializing multi-KB signature proofs on every slot.
• Batch attestation data inserts: correct, with the ordering verified safe (data-only insert before update_head doesn't affect fork choice).
• Unified validator_indices: semantically identical to both implementations it replaces.
• Derived MILLISECONDS_PER_SLOT: algebraically equivalent in tests, now matches the production formula.

The only non-trivial concern is the silent swallowing of storage errors in iter_aggregated_payload_keys, which is pre-existing behavior that should eventually be addressed, but doesn't need to block this PR.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Summary

This PR delivers four targeted performance optimizations and code cleanup changes to the blockchain fork choice implementation, with no behavior changes:

• Key-only iterators: Eliminated unnecessary deserialization of multi-KB StoredAggregatedPayload objects in update_safe_target, which runs every slot (4 seconds). The old code deserialized full payloads only to discard them and use keys. Now uses HashSet<SignatureKey> for efficient deduplication.

• Batched attestation inserts: Reduced N+1 database round-trips in on_block_core by collecting all attestation data entries (block body + proposer) and inserting them in a single batch commit instead of individual writes.

• Unified validator indices extraction: Consolidated two duplicate implementations of bitfield-to-indices conversion into a shared validator_indices() iterator in the types module, eliminating code duplication.

• Derived time constants: Removed redundant SECONDS_PER_SLOT and made MILLISECONDS_PER_SLOT derived from base constants (MILLISECONDS_PER_INTERVAL * INTERVALS_PER_SLOT) to prevent inconsistency if base values change.

All changes preserve existing behavior and pass the full test suite (102 tests).

Confidence Score: 5/5

• This PR is safe to merge with no identified risks
• Score reflects well-isolated performance optimizations with comprehensive test coverage (102 passing tests). All changes preserve existing behavior while improving efficiency: key-only iterators eliminate wasteful deserialization, batch inserts reduce database round-trips, and constant derivation prevents future inconsistencies. The refactoring to unify validator_indices extraction eliminates code duplication without changing semantics. Changes are focused on hot-path optimizations (runs every slot/block) with clear performance benefits.
• No files require special attention

Important Files Changed

Filename	Overview
crates/blockchain/src/lib.rs	Removed redundant SECONDS_PER_SLOT constant and derived MILLISECONDS_PER_SLOT from base constants to prevent inconsistency
crates/storage/src/store.rs	Added key-only iterators for aggregated payloads and batch insert method for attestation data to optimize hot-path operations
crates/blockchain/src/store.rs	Replaced full payload deserialization with key-only iterators in update_safe_target, batched attestation inserts, unified validator indices extraction

_{Last reviewed commit: e083b8b}