diff --git a/Cargo.lock b/Cargo.lock index b5a8fd20cc7..7b488140a29 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10090,6 +10090,24 @@ dependencies = [ "tokio", ] +[[package]] +name = "ic-nervous-system-long-message" +version = "0.0.1" +dependencies = [ + "candid", + "canister-test", + "dfn_candid", + "ic-cdk 0.16.0", + "ic-cdk-timers", + "ic-config", + "ic-nervous-system-temporary", + "ic-nns-test-utils", + "ic-registry-subnet-type", + "ic-state-machine-tests", + "ic-types", + "serde", +] + [[package]] name = "ic-nervous-system-proto" version = "0.9.0" @@ -10298,6 +10316,7 @@ dependencies = [ "ic-nervous-system-common-test-utils", "ic-nervous-system-governance", "ic-nervous-system-linear-map", + "ic-nervous-system-long-message", "ic-nervous-system-proto", "ic-nervous-system-root", "ic-nervous-system-runtime", diff --git a/rs/nervous_system/long_message/src/lib.rs b/rs/nervous_system/long_message/src/lib.rs index 01a20974fbb..4681948b9c8 100644 --- a/rs/nervous_system/long_message/src/lib.rs +++ b/rs/nervous_system/long_message/src/lib.rs @@ -6,6 +6,7 @@ use ic_cdk::query; use ic_nervous_system_temporary::Temporary; #[cfg(not(target_arch = "wasm32"))] use std::cell::{Cell, RefCell}; +use std::fmt::{Debug, Display, Formatter}; #[query(hidden = true)] fn __long_message_noop() { @@ -71,6 +72,21 @@ pub fn is_message_over_threshold(instructions_threshold: u64) -> bool { instructions_used >= instructions_threshold } +#[derive(Debug)] +pub struct OverCallContextError { + limit: u64, +} + +impl Display for OverCallContextError { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!( + f, + "Call context instruction counter exceeded the limit of {}", + self.limit + ) + } +} + /// Breaks the message into smaller parts if the number of instructions used /// exceeds the given threshold. /// The upper bound is used to determine the maximum number of instructions @@ -82,19 +98,20 @@ pub fn is_message_over_threshold(instructions_threshold: u64) -> bool { /// # Panics if the number of instructions used exceeds the given panic threshold. pub async fn noop_self_call_if_over_instructions( message_threshold: u64, - panic_threshold: Option, -) { + call_context_threshold: Option, +) -> Result<(), OverCallContextError> { + // We may still need a new message context for whatever cleanupis needed, but also we will return + // an error if the call context is over the threshold. + if is_message_over_threshold(message_threshold) { + make_noop_call().await; + } + // first we check the upper bound to see if we should panic. - if let Some(upper_bound) = panic_threshold { + if let Some(upper_bound) = call_context_threshold { if is_call_context_over_threshold(upper_bound) { - panic!( - "Canister call exceeded the limit of {} instructions in the call context.", - upper_bound - ); + return Err(OverCallContextError { limit: upper_bound }); } } - if is_message_over_threshold(message_threshold) { - make_noop_call().await; - } + Ok(()) } diff --git a/rs/nervous_system/long_message/tests/test_canisters/long_message_canister.rs b/rs/nervous_system/long_message/tests/test_canisters/long_message_canister.rs index 9e061500559..781e809d963 100644 --- a/rs/nervous_system/long_message/tests/test_canisters/long_message_canister.rs +++ b/rs/nervous_system/long_message/tests/test_canisters/long_message_canister.rs @@ -44,7 +44,9 @@ async fn test_next_message_if_over_instructions(params: BreakMessageParams) { // Fib(17) was benchmarked at about 80k instructions fib(17); if use_break { - noop_self_call_if_over_instructions(message_threshold, upper_bound).await; + noop_self_call_if_over_instructions(message_threshold, upper_bound) + .await + .expect("Over upper bound"); } } } diff --git a/rs/nervous_system/long_message/tests/test_message_breaks.rs b/rs/nervous_system/long_message/tests/test_message_breaks.rs index 7b586c1db92..0de8f394f59 100644 --- a/rs/nervous_system/long_message/tests/test_message_breaks.rs +++ b/rs/nervous_system/long_message/tests/test_message_breaks.rs @@ -100,17 +100,7 @@ fn test_upper_bound() { PrincipalId::new_anonymous(), ) .unwrap_err(); - assert!( - err.contains( - format!( - "Canister call exceeded the limit of {} instructions in the call context.", - 700_000 - ) - .as_str() - ), - "Error was: {:?}", - err - ); + assert!(err.contains("OverCallContextError"), "Error was: {:?}", err); update_with_sender::( &state_machine, diff --git a/rs/nns/common/src/lib.rs b/rs/nns/common/src/lib.rs index 0673eda990c..5d5d95619be 100644 --- a/rs/nns/common/src/lib.rs +++ b/rs/nns/common/src/lib.rs @@ -1,7 +1,6 @@ -use crate::pb::v1::NeuronId; +use crate::pb::v1::{NeuronId, ProposalId}; use ic_crypto_sha2::Sha256; -use ic_stable_structures::storable::Bound; -use ic_stable_structures::Storable; +use ic_stable_structures::{storable::Bound, Storable}; use num_traits::bounds::{LowerBounded, UpperBounded}; use std::{borrow::Cow, convert::TryInto}; @@ -44,7 +43,24 @@ impl Storable for NeuronId { } fn from_bytes(bytes: Cow<[u8]>) -> Self { - NeuronId { + Self { + id: u64::from_bytes(bytes), + } + } + + const BOUND: Bound = Bound::Bounded { + max_size: std::mem::size_of::() as u32, + is_fixed_size: true, + }; +} + +impl Storable for ProposalId { + fn to_bytes(&self) -> Cow<[u8]> { + self.id.to_bytes() + } + + fn from_bytes(bytes: Cow<[u8]>) -> Self { + Self { id: u64::from_bytes(bytes), } } diff --git a/rs/nns/governance/BUILD.bazel b/rs/nns/governance/BUILD.bazel index 8d23a6dc44a..ec8bc8609e1 100644 --- a/rs/nns/governance/BUILD.bazel +++ b/rs/nns/governance/BUILD.bazel @@ -37,6 +37,7 @@ DEPENDENCIES = [ "//rs/nervous_system/common", "//rs/nervous_system/governance", "//rs/nervous_system/linear_map", + "//rs/nervous_system/long_message", "//rs/nervous_system/neurons_fund", "//rs/nervous_system/proto", "//rs/nervous_system/root", diff --git a/rs/nns/governance/Cargo.toml b/rs/nns/governance/Cargo.toml index ec01ac9602e..8e6cc0f176d 100644 --- a/rs/nns/governance/Cargo.toml +++ b/rs/nns/governance/Cargo.toml @@ -51,6 +51,7 @@ ic-nervous-system-common = { path = "../../nervous_system/common" } ic-nervous-system-common-build-metadata = { path = "../../nervous_system/common/build_metadata" } ic-nervous-system-governance = { path = "../../nervous_system/governance" } ic-nervous-system-linear-map = { path = "../../nervous_system/linear_map" } +ic-nervous-system-long-message = { path = "../../nervous_system/long_message" } ic-nervous-system-root = { path = "../../nervous_system/root" } ic-nervous-system-runtime = { path = "../../nervous_system/runtime" } ic-nervous-system-proto = { path = "../../nervous_system/proto" } diff --git a/rs/nns/governance/canbench/canbench_results.yml b/rs/nns/governance/canbench/canbench_results.yml index 27e4cdbe820..217d97bd62a 100644 --- a/rs/nns/governance/canbench/canbench_results.yml +++ b/rs/nns/governance/canbench/canbench_results.yml @@ -1,127 +1,127 @@ benches: add_neuron_active_maximum: total: - instructions: 36059517 + instructions: 36184626 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_active_typical: total: - instructions: 1830145 + instructions: 1835593 heap_increase: 0 stable_memory_increase: 0 scopes: {} add_neuron_inactive_maximum: total: - instructions: 96070124 + instructions: 96163372 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_inactive_typical: total: - instructions: 7372921 + instructions: 7371198 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_all_heap: total: - instructions: 34047728 + instructions: 34443866 heap_increase: 0 - stable_memory_increase: 0 + stable_memory_increase: 128 scopes: {} cascading_vote_heap_neurons_stable_index: total: - instructions: 56554267 + instructions: 56578763 heap_increase: 0 - stable_memory_increase: 0 + stable_memory_increase: 128 scopes: {} cascading_vote_stable_everything: total: - instructions: 162950875 + instructions: 370858158 heap_increase: 0 - stable_memory_increase: 0 + stable_memory_increase: 128 scopes: {} cascading_vote_stable_neurons_with_heap_index: total: - instructions: 140555145 + instructions: 348673022 heap_increase: 0 - stable_memory_increase: 0 + stable_memory_increase: 128 scopes: {} centralized_following_all_stable: total: - instructions: 68428123 + instructions: 173332865 heap_increase: 0 - stable_memory_increase: 0 + stable_memory_increase: 128 scopes: {} compute_ballots_for_new_proposal_with_stable_neurons: total: - instructions: 1798483 + instructions: 1811023 heap_increase: 0 stable_memory_increase: 0 scopes: {} draw_maturity_from_neurons_fund_heap: total: - instructions: 7244019 + instructions: 7254719 heap_increase: 0 stable_memory_increase: 0 scopes: {} draw_maturity_from_neurons_fund_stable: total: - instructions: 10264384 + instructions: 10293400 heap_increase: 0 stable_memory_increase: 0 scopes: {} list_neurons_ready_to_unstake_maturity_heap: total: - instructions: 474237 + instructions: 471240 heap_increase: 0 stable_memory_increase: 0 scopes: {} list_neurons_ready_to_unstake_maturity_stable: total: - instructions: 37739237 + instructions: 36814392 heap_increase: 0 stable_memory_increase: 0 scopes: {} list_ready_to_spawn_neuron_ids_heap: total: - instructions: 462350 + instructions: 459353 heap_increase: 0 stable_memory_increase: 0 scopes: {} list_ready_to_spawn_neuron_ids_stable: total: - instructions: 37727938 + instructions: 36803093 heap_increase: 0 stable_memory_increase: 0 scopes: {} neuron_metrics_calculation_heap: total: - instructions: 529394 + instructions: 528806 heap_increase: 0 stable_memory_increase: 0 scopes: {} neuron_metrics_calculation_stable: total: - instructions: 1865928 + instructions: 1837632 heap_increase: 0 stable_memory_increase: 0 scopes: {} range_neurons_performance: total: - instructions: 47978708 + instructions: 47691642 heap_increase: 0 stable_memory_increase: 0 scopes: {} single_vote_all_stable: total: - instructions: 2690730 + instructions: 2474894 heap_increase: 0 - stable_memory_increase: 0 + stable_memory_increase: 128 scopes: {} update_recent_ballots_stable_memory: total: - instructions: 13454675 + instructions: 236883 heap_increase: 0 stable_memory_increase: 0 scopes: {} diff --git a/rs/nns/governance/canister/canister.rs b/rs/nns/governance/canister/canister.rs index 0f95b56cb8f..d459b27bf95 100644 --- a/rs/nns/governance/canister/canister.rs +++ b/rs/nns/governance/canister/canister.rs @@ -168,6 +168,7 @@ fn schedule_timers() { schedule_prune_following(Duration::from_secs(0)); schedule_spawn_neurons(); schedule_unstake_maturity_of_dissolved_neurons(); + schedule_vote_processing(); // TODO(NNS1-3446): Delete. (This only needs to be run once, but can safely be run multiple times). schedule_backfill_voting_power_refreshed_timestamps(Duration::from_secs(0)); @@ -327,6 +328,15 @@ fn schedule_unstake_maturity_of_dissolved_neurons() { }); } +/// The interval at which the voting state machines are processed. +const VOTE_PROCESSING_INTERVAL: Duration = Duration::from_secs(3); + +fn schedule_vote_processing() { + ic_cdk_timers::set_timer_interval(VOTE_PROCESSING_INTERVAL, || { + spawn(governance_mut().process_voting_state_machines()); + }); +} + struct CanisterEnv { rng: Option, time_warp: GovTimeWarp, 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 2d22cff4560..9def884ba5d 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 @@ -3005,3 +3005,13 @@ message ArchivedMonthlyNodeProviderRewards { ic_nns_governance.pb.v1.MonthlyNodeProviderRewards rewards = 1; } } + +// Internal type to allow ProposalVotingStateMachine to be stored +// in stable memory. +message ProposalVotingStateMachine { + ic_nns_common.pb.v1.ProposalId proposal_id = 1; + Topic topic = 2; + repeated ic_nns_common.pb.v1.NeuronId neurons_to_check_followers = 3; + repeated ic_nns_common.pb.v1.NeuronId followers_to_check = 4; + map recent_neuron_ballots_to_record = 5; +} 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 3211fb7a5b6..983afdaa993 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 @@ -4757,6 +4757,29 @@ pub mod archived_monthly_node_provider_rewards { Version1(V1), } } +/// Internal type to allow ProposalVotingStateMachine to be stored +/// in stable memory. +#[derive( + candid::CandidType, + candid::Deserialize, + serde::Serialize, + comparable::Comparable, + Clone, + PartialEq, + ::prost::Message, +)] +pub struct ProposalVotingStateMachine { + #[prost(message, optional, tag = "1")] + pub proposal_id: ::core::option::Option<::ic_nns_common::pb::v1::ProposalId>, + #[prost(enumeration = "Topic", tag = "2")] + pub topic: i32, + #[prost(message, repeated, tag = "3")] + pub neurons_to_check_followers: ::prost::alloc::vec::Vec<::ic_nns_common::pb::v1::NeuronId>, + #[prost(message, repeated, tag = "4")] + pub followers_to_check: ::prost::alloc::vec::Vec<::ic_nns_common::pb::v1::NeuronId>, + #[prost(map = "uint64, enumeration(Vote)", tag = "5")] + pub recent_neuron_ballots_to_record: ::std::collections::HashMap, +} /// Proposal types are organized into topics. Neurons can automatically /// vote based on following other neurons, and these follow /// relationships are defined per topic. diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index a3e4cc5b8e4..d8185870908 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.rs @@ -141,6 +141,7 @@ pub mod tla_macros; #[cfg(feature = "tla")] pub mod tla; +use crate::storage::with_voting_state_machines_mut; #[cfg(feature = "tla")] use std::collections::BTreeSet; #[cfg(feature = "tla")] @@ -1227,6 +1228,11 @@ impl ProposalData { now_seconds < self.get_deadline_timestamp_seconds(voting_period_seconds) } + /// Returns true if the proposal has unprocessed votes in the state machine. + pub fn has_unprocessed_votes(&self) -> bool { + with_voting_state_machines_mut(|vsm| vsm.machine_has_votes_to_process(self.id.unwrap())) + } + pub fn evaluate_wait_for_quiet( &mut self, now_seconds: u64, @@ -1393,7 +1399,13 @@ impl ProposalData { // equivalent to (2 * yes > total) || (2 * no >= total). let majority = (tally.yes > tally.total - tally.yes) || (tally.no >= tally.total - tally.no); - let expired = !self.accepts_vote(now_seconds, voting_period_seconds); + let can_accept_votes = self.accepts_vote(now_seconds, voting_period_seconds); + let votes_still_processing = self.has_unprocessed_votes(); + let polls_open_or_still_counting = can_accept_votes || votes_still_processing; + let expired = !polls_open_or_still_counting; + + // NOTE: expired is not exactly the right concept in the case where votes are still + // processing. let decision_reason = match (majority, expired) { (true, true) => Some("majority and expiration"), (true, false) => Some("majority"), @@ -7020,7 +7032,7 @@ impl Governance { /// can no longer accept votes for the purpose of rewards and that have /// not yet been considered in a reward event. /// * Associate those proposals to the new reward event - fn distribute_rewards(&mut self, supply: Tokens) { + pub(crate) fn distribute_rewards(&mut self, supply: Tokens) { println!("{}distribute_rewards. Supply: {:?}", LOG_PREFIX, supply); let now = self.env.now(); @@ -7094,19 +7106,29 @@ impl Governance { // reward weight of proposals being voted on. let mut actually_distributed_e8s_equivalent = 0_u64; - let proposals = considered_proposals.iter().filter_map(|proposal_id| { - let result = self.heap_data.proposals.get(&proposal_id.id); - if result.is_none() { - println!( - "{}ERROR: Trying to give voting rewards for proposal {}, \ - but it was not found.", - LOG_PREFIX, proposal_id.id, - ); - } - result - }); - let (voters_to_used_voting_right, total_voting_rights) = - sum_weighted_voting_power(proposals); + // We filter out proposals with votes that still need to be propogated to the ballots. + let considered_proposals: Vec = considered_proposals + .into_iter() + .filter( + |proposal_id| match self.heap_data.proposals.get(&proposal_id.id) { + None => { + println!( + "{}ERROR: Trying to give voting rewards for proposal {}, \ + but it was not found.", + LOG_PREFIX, proposal_id.id, + ); + false + } + Some(proposal_data) => !proposal_data.has_unprocessed_votes(), + }, + ) + .collect(); + + let (voters_to_used_voting_right, total_voting_rights) = sum_weighted_voting_power( + considered_proposals + .iter() + .flat_map(|proposal_id| self.heap_data.proposals.get(&proposal_id.id)), + ); // Increment neuron maturities (and actually_distributed_e8s_equivalent). // diff --git a/rs/nns/governance/src/governance/benches.rs b/rs/nns/governance/src/governance/benches.rs index 98d808ad189..54458386752 100644 --- a/rs/nns/governance/src/governance/benches.rs +++ b/rs/nns/governance/src/governance/benches.rs @@ -467,21 +467,6 @@ fn centralized_following_all_stable() -> BenchResult { ) } -/// Benchmark the `cascading_vote` function with stable neurons and a heap index. -/// Before we do the migration of the ballots function to be more efficient: -/// Benchmark: compute_ballots_for_new_proposal_with_stable_neurons (new) -// total: -// instructions: 78.49 M (new) -// heap_increase: 0 pages (new) -// stable_memory_increase: 0 pages (new) -// -// After we migrate to be more efficient: -// Benchmark: compute_ballots_for_new_proposal_with_stable_neurons (new) -// total: -// instructions: 1.50 M (new) -// heap_increase: 0 pages (new) -// stable_memory_increase: 0 pages (new) -// #[bench(raw)] fn compute_ballots_for_new_proposal_with_stable_neurons() -> BenchResult { let now_seconds = 1732817584; diff --git a/rs/nns/governance/src/governance/tests/mod.rs b/rs/nns/governance/src/governance/tests/mod.rs index 0fe044c1d31..0e04e9c710e 100644 --- a/rs/nns/governance/src/governance/tests/mod.rs +++ b/rs/nns/governance/src/governance/tests/mod.rs @@ -844,6 +844,7 @@ mod convert_create_service_nervous_system_proposal_to_sns_init_payload_tests_wit mod metrics_tests { + use ic_nns_common::pb::v1::ProposalId; use maplit::btreemap; use crate::{ @@ -858,6 +859,7 @@ mod metrics_tests { #[test] fn test_metrics_total_voting_power() { let proposal_1 = ProposalData { + id: Some(ProposalId { id: 1 }), proposal: Some(Proposal { title: Some("Foo Foo Bar".to_string()), action: Some(proposal::Action::Motion(Motion { @@ -875,6 +877,7 @@ mod metrics_tests { }; let proposal_2 = ProposalData { + id: Some(ProposalId { id: 2 }), proposal: Some(Proposal { title: Some("Foo Foo Bar".to_string()), action: Some(proposal::Action::ManageNeuron(Box::default())), @@ -923,6 +926,7 @@ mod metrics_tests { }); let open_proposal = ProposalData { + id: Some(ProposalId { id: 1 }), proposal: Some(Proposal { title: Some("open_proposal".to_string()), action: Some(manage_neuron_action.clone()), @@ -932,6 +936,7 @@ mod metrics_tests { }; let rejected_proposal = ProposalData { + id: Some(ProposalId { id: 2 }), proposal: Some(Proposal { title: Some("rejected_proposal".to_string()), action: Some(manage_neuron_action.clone()), @@ -942,6 +947,7 @@ mod metrics_tests { }; let motion_proposal = ProposalData { + id: Some(ProposalId { id: 3 }), proposal: Some(Proposal { title: Some("Foo Foo Bar".to_string()), action: Some(motion_action.clone()), diff --git a/rs/nns/governance/src/neuron_store/benches.rs b/rs/nns/governance/src/neuron_store/benches.rs index 76784117942..dabe4221fe3 100644 --- a/rs/nns/governance/src/neuron_store/benches.rs +++ b/rs/nns/governance/src/neuron_store/benches.rs @@ -124,6 +124,7 @@ fn build_neuron(rng: &mut impl RngCore, location: NeuronLocation, size: NeuronSi vote: Vote::Yes as i32, }) .collect(); + neuron.recent_ballots_next_entry_index = Some(0); neuron } diff --git a/rs/nns/governance/src/storage.rs b/rs/nns/governance/src/storage.rs index 3c8f886d547..fb0c1b03284 100644 --- a/rs/nns/governance/src/storage.rs +++ b/rs/nns/governance/src/storage.rs @@ -1,6 +1,6 @@ use crate::{governance::LOG_PREFIX, pb::v1::AuditEvent}; -use crate::pb::v1::ArchivedMonthlyNodeProviderRewards; +use crate::{pb::v1::ArchivedMonthlyNodeProviderRewards, voting::VotingStateMachines}; use ic_cdk::println; use ic_stable_structures::{ memory_manager::{MemoryId, MemoryManager, VirtualMemory}, @@ -29,6 +29,8 @@ const NEURON_ACCOUNT_ID_INDEX_MEMORY_ID: MemoryId = MemoryId::new(13); const NODE_PROVIDER_REWARDS_LOG_INDEX_MEMORY_ID: MemoryId = MemoryId::new(14); const NODE_PROVIDER_REWARDS_LOG_DATA_MEMORY_ID: MemoryId = MemoryId::new(15); +const VOTING_STATE_MACHINES_MEMORY_ID: MemoryId = MemoryId::new(16); + pub mod neuron_indexes; pub mod neurons; @@ -39,6 +41,15 @@ thread_local! { RefCell::new(MemoryManager::init(DefaultMemoryImpl::default())); static STATE: RefCell = RefCell::new(State::new()); + + // Cannot be part of STATE because it also needs to borrow things in STATE + // when being used. + static VOTING_STATE_MACHINES: RefCell> = RefCell::new({ + MEMORY_MANAGER.with(|memory_manager| { + let memory = memory_manager.borrow().get(VOTING_STATE_MACHINES_MEMORY_ID); + VotingStateMachines::new(memory) + }) + }) } struct State { @@ -185,6 +196,15 @@ pub(crate) fn with_node_provider_rewards_log( }) } +pub(crate) fn with_voting_state_machines_mut( + f: impl FnOnce(&mut VotingStateMachines) -> R, +) -> R { + VOTING_STATE_MACHINES.with(|voting_state_machines| { + let voting_state_machines = &mut voting_state_machines.borrow_mut(); + f(voting_state_machines) + }) +} + /// Validates that some of the data in stable storage can be read, in order to prevent broken /// schema. Should only be called in post_upgrade. pub fn validate_stable_storage() { @@ -219,6 +239,14 @@ where pub fn reset_stable_memory() { MEMORY_MANAGER.with(|mm| *mm.borrow_mut() = MemoryManager::init(DefaultMemoryImpl::default())); STATE.with(|cell| *cell.borrow_mut() = State::new()); + VOTING_STATE_MACHINES.with(|cell| { + *cell.borrow_mut() = { + MEMORY_MANAGER.with(|memory_manager| { + let memory = memory_manager.borrow().get(VOTING_STATE_MACHINES_MEMORY_ID); + VotingStateMachines::new(memory) + }) + } + }); } pub fn grow_upgrades_memory_to(target_pages: u64) { diff --git a/rs/nns/governance/src/test_utils.rs b/rs/nns/governance/src/test_utils.rs index 6bed69d16f8..7764f856b3a 100644 --- a/rs/nns/governance/src/test_utils.rs +++ b/rs/nns/governance/src/test_utils.rs @@ -162,6 +162,14 @@ impl MockEnvironment { now: Arc::new(Mutex::new(now)), } } + + pub fn now_setter(&self) -> impl Fn(u64) { + let arc = self.now.clone(); + move |new_now| { + let mut now = arc.lock().unwrap(); + *now = new_now; + } + } } impl Default for MockEnvironment { diff --git a/rs/nns/governance/src/voting.rs b/rs/nns/governance/src/voting.rs index d0f54818e5b..7b2ac38d4d4 100644 --- a/rs/nns/governance/src/voting.rs +++ b/rs/nns/governance/src/voting.rs @@ -1,17 +1,89 @@ use crate::{ - governance::Governance, + governance::{Governance, LOG_PREFIX}, neuron_store::NeuronStore, - pb::v1::{Ballot, Topic, Topic::NeuronManagement, Vote}, + pb::v1::{Ballot, ProposalData, Topic, Topic::NeuronManagement, Vote}, + storage::with_voting_state_machines_mut, }; +#[cfg(not(test))] +use ic_nervous_system_long_message::is_message_over_threshold; +#[cfg(test)] +use ic_nervous_system_temporary::Temporary; use ic_nns_common::pb::v1::{NeuronId, ProposalId}; +use ic_stable_structures::{storable::Bound, StableBTreeMap, Storable}; +use prost::Message; use std::{ - cell::RefCell, + borrow::Cow, collections::{BTreeMap, BTreeSet, HashMap}, }; +const BILLION: u64 = 1_000_000_000; + +/// The hard limit for the number of instructions that can be executed in a single call context. +/// This leaves room for 750 thousand neurons with complex following. +const HARD_VOTING_INSTRUCTIONS_LIMIT: u64 = 750 * BILLION; +// For production, we want this higher so that we can process more votes, but without affecting +// the overall responsiveness of the canister. 1 Billion seems like a reasonable compromise. +const SOFT_VOTING_INSTRUCTIONS_LIMIT: u64 = if cfg!(feature = "test") { + 1_000_000 +} else { + BILLION +}; + +#[cfg(not(test))] +fn over_soft_message_limit() -> bool { + is_message_over_threshold(SOFT_VOTING_INSTRUCTIONS_LIMIT) +} + +// The following test methods let us test this internally +#[cfg(test)] thread_local! { - static VOTING_STATE_MACHINES: RefCell = RefCell::new(VotingStateMachines::new()); + static OVER_SOFT_MESSAGE_LIMIT: std::cell::Cell = const { std::cell::Cell::new(false) } +} + +#[cfg(test)] +fn temporarily_set_over_soft_message_limit(over: bool) -> Temporary { + Temporary::new(&OVER_SOFT_MESSAGE_LIMIT, over) } + +#[cfg(test)] +fn over_soft_message_limit() -> bool { + OVER_SOFT_MESSAGE_LIMIT.with(|over| over.get()) +} + +async fn noop_self_call_if_over_instructions( + message_threshold: u64, + panic_threshold: Option, +) -> Result<(), String> { + // canbench doesn't currently support query calls inside of benchmarks + // We send a no-op message to self to break up the call context into more messages + if cfg!(not(feature = "canbench-rs")) { + ic_nervous_system_long_message::noop_self_call_if_over_instructions( + message_threshold, + panic_threshold, + ) + .await + .map_err(|e| e.to_string()) + } else { + Ok(()) + } +} + +fn proposal_ballots( + proposals: &mut BTreeMap, + proposal_id: u64, +) -> Result<&mut HashMap, String> { + match proposals.get_mut(&proposal_id) { + Some(proposal) => Ok(&mut proposal.ballots), + None => { + eprintln!( + "{} Proposal {} not found, cannot operate on ballots", + LOG_PREFIX, proposal_id + ); + Err(format!("Proposal {} not found.", proposal_id)) + } + } +} + impl Governance { pub async fn cast_vote_and_cascade_follow( &mut self, @@ -22,37 +94,141 @@ impl Governance { ) { let voting_started = self.env.now(); - let neuron_store = &mut self.neuron_store; - let ballots = match self.heap_data.proposals.get_mut(&proposal_id.id) { - Some(proposal) => &mut proposal.ballots, - None => { - // This is a critical error, but there is nothing that can be done about it - // at this place. We somehow have a vote for a proposal that doesn't exist. - eprintln!("error in cast_vote_and_cascade_follow when gathering induction votes: Proposal not found"); - return; + if !self.heap_data.proposals.contains_key(&proposal_id.id) { + // This is a critical error, but there is nothing that can be done about it + // at this place. We somehow have a vote for a proposal that doesn't exist. + eprintln!( + "error in cast_vote_and_cascade_follow: Proposal not found: {}", + proposal_id.id + ); + return; + } + + // First we cast the ballot. + self.record_neuron_vote(proposal_id, voting_neuron_id, vote_of_neuron, topic); + + // We process until voting is finished, and then do any other work that fits into the soft + // limit of the current message. Votes are guaranteed to be recorded before the function + // returns, but recent_ballots for neurons might be recorded later in a timer job. This + // ensures we return to the caller in a reasonable amount of time. + let mut is_voting_finished = false; + + while !is_voting_finished { + // Now we process until we are done or we are over a limit and need to + // make a self-call. + with_voting_state_machines_mut(|voting_state_machines| { + voting_state_machines.with_machine(proposal_id, topic, |machine| { + self.process_machine_until_soft_limit(machine, over_soft_message_limit); + is_voting_finished = machine.is_voting_finished(); + }); + }); + + // This returns an error if we hit the hard limit, which should basically never happen + // in production, but we need a way out of this loop in the worst case to prevent + // the canister from being unable to upgrade. + if let Err(e) = noop_self_call_if_over_instructions( + SOFT_VOTING_INSTRUCTIONS_LIMIT, + Some(HARD_VOTING_INSTRUCTIONS_LIMIT), + ) + .await + { + println!( + "Error in cast_vote_and_cascade_follow, \ + voting will be processed in timers: {}", + e + ); + break; } - }; + } + // We use the time from the beginning of the function to retain the behaviors needed + // for wait for quiet even when votes can be processed asynchronously. + self.recompute_proposal_tally(proposal_id, voting_started); + } - // Use of thread local storage to store the state machines prevents - // more than one state machine per proposal, which limits the overall - // memory usage for voting, which will be relevant when this can be used - // across multiple messages, which would cause the memory usage to accumulate. - VOTING_STATE_MACHINES.with(|vsm| { - let mut voting_state_machines = vsm.borrow_mut(); - let proposal_voting_machine = - voting_state_machines.get_or_create_machine(proposal_id, topic); + /// Record a neuron vote into the voting state machine, then do nothing else. + fn record_neuron_vote( + &mut self, + proposal_id: ProposalId, + voting_neuron_id: NeuronId, + vote: Vote, + topic: Topic, + ) { + with_voting_state_machines_mut(|voting_state_machines| { + if let Ok(ballots) = proposal_ballots(&mut self.heap_data.proposals, proposal_id.id) { + voting_state_machines.with_machine(proposal_id, topic, |machine| { + machine.cast_vote(ballots, voting_neuron_id, vote) + }); + } + }); + } - proposal_voting_machine.cast_vote(ballots, voting_neuron_id, vote_of_neuron); + /// Process a single voting state machine until it is over the soft limit or finished, then return + fn process_machine_until_soft_limit( + &mut self, + machine: &mut ProposalVotingStateMachine, + is_over_soft_limit: fn() -> bool, + ) { + let proposal_id = machine.proposal_id; + while !machine.is_completely_finished() { + if let Ok(ballots) = proposal_ballots(&mut self.heap_data.proposals, proposal_id.id) { + machine.continue_processing(&mut self.neuron_store, ballots, is_over_soft_limit); + } else { + eprintln!( + "{} Proposal {} for voting machine not found. Machine cannot complete work.", + LOG_PREFIX, proposal_id.id + ); + break; + } - while !proposal_voting_machine.is_done() { - proposal_voting_machine.continue_processing(neuron_store, ballots); + if is_over_soft_limit() { + break; } + } + } - voting_state_machines.remove_if_done(&proposal_id); + /// Process all voting state machines. This function is called in the timer job. + /// It processes voting state machines until the soft limit is reached or there is no work to do. + pub async fn process_voting_state_machines(&mut self) { + let mut proposals_with_new_votes_cast = vec![]; + with_voting_state_machines_mut(|voting_state_machines| loop { + if voting_state_machines + .with_next_machine(|(proposal_id, machine)| { + if !machine.is_voting_finished() { + proposals_with_new_votes_cast.push(proposal_id); + } + // We need to keep track of which proposals we processed + self.process_machine_until_soft_limit(machine, over_soft_message_limit); + }) + .is_none() + { + break; + }; + + if over_soft_message_limit() { + break; + } }); - // We use the time from the beginning of the function to retain the behaviors needed - // for wait for quiet even when votes can be processed asynchronously. - self.recompute_proposal_tally(proposal_id, voting_started); + + // Most of the time, we are not going to see new votes cast in this function, but this + // is here to make sure the normal proposal processing still applies + for proposal_id in proposals_with_new_votes_cast { + self.recompute_proposal_tally(proposal_id, self.env.now()); + self.process_proposal(proposal_id.id); + + if let Err(e) = noop_self_call_if_over_instructions( + SOFT_VOTING_INSTRUCTIONS_LIMIT, + Some(HARD_VOTING_INSTRUCTIONS_LIMIT), + ) + .await + { + println!( + "Used too many instructions in process_voting_state_machines, \ + exiting before finishing: {}", + e + ); + break; + } + } } /// Recompute the tally for a proposal, using the time provided as the current time. @@ -76,41 +252,88 @@ impl Governance { } } -struct VotingStateMachines { +pub(crate) struct VotingStateMachines +where + Memory: ic_stable_structures::Memory, +{ // Up to one machine per proposal, to avoid having to do unnecessary checks for followers that // might follow. This allows the state machines to be used across multiple messages // without duplicating state and memory usage. - machines: BTreeMap, + machines: StableBTreeMap, } -impl VotingStateMachines { - fn new() -> Self { +impl VotingStateMachines { + pub(crate) fn new(memory: Memory) -> Self { Self { - machines: BTreeMap::new(), + machines: StableBTreeMap::init(memory), } } - fn get_or_create_machine( + /// Optionally executes callback on the next machine, if one exists. Otherwise, this does + /// nothing. + fn with_next_machine( + &mut self, + mut callback: impl FnMut((ProposalId, &mut ProposalVotingStateMachine)) -> R, + ) -> Option { + if let Some((proposal_id, proto)) = self.machines.pop_first() { + let mut machine = ProposalVotingStateMachine::try_from(proto).unwrap(); + let result = callback((proposal_id, &mut machine)); + if !machine.is_completely_finished() { + self.machines.insert( + proposal_id, + crate::pb::v1::ProposalVotingStateMachine::from(machine), + ); + } + Some(result) + } else { + None + } + } + + /// Perform a callback with a given voting machine. If the machine is finished, it is removed + /// after the callback. + pub(crate) fn with_machine( &mut self, proposal_id: ProposalId, topic: Topic, - ) -> &mut ProposalVotingStateMachine { - self.machines - .entry(proposal_id) - .or_insert_with(|| ProposalVotingStateMachine::try_new(proposal_id, topic).unwrap()) + callback: impl FnOnce(&mut ProposalVotingStateMachine) -> R, + ) -> R { + // We use remove here because we delete machines if they're done. + // This reduces stable memory calls in the case where the machine is completed, + // as we do not need to get it and then remove it later. + let mut machine = self + .machines + .remove(&proposal_id) + // This unwrap should be safe because we only write valid machines below. + .map(|proto| ProposalVotingStateMachine::try_from(proto).unwrap()) + .unwrap_or(ProposalVotingStateMachine::new(proposal_id, topic)); + + let result = callback(&mut machine); + + // Save the machine again if it's not finished. + if !machine.is_completely_finished() { + self.machines.insert( + proposal_id, + crate::pb::v1::ProposalVotingStateMachine::from(machine), + ); + } + result } - fn remove_if_done(&mut self, proposal_id: &ProposalId) { - if let Some(machine) = self.machines.get(proposal_id) { - if machine.is_done() { - self.machines.remove(proposal_id); - } + /// Returns true if the machine has votes to process. + pub(crate) fn machine_has_votes_to_process(&self, proposal_id: ProposalId) -> bool { + if let Some(proto) = self.machines.get(&proposal_id) { + !ProposalVotingStateMachine::try_from(proto) + .unwrap() + .is_voting_finished() + } else { + false } } } -#[derive(Debug, PartialEq, Default)] -struct ProposalVotingStateMachine { +#[derive(Clone, Debug, PartialEq, Default)] +pub(crate) struct ProposalVotingStateMachine { // The proposal ID that is being voted on. proposal_id: ProposalId, // The topic of the proposal. @@ -123,25 +346,83 @@ struct ProposalVotingStateMachine { recent_neuron_ballots_to_record: BTreeMap, } -impl ProposalVotingStateMachine { - fn try_new(proposal_id: ProposalId, topic: Topic) -> Result { - if topic == Topic::Unspecified { - return Err("Topic must be specified".to_string()); +impl From for crate::pb::v1::ProposalVotingStateMachine { + fn from(value: ProposalVotingStateMachine) -> Self { + Self { + proposal_id: Some(value.proposal_id), + topic: value.topic as i32, + neurons_to_check_followers: value.neurons_to_check_followers.into_iter().collect(), + followers_to_check: value.followers_to_check.into_iter().collect(), + recent_neuron_ballots_to_record: value + .recent_neuron_ballots_to_record + .into_iter() + .map(|(n, v)| (n.id, v as i32)) + .collect(), } + } +} + +impl TryFrom for ProposalVotingStateMachine { + type Error = String; + fn try_from(value: crate::pb::v1::ProposalVotingStateMachine) -> Result { Ok(Self { + proposal_id: value.proposal_id.ok_or("Proposal ID must be specified")?, + topic: Topic::try_from(value.topic).map_err(|e| e.to_string())?, + neurons_to_check_followers: value.neurons_to_check_followers.into_iter().collect(), + followers_to_check: value.followers_to_check.into_iter().collect(), + recent_neuron_ballots_to_record: value + .recent_neuron_ballots_to_record + .into_iter() + .map(|(n, v)| { + let neuron_id = NeuronId::from_u64(n); + let vote = Vote::try_from(v).map_err(|e| e.to_string())?; // Propagate the error directly + Ok((neuron_id, vote)) + }) + .collect::>()?, + }) + } +} + +impl Storable for crate::pb::v1::ProposalVotingStateMachine { + fn to_bytes(&self) -> Cow<'_, [u8]> { + Cow::from(self.encode_to_vec()) + } + + fn from_bytes(bytes: Cow<'_, [u8]>) -> Self { + Self::decode(&bytes[..]) + // Convert from Result to Self. (Unfortunately, it seems that + // panic is unavoidable in the case of Err.) + .expect("Unable to deserialize ProposalVotingStateMachine.") + } + + const BOUND: Bound = Bound::Unbounded; +} + +impl ProposalVotingStateMachine { + fn new(proposal_id: ProposalId, topic: Topic) -> Self { + Self { proposal_id, topic, - ..Default::default() - }) + neurons_to_check_followers: Default::default(), + followers_to_check: Default::default(), + recent_neuron_ballots_to_record: Default::default(), + } } - fn is_done(&self) -> bool { + /// Returns true if this machine has no more work to do. + fn is_completely_finished(&self) -> bool { self.neurons_to_check_followers.is_empty() && self.followers_to_check.is_empty() && self.recent_neuron_ballots_to_record.is_empty() } + /// Returns true if all following is processed and all votes are cast on the proposal. + /// Recent neuron ballots could still be left, however. + pub(crate) fn is_voting_finished(&self) -> bool { + self.neurons_to_check_followers.is_empty() && self.followers_to_check.is_empty() + } + fn add_followers_to_check( &mut self, neuron_store: &NeuronStore, @@ -190,47 +471,66 @@ impl ProposalVotingStateMachine { &mut self, neuron_store: &mut NeuronStore, ballots: &mut HashMap, + is_over_instructions_limit: fn() -> bool, ) { - while let Some(neuron_id) = self.neurons_to_check_followers.pop_first() { - self.add_followers_to_check(neuron_store, neuron_id, self.topic); - } + let voting_finished = self.is_voting_finished(); - // Memory optimization, will not cause tests to fail if removed - retain_neurons_with_castable_ballots(&mut self.followers_to_check, ballots); + if !voting_finished { + while let Some(neuron_id) = self.neurons_to_check_followers.pop_first() { + self.add_followers_to_check(neuron_store, neuron_id, self.topic); - while let Some(follower) = self.followers_to_check.pop_first() { - let vote = match neuron_store.neuron_would_follow_ballots(follower, self.topic, ballots) - { - Ok(vote) => vote, - Err(e) => { - // This is a bad inconsistency, but there is - // nothing that can be done about it at this - // place. We somehow have followers recorded that don't exist. - eprintln!("error in cast_vote_and_cascade_follow when gathering induction votes: {:?}", e); - Vote::Unspecified + // Before we check the next one, see if we're over the limit. + if is_over_instructions_limit() { + return; } - }; - // Casting vote immediately might affect other follower votes, which makes - // voting resolution take fewer iterations. - // Vote::Unspecified is ignored by cast_vote. - self.cast_vote(ballots, follower, vote); - } - - while let Some((neuron_id, vote)) = self.recent_neuron_ballots_to_record.pop_first() { - 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 - // nothing that can be done about it at this - // place. We somehow have followers recorded that don't exist. - eprintln!("error in cast_vote_and_cascade_follow when gathering induction votes: {:?}", e); + } + + // Memory optimization, will not cause tests to fail if removed + retain_neurons_with_castable_ballots(&mut self.followers_to_check, ballots); + + while let Some(follower) = self.followers_to_check.pop_first() { + let vote = match neuron_store + .neuron_would_follow_ballots(follower, self.topic, ballots) + { + Ok(vote) => vote, + Err(e) => { + // This is a bad inconsistency, but there is + // nothing that can be done about it at this + // place. We somehow have followers recorded that don't exist. + eprintln!("error in cast_vote_and_cascade_follow when gathering induction votes: {:?}", e); + Vote::Unspecified + } + }; + // Casting vote immediately might affect other follower votes, which makes + // voting resolution take fewer iterations. + // Vote::Unspecified is ignored by cast_vote. + self.cast_vote(ballots, follower, vote); + + if is_over_instructions_limit() { + return; } - }; + } + } else { + while let Some((neuron_id, vote)) = self.recent_neuron_ballots_to_record.pop_first() { + 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 + // nothing that can be done about it at this + // place. We somehow have followers recorded that don't exist. + eprintln!("error in cast_vote_and_cascade_follow when gathering induction votes: {:?}", e); + } + }; + + if is_over_instructions_limit() { + return; + } + } } } } @@ -254,18 +554,33 @@ fn retain_neurons_with_castable_ballots( #[cfg(test)] mod test { use crate::{ - governance::{Governance, MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS}, + governance::{ + Governance, MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS, + REWARD_DISTRIBUTION_PERIOD_SECONDS, + }, neuron::{DissolveStateAndAge, Neuron, NeuronBuilder}, neuron_store::NeuronStore, pb::v1::{ - neuron::Followees, Ballot, ProposalData, Tally, Topic, Vote, VotingPowerEconomics, + neuron::Followees, proposal::Action, Ballot, Governance as GovernanceProto, Motion, + Proposal, ProposalData, Tally, Topic, Vote, VotingPowerEconomics, WaitForQuietState, + }, + storage::with_voting_state_machines_mut, + test_utils::{ + ExpectedCallCanisterMethodCallArguments, MockEnvironment, StubCMC, StubIcpLedger, + }, + voting::{ + temporarily_set_over_soft_message_limit, ProposalVotingStateMachine, + VotingStateMachines, }, - test_utils::{MockEnvironment, StubCMC, StubIcpLedger}, - voting::ProposalVotingStateMachine, }; + use candid::Encode; use futures::FutureExt; use ic_base_types::PrincipalId; + use ic_ledger_core::Tokens; + use ic_nervous_system_long_message::in_test_temporarily_set_call_context_over_threshold; use ic_nns_common::pb::v1::{NeuronId, ProposalId}; + use ic_nns_constants::GOVERNANCE_CANISTER_ID; + use ic_stable_structures::DefaultMemoryImpl; use icp_ledger::Subaccount; use maplit::{btreemap, hashmap}; use std::collections::{BTreeMap, BTreeSet, HashMap}; @@ -533,15 +848,13 @@ mod test { } fn add_neuron_with_ballot( - neuron_store: &mut NeuronStore, + neurons: &mut BTreeMap, ballots: &mut HashMap, neuron: Neuron, ) { let cached_stake = neuron.cached_neuron_stake_e8s; let id = neuron.id().id; - neuron_store - .add_neuron(neuron) - .expect("Couldn't add neuron"); + neurons.insert(id, neuron); ballots.insert( id, Ballot { @@ -552,15 +865,7 @@ mod test { } #[test] - fn test_invalid_topic() { - let err = ProposalVotingStateMachine::try_new(ProposalId { id: 0 }, Topic::Unspecified) - .unwrap_err(); - - assert_eq!(err, "Topic must be specified"); - } - - #[test] - fn test_is_done() { + fn test_is_completely_finished() { let mut state_machine = ProposalVotingStateMachine { proposal_id: ProposalId { id: 0 }, topic: Topic::Governance, @@ -569,41 +874,36 @@ mod test { recent_neuron_ballots_to_record: BTreeMap::new(), }; - assert!(state_machine.is_done()); + assert!(state_machine.is_completely_finished()); state_machine .neurons_to_check_followers .insert(NeuronId { id: 0 }); - assert!(!state_machine.is_done()); + assert!(!state_machine.is_completely_finished()); state_machine.neurons_to_check_followers.clear(); state_machine.followers_to_check.insert(NeuronId { id: 0 }); - assert!(!state_machine.is_done()); + assert!(!state_machine.is_completely_finished()); state_machine.followers_to_check.clear(); state_machine .recent_neuron_ballots_to_record .insert(NeuronId { id: 0 }, Vote::Yes); - assert!(!state_machine.is_done()); + assert!(!state_machine.is_completely_finished()); state_machine.recent_neuron_ballots_to_record.clear(); } #[test] fn test_continue_processsing() { let mut state_machine = - ProposalVotingStateMachine::try_new(ProposalId { id: 0 }, Topic::NetworkEconomics) - .unwrap(); + ProposalVotingStateMachine::new(ProposalId { id: 0 }, Topic::NetworkEconomics); let mut ballots = HashMap::new(); - let mut neuron_store = NeuronStore::new(btreemap! {}); + let mut neurons = BTreeMap::new(); + add_neuron_with_ballot(&mut neurons, &mut ballots, make_neuron(1, 101, hashmap! {})); add_neuron_with_ballot( - &mut neuron_store, - &mut ballots, - make_neuron(1, 101, hashmap! {}), - ); - add_neuron_with_ballot( - &mut neuron_store, + &mut neurons, &mut ballots, make_neuron( 2, @@ -613,9 +913,10 @@ mod test { }}, ), ); + let mut neuron_store = NeuronStore::new(neurons); state_machine.cast_vote(&mut ballots, NeuronId { id: 1 }, Vote::Yes); - state_machine.continue_processing(&mut neuron_store, &mut ballots); + state_machine.continue_processing(&mut neuron_store, &mut ballots, || false); assert_eq!( ballots, @@ -623,26 +924,35 @@ mod test { 1 => Ballot { vote: Vote::Yes as i32, voting_power: 101 }, 2 => Ballot { vote: Vote::Yes as i32, voting_power: 102 }} ); + + // First, we see not finished at all + assert!(!state_machine.is_completely_finished()); + assert!(!state_machine.is_voting_finished()); + + // Now we see voting finished but not recording recent ballots finished + state_machine.continue_processing(&mut neuron_store, &mut ballots, || false); + assert!(!state_machine.is_completely_finished()); + assert!(state_machine.is_voting_finished()); + assert_eq!( neuron_store .with_neuron(&NeuronId { id: 1 }, |n| { - n.recent_ballots.first().unwrap().vote + n.recent_ballots.first().cloned() }) .unwrap(), - Vote::Yes as i32 + None ); assert_eq!( neuron_store .with_neuron(&NeuronId { id: 2 }, |n| { - n.recent_ballots.first().unwrap().vote + n.recent_ballots.first().cloned() }) .unwrap(), - Vote::Yes as i32 + None ); - assert!(!state_machine.is_done()); - - state_machine.continue_processing(&mut neuron_store, &mut ballots); + state_machine.continue_processing(&mut neuron_store, &mut ballots, || false); + assert!(state_machine.is_completely_finished()); assert_eq!( ballots, @@ -666,20 +976,44 @@ mod test { .unwrap(), Vote::Yes as i32 ); - assert!(state_machine.is_done()); + } + + #[test] + fn test_machine_has_votes_to_process() { + let mut voting_state_machines: VotingStateMachines = + VotingStateMachines::new(Default::default()); + let proposal_id = ProposalId { id: 0 }; + let topic = Topic::NetworkEconomics; + let mut state_machine = ProposalVotingStateMachine::new(proposal_id, topic); + + assert!(!voting_state_machines.machine_has_votes_to_process(proposal_id)); + + voting_state_machines.machines.insert( + proposal_id, + crate::pb::v1::ProposalVotingStateMachine::from(state_machine.clone()), + ); + assert!(!voting_state_machines.machine_has_votes_to_process(proposal_id)); + + state_machine + .neurons_to_check_followers + .insert(NeuronId { id: 0 }); + voting_state_machines.machines.insert( + proposal_id, + crate::pb::v1::ProposalVotingStateMachine::from(state_machine), + ); + assert!(voting_state_machines.machine_has_votes_to_process(proposal_id)); } #[test] fn test_cyclic_following_will_terminate() { let mut state_machine = - ProposalVotingStateMachine::try_new(ProposalId { id: 0 }, Topic::NetworkEconomics) - .unwrap(); + ProposalVotingStateMachine::new(ProposalId { id: 0 }, Topic::NetworkEconomics); let mut ballots = HashMap::new(); - let mut neuron_store = NeuronStore::new(btreemap! {}); + let mut neurons = BTreeMap::new(); add_neuron_with_ballot( - &mut neuron_store, + &mut neurons, &mut ballots, make_neuron( 1, @@ -690,7 +1024,7 @@ mod test { ), ); add_neuron_with_ballot( - &mut neuron_store, + &mut neurons, &mut ballots, make_neuron( 2, @@ -701,15 +1035,528 @@ mod test { ), ); + let mut neuron_store = NeuronStore::new(neurons); + // We assert it is immediately done after casting an unspecified vote b/c there // is no work to do. state_machine.cast_vote(&mut ballots, NeuronId { id: 1 }, Vote::Unspecified); - assert!(state_machine.is_done()); + assert!(state_machine.is_completely_finished()); // We assert it is done after checking both sets of followers state_machine.cast_vote(&mut ballots, NeuronId { id: 1 }, Vote::Yes); - state_machine.continue_processing(&mut neuron_store, &mut ballots); - state_machine.continue_processing(&mut neuron_store, &mut ballots); - assert!(state_machine.is_done()); + state_machine.continue_processing(&mut neuron_store, &mut ballots, || false); + state_machine.continue_processing(&mut neuron_store, &mut ballots, || false); + state_machine.continue_processing(&mut neuron_store, &mut ballots, || false); + assert!(state_machine.is_completely_finished()); + } + + #[test] + fn test_cast_vote_and_cascade_follow_always_finishes_processing_ballots() { + let _a = temporarily_set_over_soft_message_limit(true); + let topic = Topic::NetworkEconomics; + let mut neurons = BTreeMap::new(); + let mut ballots = HashMap::new(); + for i in 1..=100 { + let mut followees = HashMap::new(); + if i != 1 { + // cascading followees + followees.insert( + topic as i32, + Followees { + followees: vec![NeuronId { id: i - 1 }], + }, + ); + } + add_neuron_with_ballot(&mut neurons, &mut ballots, make_neuron(i, 100, followees)); + } + + let governance_proto = GovernanceProto { + proposals: btreemap! { + 1 => ProposalData { + id: Some(ProposalId {id: 1}), + ballots, + ..Default::default() + } + }, + neurons: neurons + .into_iter() + .map(|(id, n)| (id, n.into_proto(&VotingPowerEconomics::DEFAULT, u64::MAX))) + .collect(), + ..Default::default() + }; + let mut governance = Governance::new( + governance_proto, + Box::new(MockEnvironment::new(Default::default(), 0)), + Box::new(StubIcpLedger {}), + Box::new(StubCMC {}), + ); + + // In our test configuration, we always return "true" for is_over_instructions_limit() + // So our logic is at least resilient to not having enough instructions, and is able + // to continue working. Without this, it could be possible to choose wrong settings + // that make it impossible to advance. + governance + .cast_vote_and_cascade_follow( + ProposalId { id: 1 }, + NeuronId { id: 1 }, + Vote::Yes, + topic, + ) + .now_or_never() + .unwrap(); + + with_voting_state_machines_mut(|voting_state_machines| { + // We are asserting here that the machine is cleaned up after it is done. + assert!( + !voting_state_machines.machines.is_empty(), + "Voting StateMachines? {:?}", + voting_state_machines.machines.first_key_value() + ); + + voting_state_machines.with_machine(ProposalId { id: 1 }, topic, |machine| { + assert!(!machine.is_completely_finished()); + assert!(machine.is_voting_finished()); + }); + }); + + let ballots = &governance.heap_data.proposals.get(&1).unwrap().ballots; + assert_eq!(ballots.len(), 100); + for (_, ballot) in ballots.iter() { + assert_eq!(ballot.vote, Vote::Yes as i32); + } + } + + #[test] + fn test_cast_vote_and_cascade_breaks_if_over_hard_limit() { + let topic = Topic::NetworkEconomics; + let mut neurons = BTreeMap::new(); + let mut ballots = HashMap::new(); + for i in 1..=10 { + let mut followees = HashMap::new(); + if i != 1 { + // cascading followees + followees.insert( + topic as i32, + Followees { + followees: vec![NeuronId { id: i - 1 }], + }, + ); + } + add_neuron_with_ballot(&mut neurons, &mut ballots, make_neuron(i, 100, followees)); + } + + let governance_proto = GovernanceProto { + proposals: btreemap! { + 1 => ProposalData { + id: Some(ProposalId {id: 1}), + ballots, + ..Default::default() + } + }, + neurons: neurons + .into_iter() + .map(|(id, n)| (id, n.into_proto(&VotingPowerEconomics::DEFAULT, u64::MAX))) + .collect(), + ..Default::default() + }; + let mut governance = Governance::new( + governance_proto, + Box::new(MockEnvironment::new(Default::default(), 0)), + Box::new(StubIcpLedger {}), + Box::new(StubCMC {}), + ); + + let _e = temporarily_set_over_soft_message_limit(true); + let _f = in_test_temporarily_set_call_context_over_threshold(); + governance + .cast_vote_and_cascade_follow( + ProposalId { id: 1 }, + NeuronId { id: 1 }, + Vote::Yes, + topic, + ) + .now_or_never() + .unwrap(); + + with_voting_state_machines_mut(|voting_state_machines| { + assert!(!voting_state_machines.machines.is_empty()); + assert!(voting_state_machines.machine_has_votes_to_process(ProposalId { id: 1 })); + }); + } + + #[test] + fn test_cast_vote_and_cascade_follow_doesnt_record_recent_ballots_after_first_soft_limit() { + let _a = temporarily_set_over_soft_message_limit(true); + let topic = Topic::NetworkEconomics; + let mut neurons = BTreeMap::new(); + let mut ballots = HashMap::new(); + + for i in 1..=9 { + let mut followees = HashMap::new(); + if i != 1 { + // cascading followees + followees.insert( + topic as i32, + Followees { + followees: vec![NeuronId { id: i - 1 }], + }, + ); + } + add_neuron_with_ballot(&mut neurons, &mut ballots, make_neuron(i, 100, followees)); + } + + let governance_proto = GovernanceProto { + proposals: btreemap! { + 1 => ProposalData { + id: Some(ProposalId {id: 1}), + ballots, + ..Default::default() + } + }, + neurons: neurons + .into_iter() + .map(|(id, n)| (id, n.into_proto(&VotingPowerEconomics::DEFAULT, u64::MAX))) + .collect(), + ..Default::default() + }; + let mut governance = Governance::new( + governance_proto, + Box::new(MockEnvironment::new(Default::default(), 0)), + Box::new(StubIcpLedger {}), + Box::new(StubCMC {}), + ); + + // In test mode, we are always saying we're over the soft-message limit, so we know that + // this will hit that limit and not record any recent ballots. + governance + .cast_vote_and_cascade_follow( + ProposalId { id: 1 }, + NeuronId { id: 1 }, + Vote::Yes, + topic, + ) + .now_or_never() + .unwrap(); + + with_voting_state_machines_mut(|voting_state_machines| { + assert!(!voting_state_machines.machines.is_empty(),); + }); + + let ballots = &governance.heap_data.proposals.get(&1).unwrap().ballots; + assert_eq!(ballots.len(), 9); + for (_, ballot) in ballots.iter() { + assert_eq!(ballot.vote, Vote::Yes as i32); + } + + for i in 1..=9 { + let recent_ballots = governance + .neuron_store + .with_neuron(&NeuronId { id: i }, |n| n.recent_ballots.clone()) + .unwrap(); + assert_eq!(recent_ballots.len(), 0, "Neuron {} has recent ballots", i); + } + + // Now let's run the "timer job" to make sure it eventually drains everything. + for _ in 1..20 { + governance + .process_voting_state_machines() + .now_or_never() + .unwrap(); + } + + with_voting_state_machines_mut(|voting_state_machines| { + // We are asserting here that the machine is cleaned up after it is done. + assert!( + voting_state_machines.machines.is_empty(), + "Voting StateMachines? {:?}", + voting_state_machines.machines.first_key_value() + ); + }); + + for i in 1..=9 { + let recent_ballots = governance + .neuron_store + .with_neuron(&NeuronId { id: i }, |n| n.recent_ballots.clone()) + .unwrap(); + assert_eq!(recent_ballots.len(), 1, "Neuron {} has recent ballots", i); + } + } + + #[test] + fn test_process_voting_state_machines_processes_votes_and_executes_proposals() { + let topic = Topic::Governance; + let mut neurons = BTreeMap::new(); + let mut ballots = HashMap::new(); + + for i in 1..=9 { + let mut followees = HashMap::new(); + if i != 1 { + // cascading followees + followees.insert( + topic as i32, + Followees { + followees: vec![NeuronId { id: i - 1 }], + }, + ); + } + add_neuron_with_ballot(&mut neurons, &mut ballots, make_neuron(i, 100, followees)); + } + + let motion = Motion { + motion_text: "".to_string(), + }; + let action = Action::Motion(motion); + let proposal = Proposal { + title: Some("".to_string()), + summary: "".to_string(), + url: "".to_string(), + action: Some(action), + }; + let governance_proto = GovernanceProto { + proposals: btreemap! { + 1 => ProposalData { + id: Some(ProposalId {id: 1}), + ballots, + proposal: Some(proposal), + ..Default::default() + } + }, + neurons: neurons + .into_iter() + .map(|(id, n)| (id, n.into_proto(&VotingPowerEconomics::DEFAULT, u64::MAX))) + .collect(), + ..Default::default() + }; + let mut governance = Governance::new( + governance_proto, + Box::new(MockEnvironment::new(Default::default(), 1234)), + Box::new(StubIcpLedger {}), + Box::new(StubCMC {}), + ); + + governance.record_neuron_vote(ProposalId { id: 1 }, NeuronId { id: 1 }, Vote::Yes, topic); + + with_voting_state_machines_mut(|voting_state_machines| { + assert!(!voting_state_machines.machines.is_empty(),); + voting_state_machines.with_machine(ProposalId { id: 1 }, topic, |machine| { + assert!(!machine.is_voting_finished()); + }); + }); + + // In test mode, we are always saying we're over the soft-message limit, so we know that + // this will hit that limit and not record any recent ballots. + governance + .process_voting_state_machines() + .now_or_never() + .unwrap(); + + with_voting_state_machines_mut(|voting_state_machines| { + assert!(voting_state_machines.machines.is_empty(),); + }); + + let ballots = &governance.heap_data.proposals.get(&1).unwrap().ballots; + assert_eq!(ballots.len(), 9); + for (_, ballot) in ballots.iter() { + assert_eq!(ballot.vote, Vote::Yes as i32); + } + + for i in 1..=9 { + let recent_ballots = governance + .neuron_store + .with_neuron(&NeuronId { id: i }, |n| n.recent_ballots.clone()) + .unwrap(); + assert_eq!(recent_ballots.len(), 1, "Neuron {} has recent ballots", i); + } + + let proposal = governance.heap_data.proposals.get(&1).unwrap(); + assert_eq!(proposal.latest_tally.unwrap().yes, 900); + // It is now "decided" + assert_eq!(proposal.decided_timestamp_seconds, 1234); + } + + #[test] + fn test_rewards_distribution_is_blocked_on_votes_not_cast_in_state_machine() { + let now = 1733433219; + let topic = Topic::Governance; + let mut neurons = BTreeMap::new(); + let mut ballots = HashMap::new(); + + for i in 1..=9 { + let mut followees = HashMap::new(); + if i != 1 { + // cascading followees + followees.insert( + topic as i32, + Followees { + followees: vec![NeuronId { id: i - 1 }], + }, + ); + } + add_neuron_with_ballot( + &mut neurons, + &mut ballots, + make_neuron(i, 100_000_000, followees), + ); + } + + // Neurons should all get rewards. + for ballot in ballots.iter_mut() { + ballot.1.vote = Vote::Yes as i32; + } + + add_neuron_with_ballot( + &mut neurons, + &mut ballots, + make_neuron( + 100, + 100_000_000, + hashmap! { + topic as i32 => Followees { followees: vec![NeuronId { id: 1 }] }}, + ), + ); + + let motion = Motion { + motion_text: "".to_string(), + }; + let action = Action::Motion(motion); + let proposal = Proposal { + title: Some("".to_string()), + summary: "".to_string(), + url: "".to_string(), + action: Some(action), + }; + let governance_proto = GovernanceProto { + genesis_timestamp_seconds: now - REWARD_DISTRIBUTION_PERIOD_SECONDS, + proposals: btreemap! { + 1 => ProposalData { + id: Some(ProposalId {id: 1}), + ballots, + wait_for_quiet_state: Some(WaitForQuietState { + current_deadline_timestamp_seconds: now - 100 + }), + proposal: Some(proposal), + decided_timestamp_seconds: now - 100, + ..Default::default() + } + }, + neurons: neurons + .into_iter() + .map(|(id, n)| (id, n.into_proto(&VotingPowerEconomics::DEFAULT, u64::MAX))) + .collect(), + ..Default::default() + }; + let environment = MockEnvironment::new( + vec![( + ExpectedCallCanisterMethodCallArguments::new( + GOVERNANCE_CANISTER_ID, + "get_build_metadata", + Encode!().unwrap(), + ), + Ok(vec![]), + )], + now, + ); + let now_setter = environment.now_setter(); + + let mut governance = Governance::new( + governance_proto, + Box::new(environment), + Box::new(StubIcpLedger {}), + Box::new(StubCMC {}), + ); + + assert_eq!( + governance + .neuron_store + .with_neuron(&NeuronId { id: 1 }, |n| n.maturity_e8s_equivalent) + .unwrap(), + 0 + ); + + governance.record_neuron_vote(ProposalId { id: 1 }, NeuronId { id: 100 }, Vote::Yes, topic); + + with_voting_state_machines_mut(|voting_state_machines| { + voting_state_machines.with_machine(ProposalId { id: 1 }, topic, |machine| { + assert!(!machine.is_voting_finished()); + }); + }); + + assert_eq!( + governance + .neuron_store + .with_neuron(&NeuronId { id: 1 }, |n| n.maturity_e8s_equivalent) + .unwrap(), + 0 + ); + + governance.distribute_rewards(Tokens::from_e8s(100_000_000)); + + assert_eq!( + governance + .neuron_store + .with_neuron(&NeuronId { id: 1 }, |n| n.maturity_e8s_equivalent) + .unwrap(), + 0 + ); + + now_setter(now + REWARD_DISTRIBUTION_PERIOD_SECONDS + 1); + // Finish processing the vote + governance + .process_voting_state_machines() + .now_or_never() + .unwrap(); + + // Now rewards should be able to be distributed + governance.distribute_rewards(Tokens::from_e8s(100_000_000)); + + assert_eq!( + governance + .neuron_store + .with_neuron(&NeuronId { id: 1 }, |n| n.maturity_e8s_equivalent) + .unwrap(), + 5474 + ); + } + + #[test] + fn test_can_make_decision_is_false_if_expired_but_votes_not_recorded() { + let now = 99; + // We are using this value b/c it's irrelevant to the test if + // wait_for_quiet_state is set. + let voting_period_seconds = u64::MAX; + let mut proposal_data = ProposalData { + id: Some(ProposalId { id: 1 }), + latest_tally: Some(Tally { + timestamp_seconds: 100, + yes: 51, + no: 0, + total: 100, + }), + wait_for_quiet_state: Some(WaitForQuietState { + current_deadline_timestamp_seconds: 100, + }), + ..Default::default() + }; + + // First we test majority switch + assert!(proposal_data.can_make_decision(now, voting_period_seconds)); + proposal_data.latest_tally.as_mut().unwrap().yes = 49; + assert!(!proposal_data.can_make_decision(now, voting_period_seconds)); + + let now = 101; + assert!(proposal_data.can_make_decision(now, voting_period_seconds)); + + with_voting_state_machines_mut(|vsm| { + vsm.with_machine(ProposalId { id: 1 }, Topic::Governance, |machine| { + // We don't actually care about the vote being persisted, we just need to represent work still to be done. + machine.cast_vote( + &mut hashmap! { 1 => Ballot {vote: Vote::Unspecified as i32, voting_power: 100}}, + NeuronId { id: 1 }, + Vote::Yes, + ); + }) + }); + // Now we should no longer be able to make a decision. + assert!(!proposal_data.can_make_decision(now, voting_period_seconds)); } } diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs index f4794c4f557..36d613be47b 100644 --- a/rs/nns/governance/tests/governance.rs +++ b/rs/nns/governance/tests/governance.rs @@ -220,6 +220,12 @@ fn check_proposal_status_after_voting_and_after_expiration_new( .unwrap(), )); nns.governance.run_periodic_tasks().now_or_never(); + // We need to process the timer to make sure recent ballots record. + nns.governance + .process_voting_state_machines() + .now_or_never() + .unwrap(); + let after_expiration = nns.governance.get_proposal_data(pid).unwrap(); assert_eq!( @@ -1112,6 +1118,9 @@ async fn test_cascade_following_new() { ProposalId { id: 1 }, Vote::Yes, ); + // Recent ballots are not immediately recorded in every case (such as when there are + // many votes in the cascading, it is done later through timer tasks) + nns.governance.process_voting_state_machines().await; assert_changes!( nns, @@ -1166,6 +1175,10 @@ async fn test_cascade_following_new() { Vote::Yes, ); + // Recent ballots are not immediately recorded in every case (such as when there are + // many votes in the cascading, it is done later through timer tasks) + nns.governance.process_voting_state_machines().await; + // Check that the vote for neuron 2 is registered in the proposal assert_eq!( (Vote::Yes as i32), @@ -1455,6 +1468,10 @@ async fn test_cascade_following() { Vote::Yes, ); + // Recent ballots are not immediately recorded in every case (such as when there are + // many votes in the cascading, it is done later through timer tasks) + gov.process_voting_state_machines().await; + // Check that the vote for neuron 2 is registered in the proposal assert_eq!( (Vote::Yes as i32), @@ -3895,7 +3912,7 @@ fn test_random_voting_rewards_scenarios() { proposals } - const SCENARIO_COUNT: u64 = 1000; + const SCENARIO_COUNT: u64 = 500; let mut unique_scenarios = HashSet::new(); for seed in 1..=SCENARIO_COUNT { unique_scenarios.insert(helper(seed)); diff --git a/rs/nns/integration_tests/src/neuron_voting.rs b/rs/nns/integration_tests/src/neuron_voting.rs index 633568813c6..d5b6fb3f37e 100644 --- a/rs/nns/integration_tests/src/neuron_voting.rs +++ b/rs/nns/integration_tests/src/neuron_voting.rs @@ -1,10 +1,15 @@ use assert_matches::assert_matches; use ic_base_types::PrincipalId; -use ic_nns_common::types::ProposalId; +use ic_nervous_system_common_test_keys::{TEST_NEURON_1_ID, TEST_NEURON_1_OWNER_PRINCIPAL}; +use ic_nns_common::{pb::v1::NeuronId, types::ProposalId}; +use ic_nns_governance::pb::v1::{ + neuron::{DissolveState, Followees}, + Neuron, Topic, +}; use ic_nns_governance_api::pb::v1::{ governance_error::ErrorType, manage_neuron_response::{Command, RegisterVoteResponse}, - Vote, + BallotInfo, ListNeurons, Vote, }; use ic_nns_test_utils::{ common::NnsInitPayloadsBuilder, @@ -13,11 +18,14 @@ use ic_nns_test_utils::{ get_unauthorized_neuron, submit_proposal, }, state_test_helpers::{ - get_pending_proposals, nns_cast_vote, nns_governance_get_full_neuron, + get_pending_proposals, list_neurons, nns_cast_vote, nns_governance_get_full_neuron, nns_governance_make_proposal, setup_nns_canisters, state_machine_builder_for_nns_tests, }, }; use ic_state_machine_tests::StateMachine; +use icp_ledger::Subaccount; +use maplit::hashmap; +use std::{collections::HashMap, time::Duration}; const INVALID_PROPOSAL_ID: u64 = 69420; @@ -326,3 +334,113 @@ fn cannot_vote_on_future_proposal() { assert_eq!(proposal.ballots[&n1.neuron_id.id].vote(), Vote::Unspecified); } } + +fn neuron_with_followees( + id: u64, + followees: HashMap, +) -> ic_nns_governance_api::pb::v1::Neuron { + const TWELVE_MONTHS_SECONDS: u64 = 30 * 12 * 24 * 60 * 60; + + let neuron_id = NeuronId::from_u64(id); + let mut account = vec![0; 32]; + for (destination, data) in account.iter_mut().zip(id.to_le_bytes().iter().cycle()) { + *destination = *data; + } + let subaccount = Subaccount::try_from(account.as_slice()).unwrap(); + + Neuron { + id: Some(neuron_id), + controller: Some(PrincipalId::new_user_test_id(id)), + hot_keys: vec![*TEST_NEURON_1_OWNER_PRINCIPAL], + // Use large values to avoid the possibility of collisions with other self-authenticating hotkeys + dissolve_state: Some(DissolveState::DissolveDelaySeconds(TWELVE_MONTHS_SECONDS)), + cached_neuron_stake_e8s: 1_000_000_000, + account: subaccount.to_vec(), + followees, + ..Default::default() + } + .into() +} + +#[test] +fn test_voting_can_span_multiple_rounds() { + let topic = Topic::ProtocolCanisterManagement; + let neurons = (1..1000u64).map(|i| { + let followees = if i != 1 { + hashmap! {topic as i32 => Followees { followees: vec![NeuronId { id: i -1 }] }} + } else { + hashmap! {topic as i32 => Followees { followees: vec![NeuronId {id : TEST_NEURON_1_ID}] }} + }; + neuron_with_followees(i, followees) + }).collect(); + + let state_machine = state_machine_builder_for_nns_tests().build(); + let nns_init_payloads = NnsInitPayloadsBuilder::new() + .with_test_neurons() + .with_additional_neurons(neurons) + .build(); + setup_nns_canisters(&state_machine, nns_init_payloads); + + let proposal = get_some_proposal(); + + let response = nns_governance_make_proposal( + &state_machine, + *TEST_NEURON_1_OWNER_PRINCIPAL, + NeuronId { + id: TEST_NEURON_1_ID, + }, + &proposal, + ); + + assert_matches!(response.command, Some(Command::MakeProposal(_))); + + let listed_neurons = list_neurons( + &state_machine, + *TEST_NEURON_1_OWNER_PRINCIPAL, + ListNeurons { + neuron_ids: (0..1000u64).collect(), + include_neurons_readable_by_caller: false, + include_empty_neurons_readable_by_caller: None, + include_public_neurons_in_full_neurons: None, + }, + ); + + assert_eq!(listed_neurons.full_neurons.len(), 999); + + // No recent ballots, bc ran out of instructions, should wait til next round. + for neuron in listed_neurons.full_neurons { + assert_eq!(neuron.recent_ballots, vec![], "Neuron: {:?}", neuron); + } + + // The timer should run, which should record all the ballots. + // Because we set the instructions limit very low, we need to run the timer multiple times. + for _ in 0..1000 { + state_machine.advance_time(Duration::from_secs(3)); + state_machine.tick(); + } + + let listed_neurons = list_neurons( + &state_machine, + *TEST_NEURON_1_OWNER_PRINCIPAL, + ListNeurons { + neuron_ids: (0..1000u64).collect(), + include_neurons_readable_by_caller: false, + include_empty_neurons_readable_by_caller: None, + include_public_neurons_in_full_neurons: None, + }, + ); + + assert_eq!(listed_neurons.full_neurons.len(), 999); + + for neuron in listed_neurons.full_neurons { + assert_eq!( + neuron.recent_ballots, + vec![BallotInfo { + proposal_id: Some(ic_nns_common::pb::v1::ProposalId { id: 1 }), + vote: 1 + }], + "Neuron: {:?}", + neuron + ); + } +} diff --git a/rs/nns/integration_tests/src/voting_rewards.rs b/rs/nns/integration_tests/src/voting_rewards.rs index 07ce53d3efa..c86d37e984c 100644 --- a/rs/nns/integration_tests/src/voting_rewards.rs +++ b/rs/nns/integration_tests/src/voting_rewards.rs @@ -2,7 +2,7 @@ use canister_test::Runtime; use dfn_candid::{candid, candid_one}; use ic_canister_client_sender::Sender; use ic_nervous_system_common_test_keys::{TEST_NEURON_1_ID, TEST_NEURON_1_OWNER_KEYPAIR}; -use ic_nns_common::types::NeuronId; +use ic_nns_common::{pb::v1::ProposalId, types::NeuronId}; use ic_nns_governance::governance::REWARD_DISTRIBUTION_PERIOD_SECONDS; use ic_nns_governance_api::pb::v1::{ Ballot, Governance as GovernanceProto, GovernanceError, NetworkEconomics, Neuron, ProposalData, @@ -57,6 +57,7 @@ fn test_increase_maturity_just_after_init() { proposals: once(( 1, ProposalData { + id: Some(ProposalId { id: 1 }), proposal_timestamp_seconds: genesis_timestamp_secs, ballots: once(( TEST_NEURON_1_ID,