diff --git a/rs/nns/governance/canbench/canbench_results.yml b/rs/nns/governance/canbench/canbench_results.yml index 199502c1e69..108fdf47ca3 100644 --- a/rs/nns/governance/canbench/canbench_results.yml +++ b/rs/nns/governance/canbench/canbench_results.yml @@ -1,61 +1,61 @@ benches: add_neuron_active_maximum: total: - instructions: 35992698 + instructions: 36149096 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_active_typical: total: - instructions: 1838632 + instructions: 1834158 heap_increase: 0 stable_memory_increase: 0 scopes: {} add_neuron_inactive_maximum: total: - instructions: 98666560 + instructions: 96285952 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_inactive_typical: total: - instructions: 7577551 + instructions: 7381170 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_all_heap: total: - instructions: 32302982 + instructions: 31887311 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_heap_neurons_stable_index: total: - instructions: 54494918 + instructions: 54233977 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_stable_everything: total: - instructions: 1969690886 + instructions: 160313292 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_stable_neurons_with_heap_index: total: - instructions: 1947188278 + instructions: 138075846 heap_increase: 0 stable_memory_increase: 0 scopes: {} centralized_following_all_stable: total: - instructions: 2047356165 + instructions: 66264382 heap_increase: 0 stable_memory_increase: 0 scopes: {} compute_ballots_for_new_proposal_with_stable_neurons: total: - instructions: 1743731 + instructions: 1690958 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -85,31 +85,31 @@ benches: scopes: {} neuron_metrics_calculation_heap: total: - instructions: 845137 + instructions: 837546 heap_increase: 0 stable_memory_increase: 0 scopes: {} neuron_metrics_calculation_stable: total: - instructions: 1835769 + instructions: 1843015 heap_increase: 0 stable_memory_increase: 0 scopes: {} range_neurons_performance: total: - instructions: 47339494 + instructions: 47360359 heap_increase: 0 stable_memory_increase: 0 scopes: {} single_vote_all_stable: total: - instructions: 13161102 + instructions: 364240 heap_increase: 0 stable_memory_increase: 0 scopes: {} - update_recent_ballots: + update_recent_ballots_stable_memory: total: - instructions: 16208293 + instructions: 236573 heap_increase: 0 stable_memory_increase: 0 scopes: {} diff --git a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto index 09b44599bb0..1e52b445a7f 100644 --- a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto +++ b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto @@ -458,6 +458,11 @@ message Neuron { // refreshed, this will be set to 2024-11-05T00:00:01 UTC (1730764801 seconds // after the UNIX epoch). optional uint64 voting_power_refreshed_timestamp_seconds = 24; + + // The index of the next entry in the recent_ballots list that will be + // used for the circular buffer. This is used to determine which entry + // to overwrite next. + optional uint32 recent_ballots_next_entry_index = 25; } // Subset of Neuron that has no collections or big fields that might not exist in most neurons, and @@ -484,6 +489,7 @@ message AbridgedNeuron { optional NeuronType neuron_type = 22; optional Visibility visibility = 23; optional uint64 voting_power_refreshed_timestamp_seconds = 24; + optional uint32 recent_ballots_next_entry_index = 25; reserved 1; reserved "id"; diff --git a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs index cf4c5ff9332..2f4d4c246c0 100644 --- a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs +++ b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs @@ -306,6 +306,11 @@ pub struct Neuron { /// after the UNIX epoch). #[prost(uint64, optional, tag = "24")] pub voting_power_refreshed_timestamp_seconds: ::core::option::Option, + /// The index of the next entry in the recent_ballots list that will be + /// used for the circular buffer. This is used to determine which entry + /// to overwrite next. + #[prost(uint32, optional, tag = "25")] + pub recent_ballots_next_entry_index: ::core::option::Option, /// At any time, at most one of `when_dissolved` and /// `dissolve_delay` are specified. /// @@ -433,6 +438,8 @@ pub struct AbridgedNeuron { pub visibility: ::core::option::Option, #[prost(uint64, optional, tag = "24")] pub voting_power_refreshed_timestamp_seconds: ::core::option::Option, + #[prost(uint32, optional, tag = "25")] + pub recent_ballots_next_entry_index: ::core::option::Option, #[prost(oneof = "abridged_neuron::DissolveState", tags = "9, 10")] pub dissolve_state: ::core::option::Option, } diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index e5cad36971b..b9b4a312200 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.rs @@ -2313,7 +2313,13 @@ impl Governance { && neuron.visibility() == Some(Visibility::Public) ); if let_caller_read_full_neuron { - full_neurons.push(NeuronProto::from(neuron.clone())); + let mut proto = NeuronProto::from(neuron.clone()); + // We get the recent_ballots from the neuron itself, because + // we are using a circular buffer to store them. This solution is not ideal, but + // we need to do a larger refactoring to use the correct API types instead of the internal + // governance proto at this level. + proto.recent_ballots = neuron.sorted_recent_ballots(); + full_neurons.push(proto); } }); } diff --git a/rs/nns/governance/src/neuron/types.rs b/rs/nns/governance/src/neuron/types.rs index 6c9ad6c4610..8487d63ca42 100644 --- a/rs/nns/governance/src/neuron/types.rs +++ b/rs/nns/governance/src/neuron/types.rs @@ -125,6 +125,10 @@ pub struct Neuron { /// month. After that, following is cleared, except for ManageNeuron /// proposals. voting_power_refreshed_timestamp_seconds: u64, + /// This field is used to store the index of the most recent ballot in the + /// `recent_ballots` circular buffer. This is used to optimize insertions + /// into stable memory, to avoid rewriting all the data. + pub recent_ballots_next_entry_index: Option, } /// This is mostly the same as the version of PartialEq generated by derive. The @@ -160,6 +164,7 @@ impl PartialEq for Neuron { voting_power_refreshed_timestamp_seconds: &'a u64, visibility: Visibility, + recent_ballots_next_entry_index: Option, } impl<'a> Normalized<'a> { @@ -188,6 +193,7 @@ impl PartialEq for Neuron { voting_power_refreshed_timestamp_seconds, visibility: _, + recent_ballots_next_entry_index, } = src; let visibility = src.visibility().unwrap_or(Visibility::Private); @@ -216,6 +222,7 @@ impl PartialEq for Neuron { voting_power_refreshed_timestamp_seconds, visibility, + recent_ballots_next_entry_index: *recent_ballots_next_entry_index, } } } @@ -429,6 +436,23 @@ impl Neuron { std::cmp::min(ad_stake, u64::MAX as u128) as u64 } + /// Get the recent ballots, with most recent ballots first + pub(crate) fn sorted_recent_ballots(&self) -> Vec { + if let Some(index) = self.recent_ballots_next_entry_index { + // We store ballots in a circular buffer with oldest first, so we need to reverse + // this as well as rearrange it before returning it. + let recent_ballots = &self.recent_ballots; + recent_ballots[index..] + .iter() + .chain(recent_ballots[..index].iter()) + .rev() + .cloned() + .collect() + } else { + self.recent_ballots.clone() + } + } + /// Given the specified `ballots`: determine how this neuron would /// vote on a proposal of `topic` based on which neurons this /// neuron follows on this topic (or on the default topic if this @@ -489,6 +513,7 @@ impl Neuron { /// Register that this neuron has cast a ballot for a /// proposal. Don't include votes on "real time" topics (such as /// setting the ICP/SDR exchange rate). + /// TODO(NNS1-3479) delete this method after all neurons are migrated to stable memory pub(crate) fn register_recent_ballot( &mut self, topic: Topic, @@ -500,20 +525,29 @@ impl Neuron { if topic == Topic::ExchangeRate { return; } + + // Data migration for updating the recent ballots so we can use a circular buffer here. + let next_entry_index = if let Some(index) = self.recent_ballots_next_entry_index { + index + } else { + self.recent_ballots.reverse(); + self.recent_ballots.len() % MAX_NEURON_RECENT_BALLOTS + }; + let ballot_info = BallotInfo { proposal_id: Some(*proposal_id), vote: vote as i32, }; - // We would really like to have a circular buffer here. As - // we're dealing with a simple vector, we insert at the - // beginning and remove at the end once we have reached - // the maximum number of votes to keep track of. - self.recent_ballots.insert(0, ballot_info); - // Pop and discard elements from the end until we reach - // the maximum allowed length of the vector. - while self.recent_ballots.len() > MAX_NEURON_RECENT_BALLOTS { - self.recent_ballots.pop(); + + // Vector is full + if self.recent_ballots.len() >= MAX_NEURON_RECENT_BALLOTS { + self.recent_ballots[next_entry_index] = ballot_info; + } else { + self.recent_ballots.push(ballot_info); } + // Advance the index + self.recent_ballots_next_entry_index = + Some((next_entry_index + 1) % MAX_NEURON_RECENT_BALLOTS); } pub(crate) fn refresh_voting_power(&mut self, now_seconds: u64) { @@ -875,7 +909,7 @@ impl Neuron { || self.visibility() == Some(Visibility::Public) || self.is_hotkey_or_controller(&requester); if show_full { - recent_ballots.append(&mut self.recent_ballots.clone()); + recent_ballots.append(&mut self.sorted_recent_ballots()); joined_community_fund_timestamp_seconds = self.joined_community_fund_timestamp_seconds; } @@ -1098,7 +1132,6 @@ impl Neuron { impl From for NeuronProto { fn from(neuron: Neuron) -> Self { let visibility = neuron.visibility().map(|visibility| visibility as i32); - let Neuron { id, subaccount, @@ -1122,6 +1155,7 @@ impl From for NeuronProto { neuron_type, visibility: _, voting_power_refreshed_timestamp_seconds, + recent_ballots_next_entry_index, } = neuron; let id = Some(id); @@ -1158,6 +1192,7 @@ impl From for NeuronProto { neuron_type, visibility, voting_power_refreshed_timestamp_seconds, + recent_ballots_next_entry_index: recent_ballots_next_entry_index.map(|x| x as u32), } } } @@ -1190,6 +1225,7 @@ impl TryFrom for Neuron { neuron_type, visibility, voting_power_refreshed_timestamp_seconds, + recent_ballots_next_entry_index, } = proto; let id = id.ok_or("Neuron ID is missing")?; @@ -1235,6 +1271,7 @@ impl TryFrom for Neuron { neuron_type, visibility, voting_power_refreshed_timestamp_seconds, + recent_ballots_next_entry_index: recent_ballots_next_entry_index.map(|x| x as usize), }) } } @@ -1328,6 +1365,7 @@ impl TryFrom for DecomposedNeuron { neuron_type, visibility, voting_power_refreshed_timestamp_seconds, + recent_ballots_next_entry_index, } = source; let account = subaccount.to_vec(); @@ -1359,6 +1397,15 @@ impl TryFrom for DecomposedNeuron { dissolve_state, visibility, voting_power_refreshed_timestamp_seconds, + // Conversion to u32 is safe because the value cannot be very large. If it overflowed + // u32 max, we would have spent 68GB on recent ballots for this single neuron. + recent_ballots_next_entry_index: recent_ballots_next_entry_index + .map(|x| { + u32::try_from(x).map_err(|e| NeuronStoreError::InvalidData { + reason: format!("Failed to convert recent_ballots_next_entry_index: {}", e), + }) + }) + .transpose()?, }; Ok(Self { @@ -1409,6 +1456,7 @@ impl From for Neuron { dissolve_state, visibility, voting_power_refreshed_timestamp_seconds, + recent_ballots_next_entry_index, } = main; let subaccount = @@ -1447,6 +1495,8 @@ impl From for Neuron { neuron_type, visibility, voting_power_refreshed_timestamp_seconds, + // usize is always at least u32, so this is safe. + recent_ballots_next_entry_index: recent_ballots_next_entry_index.map(|x| x as usize), } } } @@ -1716,6 +1766,7 @@ impl NeuronBuilder { neuron_type, visibility, voting_power_refreshed_timestamp_seconds, + recent_ballots_next_entry_index: None, } } } diff --git a/rs/nns/governance/src/neuron/types/tests.rs b/rs/nns/governance/src/neuron/types/tests.rs index 3678f07600a..00371c694da 100644 --- a/rs/nns/governance/src/neuron/types/tests.rs +++ b/rs/nns/governance/src/neuron/types/tests.rs @@ -631,3 +631,140 @@ fn test_adjust_voting_power_disabled() { ); } } + +#[test] +fn test_conversion_from_old_ballot_storage_full() { + let principal_id = PrincipalId::new_user_test_id(42); + let created_timestamp_seconds = 1729791574; + + let recent_ballots: Vec<_> = (0..100) + .map(|id| BallotInfo { + proposal_id: Some(ProposalId { id }), + vote: Vote::Yes as i32, + }) + .collect(); + + let mut neuron = NeuronBuilder::new( + NeuronId { id: 42 }, + Subaccount::try_from(vec![42u8; 32].as_slice()).unwrap(), + principal_id, + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: 12 * ONE_MONTH_SECONDS, + aging_since_timestamp_seconds: created_timestamp_seconds + 42, + }, + created_timestamp_seconds, // created + ) + .with_recent_ballots(recent_ballots.clone()) + .build(); + neuron.recent_ballots_next_entry_index = None; + + assert_eq!(neuron.recent_ballots, recent_ballots); + + neuron.register_recent_ballot(Topic::NetworkEconomics, &ProposalId { id: 100 }, Vote::No); + + assert_eq!(neuron.recent_ballots_next_entry_index, Some(1)); + + let expected_updated_ballots = { + let mut recent_ballots = recent_ballots.clone(); + recent_ballots.reverse(); + recent_ballots[0] = BallotInfo { + proposal_id: Some(ProposalId { id: 100 }), + vote: Vote::No as i32, + }; + recent_ballots + }; + + assert_eq!(neuron.recent_ballots, expected_updated_ballots); +} + +#[test] +fn test_conversion_from_old_ballot_storage_not_full() { + let principal_id = PrincipalId::new_user_test_id(42); + let created_timestamp_seconds = 1729791574; + + let recent_ballots: Vec<_> = (0..75) + .map(|id| BallotInfo { + proposal_id: Some(ProposalId { id }), + vote: Vote::Yes as i32, + }) + .collect(); + + let mut neuron = NeuronBuilder::new( + NeuronId { id: 42 }, + Subaccount::try_from(vec![42u8; 32].as_slice()).unwrap(), + principal_id, + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: 12 * ONE_MONTH_SECONDS, + aging_since_timestamp_seconds: created_timestamp_seconds + 42, + }, + created_timestamp_seconds, // created + ) + .with_recent_ballots(recent_ballots.clone()) + .build(); + neuron.recent_ballots_next_entry_index = None; + + assert_eq!(neuron.recent_ballots, recent_ballots); + + neuron.register_recent_ballot(Topic::NetworkEconomics, &ProposalId { id: 100 }, Vote::No); + + assert_eq!(neuron.recent_ballots_next_entry_index, Some(76)); + + let expected_updated_ballots = { + let mut recent_ballots = recent_ballots.clone(); + recent_ballots.reverse(); + recent_ballots.push(BallotInfo { + proposal_id: Some(ProposalId { id: 100 }), + vote: Vote::No as i32, + }); + recent_ballots + }; + + assert_eq!(neuron.recent_ballots, expected_updated_ballots); +} + +#[test] +fn test_recent_ballots_accessor_pre_and_post_migration() { + let principal_id = PrincipalId::new_user_test_id(42); + let created_timestamp_seconds = 1729791574; + + let recent_ballots: Vec<_> = (0..100) + .map(|id| BallotInfo { + proposal_id: Some(ProposalId { id }), + vote: Vote::Yes as i32, + }) + .collect(); + + let mut neuron = NeuronBuilder::new( + NeuronId { id: 42 }, + Subaccount::try_from(vec![42u8; 32].as_slice()).unwrap(), + principal_id, + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: 12 * ONE_MONTH_SECONDS, + aging_since_timestamp_seconds: created_timestamp_seconds + 42, + }, + created_timestamp_seconds, // created + ) + .with_recent_ballots(recent_ballots.clone()) + .build(); + neuron.recent_ballots_next_entry_index = None; + + assert_eq!(neuron.sorted_recent_ballots(), recent_ballots); + + neuron.register_recent_ballot(Topic::NetworkEconomics, &ProposalId { id: 100 }, Vote::No); + assert_eq!(neuron.recent_ballots_next_entry_index, Some(1)); + + let expected_updated_ballots = { + let mut recent_ballots = recent_ballots.clone(); + recent_ballots.insert( + 0, + BallotInfo { + proposal_id: Some(ProposalId { id: 100 }), + vote: Vote::No as i32, + }, + ); + recent_ballots.pop(); + recent_ballots + }; + + assert_eq!(neuron.sorted_recent_ballots(), expected_updated_ballots); +} diff --git a/rs/nns/governance/src/neuron_store.rs b/rs/nns/governance/src/neuron_store.rs index 2f3745898b9..e4c794e11ac 100644 --- a/rs/nns/governance/src/neuron_store.rs +++ b/rs/nns/governance/src/neuron_store.rs @@ -25,7 +25,7 @@ use ic_nervous_system_governance::index::{ neuron_following::{HeapNeuronFollowingIndex, NeuronFollowingIndex}, neuron_principal::NeuronPrincipalIndex, }; -use ic_nns_common::pb::v1::NeuronId; +use ic_nns_common::pb::v1::{NeuronId, ProposalId}; use icp_ledger::{AccountIdentifier, Subaccount}; use std::{ borrow::Cow, @@ -1114,6 +1114,31 @@ impl NeuronStore { }) } + pub fn register_recent_neuron_ballot( + &mut self, + neuron_id: NeuronId, + topic: Topic, + proposal_id: ProposalId, + vote: Vote, + ) -> Result<(), NeuronStoreError> { + if self.heap_neurons.contains_key(&neuron_id.id) { + self.with_neuron_mut(&neuron_id, |neuron| { + neuron.register_recent_ballot(topic, &proposal_id, vote) + })?; + } else { + with_stable_neuron_store_mut(|stable_neuron_store| { + stable_neuron_store.register_recent_neuron_ballot( + neuron_id, + topic, + proposal_id, + vote, + ) + })?; + } + + Ok(()) + } + // Below are indexes related methods. They don't have a unified interface yet, but NNS1-2507 will change that. // Read methods for indexes. diff --git a/rs/nns/governance/src/neuron_store/benches.rs b/rs/nns/governance/src/neuron_store/benches.rs index 74cb0821206..e0d592d74c2 100644 --- a/rs/nns/governance/src/neuron_store/benches.rs +++ b/rs/nns/governance/src/neuron_store/benches.rs @@ -182,12 +182,13 @@ fn add_neuron_inactive_typical() -> BenchResult { } #[bench(raw)] -fn update_recent_ballots() -> BenchResult { +fn update_recent_ballots_stable_memory() -> BenchResult { let _a = temporarily_enable_active_neurons_in_stable_memory(); let _b = temporarily_enable_stable_memory_following_index(); let mut rng = new_rng(); let mut neuron_store = set_up_neuron_store(&mut rng, 100, 200); let neuron = build_neuron(&mut rng, NeuronLocation::Heap, NeuronSize::Maximum); + let id = neuron.id(); assert_eq!(neuron.recent_ballots.len(), MAX_NEURON_RECENT_BALLOTS); @@ -196,13 +197,12 @@ fn update_recent_ballots() -> BenchResult { bench_fn(|| { neuron_store - .with_neuron_mut(&id, |neuron| { - neuron.register_recent_ballot( - Topic::NetworkEconomics, - &ProposalId { id: rng.next_u64() }, - Vote::Yes, - ) - }) + .register_recent_neuron_ballot( + id, + Topic::NetworkEconomics, + ProposalId { id: rng.next_u64() }, + Vote::Yes, + ) .unwrap(); }) } diff --git a/rs/nns/governance/src/pb/conversions.rs b/rs/nns/governance/src/pb/conversions.rs index ea6336ffc26..89f51dba469 100644 --- a/rs/nns/governance/src/pb/conversions.rs +++ b/rs/nns/governance/src/pb/conversions.rs @@ -179,6 +179,8 @@ impl From for pb::Neuron { dissolve_state: item.dissolve_state.map(|x| x.into()), visibility: item.visibility, voting_power_refreshed_timestamp_seconds: item.voting_power_refreshed_timestamp_seconds, + // This field is internal only and should not be read from API types. + recent_ballots_next_entry_index: None, } } } diff --git a/rs/nns/governance/src/storage/neurons.rs b/rs/nns/governance/src/storage/neurons.rs index a33881c8288..868e48c2b1d 100644 --- a/rs/nns/governance/src/storage/neurons.rs +++ b/rs/nns/governance/src/storage/neurons.rs @@ -8,7 +8,7 @@ use crate::{ }; use candid::Principal; use ic_base_types::PrincipalId; -use ic_nns_common::pb::v1::NeuronId; +use ic_nns_common::pb::v1::{NeuronId, ProposalId}; use ic_stable_structures::{storable::Bound, StableBTreeMap, Storable}; use itertools::Itertools; use lazy_static::lazy_static; @@ -16,7 +16,7 @@ use maplit::hashmap; use prost::Message; use std::{ borrow::Cow, - collections::{BTreeMap as HeapBTreeMap, BTreeSet as HeapBTreeSet, HashMap}, + collections::{btree_map::Entry, BTreeMap as HeapBTreeMap, HashMap}, iter::Peekable, ops::{Bound as RangeBound, RangeBounds}, }; @@ -230,6 +230,58 @@ where Ok(self.reconstitute_neuron(neuron_id, main_neuron_part, sections)) } + pub fn register_recent_neuron_ballot( + &mut self, + neuron_id: NeuronId, + topic: Topic, + proposal_id: ProposalId, + vote: Vote, + ) -> Result<(), NeuronStoreError> { + if topic == Topic::ExchangeRate { + return Ok(()); + } + + let main_neuron_part = self + .main + .get(&neuron_id) + // Deal with no entry by blaming it on the caller. + .ok_or_else(|| NeuronStoreError::not_found(neuron_id))?; + + let recent_entry_index = main_neuron_part.recent_ballots_next_entry_index; + + let next_entry_index = if let Some(recent_entry_index) = recent_entry_index { + recent_entry_index as usize + } else { + let mut ballots = read_repeated_field(neuron_id, &self.recent_ballots_map); + ballots.reverse(); + let next_entry = ballots.len() % MAX_NEURON_RECENT_BALLOTS; + update_repeated_field(neuron_id, ballots, &mut self.recent_ballots_map); + next_entry + }; + // We cannot error after this, or we risk creating some chaos with the ordering + // of the ballots b/c of the migration code above. + + let ballot_info = BallotInfo { + proposal_id: Some(proposal_id), + vote: vote as i32, + }; + + insert_element_in_repeated_field( + neuron_id, + next_entry_index as u64, + ballot_info, + &mut self.recent_ballots_map, + ); + + // update the main part now + let mut main_neuron_part = main_neuron_part; + main_neuron_part.recent_ballots_next_entry_index = + Some(((next_entry_index + 1) % MAX_NEURON_RECENT_BALLOTS) as u32); + self.main.insert(neuron_id, main_neuron_part); + + Ok(()) + } + /// Changes an existing entry. /// /// If the entry does not already exist, returns a NotFound Err. @@ -630,6 +682,7 @@ pub struct NeuronStorageLens { pub known_neuron_data: u64, } +use crate::{governance::MAX_NEURON_RECENT_BALLOTS, pb::v1::Vote}; #[cfg(test)] use ic_stable_structures::VectorMemory; @@ -798,7 +851,7 @@ fn update_repeated_field( new_elements: Vec, map: &mut StableBTreeMap<(NeuronId, /* index */ u64), Element, Memory>, ) where - Element: Storable, + Element: Storable + PartialEq, Memory: ic_stable_structures::Memory, { let new_entries = new_elements @@ -819,31 +872,53 @@ fn update_repeated_field( update_range(new_entries, range, map) } +fn insert_element_in_repeated_field( + neuron_id: NeuronId, + index: u64, + element: Element, + map: &mut StableBTreeMap<(NeuronId, /* index */ u64), Element, Memory>, +) where + Element: Storable + PartialEq, + Memory: ic_stable_structures::Memory, +{ + let key = (neuron_id, index); + map.insert(key, element); +} + /// Replaces the contents of map where keys are in range with new_entries. // TODO(NNS1-2513): To avoid the caller passing an incorrect range (e.g. too // small, or to big), derive range from NeuronId. fn update_range( - new_entries: HeapBTreeMap, + mut new_entries: HeapBTreeMap, range: impl RangeBounds, map: &mut StableBTreeMap, ) where Key: Storable + Ord + Clone, - Value: Storable, + Value: Storable + PartialEq, Memory: ic_stable_structures::Memory, { - let new_keys = new_entries.keys().cloned().collect::>(); + let mut to_remove = vec![]; + for (key, value) in map.range(range) { + match new_entries.entry(key.clone()) { + // If our new entries do not include a key in existing, we remove it from existing. + Entry::Vacant(_) => { + to_remove.push(key); + } + Entry::Occupied(entry) => { + // If our new entries have the same value as what exists, we do not want to insert + // it, but instead remove it from the list of new entries, since it's present. + if *entry.get() == value { + entry.remove(); + } + } + }; + } for (new_key, new_value) in new_entries { map.insert(new_key, new_value); } - let obsolete_keys = map - .range(range) - .filter(|(key, _value)| !new_keys.contains(key)) - .map(|(key, _value)| key) - .collect::>(); - - for obsolete_key in obsolete_keys { + for obsolete_key in to_remove { map.remove(&obsolete_key); } } diff --git a/rs/nns/governance/src/storage/neurons/neurons_tests.rs b/rs/nns/governance/src/storage/neurons/neurons_tests.rs index df650149d78..5580435307f 100644 --- a/rs/nns/governance/src/storage/neurons/neurons_tests.rs +++ b/rs/nns/governance/src/storage/neurons/neurons_tests.rs @@ -506,12 +506,13 @@ fn test_abridged_neuron_size() { dissolve_state: Some(DissolveState::WhenDissolvedTimestampSeconds(u64::MAX)), visibility: None, voting_power_refreshed_timestamp_seconds: Some(u64::MAX), + recent_ballots_next_entry_index: Some(100), }; assert!(abridged_neuron.encoded_len() as u32 <= AbridgedNeuron::BOUND.max_size()); // This size can be updated. This assertion is here to make sure we are very aware of growth. // Reminder: the amount we allocated for AbridgedNeuron is 380 bytes. - assert_eq!(abridged_neuron.encoded_len(), 196); + assert_eq!(abridged_neuron.encoded_len(), 199); } #[test] @@ -564,3 +565,129 @@ fn test_range_neurons_ranges_work_correctly() { .collect::>(); assert_eq!(result, neurons[2..3]); } + +#[test] +fn test_register_recent_neuron_ballot_migration_full() { + // Set up with 100 ballots, and ensure that the pointer is in the right place and the ballots are reversed + let mut store = new_heap_based(); + let mut neuron = create_model_neuron(1); + neuron.recent_ballots_next_entry_index = None; + + let recent_ballots = (0..100) + .map(|i| BallotInfo { + proposal_id: Some(ProposalId { id: i as u64 }), + vote: Vote::Yes as i32, + }) + .collect::>(); + + neuron.recent_ballots = recent_ballots.clone(); + + store.create(neuron.clone()).unwrap(); + + let retrieved_neuron = store.read(neuron.id(), NeuronSections::all()).unwrap(); + assert_eq!(retrieved_neuron, neuron); + + store + .register_recent_neuron_ballot( + neuron.id(), + Topic::NetworkEconomics, + ProposalId { id: 100 }, + Vote::No, + ) + .unwrap(); + + let mut expected_updated_ballots = { + let mut recent_ballots = recent_ballots.clone(); + recent_ballots.reverse(); + recent_ballots[0] = BallotInfo { + proposal_id: Some(ProposalId { id: 100 }), + vote: Vote::No as i32, + }; + recent_ballots + }; + + let retrieved_neuron = store.read(neuron.id(), NeuronSections::all()).unwrap(); + assert_eq!(retrieved_neuron.recent_ballots, expected_updated_ballots); + assert_eq!(retrieved_neuron.recent_ballots_next_entry_index, Some(1)); + + // Now, let's add another ballot and ensure that the pointer is updated correctly and ballots + // are not reversed again + store + .register_recent_neuron_ballot( + neuron.id(), + Topic::NetworkEconomics, + ProposalId { id: 101 }, + Vote::Yes, + ) + .unwrap(); + expected_updated_ballots[1] = BallotInfo { + proposal_id: Some(ProposalId { id: 101 }), + vote: Vote::Yes as i32, + }; + let retrieved_neuron = store.read(neuron.id(), NeuronSections::all()).unwrap(); + assert_eq!(retrieved_neuron.recent_ballots, expected_updated_ballots); + assert_eq!(retrieved_neuron.recent_ballots_next_entry_index, Some(2)); +} + +#[test] +fn test_register_recent_neuron_ballot_migration_notfull() { + // Set up with 100 ballots, and ensure that the pointer is in the right place and the ballots are reversed + let mut store = new_heap_based(); + let mut neuron = create_model_neuron(1); + neuron.recent_ballots_next_entry_index = None; + + let recent_ballots = (0..20) + .map(|i| BallotInfo { + proposal_id: Some(ProposalId { id: i as u64 }), + vote: Vote::Yes as i32, + }) + .collect::>(); + + neuron.recent_ballots = recent_ballots.clone(); + + store.create(neuron.clone()).unwrap(); + + let retrieved_neuron = store.read(neuron.id(), NeuronSections::all()).unwrap(); + assert_eq!(retrieved_neuron, neuron); + + store + .register_recent_neuron_ballot( + neuron.id(), + Topic::NetworkEconomics, + ProposalId { id: 100 }, + Vote::No, + ) + .unwrap(); + + let mut expected_updated_ballots = { + let mut recent_ballots = recent_ballots.clone(); + recent_ballots.reverse(); + recent_ballots.push(BallotInfo { + proposal_id: Some(ProposalId { id: 100 }), + vote: Vote::No as i32, + }); + recent_ballots + }; + + let retrieved_neuron = store.read(neuron.id(), NeuronSections::all()).unwrap(); + assert_eq!(retrieved_neuron.recent_ballots, expected_updated_ballots); + assert_eq!(retrieved_neuron.recent_ballots_next_entry_index, Some(21)); + + // Now, let's add another ballot and ensure that the pointer is updated correctly and ballots + // are not reversed again + store + .register_recent_neuron_ballot( + neuron.id(), + Topic::NetworkEconomics, + ProposalId { id: 101 }, + Vote::Yes, + ) + .unwrap(); + expected_updated_ballots.push(BallotInfo { + proposal_id: Some(ProposalId { id: 101 }), + vote: Vote::Yes as i32, + }); + let retrieved_neuron = store.read(neuron.id(), NeuronSections::all()).unwrap(); + assert_eq!(retrieved_neuron.recent_ballots, expected_updated_ballots); + assert_eq!(retrieved_neuron.recent_ballots_next_entry_index, Some(22)); +} diff --git a/rs/nns/governance/src/voting.rs b/rs/nns/governance/src/voting.rs index 18bfde37a63..d9576d8e5ba 100644 --- a/rs/nns/governance/src/voting.rs +++ b/rs/nns/governance/src/voting.rs @@ -188,9 +188,12 @@ impl ProposalVotingStateMachine { } while let Some((neuron_id, vote)) = self.recent_neuron_ballots_to_record.pop_first() { - match neuron_store.with_neuron_mut(&neuron_id, |neuron| { - neuron.register_recent_ballot(self.topic, &self.proposal_id, vote) - }) { + match neuron_store.register_recent_neuron_ballot( + neuron_id, + self.topic, + self.proposal_id, + vote, + ) { Ok(_) => {} Err(e) => { // This is a bad inconsistency, but there is diff --git a/rs/nns/governance/tests/fixtures/mod.rs b/rs/nns/governance/tests/fixtures/mod.rs index b8bc5ffaeb1..72b6087cc94 100755 --- a/rs/nns/governance/tests/fixtures/mod.rs +++ b/rs/nns/governance/tests/fixtures/mod.rs @@ -382,6 +382,7 @@ impl NeuronBuilder { joined_community_fund_timestamp_seconds: self.joined_community_fund, spawn_at_timestamp_seconds: self.spawn_at_timestamp_seconds, neuron_type: self.neuron_type, + recent_ballots_next_entry_index: Some(0), ..Neuron::default() } } diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs index 64fbc4b283b..7f53af80cb9 100644 --- a/rs/nns/governance/tests/governance.rs +++ b/rs/nns/governance/tests/governance.rs @@ -11,7 +11,9 @@ use assert_matches::assert_matches; use async_trait::async_trait; use candid::{Decode, Encode}; use common::increase_dissolve_delay_raw; -use comparable::{Changed, I32Change, MapChange, OptionChange, StringChange, U64Change, VecChange}; +use comparable::{ + Changed, I32Change, MapChange, OptionChange, StringChange, U32Change, U64Change, VecChange, +}; use fixtures::{ account, environment_fixture::CanisterCallReply, new_motion_proposal, principal, NNSBuilder, NNSStateChange, NeuronBuilder, ProposalNeuronBehavior, NNS, @@ -290,6 +292,7 @@ fn test_single_neuron_proposal_new() { ), ), ), + NeuronChange::RecentBallotsNextEntryIndex(OptionChange::BothSome(U32Change(0, 1,),),), ], )]), GovernanceChange::Proposals(vec![MapChange::Added( @@ -961,6 +964,9 @@ async fn test_cascade_following_new() { DEFAULT_TEST_START_TIMESTAMP_SECONDS, ), ),), + NeuronChange::RecentBallotsNextEntryIndex(OptionChange::BothSome(U32Change( + 0, 1, + ),),), ], )]), GovernanceChange::Proposals(vec![MapChange::Added( @@ -1115,6 +1121,9 @@ async fn test_cascade_following_new() { DEFAULT_TEST_START_TIMESTAMP_SECONDS, ), ),), + NeuronChange::RecentBallotsNextEntryIndex(OptionChange::BothSome(U32Change( + 0, 1, + ),),), ], )]), GovernanceChange::Proposals(vec![MapChange::Changed( @@ -1183,29 +1192,39 @@ async fn test_cascade_following_new() { ), MapChange::Changed( 2, - vec![NeuronChange::RecentBallots(vec![VecChange::Added( - 0, - vec![ - BallotInfoChange::ProposalId(OptionChange::Different( - None, - Some(ProposalId { id: 1 }), - )), - BallotInfoChange::Vote(I32Change(0, 1)), - ], - )])], + vec![ + NeuronChange::RecentBallots(vec![VecChange::Added( + 0, + vec![ + BallotInfoChange::ProposalId(OptionChange::Different( + None, + Some(ProposalId { id: 1 }), + )), + BallotInfoChange::Vote(I32Change(0, 1)), + ], + )]), + NeuronChange::RecentBallotsNextEntryIndex(OptionChange::BothSome( + U32Change(0, 1,), + ),), + ], ), MapChange::Changed( 3, - vec![NeuronChange::RecentBallots(vec![VecChange::Added( - 0, - vec![ - BallotInfoChange::ProposalId(OptionChange::Different( - None, - Some(ProposalId { id: 1 }), - )), - BallotInfoChange::Vote(I32Change(0, 1)), - ], - )])], + vec![ + NeuronChange::RecentBallots(vec![VecChange::Added( + 0, + vec![ + BallotInfoChange::ProposalId(OptionChange::Different( + None, + Some(ProposalId { id: 1 }), + )), + BallotInfoChange::Vote(I32Change(0, 1)), + ], + )]), + NeuronChange::RecentBallotsNextEntryIndex(OptionChange::BothSome( + U32Change(0, 1,), + ),), + ], ), MapChange::Changed( 6, @@ -1228,6 +1247,9 @@ async fn test_cascade_following_new() { DEFAULT_TEST_START_TIMESTAMP_SECONDS, ), ),), + NeuronChange::RecentBallotsNextEntryIndex(OptionChange::BothSome( + U32Change(0, 1,), + ),), ], ), ]), @@ -1783,8 +1805,7 @@ async fn test_all_follow_proposer() { })), }, ) - .now_or_never() - .unwrap() + .await .panic_if_error("Manage neuron failed"); // Assert that neuron 5's voting power was refreshed. More concretely, @@ -1815,8 +1836,7 @@ async fn test_all_follow_proposer() { })), }, ) - .now_or_never() - .unwrap() + .await .panic_if_error("Manage neuron failed"); gov.make_proposal(