Port subnet network topology from leanSpec PR #482#249
Conversation
Add --aggregate-subnet-ids CLI flag and multi-subnet subscription support. Nodes now subscribe to all subnets their validators belong to (for mesh health), and aggregators can additionally subscribe to explicit subnets. The store no longer filters attestations by subnet — it stores all gossip signatures unconditionally, since the P2P layer already ensures only relevant subnets are subscribed. Key changes: - SwarmConfig accepts all validator IDs instead of just the first - P2P layer computes subnet set from validators + explicit aggregator IDs - publish_attestation routes per-validator to correct subnet topic - Store removes ATTESTATION_COMMITTEE_COUNT constant and compute_subnet_id - on_gossip_attestation and on_block no longer take local_validator_ids
🤖 Kimi Code ReviewSecurity & DoS Risk
Input Validation
Code Duplication & Maintainability
Determinism
Metrics Accuracy
Defensive Programming
Minor
Summary: The architectural change to move subnet filtering to the P2P layer is sound, but the PR removes safety boundaries without adding compensating controls (DoS limits) and duplicates topic construction logic. Address the validation and deduplication issues before merging. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewNow I have everything I need to write the review. Review: Port subnet network topology from leanSpec PR #482Overall: Clean architectural improvement. The shift of subnet filtering from store to P2P layer is correct, and removing the broken Hardcoded
|
Greptile SummaryThis PR ports the subnet network topology from leanSpec PR #482, moving attestation subnet filtering from the fork-choice store to the P2P subscription layer and adding multi-validator / multi-subnet support. The architectural shift is sound and the four-phase decomposition (CLI → P2P config → store simplification → caller updates) is clean. Key changes:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| crates/net/p2p/src/lib.rs | Core P2P refactor: SwarmConfig gains validator_ids: Vec<u64> + aggregate_subnet_ids; multi-subnet subscription logic is correct; explicit subnet IDs lack bounds-checking against attestation_committee_count. |
| crates/net/p2p/src/gossipsub/handler.rs | publish_attestation now routes per-validator to the correct subnet topic via validator_id % committee_count; falls back to on-the-fly fanout topic construction when not subscribed. Hardcoded "devnet0" is an acknowledged pre-existing limitation. |
| crates/blockchain/src/lib.rs | Simplified on_block and on_gossip_attestation call sites; however, the existing warn! in on_gossip_attestation will now fire continuously for non-aggregator validator nodes that subscribe to attestation subnets for mesh health. |
| crates/blockchain/src/store.rs | Removed hardcoded ATTESTATION_COMMITTEE_COUNT and compute_subnet_id; gossip and proposer signatures are now stored unconditionally — subnet filtering correctly delegated to the P2P layer. |
| bin/ethlambda/src/main.rs | Adds --aggregate-subnet-ids CLI flag with requires = "is_aggregator" guard; collects all validator IDs instead of only the first — straightforward and correct. |
| crates/blockchain/tests/signature_spectests.rs | Trivial call-site update removing the now-deleted local_validator_ids parameter from on_block; no issues. |
Sequence Diagram
sequenceDiagram
participant CLI as CLI (main.rs)
participant Swarm as build_swarm (lib.rs)
participant P2P as P2PServer
participant Handler as publish_attestation
participant BC as BlockChainServer
participant Store as store.rs
CLI->>Swarm: SwarmConfig { validator_ids, aggregate_subnet_ids, committee_count }
Note over Swarm: compute validator_subnets = validator_ids.map(vid % count)
Note over Swarm: subscribe_subnets = validator_subnets ∪ explicit_ids (if aggregator)
loop for each subnet_id in subscribe_subnets
Swarm->>Swarm: gossipsub.subscribe(attestation_subnet_{id})
end
Swarm-->>P2P: BuiltSwarm { attestation_topics: HashMap, committee_count }
Note over P2P: Incoming attestation gossip
P2P->>BC: new_attestation(signed_attestation)
BC->>Store: on_gossip_attestation(attestation) [no subnet filter]
Store-->>BC: Ok (signature stored unconditionally)
Note over P2P: Outgoing attestation publish
P2P->>Handler: publish_attestation(attestation)
Handler->>Handler: subnet_id = validator_id % committee_count
alt subscribed topic found
Handler->>P2P: swarm.publish(attestation_topics[subnet_id])
else not subscribed (fanout)
Handler->>Handler: construct topic on-the-fly ("devnet0")
Handler->>P2P: swarm.publish(fanout_topic)
end
Comments Outside Diff (1)
-
crates/blockchain/src/lib.rs, line 452-455 (link)Spurious
warn!spam for non-aggregator validator nodesThis PR intentionally subscribes non-aggregator validator nodes to their attestation subnets for gossipsub mesh health (see PR notes). As a result, those nodes will now receive attestations via gossip. Every received attestation will hit this
if !self.is_aggregatorguard and emit awarn!— once per attestation per slot — producing continuous log noise for a perfectly valid configuration.The
warn!was written when non-aggregators were never subscribed to attestation topics and receiving one was truly unexpected. Now that subscribing is intentional, this should be downgraded totrace!(or dropped entirely).Prompt To Fix With AI
This is a comment left during a code review. Path: crates/blockchain/src/lib.rs Line: 452-455 Comment: **Spurious `warn!` spam for non-aggregator validator nodes** This PR intentionally subscribes non-aggregator validator nodes to their attestation subnets for gossipsub mesh health (see PR notes). As a result, those nodes will now *receive* attestations via gossip. Every received attestation will hit this `if !self.is_aggregator` guard and emit a `warn!` — once per attestation per slot — producing continuous log noise for a perfectly valid configuration. The `warn!` was written when non-aggregators were never subscribed to attestation topics and receiving one was truly unexpected. Now that subscribing is intentional, this should be downgraded to `trace!` (or dropped entirely). 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/blockchain/src/lib.rs
Line: 452-455
Comment:
**Spurious `warn!` spam for non-aggregator validator nodes**
This PR intentionally subscribes non-aggregator validator nodes to their attestation subnets for gossipsub mesh health (see PR notes). As a result, those nodes will now *receive* attestations via gossip. Every received attestation will hit this `if !self.is_aggregator` guard and emit a `warn!` — once per attestation per slot — producing continuous log noise for a perfectly valid configuration.
The `warn!` was written when non-aggregators were never subscribed to attestation topics and receiving one was truly unexpected. Now that subscribing is intentional, this should be downgraded to `trace!` (or dropped entirely).
```suggestion
if !self.is_aggregator {
trace!("Received unaggregated attestation on non-aggregator node (subscribed for mesh health), ignoring");
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/net/p2p/src/lib.rs
Line: 221-228
Comment:
**No bounds-check on explicit `aggregate_subnet_ids`**
The `--aggregate-subnet-ids` values provided via CLI are added directly to `subscribe_subnets` without being validated against `attestation_committee_count`. An operator running e.g. `--attestation-committee-count 2 --aggregate-subnet-ids 0,1,5` will silently subscribe to subnet 5, which falls outside the valid range `[0, committee_count)`. This could cause the node to subscribe to a topic that no other peer publishes to, wasting a gossipsub mesh slot, and creates a mismatch between the node's subscriptions and the routing logic in `publish_attestation` (which computes `validator % committee_count`).
Consider adding a validation step here:
```rust
if config.is_aggregator {
if let Some(ref explicit_ids) = config.aggregate_subnet_ids {
for &id in explicit_ids {
if id >= config.attestation_committee_count {
// return an error or log a warning
warn!(subnet_id = id, committee_count = config.attestation_committee_count,
"aggregate_subnet_ids contains out-of-range subnet ID");
}
}
subscribe_subnets.extend(explicit_ids);
}
...
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Port leanSpec PR #482: subnet filtering ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if config.is_aggregator { | ||
| swarm | ||
| .behaviour_mut() | ||
| .gossipsub | ||
| .subscribe(&attestation_topic)?; | ||
| info!(%attestation_topic_kind, "Subscribed to attestation subnet"); | ||
| if let Some(ref explicit_ids) = config.aggregate_subnet_ids { | ||
| subscribe_subnets.extend(explicit_ids); | ||
| } | ||
| // Aggregator with no validators and no explicit subnets: fallback to subnet 0 | ||
| if subscribe_subnets.is_empty() { | ||
| subscribe_subnets.insert(0); | ||
| } |
There was a problem hiding this comment.
aggregate_subnet_ids
The --aggregate-subnet-ids values provided via CLI are added directly to subscribe_subnets without being validated against attestation_committee_count. An operator running e.g. --attestation-committee-count 2 --aggregate-subnet-ids 0,1,5 will silently subscribe to subnet 5, which falls outside the valid range [0, committee_count). This could cause the node to subscribe to a topic that no other peer publishes to, wasting a gossipsub mesh slot, and creates a mismatch between the node's subscriptions and the routing logic in publish_attestation (which computes validator % committee_count).
Consider adding a validation step here:
if config.is_aggregator {
if let Some(ref explicit_ids) = config.aggregate_subnet_ids {
for &id in explicit_ids {
if id >= config.attestation_committee_count {
// return an error or log a warning
warn!(subnet_id = id, committee_count = config.attestation_committee_count,
"aggregate_subnet_ids contains out-of-range subnet ID");
}
}
subscribe_subnets.extend(explicit_ids);
}
...
}Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/p2p/src/lib.rs
Line: 221-228
Comment:
**No bounds-check on explicit `aggregate_subnet_ids`**
The `--aggregate-subnet-ids` values provided via CLI are added directly to `subscribe_subnets` without being validated against `attestation_committee_count`. An operator running e.g. `--attestation-committee-count 2 --aggregate-subnet-ids 0,1,5` will silently subscribe to subnet 5, which falls outside the valid range `[0, committee_count)`. This could cause the node to subscribe to a topic that no other peer publishes to, wasting a gossipsub mesh slot, and creates a mismatch between the node's subscriptions and the routing logic in `publish_attestation` (which computes `validator % committee_count`).
Consider adding a validation step here:
```rust
if config.is_aggregator {
if let Some(ref explicit_ids) = config.aggregate_subnet_ids {
for &id in explicit_ids {
if id >= config.attestation_committee_count {
// return an error or log a warning
warn!(subnet_id = id, committee_count = config.attestation_committee_count,
"aggregate_subnet_ids contains out-of-range subnet ID");
}
}
subscribe_subnets.extend(explicit_ids);
}
...
}
```
How can I resolve this? If you propose a fix, please make it concise.Deduplicates topic string construction between build_swarm and publish_attestation, and consolidates the hardcoded devnet0 network name into a single constant in gossipsub/messages.rs.
Co-authored-by: Tomás Grüner <47506558+MegaRedHand@users.noreply.github.com>
2 participants
Motivation
leanSpec PR #482 (merged 2025-03-25) introduces
--aggregate-subnet-idsand changes how nodes subscribe to attestation subnets. The key architectural shift: subnet filtering moves from the fork choice store to the P2P subscription layer.Before this PR, ethlambda had several gaps:
ATTESTATION_COMMITTEE_COUNT = 1constant (ignored the CLI parameter)--aggregate-subnet-idsCLI flag existedDescription
Phase 1: CLI (
bin/ethlambda/src/main.rs)--aggregate-subnet-idsCLI flag (comma-separated,requires = "is_aggregator"via clap)validator_ids: Vec<u64>andaggregate_subnet_idstoSwarmConfigPhase 2: P2P layer (
crates/net/p2p/src/lib.rs,crates/net/p2p/src/gossipsub/handler.rs)SwarmConfig:validator_id: Option<u64>→validator_ids: Vec<u64>+aggregate_subnet_ids: Option<Vec<u64>>BuiltSwarm/P2PServer: singleattestation_topic→attestation_topics: HashMap<u64, IdentTopic>+attestation_committee_count: u64--aggregate-subnet-idspublish_attestation: routes per-validator to the correct subnet topic (validator_id % committee_count); if the subnet isn't subscribed, constructs the topic on-the-fly for gossipsub fanoutlean_attestation_committee_subnetreports the lowest validator subnet (backward-compatible)Phase 3: Store simplification (
crates/blockchain/src/store.rs)ATTESTATION_COMMITTEE_COUNTconstant andcompute_subnet_id()helperon_gossip_attestation: removedlocal_validator_idsparameter; stores gossip signatures unconditionally (subnet filtering already handled at P2P layer)on_block/on_block_core: removedlocal_validator_idsparameter; stores proposer signature unconditionallyPhase 4: Caller updates (
crates/blockchain/src/lib.rs,crates/blockchain/tests/signature_spectests.rs)How to test
For devnet testing:
Verify via logs:
Subscribed to attestation subnetlines (one per subscribed subnet)Published attestation to gossipsubnow includessubnet_idfieldNotes
"devnet0"network name: existing limitation carried forward in the fanout topic construction. Should be addressed separately.lean_attestation_committee_subnetis a single gauge. With multiple subnets, we report the lowest validator's subnet for backward compatibility. Can be improved in a follow-up.Related