-
Notifications
You must be signed in to change notification settings - Fork 21
Port subnet network topology from leanSpec PR #482 #249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
15a1ca7
827457a
259c8c3
853f390
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ use tracing::{info, trace, warn}; | |
|
|
||
| use crate::{ | ||
| gossipsub::{ | ||
| AGGREGATION_TOPIC_KIND, ATTESTATION_SUBNET_TOPIC_PREFIX, BLOCK_TOPIC_KIND, | ||
| AGGREGATION_TOPIC_KIND, BLOCK_TOPIC_KIND, NETWORK_NAME, attestation_subnet_topic, | ||
| publish_aggregated_attestation, publish_attestation, publish_block, | ||
| }, | ||
| req_resp::{ | ||
|
|
@@ -78,15 +78,17 @@ pub struct SwarmConfig { | |
| pub node_key: Vec<u8>, | ||
| pub bootnodes: Vec<Bootnode>, | ||
| pub listening_socket: SocketAddr, | ||
| pub validator_id: Option<u64>, | ||
| pub validator_ids: Vec<u64>, | ||
| pub attestation_committee_count: u64, | ||
| pub is_aggregator: bool, | ||
| pub aggregate_subnet_ids: Option<Vec<u64>>, | ||
| } | ||
|
|
||
| /// Result of building the swarm — contains all pieces needed to start the P2P actor. | ||
| pub struct BuiltSwarm { | ||
| pub(crate) swarm: libp2p::Swarm<Behaviour>, | ||
| pub(crate) attestation_topic: libp2p::gossipsub::IdentTopic, | ||
| pub(crate) attestation_topics: HashMap<u64, libp2p::gossipsub::IdentTopic>, | ||
| pub(crate) attestation_committee_count: u64, | ||
| pub(crate) block_topic: libp2p::gossipsub::IdentTopic, | ||
| pub(crate) aggregation_topic: libp2p::gossipsub::IdentTopic, | ||
| pub(crate) bootnode_addrs: HashMap<PeerId, Multiaddr>, | ||
|
|
@@ -184,10 +186,8 @@ pub fn build_swarm( | |
| .listen_on(addr) | ||
| .expect("failed to bind gossipsub listening address"); | ||
|
|
||
| let network = "devnet0"; | ||
|
|
||
| // Subscribe to block topic (all nodes) | ||
| let block_topic_str = format!("/leanconsensus/{network}/{BLOCK_TOPIC_KIND}/ssz_snappy"); | ||
| let block_topic_str = format!("/leanconsensus/{NETWORK_NAME}/{BLOCK_TOPIC_KIND}/ssz_snappy"); | ||
| let block_topic = libp2p::gossipsub::IdentTopic::new(block_topic_str); | ||
| swarm | ||
| .behaviour_mut() | ||
|
|
@@ -197,42 +197,54 @@ pub fn build_swarm( | |
|
|
||
| // Subscribe to aggregation topic (all validators) | ||
| let aggregation_topic_str = | ||
| format!("/leanconsensus/{network}/{AGGREGATION_TOPIC_KIND}/ssz_snappy"); | ||
| format!("/leanconsensus/{NETWORK_NAME}/{AGGREGATION_TOPIC_KIND}/ssz_snappy"); | ||
| let aggregation_topic = libp2p::gossipsub::IdentTopic::new(aggregation_topic_str); | ||
| swarm | ||
| .behaviour_mut() | ||
| .gossipsub | ||
| .subscribe(&aggregation_topic) | ||
| .unwrap(); | ||
|
|
||
| // Build attestation subnet topic (needed for publishing even without subscribing) | ||
| // attestation_committee_count is validated to be >= 1 by clap at CLI parse time. | ||
| let subnet_id = config | ||
| .validator_id | ||
| // Compute the set of subnets to subscribe to. | ||
| // Validators subscribe for gossipsub mesh health; aggregators additionally | ||
| // subscribe to any explicitly requested subnets. | ||
| let validator_subnets: HashSet<u64> = config | ||
| .validator_ids | ||
| .iter() | ||
| .map(|vid| vid % config.attestation_committee_count) | ||
| .unwrap_or(0); | ||
| metrics::set_attestation_committee_subnet(subnet_id); | ||
| .collect(); | ||
|
|
||
| let attestation_topic_kind = format!("{ATTESTATION_SUBNET_TOPIC_PREFIX}_{subnet_id}"); | ||
| let attestation_topic_str = | ||
| format!("/leanconsensus/{network}/{attestation_topic_kind}/ssz_snappy"); | ||
| let attestation_topic = libp2p::gossipsub::IdentTopic::new(attestation_topic_str); | ||
| let mut subscribe_subnets: HashSet<u64> = validator_subnets.clone(); | ||
|
|
||
| // Only aggregators subscribe to attestation subnets; non-aggregators | ||
| // publish via gossipsub's fanout mechanism without subscribing. | ||
| 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); | ||
| } | ||
|
Comment on lines
219
to
+226
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The 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 AIThis 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. |
||
| } | ||
|
|
||
| // Report lowest validator subnet for backward-compatible metric | ||
| let metric_subnet = validator_subnets.iter().copied().min().unwrap_or(0); | ||
| metrics::set_attestation_committee_subnet(metric_subnet); | ||
|
|
||
| // Build topics and subscribe | ||
| let mut attestation_topics: HashMap<u64, libp2p::gossipsub::IdentTopic> = HashMap::new(); | ||
| for &subnet_id in &subscribe_subnets { | ||
| let topic = attestation_subnet_topic(subnet_id); | ||
| swarm.behaviour_mut().gossipsub.subscribe(&topic)?; | ||
| info!(subnet_id, "Subscribed to attestation subnet"); | ||
| attestation_topics.insert(subnet_id, topic); | ||
| } | ||
|
|
||
| info!(socket=%config.listening_socket, "P2P node started"); | ||
|
|
||
| Ok(BuiltSwarm { | ||
| swarm, | ||
| attestation_topic, | ||
| attestation_topics, | ||
| attestation_committee_count: config.attestation_committee_count, | ||
| block_topic, | ||
| aggregation_topic, | ||
| bootnode_addrs, | ||
|
|
@@ -255,7 +267,8 @@ impl P2P { | |
| swarm_handle, | ||
| store, | ||
| blockchain: None, | ||
| attestation_topic: built.attestation_topic, | ||
| attestation_topics: built.attestation_topics, | ||
| attestation_committee_count: built.attestation_committee_count, | ||
| block_topic: built.block_topic, | ||
| aggregation_topic: built.aggregation_topic, | ||
| connected_peers: HashSet::new(), | ||
|
|
@@ -288,7 +301,8 @@ pub struct P2PServer { | |
| // BlockChain protocol ref (set via InitBlockChain message) | ||
| pub(crate) blockchain: Option<P2PToBlockChainRef>, | ||
|
|
||
| pub(crate) attestation_topic: libp2p::gossipsub::IdentTopic, | ||
| pub(crate) attestation_topics: HashMap<u64, libp2p::gossipsub::IdentTopic>, | ||
| pub(crate) attestation_committee_count: u64, | ||
| pub(crate) block_topic: libp2p::gossipsub::IdentTopic, | ||
| pub(crate) aggregation_topic: libp2p::gossipsub::IdentTopic, | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.