fix: subscribe to subnet when aggregating only

[MegaRedHand]

Summary

• Non-aggregator nodes no longer subscribe to attestation subnet topics, reducing unnecessary gossip traffic
• Aggregator nodes retain current behavior (subscribe and collect raw attestations)
• Non-aggregators can still publish their own attestations via gossipsub's fanout mechanism

Changes

File	Change
bin/ethlambda/src/main.rs	Pass is_aggregator flag to start_p2p()
crates/net/p2p/src/lib.rs	Add is_aggregator param; wrap attestation .subscribe() in conditional

How it works

Gossipsub's fanout mechanism allows publishing to topics without subscribing. When a node publishes to an unsubscribed topic, libp2p maintains a temporary fanout set of peers that are subscribed, forwarding the message through them. This means non-aggregators only need outbound delivery (publishing), not inbound collection (subscribing).

Test plan

• cargo fmt --all -- --check
• cargo clippy --workspace -- -D warnings
• cargo test --workspace --release (all unit tests pass; spec tests require fixtures not in worktree)
• Multi-client devnet: verify aggregator nodes still collect attestations and finalize; non-aggregators publish attestations but don't receive them

Closes #159

[github-actions]

🤖 Kimi Code Review

Review Summary

The changes correctly implement the intended behavior where only aggregators subscribe to attestation subnets, while non-aggregators can still publish via gossipsub's fanout mechanism. However, there are several issues that need attention:

1. Potential Panic on Subscription Failure (Line 196)

The .unwrap() on subscription could panic if the topic is already subscribed or if there's a network issue. This is particularly problematic for blockchain clients that should be resilient.

Fix:

if let Err(e) = swarm.behaviour_mut().gossipsub.subscribe(&attestation_topic) {
    error!(%attestation_topic_kind, error = %e, "Failed to subscribe to attestation subnet");
    // Consider graceful degradation or retry logic
}

2. Race Condition in Topic Construction (Lines 183-189)

The attestation topic construction happens regardless of whether the node is an aggregator or not, but the topic string is only logged in the conditional branches. This could lead to confusion in logs.

Suggestion: Move the topic construction inside the aggregator branch or log it consistently for both cases.

3. Missing Validation for attestation_committee_count

While the comment mentions "attestation_committee_count is validated to be >= 1 by clap", there's no runtime validation here. If the CLI validation changes or this function is called from elsewhere, a division by zero could occur at line 184.

Fix:

if attestation_committee_count == 0 {
    return Err(eyre::eyre!("attestation_committee_count must be greater than 0"));
}

4. Inconsistent Logging Pattern

The logging uses different patterns - some use %variable interpolation while others use string concatenation. Consider standardizing on structured logging throughout.

5. Missing Documentation for Behavior Change

The change in behavior (non-aggregators not subscribing but still publishing) should be documented in the function's doc comment or at least have a comment explaining the gossipsub fanout mechanism.

6. Potential Network Partitioning Risk

By having non-aggregators not subscribe to attestation subnets, there's a risk that attestations might not propagate efficiently if there aren't enough aggregators in the network. Consider adding a warning log if the number of aggregators is expected to be low.

Overall, the core logic is sound, but the error handling and documentation could be improved for production readiness.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

No blocking findings in this PR.

1. Behavior change looks internally consistent: is_aggregator is now threaded from CLI to P2P startup and only aggregators subscribe to attestation subnets, while non-aggregators still can publish via fanout. This aligns with the existing store path that only retains gossip signatures for aggregation when is_aggregator is true.
   main.rs:128
   lib.rs:60
   lib.rs:194
   store.rs:390

2. Consensus-critical logic (fork choice, justification/finalization, state transition, XMSS/aggregation verification, SSZ decode paths) is not modified by this diff, so regression risk there appears low.

Residual risk / suggestion:

• Consider a stronger startup warning when is_aggregator=false on validator nodes, since a misconfigured network with too few aggregators can reduce attestation propagation quality/finality liveness.
  lib.rs:202

Validation gap:

• I could not run cargo check in this environment due rustup temp-dir permission restrictions (/home/runner/.rustup/tmp not writable).

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have a thorough understanding of the PR. Let me write the review.

---

PR #160 — fix: subscribe to subnet when aggregating only

Scope: 24 lines changed across 2 files. Targeted and well-focused.

What the PR does

Nodes that are not aggregators (--is-aggregator false) no longer subscribe to their attestation subnet. They will still publish attestations via libp2p gossipsub's fanout mechanism (publishing to a topic without being subscribed), relying on connected aggregators being in the fanout peer set.

---

Correctness

The design is sound: non-aggregating validators don't collect raw attestations for aggregation, so they don't need to be in the subnet mesh. Aggregated proofs are disseminated on the aggregation topic, which all nodes still subscribe to unconditionally (lines 170–178 of lib.rs). This matches the stated intent in CLAUDE.md.

One subtle correctness question: previously, subscribing to the attestation subnet meant non-aggregating nodes helped propagate raw attestations through the mesh, increasing coverage. Removing that subscription reduces mesh redundancy for attestation subnets when there are few aggregators. This is acceptable if the deployment assumption is "at least one aggregator is always reachable" (already a requirement for finalization per CLAUDE.md).

---

Specific Issues

Inaccurate comment (minor)

lib.rs:186 — the None arm comment reads:

// Non-validators use subnet 0 for publishing
None => format!("{ATTESTATION_SUBNET_TOPIC_PREFIX}_0"),

Non-validator nodes (no validator keys) never publish attestations. The comment is misleading in the updated context. "Default to subnet 0" or simply removing the comment would be clearer.

Fanout reliability for non-aggregators (design note)

lib.rs:192–203 — Publishing via fanout requires that gossipsub has established fanout peers for the subnet. The configured fanout_ttl is 60 seconds (lib.rs:81). If a non-aggregating validator has been idle for more than 15 slots (60s / 4s per slot), the fanout set may have expired. The next attestation publish will still work if there is a connected aggregator peer — libp2p will re-establish fanout peers on publish — but the publish could silently fail if no subscribed aggregator peer is available at that moment.

The current inspect_err + warn! in publish_attestation is the right pattern and matches project conventions. Just be aware that lost attestations are not retried.

Pre-existing: .unwrap() on subscribe

lib.rs:197–199:

swarm.behaviour_mut().gossipsub.subscribe(&attestation_topic).unwrap();

A subscription failure panics the node. This was pre-existing behavior and not introduced by this PR, but subscribing is very unlikely to fail in practice (it errors only on invalid topic hashes). Not a blocker.

---

API change

start_p2p signature gained is_aggregator: bool, and main.rs passes options.is_aggregator correctly. The call site is clean and consistent with the existing BlockChain::spawn(... options.is_aggregator) pattern.

---

Logging

The two new info! messages are well-placed and use the correct shorthand format (%attestation_topic_kind). They give useful startup diagnostics.

---

Overall

The fix is correct, minimal, and well-motivated. The one actionable item is the comment on the None subnet arm (line 186) — it says "for publishing" but non-validators never publish. Everything else is either correct or pre-existing.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Summary

This PR optimizes P2P network resource usage by restricting attestation subnet subscriptions to aggregator nodes only. Non-aggregator nodes can still publish attestations via gossipsub's fanout mechanism without subscribing to the subnet.

Key Changes:

• Added is_aggregator parameter to the start_p2p function
• Conditionally subscribe to attestation subnets only when is_aggregator is true
• Non-aggregators publish attestations via fanout (without mesh subscription)
• Added clear logging to indicate subscription status

Benefits:

• Reduces bandwidth and processing overhead for non-aggregator nodes
• Non-aggregators don't need to receive and process attestations from the network
• Maintains ability for all validators to publish their own attestations

Confidence Score: 5/5

• This PR is safe to merge with no identified risks
• The changes are minimal, well-scoped, and correctly implemented. The parameter is properly threaded through the codebase, the conditional logic is sound, and the fallback to fanout publishing for non-aggregators is a standard gossipsub pattern. No breaking changes or edge cases identified.
• No files require special attention

Important Files Changed

Filename	Overview
bin/ethlambda/src/main.rs	Passes is_aggregator flag to start_p2p function
crates/net/p2p/src/lib.rs	Conditionally subscribes to attestation subnet based on aggregator status; non-aggregators publish via fanout

_{Last reviewed commit: 5ca61c4}

[pablodeymo]

we don't need to log this message

[pablodeymo]

can we trait error properly?
maybe log an error