From 3d0b3f10417fc6708e8b5d844a0bac5e86f3e17d Mon Sep 17 00:00:00 2001 From: daniel-wong-dfinity-org <97631336+daniel-wong-dfinity-org@users.noreply.github.com> Date: Fri, 2 Aug 2024 00:33:41 +0200 Subject: [PATCH] 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 | 2 +- rs/nns/governance/src/neuron/types.rs | 38 ++++------ rs/nns/governance/src/neuron/types/tests.rs | 14 ---- rs/nns/governance/src/neuron_store.rs | 73 ++++++++----------- rs/nns/governance/src/neuron_store/metrics.rs | 2 +- .../src/neuron_store/metrics/tests.rs | 18 ++++- rs/nns/governance/tests/governance.rs | 8 +- 7 files changed, 65 insertions(+), 90 deletions(-) diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index bde89b0541e..95dc6797dc3 100644 --- a/rs/nns/governance/src/governance.rs +++ b/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())); diff --git a/rs/nns/governance/src/neuron/types.rs b/rs/nns/governance/src/neuron/types.rs index c82a0447cde..3195ae6ec39 100644 --- a/rs/nns/governance/src/neuron/types.rs +++ b/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, /// How much unprivileged principals (i.e. is neither controller, nor /// hotkey) can see about this neuron. - pub visibility: Option, -} - -#[must_use] -pub fn normalized(mut neuron: Cow) -> Cow { - 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, } 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 { + 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; diff --git a/rs/nns/governance/src/neuron/types/tests.rs b/rs/nns/governance/src/neuron/types/tests.rs index 3ad532a5c09..afda9718fca 100644 --- a/rs/nns/governance/src/neuron/types/tests.rs +++ b/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),); -} diff --git a/rs/nns/governance/src/neuron_store.rs b/rs/nns/governance/src/neuron_store.rs index af36bf6e19c..e20d8e88273 100644 --- a/rs/nns/governance/src/neuron_store.rs +++ b/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, 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. diff --git a/rs/nns/governance/src/neuron_store/metrics.rs b/rs/nns/governance/src/neuron_store/metrics.rs index f425bc7a15d..63b25f08577 100644 --- a/rs/nns/governance/src/neuron_store/metrics.rs +++ b/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; } diff --git a/rs/nns/governance/src/neuron_store/metrics/tests.rs b/rs/nns/governance/src/neuron_store/metrics/tests.rs index 8c917f36824..edf424d3036 100644 --- a/rs/nns/governance/src/neuron_store/metrics/tests.rs +++ b/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. diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs index 0c722e7a793..ab24b5bb51f 100644 --- a/rs/nns/governance/tests/governance.rs +++ b/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.