[MegaRedHand]

Motivation

Neither ethlambda beacon nor ethlambda validator took part in sync committees. Other clients' validator clients on our node got a 404 for every sync duty (seen on the fulu->gloas devnet behind #667), our validator client had no sync duties at all, and every block we produced carried an empty sync aggregate, so every sync committee member lost its reward in our slots.

Stacked on #662 and merges feat/beacon-api-missing-endpoints (#642) for POST /eth/v1/validator/duties/sync/{epoch}; the #642 commits disappear from this diff once it lands.

Beacon node

Piece	What
Gossip	sync_committee_{subnet_id} (joined on demand from sync_committee_subscriptions, until until_epoch, carried across fork digests, advertised in MetaData syncnets) and sync_committee_contribution_and_proof (already subscribed, now validated instead of ignored). Every altair rule, with seen caches and their own validation permit pool so the 1/3-slot burst does not starve attestations.
Pool	One SharedSyncCommitteePool filled by gossip and the API: messages by (slot, root, subcommittee) position, best contribution per key.
Blocks	Block N packs, for (N-1, parent_root), the best contribution per subcommittee extended with pooled messages at uncovered positions, verified exactly as process_sync_aggregate checks it (else the empty aggregate). Fulu and gloas; every retry-without-operations path drops it too.
Endpoints	POST /eth/v1/beacon/pool/sync_committees, GET /eth/v1/validator/sync_committee_contribution, POST /eth/v1/validator/contribution_and_proofs, POST /eth/v1/validator/sync_committee_subscriptions (JSON, beacon-APIs shapes, IndexedErrorMessage on partial failure).

Validator client

Sync duties per period (current and next), subnet subscriptions re-sent every epoch, a message per validator at SYNC_MESSAGE_DUE_BPS (_GLOAS from gloas) over the head root (skipped when the head is optimistic), selection proofs and contributions at CONTRIBUTION_DUE_BPS, run concurrently with attestations. New ethlambda_validator_sync_* metrics.

Deviations (in docs/spec_deviations.md)

• Gossip picks the committee by the message's slot (slot + 1's period) against the cached head state, and the domain from the fork schedule, so an honest message is not refused while the head lags a period or fork boundary.
• Subnets are joined on request only, immediately, with no ENR syncnets (ethrex's discovery server cannot replace the served record at runtime).
• Blocks pack only (slot - 1, parent_root), contributions extended with direct messages.
• The validator client signs at the deadline only, not early on a block's arrival.

Validation

• Unit tests per piece (helpers, rules, pool, packing, topics, verdicts, subnets, endpoints, VC service, signing, clock, duties).
• Spec gossip vectors gossip_sync_committee_message and gossip_sync_committee_contribution_and_proof, mainnet: 46 passed (fulu and gloas, including valid_at_period_boundary). Minimal not run locally.
• End to end (validator_client_tests.rs, fulu and gloas): the real validator client against the real router covers all 512 seats on all 4 subnets, contributions with every bit, and the next block carries a full sync aggregate that process_sync_aggregate accepts.
• --lib: p2p 318, rpc 195, state-transition 501, validator 299; clippy and fmt clean.

[github-actions]

🤖 Codex Code Review

I didn’t spot a consensus-critical bug in the sync-committee flow overall; the design is careful about re-validating pooled aggregates before block packing. I do see one correctness/perf issue worth fixing.

• crates/net/rpc/src/beacon/sync_committee.rs:332 computes the subscription clamp from wall_slot(&store) instead of the chain head/state period. That means a machine with a skewed local clock can accept or truncate until_epoch differently from the actual head period the validator duties were derived from. In the worst case, a slightly fast clock can clamp away part of the next-period subscription and cause missed sync gossip right at the boundary. This endpoint should derive the clamp from the same head state / head period used for sync duties, not wall time.

• crates/blockchain/state_transition/src/beacon/sync_committee_pool.rs:138 rebuilds aggregate signatures by cloning all message signatures into a Vec on every contribution() / sync_aggregate() read. That is probably acceptable at current subnet sizes, but it is on the proposer critical path and scales with pooled messages. If this becomes hot, consider caching the best merged contribution per (slot, root, subnet) when inserts happen rather than recomputing during block production.

• crates/net/rpc/src/beacon/validator.rs:116 keeps duplicate validator indices from the request and will emit duplicate duty entries. That is not unsafe, but it is a small API-quality issue and unnecessary work; deduping the parsed indices would make the endpoint more robust.

• crates/blockchain/state_transition/src/beacon/gossip/sync_committee.rs:365 allocates a Vec of participant pubkeys before eth_fast_aggregate_verify. That is standard and fine, but this path is attacker-facing gossip validation. If profiling shows pressure here, a no-allocation/stack-backed path would be worth considering.

Everything else I checked looks sensible:

• sync committee selection is keyed off message slot rather than stale head period, which avoids rejecting honest messages across period/fork boundaries.
• block production verifies the final SyncAggregate again before inclusion, which is the right safety backstop.
• subnet joins are bounded and aggregated by subnet, which is a good guard against RPC abuse.

---

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

[github-actions]

🤖 Claude Code Review

Review: sync committee support (PR 668)

I read the core of the PR: the gossip rules, the pool, the p2p subnet management, the Beacon API handlers and verified_sync_aggregate. I skimmed or skipped the validator-client service, the DTOs and the docs. I did not build or run the tests.

The design is sound and the rules follow the altair p2p spec closely. The departures are deliberate and documented. Findings are minor.

Correctness

• Committee selection (helpers/sync_committee.rs:42). sync_committee_for_slot keys on the period of slot + 1, which matches the spec's "signs at S for inclusion at S+1". A message from the last slot of a period maps to next.

  • slot + 1 can overflow on u64::MAX. Every current caller checks is_current_slot first, so it is unreachable today. check_message and check_contribution are pub, though. A saturating_add would remove the reliance on call order.

• Block packing (block_production.rs, verified_sync_aggregate). Re-verifying the pooled aggregate against the block's own pre-state and falling back to the empty aggregate is the right safety net. It makes a bad pooled signature cost rewards instead of the block.

  • candidate.sync_committee_bits.get(position).unwrap_or(false) is fine here. Both sides use the committee-sized bitvector.
  • This step costs one eth_fast_aggregate_verify over up to 512 pubkeys on the proposal path. Please confirm it runs off the async executor.

• Pool combining (sync_committee_pool.rs:118-164). A held contribution is merged with individual messages only at uncovered positions, so no signer is counted twice. This is correct. A validator holding several seats gets one signature per bit, as the spec requires.

  • The Err(_) => return held.cloned() fallback returns None when nothing was held, even though messages exist. That is acceptable, because the comment says everything pooled was verified.

• Seen-cache race (gossip/sync_committee.rs:125). SeenSyncContributions::record re-runs the verdict so only the first of two concurrent validations records. The message-side cache has no such guard, but insert_message keeps the first signature per position, so the outcome is the same.

Performance and robustness

• Mutex held across BLS work. SyncCommitteePool::contribution and sync_aggregate run bls::aggregate while the std::sync::Mutex is held:

  • get_sync_committee_contribution in rpc/src/beacon/sync_committee.rs:205 does this on an async task.
  • Block production calls sync_aggregate the same way, with up to four aggregations.
  • Concurrent gossip verdicts then block on insert_* for the whole aggregation, and the async task blocks an executor thread.
  • Suggestion: clone the minimal inputs (signatures and bits) under the lock, drop it, then aggregate. Alternatively, wrap the GET handler's pool call in spawn_blocking.

• Unbounded batch size. post_pool_sync_committees and post_contribution_and_proofs verify one to three BLS signatures per item, with no cap on the batch length. They run on spawn_blocking, so the executor is safe. A large body can still tie up a blocking thread. A max batch size, or a body-size limit if the router doesn't already set one, would be cheap insurance. This is the same posture as the attestation endpoint, so it is low priority.

• Pool memory. Pool entries are keyed by beacon_block_root, which is not checked against known blocks. Only validated messages get in, and the pool keeps 4 slots of 512 members, so growth is bounded. No action needed.

Idioms and nits

• join() in p2p/src/beacon/sync_committee.rs:126 pushes to joined from inside an or_insert_with closure. It works, but a match on Entry would read more plainly.
• describe() and batch_response are clean.
• Tests are thorough. They cover the seat, period and subnet edge cases, metadata_seq_number moving only on change, and the clamp on until_epoch. I found no gaps worth flagging.
• CLAUDE.md and the docs are updated to match. The warn! about advertising cgc without sync subnets became an info!, which fits the new behavior.

Verdict: looks good. I'd address the lock-held-during-BLS point before merge, and the rest is optional.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR implementing sync committee support in ethlambda. This is a substantial feature addition spanning gossip validation, pooling, block production, validator client duties, and Beacon API endpoints.

High-Level Assessment

The PR implements Altair sync committee functionality (messages, contributions, aggregation) with proper integration into gossip validation, block production, and validator client duties. The architecture follows existing patterns in the codebase (attestation pool, gossip split into cheap/stateful checks, permit pools).

---

Critical Issues

1. Race condition in SyncCommitteePool::insert_message — incorrect bounds check logic

File: crates/blockchain/state_transition/src/beacon/sync_committee_pool.rs
Lines: 78-82

if subcommittee >= SYNC_COMMITTEE_SUBNET_COUNT as u64
    || position >= SYNC_SUBCOMMITTEE_SIZE
{
    continue;
}

Issue: The position >= SYNC_SUBCOMMITTEE_SIZE check is wrong. SYNC_SUBCOMMITTEE_SIZE is the size of one subcommittee, but position is the index within the entire sync committee (0..511 for mainnet). The correct check should be position >= preset::SYNC_COMMITTEE_SIZE.

However, looking more carefully at how seats are constructed in sync_committee_seats (line 119-126 of helpers/sync_committee.rs), position is index % SYNC_SUBCOMMITTEE_SIZE — the position within the subcommittee. So the check is actually correct for the intended semantics.

Wait — re-reading: In sync_committee_seats, the returned positions are:

• subcommittee = position / SYNC_SUBCOMMITTEE_SIZE
• position_within = position % SYNC_SUBCOMMITTEE_SIZE

So position in insert_message is indeed the within-subcommittee position. The check is correct. ✅

2. Potential panic in verified_sync_aggregate — saturating_sub with slot 0

File: crates/blockchain/state_transition/src/beacon/block_production.rs
Lines: 131-132

let previous_slot = state.slot().saturating_sub(1);
let Ok(block_root) = get_block_root_at_slot(state, previous_slot) else {

Issue: For genesis slot (slot 0), previous_slot is 0, and get_block_root_at_slot(state, 0) should return the genesis block root. However, the spec's process_sync_aggregate is only called from Altair onward, and slot 0 in Altair would be unusual. More critically, if state.slot() is 0, we're asking for the block root at slot 0, which is correct (genesis block root is stored at slot 0 in block_roots).

Actually, re-checking: get_block_root_at_slot returns the block root at the given slot from the state's block_roots array. For slot 0, this is valid. The saturating_sub is defensive but the case shouldn't occur in practice since sync committees start at Altair.

3. Missing sync_committee_bits SSZ serialization fix

File: crates/common/types/src/beacon/containers/altair.rs
Lines: 90-91

The SyncAggregate struct changes from:

#[serde(serialize_with = "crate::beacon::serde_helpers::ssz_hex::serialize")]

to:

#[serde(with = "crate::beacon::serde_helpers::ssz_hex")]

This is a critical fix — the old code could only serialize but not deserialize sync_committee_bits. The new with attribute enables both directions. This is necessary for the Beacon API to accept SyncAggregate in JSON. ✅ Good fix.

---

Security Issues

4. BLS signature verification bypass in verified_sync_aggregate

File: crates/blockchain/state_transition/src/beacon/block_production.rs
Lines: 109-152

pub fn verified_sync_aggregate(state: &BeaconState, candidate: SyncAggregate) -> SyncAggregate {
    // ...
    if bls::eth_fast_aggregate_verify(
        &participants,
        signing_root,
        &candidate.sync_committee_signature,
    ) {
        candidate
    } else {
        empty_sync_aggregate()
    }
}

Issue: This re-verifies the aggregate signature, but it does not verify that candidate.sync_committee_bits matches the actual participants. A malicious or buggy pool could set bits for non-participants and provide a valid signature for a different set of participants.

However, looking at the call sites: the pool's sync_aggregate method only sets bits for which it has verified signatures. And verified_sync_aggregate is called on the result. The risk is if someone calls verified_sync_aggregate directly with a crafted SyncAggregate.

Mitigation: The function should verify that the number of set bits equals participants.len(), or more precisely, that no extra bits are set beyond what the signature covers. Currently, if bits are set for positions not in participants, the signature won't verify (since participants is derived from the bits), so this is actually safe — the participants vector is built from the bits, so extra bits would require extra pubkeys in participants, making the signature invalid.

Actually wait — re-reading: participants is built from candidate.sync_committee_bits. If bit i is set, pubkey i is included. The signature is verified against exactly those pubkeys. This is correct: you can't set a bit without including the corresponding pubkey in the verification. ✅

5. No verification that sync_committee_bits count matches signature participants in pool

File: crates/blockchain/state_transition/src/beacon/sync_committee_pool.rs
Lines: 178-205

In contribution, when extending a held contribution with messages:

for (position, signature) in messages.into_iter().flatten().enumerate() {
    let Some(signature) = signature else { continue };
    if contribution.aggregation_bits.get(position).unwrap_or(true) {
        continue;
    }
    // ...
}

Issue: The unwrap_or(true) on line 185 is dangerous. If position is out of range for aggregation_bits, it defaults to true (skip), which silently drops valid signatures. This should be unwrap_or(false) or an explicit bounds check with error.

Actually, position comes from enumerating messages which is Vec<Option<BlsSignature>> of length SYNC_SUBCOMMITTEE_SIZE (set in insert_message line 84). So position is always in range. The unwrap_or(true) is defensive but unreachable in correct operation.

However, if messages is from a different source or corrupted, this could drop signatures. Consider expect or debug_assert! instead.

---

Correctness Issues

6. Wrong epoch computation in sync_committee_for_slot

File: crates/blockchain/state_transition/src/beacon/helpers/sync_committee.rs
Lines: 50-64

pub fn sync_committee_for_slot(state: &BeaconState, slot: Slot) -> Result<&SyncCommittee> {
    let (current, next) = state.sync_committees()?;
    let signing_period = compute_sync_committee_period(compute_epoch_at_slot(slot + 1));
    let state_period = compute_sync_committee_period(get_current_epoch(state));
    // ...
}

Issue: The spec's compute_subnets_for_sync_committee uses state.slot + 1 to determine the committee, but here it's slot + 1 where slot is the message's slot. The comment says this is intentional (a deviation documented in spec_deviations.md), but let's verify:

For a message at slot S to be included in a block at slot S+1, the signing committee is determined by S+1. The spec's get_sync_subcommittee_pubkeys takes state and subcommittee_index, and uses state.slot + 1 implicitly through get_sync_committee_for_slot(state) which uses state.slot.

The deviation: using the message's slot rather than state.slot + 1. This is correct for the stated reason (head lagging across boundary), but the function name and docs should be very clear this is a deviation.

✅ Properly documented in spec_deviations.md and code comments.

7. compute_sync_committee_period uses wrong preset constant

File: crates/blockchain/state_transition/src/beacon/helpers/altair.rs
Lines: 184-189

pub fn compute_sync_committee_period(epoch: Epoch) -> u64 {
    epoch / preset::EPOCHS_PER_SYNC_COMMITTEE_PERIOD
}

Wait — this is correct per spec. But let me check if EPOCHS_PER_SYNC_COMMITTEE_PERIOD is defined in the right preset. Looking at ethlambda_types::beacon::preset, this should be EPOCHS_PER_SYNC_COMMITTEE_PERIOD which is 256 for mainnet. ✅

8. Missing check for SyncCommitteeContribution slot matching in pool

File: crates/blockchain/state_transition/src/beacon/sync_committee_pool.rs
Lines: 98-115

In insert_contribution:

pub fn insert_contribution(&mut self, contribution: SyncCommitteeContribution) -> bool {
    self.prune_before(contribution.slot.saturating_sub(RETAINED_SLOTS));
    if contribution.subcommittee_index >= SYNC_COMMITTEE_SUBNET_COUNT as u64 {
        return false;
    }
    // ...
}

No verification that the contribution's signatures actually match the claimed aggregation_bits. This is by design — the pool only accepts pre-validated contributions. But the function is pub, so external callers could insert invalid contributions.

Recommendation: Add debug_assert! or document the invariant more prominently. The current doc comment says "Only objects that already passed validation go in", which is sufficient if adhered to.

---

Performance Issues

9. Inefficient pubkey lookup in test helper

File: crates/blockchain/state_transition/src/beacon/block_production.rs
Lines: 718-724 (in pooled_aggregate)

let index = (0..64)
    .find(|&index| state.validator(index).unwrap().pubkey == pubkey)
    .expect("the committee is drawn from the registry");

This is test-only code, but it's O(validators) per committee member. For tests with small validator sets this is fine. Not a production issue.

10. SyncCommitteePool uses Mutex not RwLock

File: crates/blockchain/state_transition/src/beacon/sync_committee_pool.rs
Line: 39

pub type SharedSyncCommitteePool = Arc<Mutex<SyncCommitteePool>>;

The pool is read by block production and the contribution endpoint, and written by gossip/Beacon API. A RwLock would allow concurrent reads. However, the operations are fast (BTreeMap lookups), and the attestation pool uses the same pattern (Arc<Mutex<_>>). Consistency with existing code is reasonable.

---

Rust Best Practices

11. Unnecessary Clone bound in test

File: crates/blockchain/state_transition/src/beacon/gossip/sync_committee.rs
Lines: 535-543

let mut bits = <SyncCommitteeContribution as Clone>::clone(&SyncCommitteeContribution {
    slot: SLOT,
    // ...
});

This is unnecessarily verbose. SyncCommitteeContribution::clone(&default_value) or just constructing directly would be cleaner. Actually, this is constructing a base then modifying it. Could use ..Default::default() syntax if SyncCommitteeContribution derives Default.

Looking at the type, it does derive Default. So this could be:

let mut bits = SyncCommitteeContribution {
    slot: SLOT,
    beacon_block_root: root(),
    subcommittee_index: subcommittee,
    ..Default::default()
};

Minor style issue, test-only.

12. Inconsistent error handling for poisoned locks

File: crates/net/p2p/src/beacon/sync_committee.rs
Lines: 376-379

let mut pool = server.sync_committee_pool.lock().expect("sync committee pool lock poisoned");

This panics on poison. Some places use .unwrap_or_else(|e| e.into_inner()) to recover. The attestation pool uses expect consistently, so this is consistent. ✅

13. Flexible deserializer accepts negative numbers

File: crates/net/rpc/src/beacon/sync_committee.rs
Lines: 312-340

impl<'de> Deserialize<'de> for Flexible {
    fn deserialize<D: Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
        // ...
        fn visit_u64<E: serde::de::Error>(self, value: u64) -> Result<Flexible, E> {
            Ok(Flexible(value))
        }
        fn visit_str<E: serde::de::Error>(self, value: &str) -> Result<Flexible, E> {
            value.trim().parse().map(Flexible).map_err(E::custom)
        }
    }
}

visit_i64 is not implemented, so negative numbers would fail. But visit_str with "-1" would parse to u64 and fail with a parse error. Actually, str::parse::<u64>() on "-1" returns Err. So this is safe. ✅

---

Consensus-Critical Issues

14. Fork choice rule for sync committee — no issue, correctly absent

Sync committee messages have no fork-choice weight. The PR correctly does not send them to the chain actor (see verdict.rs lines 250-275, and test at line 1407-1428 confirming received.try_recv().is_err()). ✅

15. Empty aggregate uses point at infinity — spec compliance

File: crates/blockchain/state_transition/src/beacon/block_production.rs
Lines: 100-107

pub fn empty_sync_aggregate() -> SyncAggregate {
    SyncAggregate {
        sync_committee_bits: SyncCommitteeBits::default(),
        sync_committee_signature: BlsSignature(bls::G2_POINT_AT_INFINITY),
    }
}

The spec requires the empty aggregate to have the point at infinity as signature. ✅ Verified by test at line 795-801.

16. Period boundary handling in sync_committee_for_slot

File: crates/blockchain/state_transition/src/beacon/helpers/sync_committee.rs
Lines: 50-64

At the last slot of a period, slot + 1 is in the next period, so next_sync_committee is used. This is correct per spec. The test at lines 268-283 verifies this. ✅

---

Documentation and Maintainability

17. Typo in CLAUDE.md

File: CLAUDE.md
Line: 337 (approximate)

"Gloas digests add two more topics," — should this be "Global digests" or is "Gloas" intentional? Looking at context, "Gloas" is the fork name, so this is correct. ✅

18. Inconsistent comment about syncnets in ENR

File: docs/beacon_wire.md
Lines: 639-642

The doc says "the ENR has no syncnets entry" but also mentions cgc is advertised. The PR correctly does not add syncnets to ENR (runtime modification not supported). ✅

---

Summary

Category	Count	Notes
Critical	0	No critical security bugs found
Security	0	All signature verification paths are correct
Correctness	0	Spec deviations are documented and intentional
Performance	1	Could use RwLock for pool, but follows existing pattern
Style/Rust	2	Minor test code issues
Documentation	0	Clear and comprehensive

Recommendations (Non-Blocking)

1. Add debug_assert! for position bounds in SyncCommitteePool::contribution (line 185) to catch any invariant violations in testing.

2. Consider RwLock instead of Mutex for SharedSyncCommitteePool if read contention becomes measurable.

3. The unwrap_or(true) in contribution at line 185 could be expect since it should be unreachable — or document why true (skip) is the safe fallback.

4. Test verified_sync_aggregate with mismatched bits/signature — add a test where bits claim more participants than the signature covers, to ensure it returns empty aggregate.

Overall Assessment

Approve with minor suggestions. The PR is well-structured, thoroughly tested, correctly implements the spec with documented deviations, and follows the codebase's architectural patterns. The sync committee integration is complete from gossip through block production to validator client duties.

---

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