diff --git a/Cargo.lock b/Cargo.lock index 64a1cbb85dd..df4d4e556c7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8751,6 +8751,7 @@ dependencies = [ "lazy_static", "maplit", "mockall", + "num-traits", "on_wire", "pretty_assertions", "proptest", diff --git a/rs/nns/common/src/lib.rs b/rs/nns/common/src/lib.rs index 6e24e511936..e0788ede0eb 100644 --- a/rs/nns/common/src/lib.rs +++ b/rs/nns/common/src/lib.rs @@ -22,6 +22,10 @@ impl NeuronId { }, } } + + pub fn next(&self) -> Option { + self.id.checked_add(1).map(|id| NeuronId { id }) + } } impl From for u64 { diff --git a/rs/nns/governance/BUILD.bazel b/rs/nns/governance/BUILD.bazel index b053780fdcc..99f37458d38 100644 --- a/rs/nns/governance/BUILD.bazel +++ b/rs/nns/governance/BUILD.bazel @@ -49,6 +49,7 @@ BASE_DEPENDENCIES = [ "@crate_index//:lazy_static", "@crate_index//:maplit", "@crate_index//:mockall", + "@crate_index//:num-traits", "@crate_index//:pretty_assertions", "@crate_index//:prost", "@crate_index//:rand_0_8_4", diff --git a/rs/nns/governance/Cargo.toml b/rs/nns/governance/Cargo.toml index 79d42a8e40f..85d4c899283 100644 --- a/rs/nns/governance/Cargo.toml +++ b/rs/nns/governance/Cargo.toml @@ -64,6 +64,7 @@ itertools = { workspace = true } lazy_static = "1.4.0" maplit = "1.0.2" mockall = "0.11.1" +num-traits = "0.2.12" on_wire = { path = "../../rust_canisters/on_wire" } pretty_assertions = { workspace = true } prost = { workspace = true } diff --git a/rs/nns/governance/src/neuron_data_validation.rs b/rs/nns/governance/src/neuron_data_validation.rs index c8c4db25f0f..7bc4b87cddf 100644 --- a/rs/nns/governance/src/neuron_data_validation.rs +++ b/rs/nns/governance/src/neuron_data_validation.rs @@ -1,7 +1,7 @@ use crate::{ - neuron_store::{get_neuron_subaccount, NeuronStore, NeuronStoreError}, - pb::v1::{Neuron, Topic}, - storage::{with_stable_neuron_indexes, with_stable_neuron_store, Signed32}, + neuron_store::{get_neuron_subaccount, NeuronStore}, + pb::v1::Topic, + storage::{with_stable_neuron_indexes, Signed32}, }; use candid::{CandidType, Deserialize}; @@ -231,7 +231,7 @@ impl ValidationInProgress { KnownNeuronIndexValidator, >::new())); - tasks.push_back(Box::new(NeuronCopyValidator::new( + tasks.push_back(Box::new(StableNeuronStoreValidator::new( INACTIVE_NEURON_VALIDATION_CHUNK_SIZE, ))); @@ -641,54 +641,22 @@ impl CardinalityAndRangeValidator for KnownNeuronIndexValidator { } } -/// A validator for all neuron copies for times when they exists in both stable storage -/// and heap. -/// This validator will not be needed once neurons are only stored in one or the other. -struct NeuronCopyValidator { +/// A validator to validate that all neurons in stable storage are inactive. +struct StableNeuronStoreValidator { next_neuron_id: Option, chunk_size: usize, } -impl NeuronCopyValidator { +impl StableNeuronStoreValidator { fn new(chunk_size: usize) -> Self { Self { next_neuron_id: Some(NeuronId { id: 0 }), chunk_size, } } - - /// Validate that the neuron in stable storage and heap match exactly. - fn validate_neuron_matches_in_both_stores( - stable_neuron: Neuron, - neuron_store: &NeuronStore, - ) -> Option { - let neuron_id = match stable_neuron.id.as_ref() { - // Should be impossible, as stable_neuron should be coming straight from the store. - None => { - return Some(ValidationIssue::NeuronStoreError( - NeuronStoreError::NeuronIdIsNone.to_string(), - )); - } - Some(id) => id, - }; - match neuron_store.with_neuron(neuron_id, |neuron| { - if *neuron != stable_neuron { - Some(ValidationIssue::NeuronCopyValueNotMatch( - stable_neuron.id.unwrap(), - )) - } else { - None - } - }) { - Ok(maybe_issue) => maybe_issue, - Err(neuron_store_error) => Some(ValidationIssue::NeuronStoreError( - neuron_store_error.to_string(), - )), - } - } } -impl ValidationTask for NeuronCopyValidator { +impl ValidationTask for StableNeuronStoreValidator { fn is_done(&self) -> bool { self.next_neuron_id.is_none() } @@ -699,23 +667,14 @@ impl ValidationTask for NeuronCopyValidator { None => return vec![], }; - with_stable_neuron_store(|stable_neuron_store| { - stable_neuron_store - .range_neurons(next_neuron_id..) - .take(self.chunk_size) - .flat_map(|neuron| { - let next_neuron_id = NeuronId { - id: neuron.id.unwrap().id + 1, - }; - - let result = Self::validate_neuron_matches_in_both_stores(neuron, neuron_store); + let (invalid_neuron_ids, neuron_id_for_next_batch) = neuron_store + .batch_validate_neurons_in_stable_store_are_inactive(next_neuron_id, self.chunk_size); - self.next_neuron_id = Some(next_neuron_id); - - result - }) - .collect() - }) + self.next_neuron_id = neuron_id_for_next_batch; + invalid_neuron_ids + .into_iter() + .map(|neuron_id| ValidationIssue::NeuronCopyValueNotMatch(neuron_id)) + .collect() } } @@ -729,7 +688,7 @@ mod tests { use maplit::{btreemap, hashmap}; use crate::{ - pb::v1::{neuron::Followees, KnownNeuronData, Neuron}, + pb::v1::{neuron::DissolveState, neuron::Followees, KnownNeuronData, Neuron}, storage::{with_stable_neuron_indexes_mut, with_stable_neuron_store_mut}, }; @@ -814,102 +773,6 @@ mod tests { assert!(validation.maybe_validate(86400 + 100, &neuron_store)); } - #[test] - fn test_neuron_copy_validation_when_valid() { - let idle_neurons = (0..8).map(|_| next_test_neuron()).collect::>(); - - let mut btree_map: BTreeMap = idle_neurons - .clone() - .into_iter() - .map(|neuron| (neuron.id.unwrap().id, neuron)) - .collect(); - - let non_idle = Neuron { - cached_neuron_stake_e8s: 1000, - ..next_test_neuron() - }; - btree_map.insert(non_idle.id.unwrap().id, non_idle); - - let neuron_store = NeuronStore::new(btree_map); - - with_stable_neuron_store_mut(|stable_neuron_store| { - for neuron in idle_neurons { - stable_neuron_store.create(neuron).expect("Couldn't create"); - } - }); - - let mut validator = NeuronCopyValidator::new(2); - - for _ in 0..4 { - let defects = validator.validate_next_chunk(&neuron_store); - assert_eq!(defects, vec![]); - assert!(!validator.is_done()); - } - let defects = validator.validate_next_chunk(&neuron_store); - assert_eq!(defects, vec![]); - assert!(validator.is_done()); - } - - #[test] - fn test_neuron_copy_validation_when_bad_neurons() { - let idle_neurons = (0..8).map(|_| next_test_neuron()).collect::>(); - - let mut btree_map: BTreeMap = idle_neurons - .clone() - .into_iter() - .map(|neuron| (neuron.id.unwrap().id, neuron)) - .collect(); - - let non_idle = Neuron { - cached_neuron_stake_e8s: 1000, - ..next_test_neuron() - }; - btree_map.insert(non_idle.id.unwrap().id, non_idle); - - // Create some defects (mismatches) - btree_map.get_mut(&1).unwrap().cached_neuron_stake_e8s += 1; - btree_map.get_mut(&2).unwrap().cached_neuron_stake_e8s += 1; - btree_map.remove(&3); - - let neuron_store = NeuronStore::new(btree_map); - - with_stable_neuron_store_mut(|stable_neuron_store| { - for neuron in idle_neurons { - stable_neuron_store.create(neuron).expect("Couldn't create"); - } - }); - - let mut validator = NeuronCopyValidator::new(2); - - // Our first 2 entries should be no bueno - let defects = validator.validate_next_chunk(&neuron_store); - assert_eq!( - defects, - vec![ - ValidationIssue::NeuronCopyValueNotMatch(NeuronId { id: 1 }), - ValidationIssue::NeuronCopyValueNotMatch(NeuronId { id: 2 }) - ] - ); - // Our 3rd entry is missing from heap entirely - let defects = validator.validate_next_chunk(&neuron_store); - assert_eq!( - defects, - vec![ValidationIssue::NeuronStoreError( - "Neuron not found: NeuronId { id: 3 }".to_string() - ),] - ); - - // No further issues should be found - for _ in 0..2 { - let defects = validator.validate_next_chunk(&neuron_store); - assert_eq!(defects, vec![]); - assert!(!validator.is_done()); - } - let defects = validator.validate_next_chunk(&neuron_store); - assert_eq!(defects, vec![]); - assert!(validator.is_done()); - } - #[test] fn test_validator_valid() { // Both followees and principals (controller is a hot key) have duplicates since we do allow @@ -948,9 +811,23 @@ mod tests { fn test_validator_invalid_issues() { // Step 1: Cause as many issues as possible by having an inactive neuron (without adding it // to stable_neuron_store, and remove the only neuron from indexes). - let neuron = next_test_neuron(); + let neuron = Neuron { + cached_neuron_stake_e8s: 0, + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), + ..next_test_neuron() + }; let neuron_store = NeuronStore::new(btreemap! {neuron.id.unwrap().id => neuron.clone()}); + // Remove the neuron from indexes to cause issues with indexes validation. with_stable_neuron_indexes_mut(|indexes| indexes.remove_neuron(&neuron)).unwrap(); + // Make the neuron active while it's still in stable storage, to cause the issue with stable neuron store validation. + with_stable_neuron_store_mut(|stable_neuron_store| { + stable_neuron_store + .update(Neuron { + cached_neuron_stake_e8s: 1, + ..neuron + }) + .unwrap() + }); // Step 2: Validate and get validation summary. let mut validator = NeuronDataValidator::new(); @@ -962,7 +839,7 @@ mod tests { // Step 3: Check validation summary for current issues. let current_issue_groups = summary.current_issues_summary.unwrap().issue_groups; - assert_eq!(current_issue_groups.len(), 8); + assert_eq!(current_issue_groups.len(), 9); assert!(current_issue_groups .iter() .any(|issue_group| issue_group.issues_count == 1 @@ -1023,6 +900,11 @@ mod tests { issue_group.example_issues[0], ValidationIssue::KnownNeuronMissingFromIndex { .. } ))); + assert!(current_issue_groups + .iter() + .any(|issue_group| issue_group.issues_count == 1 + && issue_group.example_issues[0] + == ValidationIssue::NeuronCopyValueNotMatch(neuron.id.unwrap()))); // Step 4: check that previous issues is empty and no running validation. assert_eq!(summary.previous_issues_summary, None); @@ -1036,7 +918,7 @@ mod tests { let summary = validator.summary(); assert_eq!( summary.previous_issues_summary.unwrap().issue_groups.len(), - 8 + 9 ); assert_eq!(summary.current_validation_started_time_seconds, Some(now)); } diff --git a/rs/nns/governance/src/neuron_store.rs b/rs/nns/governance/src/neuron_store.rs index 8cabb2d52f6..6251302037b 100644 --- a/rs/nns/governance/src/neuron_store.rs +++ b/rs/nns/governance/src/neuron_store.rs @@ -674,6 +674,42 @@ impl NeuronStore { }) } + /// Validates a batch of neurons in stable neuron store are all inactive. + /// + /// The batch is defined as the `next_neuron_id` to start and the `batch_size` for the upper + /// bound of the number of neurons to validate. + /// + /// Returns the neuron id the next batch will start with (the neuron id last validated + 1). If + /// no neuron is validated in this batch, returns None. + pub fn batch_validate_neurons_in_stable_store_are_inactive( + &self, + next_neuron_id: NeuronId, + batch_size: usize, + ) -> (Vec, Option) { + let mut neuron_id_for_next_batch = None; + let active_neurons_in_stable_store = with_stable_neuron_store(|stable_neuron_store| { + stable_neuron_store + .range_neurons(next_neuron_id..) + .take(batch_size) + .flat_map(|neuron| { + let current_neuron_id = neuron.id.unwrap(); + neuron_id_for_next_batch = current_neuron_id.next(); + + let is_neuron_inactive = neuron.is_inactive(self.now()); + + if is_neuron_inactive { + None + } else { + // An active neuron in stable neuron store is invalid. + Some(current_neuron_id) + } + }) + .collect() + }); + + (active_neurons_in_stable_store, neuron_id_for_next_batch) + } + // Census pub fn stable_neuron_store_len(&self) -> u64 { diff --git a/rs/nns/governance/src/neuron_store/neuron_store_tests.rs b/rs/nns/governance/src/neuron_store/neuron_store_tests.rs index 5232877ab01..545ac7b77a4 100644 --- a/rs/nns/governance/src/neuron_store/neuron_store_tests.rs +++ b/rs/nns/governance/src/neuron_store/neuron_store_tests.rs @@ -9,6 +9,7 @@ use crate::{ }; use ic_nervous_system_common::{cmc::MockCMC, ledger::MockIcpLedger}; use maplit::{btreemap, hashmap, hashset}; +use num_traits::bounds::LowerBounded; use std::time::{SystemTime, UNIX_EPOCH}; fn simple_neuron(id: u64) -> Neuron { @@ -411,3 +412,71 @@ fn test_neuron_store_new_then_restore() { ); } } + +#[test] +fn test_batch_validate_neurons_in_stable_store_are_inactive() { + // Create a neuron store with 80 neurons. + let mut neuron_store = NeuronStore::new(BTreeMap::new()); + for i in 1..=80 { + // The dissolve state timestamp is chosen so that it meets the inactive neuron criteria. + neuron_store + .add_neuron(Neuron { + cached_neuron_stake_e8s: 0, + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), + ..simple_neuron(i) + }) + .unwrap(); + } + + // Validate 8 batches with 10 each batch. + let mut next_neuron_id = NeuronId::min_value(); + for _ in 0..8 { + let (invalid_neuron_ids, neuron_id_for_next_batch) = + neuron_store.batch_validate_neurons_in_stable_store_are_inactive(next_neuron_id, 10); + + // No invalid neuron ids should be found. + assert_eq!(invalid_neuron_ids, vec![]); + + // There should always be the next neuron id. + next_neuron_id = neuron_id_for_next_batch.unwrap(); + } + + // Validate one more time and there shouldn't be any validation done for this round. + let (invalid_neuron_ids, neuron_id_for_next_batch) = + neuron_store.batch_validate_neurons_in_stable_store_are_inactive(next_neuron_id, 10); + assert_eq!(invalid_neuron_ids, vec![]); + assert_eq!(neuron_id_for_next_batch, None); +} + +#[test] +fn test_batch_validate_neurons_in_stable_store_are_inactive_invalid() { + // Step 1.1: set up 1 inactive neuron. + let neuron = Neuron { + cached_neuron_stake_e8s: 0, + dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(1)), + ..simple_neuron(1) + }; + + // Step 1.2: create neuron store with no neurons. + let mut neuron_store = NeuronStore::new(BTreeMap::new()); + + // Step 1.3: add the inactive neuron into neuron store. + neuron_store.add_neuron(neuron.clone()).unwrap(); + + // Step 1.4: modify the inactive in stable neuron store to make it actually active. + with_stable_neuron_store_mut(|stable_neuron_store| { + stable_neuron_store + .update(Neuron { + cached_neuron_stake_e8s: 1, + ..neuron + }) + .unwrap() + }); + + // Step 2: calls `batch_validate_neurons_in_stable_store_are_inactive` to validate. + let (invalid_neuron_ids, _) = + neuron_store.batch_validate_neurons_in_stable_store_are_inactive(NeuronId::min_value(), 10); + + // Step 3: verifies the results - the active neuron in stable storage should be found as invalid. + assert_eq!(invalid_neuron_ids, vec![neuron.id.unwrap()]); +}