fix(nns): Fixed a bug where known neuron is not seen as public. (#699)

The problem is that there are (many) paths where normalization is not
applied. This is a bug from a recent change that I did.

The solution (implemented here) is to make the Neuron::visibility field
private, and add a (public) getter.

This obsoletes the previous solution: `fn normalized`. Thus, I deleted
`normalized`.

This bug was discovered by strengthening an existing test (for public
neuron metrics), which then started failing.

rs/nns/governance/src/governance.rs

@@ -2166,7 +2166,7 @@ impl Governance {
                         // neuron is public, and the caller requested that
                         // public neurons be included (in full_neurons).
                         || (include_public_neurons_in_full_neurons
-                            && neuron.visibility == Some(Visibility::Public)
+                            && neuron.visibility() == Some(Visibility::Public)
                         );
                 if let_caller_read_full_neuron {
                     full_neurons.push(NeuronProto::from(neuron.clone()));

rs/nns/governance/src/neuron/types.rs

@@ -21,10 +21,7 @@ use ic_base_types::PrincipalId;
 use ic_nervous_system_common::ONE_DAY_SECONDS;
 use ic_nns_common::pb::v1::{NeuronId, ProposalId};
 use icp_ledger::Subaccount;
-use std::{
-    borrow::Cow,
-    collections::{BTreeSet, HashMap},
-};
+use std::collections::{BTreeSet, HashMap};
 
 /// A neuron type internal to the governance crate. Currently, this type is identical to the
 /// prost-generated Neuron type (except for derivations for prost). Gradually, this type will evolve
@@ -115,27 +112,7 @@ pub struct Neuron {
     pub neuron_type: Option<i32>,
     /// How much unprivileged principals (i.e. is neither controller, nor
     /// hotkey) can see about this neuron.
-    pub visibility: Option<Visibility>,
-}
-
-#[must_use]
-pub fn normalized(mut neuron: Cow<Neuron>) -> Cow<Neuron> {
-    if neuron.known_neuron_data.is_some() {
-        // Log if there is an inconsistency, but otherwise, do not interrupt the flow.
-        if neuron.visibility == Some(Visibility::Private) {
-            println!(
-                "{}WARNING: Neuron {:?} is a known neuron, but its visibility field is \
-                 set to private. This in-memory neuron will now quietly be set to public. \
-                 However, the underlying source of this inconsistent neuron is not \
-                 being updated.",
-                LOG_PREFIX, neuron.id,
-            );
-        }
-
-        neuron.to_mut().visibility = Some(Visibility::Public);
-    }
-
-    neuron
+    visibility: Option<Visibility>,
 }
 
 impl Neuron {
@@ -164,6 +141,17 @@ impl Neuron {
         self.dissolve_state_and_age
     }
 
+    /// When we turn on enforcement of private neurons, this will only return
+    /// Public or Private, not None. When that happens, we should define another
+    /// Visibility that does NOT have Unspecified.
+    pub fn visibility(&self) -> Option<Visibility> {
+        if self.known_neuron_data.is_some() {
+            return Some(Visibility::Public);
+        }
+
+        self.visibility
+    }
+
     /// Sets a neuron's dissolve state and age.
     pub fn set_dissolve_state_and_age(&mut self, dissolve_state_and_age: DissolveStateAndAge) {
         self.dissolve_state_and_age = dissolve_state_and_age;

rs/nns/governance/src/neuron/types/tests.rs

@@ -450,17 +450,3 @@ fn test_neuron_configure_dissolve_delay() {
     let now = now + 1;
     assert_eq!(neuron.state(now), NeuronState::Dissolved);
 }
-
-#[test]
-fn test_normalize_makes_known_neurons_always_public() {
-    let mut neuron =
-        create_neuron_with_dissolve_state_and_age(DissolveStateAndAge::DissolvingOrDissolved {
-            when_dissolved_timestamp_seconds: 1721408743,
-        });
-    neuron.known_neuron_data = Some(Default::default());
-    let neuron: std::borrow::Cow<'_, Neuron> = Cow::Owned(neuron);
-
-    let neuron = normalized(neuron);
-
-    assert_eq!(neuron.visibility, Some(Visibility::Public),);
-}

rs/nns/governance/src/neuron_store.rs

@@ -2,10 +2,7 @@ use crate::{
     governance::{
         Environment, TimeWarp, LOG_PREFIX, MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS,
     },
-    neuron::{
-        neuron_id_range_to_u64_range,
-        types::{normalized, Neuron},
-    },
+    neuron::{neuron_id_range_to_u64_range, types::Neuron},
     pb::v1::{
         governance::{followers_map::Followers, FollowersMap},
         governance_error::ErrorType,
@@ -597,48 +594,42 @@ impl NeuronStore {
         neuron_id: NeuronId,
         sections: NeuronSections,
     ) -> Result<(Cow<Neuron>, StorageLocation), NeuronStoreError> {
-        let main = || {
-            let heap_neuron = self.heap_neurons.get(&neuron_id.id).map(Cow::Borrowed);
-
-            if let Some(heap_neuron) = heap_neuron.clone() {
-                // If the neuron is active on heap, return early to avoid any operation on stable
-                // storage. The StableStorageNeuronValidator ensures that active neuron cannot also be
-                // on stable storage.
-                if !heap_neuron.is_inactive(self.now()) {
-                    return Ok((heap_neuron, StorageLocation::Heap));
-                }
+        let heap_neuron = self.heap_neurons.get(&neuron_id.id).map(Cow::Borrowed);
+
+        if let Some(heap_neuron) = heap_neuron.clone() {
+            // If the neuron is active on heap, return early to avoid any operation on stable
+            // storage. The StableStorageNeuronValidator ensures that active neuron cannot also be
+            // on stable storage.
+            if !heap_neuron.is_inactive(self.now()) {
+                return Ok((heap_neuron, StorageLocation::Heap));
             }
+        }
 
-            let stable_neuron = with_stable_neuron_store(|stable_neuron_store| {
-                stable_neuron_store
-                    .read(neuron_id, sections)
-                    .ok()
-                    .map(Cow::Owned)
-            });
-
-            match (stable_neuron, heap_neuron) {
-                // 1 copy cases.
-                (Some(stable), None) => Ok((stable, StorageLocation::Stable)),
-                (None, Some(heap)) => Ok((heap, StorageLocation::Heap)),
-
-                // 2 copy case.
-                (Some(stable), Some(_)) => {
-                    println!(
-                        "{}WARNING: neuron {:?} is in both stable memory and heap memory, \
-                         we are at risk of having stale copies",
-                        LOG_PREFIX, neuron_id
-                    );
-                    Ok((stable, StorageLocation::Stable))
-                }
+        let stable_neuron = with_stable_neuron_store(|stable_neuron_store| {
+            stable_neuron_store
+                .read(neuron_id, sections)
+                .ok()
+                .map(Cow::Owned)
+        });
 
-                // 0 copies case.
-                (None, None) => Err(NeuronStoreError::not_found(neuron_id)),
+        match (stable_neuron, heap_neuron) {
+            // 1 copy cases.
+            (Some(stable), None) => Ok((stable, StorageLocation::Stable)),
+            (None, Some(heap)) => Ok((heap, StorageLocation::Heap)),
+
+            // 2 copies case.
+            (Some(stable), Some(_)) => {
+                println!(
+                    "{}WARNING: neuron {:?} is in both stable memory and heap memory, \
+                     we are at risk of having stale copies",
+                    LOG_PREFIX, neuron_id
+                );
+                Ok((stable, StorageLocation::Stable))
             }
-        };
 
-        let (neuron, storage_location) = main()?;
-        let neuron = normalized(neuron);
-        Ok((neuron, storage_location))
+            // 0 copies case.
+            (None, None) => Err(NeuronStoreError::not_found(neuron_id)),
+        }
     }
 
     // Loads the entire neuron from either heap or stable storage and returns its primary storage.

rs/nns/governance/src/neuron_store/metrics.rs

@@ -83,7 +83,7 @@ impl NeuronMetrics {
     }
 
     fn increment_public_neuron_subset_metrics(&mut self, now_seconds: u64, neuron: &Neuron) {
-        let is_public = neuron.visibility == Some(Visibility::Public);
+        let is_public = neuron.visibility() == Some(Visibility::Public);
         if !is_public {
             return;
         }

rs/nns/governance/src/neuron_store/metrics/tests.rs

@@ -1,7 +1,7 @@
 use super::*;
 use crate::{
     neuron::{DissolveStateAndAge, NeuronBuilder},
-    pb::v1::NeuronType,
+    pb::v1::{KnownNeuronData, NeuronType},
 };
 use ic_base_types::PrincipalId;
 use ic_nervous_system_common::{E8, ONE_DAY_SECONDS, ONE_YEAR_SECONDS};
@@ -591,7 +591,11 @@ fn test_compute_neuron_metrics_public_neurons() {
     .with_cached_neuron_stake_e8s(300_000_000)
     .with_staked_maturity_e8s_equivalent(303_000_000)
     .with_maturity_e8s_equivalent(330_000_000)
-    .with_visibility(Some(Visibility::Public))
+    // (Nominally) the neuron should be treated as public.
+    .with_known_neuron_data(Some(KnownNeuronData {
+        name: "Daniel Wong".to_string(),
+        description: Some("Best engineer of all time. Of all time.".to_string()),
+    }))
     .build();
 
     let voting_power_1 = neuron_1.voting_power(now_seconds);
@@ -609,6 +613,16 @@ fn test_compute_neuron_metrics_public_neurons() {
         2 => neuron_2,
         3 => neuron_3,
     });
+    neuron_store
+        .with_neuron(&NeuronId { id: 3 }, |neuron| {
+            assert_eq!(
+                neuron.visibility(),
+                Some(Visibility::Public),
+                "{:#?}",
+                neuron,
+            );
+        })
+        .unwrap(); // Explode if neuron is not found.
 
     // Step 2: Call code under test.
 

rs/nns/governance/tests/governance.rs

@@ -9878,7 +9878,7 @@ fn test_neuron_set_visibility() {
                 .with_neuron(&neuron_id, |neuron| neuron.clone())
                 .unwrap();
 
-            assert_eq!(neuron.visibility, expected_visibility, "{:#?}", neuron,);
+            assert_eq!(neuron.visibility(), expected_visibility, "{:#?}", neuron,);
         };
 
     assert_neuron_visibility(typical_neuron.id.unwrap(), Some(Visibility::Public));
@@ -9977,11 +9977,7 @@ fn test_include_public_neurons_in_full_neurons() {
     assert_eq!(
         list_neurons_response.full_neurons,
         vec![
-            Neuron {
-                // Thanks to normalization.
-                visibility: Some(Visibility::Public as i32),
-                ..known_neuron
-            },
+            known_neuron,
             explicitly_public_neuron,
             // In particular, legacy and explicitly_private are NOT in the result.