From 05fe793f84f1088695123f39863bd211089814c3 Mon Sep 17 00:00:00 2001 From: pawan Date: Mon, 2 Aug 2021 12:07:49 +0530 Subject: [PATCH 1/2] Fix issues from review --- .../network/src/beacon_processor/worker/gossip_methods.rs | 6 ++---- beacon_node/network/src/subnet_service/tests/mod.rs | 7 ++++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index 525e1c2a4f2..4c71e3ca0dc 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -867,8 +867,7 @@ impl Worker { /* * The aggregate had no signatures and is therefore worthless. * - * Whilst we don't gossip this attestation, this act is **not** a clear - * violation of the spec nor indication of fault. + * This is forbidden by the p2p spec. Reject the message. * */ self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); @@ -1244,8 +1243,7 @@ impl Worker { /* * The aggregate had no signatures and is therefore worthless. * - * Whilst we don't gossip this message, this act is **not** a clear - * violation of the spec nor indication of fault. + * This is forbidden by the p2p spec. Reject the message. * */ self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); diff --git a/beacon_node/network/src/subnet_service/tests/mod.rs b/beacon_node/network/src/subnet_service/tests/mod.rs index 2bba5d85af4..6ad0837642c 100644 --- a/beacon_node/network/src/subnet_service/tests/mod.rs +++ b/beacon_node/network/src/subnet_service/tests/mod.rs @@ -214,7 +214,12 @@ mod attestation_service { ]; // Wait for 1 slot duration to get the unsubscription event - let events = get_events(&mut attestation_service, None, 1).await; + let events = get_events( + &mut attestation_service, + Some(5), + (MainnetEthSpec::slots_per_epoch() * 3) as u32, + ) + .await; matches::assert_matches!( events[..3], [ From 68357f33c8b746369a1d7571ff69bf3ff8d2b66c Mon Sep 17 00:00:00 2001 From: pawan Date: Mon, 2 Aug 2021 13:44:44 +0530 Subject: [PATCH 2/2] Remove sync_committee_subnet_count from ChainSpec --- beacon_node/eth2_libp2p/src/behaviour/mod.rs | 5 +++-- beacon_node/network/src/metrics.rs | 7 +++++-- beacon_node/network/src/subnet_service/sync_subnets.rs | 3 ++- consensus/types/src/chain_spec.rs | 2 -- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/beacon_node/eth2_libp2p/src/behaviour/mod.rs b/beacon_node/eth2_libp2p/src/behaviour/mod.rs index 8c308c49605..83161c6704b 100644 --- a/beacon_node/eth2_libp2p/src/behaviour/mod.rs +++ b/beacon_node/eth2_libp2p/src/behaviour/mod.rs @@ -44,7 +44,8 @@ use std::{ task::{Context, Poll}, }; use types::{ - ChainSpec, EnrForkId, EthSpec, ForkContext, SignedBeaconBlock, Slot, SubnetId, SyncSubnetId, + consts::altair::SYNC_COMMITTEE_SUBNET_COUNT, ChainSpec, EnrForkId, EthSpec, ForkContext, + SignedBeaconBlock, Slot, SubnetId, SyncSubnetId, }; pub mod gossipsub_scoring_parameters; @@ -213,7 +214,7 @@ impl Behaviour { filter: Self::create_whitelist_filter( possible_fork_digests, chain_spec.attestation_subnet_count, - chain_spec.sync_committee_subnet_count, + SYNC_COMMITTEE_SUBNET_COUNT, ), max_subscribed_topics: 200, //TODO change this to a constant max_subscriptions_per_request: 100, //this is according to the current go implementation diff --git a/beacon_node/network/src/metrics.rs b/beacon_node/network/src/metrics.rs index baf97340df4..7ffce125422 100644 --- a/beacon_node/network/src/metrics.rs +++ b/beacon_node/network/src/metrics.rs @@ -10,7 +10,10 @@ use fnv::FnvHashMap; pub use lighthouse_metrics::*; use std::{collections::HashMap, sync::Arc}; use strum::AsStaticRef; -use types::{subnet_id::subnet_id_to_string, sync_subnet_id::sync_subnet_id_to_string, EthSpec}; +use types::{ + consts::altair::SYNC_COMMITTEE_SUBNET_COUNT, subnet_id::subnet_id_to_string, + sync_subnet_id::sync_subnet_id_to_string, EthSpec, +}; lazy_static! { @@ -597,7 +600,7 @@ pub fn update_gossip_metrics( .map(|v| v.set(0)); } - for subnet_id in 0..T::default_spec().sync_committee_subnet_count { + for subnet_id in 0..SYNC_COMMITTEE_SUBNET_COUNT { let _ = get_int_gauge( &MESH_PEERS_PER_SYNC_SUBNET_TOPIC, &[sync_subnet_id_to_string(subnet_id)], diff --git a/beacon_node/network/src/subnet_service/sync_subnets.rs b/beacon_node/network/src/subnet_service/sync_subnets.rs index 1bb57221d26..4162fdd167b 100644 --- a/beacon_node/network/src/subnet_service/sync_subnets.rs +++ b/beacon_node/network/src/subnet_service/sync_subnets.rs @@ -89,8 +89,9 @@ impl SyncCommitteeService { /// Return count of all currently subscribed subnets. #[cfg(test)] pub fn subscription_count(&self) -> usize { + use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; if self.subscribe_all_subnets { - self.beacon_chain.spec.sync_committee_subnet_count as usize + SYNC_COMMITTEE_SUBNET_COUNT as usize } else { self.subscriptions.len() } diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 07fa2ba5ae5..043007decdf 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -136,7 +136,6 @@ pub struct ChainSpec { pub maximum_gossip_clock_disparity_millis: u64, pub target_aggregators_per_committee: u64, pub attestation_subnet_count: u64, - pub sync_committee_subnet_count: u64, pub random_subnets_per_validator: u64, pub epochs_per_random_subnet_subscription: u64, } @@ -464,7 +463,6 @@ impl ChainSpec { network_id: 1, // mainnet network id attestation_propagation_slot_range: 32, attestation_subnet_count: 64, - sync_committee_subnet_count: 4, random_subnets_per_validator: 1, maximum_gossip_clock_disparity_millis: 500, target_aggregators_per_committee: 16,