refactor(p2p): drop NoPeersSubscribedToTopic suppression path

[MegaRedHand]

Summary

Follow-up to #293. Now that every validator subscribes to its own attestation subnet, the NoPeersSubscribedToTopic error path is no longer an expected-and-ignorable case on any publish site. Remove the suppression machinery and unify on a single publish path.

Motivation

Before #293 only aggregators subscribed to attestation subnets. A non-aggregator publishing via fanout, or an aggregator in a single-subscriber mesh, could legitimately get PublishError::NoPeersSubscribedToTopic and we handled this by gating publish_ignore_no_peers on is_aggregator. This was already slightly misaligned (Codex flagged this on #293) and is now fully obsolete: every validator that publishes an attestation is itself in the mesh for that subnet, so if gossipsub still reports no remote subscribers, it means something is actually wrong (partition, misconfigured peer) and we want the warning.

Changes

File	Change
crates/net/p2p/src/swarm_adapter.rs	Drop ignore_no_peers field on SwarmCommand::Publish and the SwarmHandle::publish_ignore_no_peers helper. The publish execution now uses a single inspect_err path that warns on any failure, including NoPeersSubscribedToTopic. Removed the now-unused PublishError import.
crates/net/p2p/src/gossipsub/handler.rs	publish_attestation no longer branches on is_aggregator; always calls swarm_handle.publish(...).
crates/net/p2p/src/lib.rs	is_aggregator field removed from BuiltSwarm and P2PServer — the only consumer was the branching in publish_attestation. SwarmConfig.is_aggregator stays (still drives subscription decisions).

Net diff: 3 files, +9/-49.

Test plan

• cargo fmt --all -- --check
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test --workspace --release --lib — 60 passed, 6 ignored
• cargo test -p ethlambda-blockchain --release --test forkchoice_spectests — 57 passed
• cargo test -p ethlambda-state-transition --release --test stf_spectests — 47 passed
• Devnet: confirm no new publish warnings under normal operation

[github-actions]

🤖 Kimi Code Review

Summary
This PR simplifies the P2P publishing API by removing the is_aggregator flag and the publish_ignore_no_peers method. While this reduces code complexity, it introduces a behavioral change where NoPeersSubscribedToTopic errors are now logged as warnings even for aggregators (who expect this condition in small networks).

---

Issues & Concerns

1. Log spam in small networks / empty subnets (crates/net/p2p/src/swarm_adapter.rs:126-130)

   • The removal of the ignore_no_peers flag means NoPeersSubscribedToTopic errors now always trigger warning logs.
   • In small testnets or when an aggregator is the only subscriber to a subnet topic, this creates benign warning spam.
   • Suggestion: Either accept the verbosity, or filter this specific error to debug! level:

     .inspect_err(|err| {
         if !matches!(err, libp2p::gossipsub::PublishError::NoPeersSubscribedToTopic) {
             warn!(%err, "Swarm adapter: publish failed");
         }
     })

2. Loss of context in comments (crates/net/p2p/src/gossipsub/handler.rs:148)

   • The removed comment explained why suppressing the error was safe (self-delivery). Without this context, future maintainers might not understand why NoPeersSubscribedToTopic is expected behavior for aggregators.
   • Suggestion: Add a brief comment noting that publish errors (including no peers) are logged but do not affect consensus.

3. Global is_aggregator flag imprecision (crates/net/p2p/src/lib.rs:95, 275, 309)

   • The removal of the is_aggregator boolean from P2PServer and BuiltSwarm is likely correct, as a global flag cannot accurately represent per-subnet aggregator status (which changes every slot).
   • Verify that no other component relies on this field for critical logic (the diff suggests it was only used for log suppression).

---

Positive Aspects

1. Idiomatic error handling (crates/net/p2p/src/swarm_adapter.rs:126-130)

   • The use of inspect_err followed by .ok() is clean and idiomatic Rust, replacing the manual if let Err match.

2. API simplification

   • Removing the conditional publish path reduces API surface area and eliminates a potential source of confusion about when to use publish vs publish_ignore_no_peers.

3. Dead code removal

   • The PublishError import removal and struct field cleanups are correct.

---

Consensus & Security Assessment

• No impact on state transition, fork choice, or attestation validation logic.
• No security issues: This change only affects error logging verbosity in the gossipsub layer; message publishing behavior remains identical.
• Attestation propagation: Attestations are still published to the correct subnet topics regardless of aggregator status.

Recommendation: Approve if the team accepts increased warning verbosity in small networks, or request filtering NoPeersSubscribedToTopic to debug! level before merging.

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. crates/net/p2p/src/swarm_adapter.rs#L126 reintroduces warn! logging for PublishError::NoPeersSubscribedToTopic, but under the current subscription model that is an expected condition for attestation publishes on sparsely populated subnets. Aggregators subscribe to their own / explicit attestation subnets in crates/net/p2p/src/lib.rs#L208, and publish_attestation() always publishes to that subnet in crates/net/p2p/src/gossipsub/handler.rs#L141. In small networks or during startup, that path can legitimately return NoPeersSubscribedToTopic; the previous implementation intentionally suppressed it. With this PR, every locally produced attestation on an empty subnet becomes a warning, which is likely to create noisy per-slot log spam and obscure real publish failures. I would keep the old special-case suppression or at least downgrade just that variant to debug!/trace!.

No other correctness, security, fork-choice, attestation-validation, STF, XMSS, or SSZ issues stood out in this diff. The rest of the change is mainly cleanup of now-unused is_aggregator plumbing.

I could not run cargo test -p net-p2p --lib in this environment because rustup tried to sync the toolchain and failed on a read-only filesystem under /home/runner/.rustup/tmp.

---

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

[github-actions]

🤖 Claude Code Review

Here is the code review for PR #298:

---

PR #298 — refactor(p2p): drop NoPeersSubscribedToTopic suppression path

Overview

Clean follow-up to #293. Now that every validator subscribes to its own attestation subnet, the NoPeersSubscribedToTopic suppression logic is dead weight. The PR removes it: the ignore_no_peers field on SwarmCommand::Publish, the publish_ignore_no_peers helper, the is_aggregator branch in publish_attestation, and the now-redundant is_aggregator field on BuiltSwarm / P2PServer. Net: +9 / -49.

---

Code Quality

swarm_adapter.rs — The replacement pattern is correct and idiomatic per project conventions:

swarm
    .behaviour_mut()
    .gossipsub
    .publish(topic, data)
    .inspect_err(|err| warn!(%err, "Swarm adapter: publish failed"))
    .ok();

Using .inspect_err for the side-effect and .ok() to discard the Result<MessageId, PublishError> is clean. The PublishError import is correctly removed since no match arm references it anymore.

handler.rs — The branch if server.is_aggregator { ... } else { ... } is correctly collapsed to a single server.swarm_handle.publish(topic, compressed). No logic is lost.

lib.rs — is_aggregator is removed from BuiltSwarm and P2PServer but correctly retained in SwarmConfig (still drives subscription decisions). The field removal is minimal and well-scoped.

---

Correctness

The underlying assumption is sound: if every validator is now subscribed to its attestation subnet mesh (post-#293), gossipsub will always have at least the local node in the mesh. A NoPeersSubscribedToTopic error therefore indicates a genuine anomaly (network partition, misconfiguration) and should surface as a warning rather than be silenced.

The previous guard is_aggregator && NoPeersSubscribedToTopic was also slightly wrong directionally — it suppressed warnings for aggregators specifically, but non-aggregators publishing via fanout could also trigger the same error. The unified path removes that inconsistency.

---

Minor Points

1. Single-node / isolated dev setups: A node running with no peers at all will now emit NoPeersSubscribedToTopic warnings on every attestation publish. This was previously suppressed for aggregators. The new behavior is arguably more correct (it tells you the node is isolated), but it may be noisy in local single-node smoke tests. Worth noting in the devnet validation step.

2. Devnet test item is still unchecked in the PR description. This is the one empirical check that confirms the assumption holds in multi-client operation — that no spurious warnings fire under normal peer conditions. The PR should not be merged until this is confirmed.

3. No issues with the .ok() discard of MessageId — the value is not used anywhere downstream and discarding it is the correct intent.

---

Summary

The logic is correct, the cleanup is complete, and the code follows project conventions (inspect_err, ownership patterns, field removal consistency). The only gate before merging is confirming the devnet run produces no unexpected NoPeersSubscribedToTopic warnings under normal operation.

---

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

[greptile-apps]

Greptile Summary

This PR removes the NoPeersSubscribedToTopic suppression machinery that was needed before #293, when only aggregators subscribed to attestation subnets. Since every validator now subscribes to its own subnet, the error is no longer an expected quiescent state and should surface as a real warning. The changes are a clean dead-code removal across three files: swarm_adapter.rs drops the ignore_no_peers enum field and publish_ignore_no_peers helper, handler.rs simplifies publish_attestation to a single unconditional call, and lib.rs removes is_aggregator from BuiltSwarm/P2PServer (the field stays in SwarmConfig for subscription decisions).

Confidence Score: 5/5

Safe to merge — clean dead-code removal with no functional regressions on the changed paths.

All findings are P2 (a stale comment). The logic change is correct: removing suppression that was only needed when non-aggregators lacked subnet subscriptions, a precondition that no longer holds after #293. No new error paths, no data loss, no broken contracts.

No files require special attention.

Important Files Changed

Filename	Overview
crates/net/p2p/src/swarm_adapter.rs	Dropped ignore_no_peers from SwarmCommand::Publish and removed publish_ignore_no_peers helper; execute_command now uses a single inspect_err warn path for all publish failures.
crates/net/p2p/src/gossipsub/handler.rs	publish_attestation simplified: removed is_aggregator branch, always calls swarm_handle.publish; stale "gossipsub fanout" comment remains but the fallback path is effectively unreachable since every validator now subscribes to its own subnet.
crates/net/p2p/src/lib.rs	Removed is_aggregator from BuiltSwarm and P2PServer; SwarmConfig.is_aggregator is intentionally retained for subscription logic in build_swarm.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[publish_attestation called] --> B[compute subnet_id]
    B --> C[look up topic in attestation_topics]
    C --> D{found?}
    D -- yes --> E[swarm_handle.publish topic]
    D -- no --> F[attestation_subnet_topic fallback]
    F --> E
    E --> G[SwarmCommand::Publish enqueued]
    G --> H[execute_command in swarm_loop]
    H --> I[gossipsub.publish]
    I --> J{error?}
    J -- no --> K[done]
    J -- yes --> L[warn! Swarm adapter: publish failed]
    L --> K

    style F stroke-dasharray: 5 5
    style F fill:#ffe0b2

Loading

Comments Outside Diff (1)

1. crates/net/p2p/src/gossipsub/handler.rs, line 141-146 (link)

   Stale "fanout" comment and dead fallback branch

   Since fix: subscribe non-aggregator validators to their attestation subnets #293 every validator subscribes to its own attestation subnet before publishing, attestation_topics will always contain the validator's subnet_id. The unwrap_or_else branch and its comment about gossipsub fanout are now unreachable in normal operation; consider updating or removing both to avoid misleading future readers.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/net/p2p/src/gossipsub/handler.rs
   Line: 141-146
   
   Comment:
   **Stale "fanout" comment and dead fallback branch**
   
   Since #293 every validator subscribes to its own attestation subnet before publishing, `attestation_topics` will always contain the validator's `subnet_id`. The `unwrap_or_else` branch and its comment about gossipsub fanout are now unreachable in normal operation; consider updating or removing both to avoid misleading future readers.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: crates/net/p2p/src/gossipsub/handler.rs
Line: 141-146

Comment:
**Stale "fanout" comment and dead fallback branch**

Since #293 every validator subscribes to its own attestation subnet before publishing, `attestation_topics` will always contain the validator's `subnet_id`. The `unwrap_or_else` branch and its comment about gossipsub fanout are now unreachable in normal operation; consider updating or removing both to avoid misleading future readers.

```suggestion
    // Look up subscribed topic; every validator subscribes to its own subnet.
    let topic = server
        .attestation_topics
        .get(&subnet_id)
        .cloned()
        .unwrap_or_else(|| attestation_subnet_topic(subnet_id));
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "refactor(p2p): drop NoPeersSubscribedToT..." | Re-trigger Greptile}