Remove redundant subscribe_all_data_column_subnets field from network (#8259)

Addresses this comment: https://github.com/sigp/lighthouse/pull/8254#discussion_r2447998786

We're currently using `subscribe_all_data_column_subnets` here to subscribe to all subnets
522bd9e9c6/beacon_node/lighthouse_network/src/types/topics.rs (L82-L92)

But its unnecessary because the else path also works for supernode (uses `sampling_subnets` instead)

The big diffs will disappear once #8254 is merged.


  


Co-Authored-By: Jimmy Chen <jchen.tc@gmail.com>

beacon_node/lighthouse_network/src/config.rs

@@ -94,9 +94,6 @@ pub struct Config {
     /// Attempt to construct external port mappings with UPnP.
     pub upnp_enabled: bool,
 
-    /// Subscribe to all data column subnets for the duration of the runtime.
-    pub subscribe_all_data_column_subnets: bool,
-
     /// Subscribe to all subnets for the duration of the runtime.
     pub subscribe_all_subnets: bool,
 
@@ -355,7 +352,6 @@ impl Default for Config {
             upnp_enabled: true,
             network_load: 3,
             private: false,
-            subscribe_all_data_column_subnets: false,
             subscribe_all_subnets: false,
             import_all_attestations: false,
             shutdown_after_sync: false,

beacon_node/lighthouse_network/src/discovery/enr.rs

@@ -159,7 +159,7 @@ pub fn build_or_load_enr<E: EthSpec>(
     local_key: Keypair,
     config: &NetworkConfig,
     enr_fork_id: &EnrForkId,
-    custody_group_count: Option<u64>,
+    custody_group_count: u64,
     next_fork_digest: [u8; 4],
     spec: &ChainSpec,
 ) -> Result<Enr, String> {
@@ -185,7 +185,7 @@ pub fn build_enr<E: EthSpec>(
     enr_key: &CombinedKey,
     config: &NetworkConfig,
     enr_fork_id: &EnrForkId,
-    custody_group_count: Option<u64>,
+    custody_group_count: u64,
     next_fork_digest: [u8; 4],
     spec: &ChainSpec,
 ) -> Result<Enr, String> {
@@ -280,15 +280,6 @@ pub fn build_enr<E: EthSpec>(
 
     // only set `cgc` and `nfd` if PeerDAS fork (Fulu) epoch has been scheduled
     if spec.is_peer_das_scheduled() {
-        let custody_group_count = if let Some(cgc) = custody_group_count {
-            cgc
-        } else if let Some(false_cgc) = config.advertise_false_custody_group_count {
-            false_cgc
-        } else if config.subscribe_all_data_column_subnets {
-            spec.number_of_custody_groups
-        } else {
-            spec.custody_requirement
-        };
         builder.add_value(PEERDAS_CUSTODY_GROUP_COUNT_ENR_KEY, &custody_group_count);
         builder.add_value(NEXT_FORK_DIGEST_ENR_KEY, &next_fork_digest);
     }
@@ -373,7 +364,7 @@ mod test {
 
     fn build_enr_with_config(
         config: NetworkConfig,
-        cgc: Option<u64>,
+        cgc: u64,
         spec: &ChainSpec,
     ) -> (Enr, CombinedKey) {
         let keypair = libp2p::identity::secp256k1::Keypair::generate();
@@ -386,56 +377,23 @@ mod test {
     #[test]
     fn test_nfd_enr_encoding() {
         let spec = make_fulu_spec();
-        let enr = build_enr_with_config(NetworkConfig::default(), None, &spec).0;
+        let enr =
+            build_enr_with_config(NetworkConfig::default(), spec.custody_requirement, &spec).0;
         assert_eq!(enr.next_fork_digest().unwrap(), TEST_NFD);
     }
 
-    #[test]
-    fn custody_group_count_default() {
-        let config = NetworkConfig {
-            subscribe_all_data_column_subnets: false,
-            ..NetworkConfig::default()
-        };
-        let spec = make_fulu_spec();
-
-        let enr = build_enr_with_config(config, None, &spec).0;
-
-        assert_eq!(
-            enr.custody_group_count::<E>(&spec).unwrap(),
-            spec.custody_requirement,
-        );
-    }
-
-    #[test]
-    fn custody_group_count_all() {
-        let config = NetworkConfig {
-            subscribe_all_data_column_subnets: true,
-            ..NetworkConfig::default()
-        };
-        let spec = make_fulu_spec();
-        let enr = build_enr_with_config(config, None, &spec).0;
-
-        assert_eq!(
-            enr.custody_group_count::<E>(&spec).unwrap(),
-            spec.number_of_custody_groups,
-        );
-    }
-
     #[test]
     fn custody_group_value() {
-        let config = NetworkConfig {
+        let config = NetworkConfig::default();
-            subscribe_all_data_column_subnets: true,
-            ..NetworkConfig::default()
-        };
         let spec = make_fulu_spec();
-        let enr = build_enr_with_config(config, Some(42), &spec).0;
+        let enr = build_enr_with_config(config, 42, &spec).0;
 
         assert_eq!(enr.custody_group_count::<E>(&spec).unwrap(), 42);
     }
 
     #[test]
     fn test_encode_decode_eth2_enr() {
-        let (enr, _key) = build_enr_with_config(NetworkConfig::default(), None, &E::default_spec());
+        let (enr, _key) = build_enr_with_config(NetworkConfig::default(), 4, &E::default_spec());
         // Check all Eth2 Mappings are decodeable
         enr.eth2().unwrap();
         enr.attestation_bitfield::<MainnetEthSpec>().unwrap();

beacon_node/lighthouse_network/src/discovery/mod.rs

@@ -1243,11 +1243,12 @@ mod tests {
         let config = Arc::new(config);
         let enr_key: CombinedKey = CombinedKey::from_secp256k1(&keypair);
         let next_fork_digest = [0; 4];
+        let custody_group_count = spec.custody_requirement;
         let enr: Enr = build_enr::<E>(
             &enr_key,
             &config,
             &EnrForkId::default(),
-            None,
+            custody_group_count,
             next_fork_digest,
             &spec,
         )
@@ -1258,7 +1259,7 @@ mod tests {
                 seq_number: 0,
                 attnets: Default::default(),
                 syncnets: Default::default(),
-                custody_group_count: spec.custody_requirement,
+                custody_group_count,
             }),
             vec![],
             false,

beacon_node/lighthouse_network/src/service/mod.rs

@@ -199,7 +199,7 @@ impl<E: EthSpec> Network<E> {
             local_keypair.clone(),
             &config,
             &ctx.enr_fork_id,
-            Some(advertised_cgc),
+            advertised_cgc,
             next_fork_digest,
             &ctx.chain_spec,
         )?;

beacon_node/lighthouse_network/src/types/globals.rs

@@ -227,7 +227,6 @@ impl<E: EthSpec> NetworkGlobals<E> {
         TopicConfig {
             enable_light_client_server: self.config.enable_light_client_server,
             subscribe_all_subnets: self.config.subscribe_all_subnets,
-            subscribe_all_data_column_subnets: self.config.subscribe_all_data_column_subnets,
             sampling_subnets: self.sampling_subnets.read().clone(),
         }
     }

beacon_node/lighthouse_network/src/types/topics.rs

@@ -29,7 +29,6 @@ pub const LIGHT_CLIENT_OPTIMISTIC_UPDATE: &str = "light_client_optimistic_update
 pub struct TopicConfig {
     pub enable_light_client_server: bool,
     pub subscribe_all_subnets: bool,
-    pub subscribe_all_data_column_subnets: bool,
     pub sampling_subnets: HashSet<DataColumnSubnetId>,
 }
 
@@ -80,14 +79,8 @@ pub fn core_topics_to_subscribe<E: EthSpec>(
     }
 
     if fork_name.fulu_enabled() {
-        if opts.subscribe_all_data_column_subnets {
+        for subnet in &opts.sampling_subnets {
-            for i in 0..spec.data_column_sidecar_subnet_count {
+            topics.push(GossipKind::DataColumnSidecar(*subnet));
-                topics.push(GossipKind::DataColumnSidecar(i.into()));
-            }
-        } else {
-            for subnet in &opts.sampling_subnets {
-                topics.push(GossipKind::DataColumnSidecar(*subnet));
-            }
         }
     }
 
@@ -125,7 +118,6 @@ pub fn all_topics_at_fork<E: EthSpec>(fork: ForkName, spec: &ChainSpec) -> Vec<G
     let opts = TopicConfig {
         enable_light_client_server: true,
         subscribe_all_subnets: true,
-        subscribe_all_data_column_subnets: true,
         sampling_subnets,
     };
     core_topics_to_subscribe::<E>(fork, &opts, spec)
@@ -520,7 +512,6 @@ mod tests {
         TopicConfig {
             enable_light_client_server: false,
             subscribe_all_subnets: false,
-            subscribe_all_data_column_subnets: false,
             sampling_subnets: sampling_subnets.clone(),
         }
     }
@@ -552,9 +543,8 @@ mod tests {
     #[test]
     fn columns_are_subscribed_in_peerdas() {
         let spec = get_spec();
-        let s = get_sampling_subnets();
+        let s = HashSet::from_iter([0.into()]);
-        let mut topic_config = get_topic_config(&s);
+        let topic_config = get_topic_config(&s);
-        topic_config.subscribe_all_data_column_subnets = true;
         assert!(
             core_topics_to_subscribe::<E>(ForkName::Fulu, &topic_config, &spec)
                 .contains(&GossipKind::DataColumnSidecar(0.into()))

beacon_node/src/config.rs

@@ -114,7 +114,6 @@ pub fn get_config<E: EthSpec>(
     let is_semi_supernode = parse_flag(cli_args, "semi-supernode");
 
     client_config.chain.node_custody_type = if is_supernode {
-        client_config.network.subscribe_all_data_column_subnets = true;
         NodeCustodyType::Supernode
     } else if is_semi_supernode {
         NodeCustodyType::SemiSupernode

lcli/src/generate_bootnode_enr.rs

@@ -43,7 +43,7 @@ pub fn run<E: EthSpec>(matches: &ArgMatches, spec: &ChainSpec) -> Result<(), Str
         &enr_key,
         &config,
         &enr_fork_id,
-        None,
+        spec.custody_requirement,
         genesis_fork_digest,
         spec,
     )