From 363e697195284ca239776674aa8d2b94248fd68e Mon Sep 17 00:00:00 2001 From: max-dfinity <100170574+max-dfinity@users.noreply.github.com> Date: Mon, 18 Nov 2024 15:02:15 -0800 Subject: [PATCH] refactor(nns): Move cast_vote_and_cascade_follow into Governance struct (#2600) This puts cast_vote_and_cascade_follow into governance, which will be necessary for the next step of making it async, and then finally adding break points to increase the voting capacity. [Prev](https://github.com/dfinity/ic/pull/2528) | Next --- .../governance/canbench/canbench_results.yml | 34 ++++---- rs/nns/governance/src/governance.rs | 45 ++++------- rs/nns/governance/src/governance/benches.rs | 42 ++++++---- rs/nns/governance/src/governance/tests/mod.rs | 77 ++++++++++++++----- rs/nns/governance/src/voting.rs | 63 ++++++++------- 5 files changed, 150 insertions(+), 111 deletions(-) diff --git a/rs/nns/governance/canbench/canbench_results.yml b/rs/nns/governance/canbench/canbench_results.yml index e44697f6069..32240ac53c7 100644 --- a/rs/nns/governance/canbench/canbench_results.yml +++ b/rs/nns/governance/canbench/canbench_results.yml @@ -1,91 +1,91 @@ benches: add_neuron_active_maximum: total: - instructions: 35970325 + instructions: 35887686 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_active_typical: total: - instructions: 1837226 + instructions: 1833590 heap_increase: 0 stable_memory_increase: 0 scopes: {} add_neuron_inactive_maximum: total: - instructions: 98527974 + instructions: 98494555 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_inactive_typical: total: - instructions: 7584030 + instructions: 7593880 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_all_heap: total: - instructions: 35794036 + instructions: 32187827 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_heap_neurons_stable_index: total: - instructions: 57972358 + instructions: 54527506 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_stable_everything: total: - instructions: 2936044986 - heap_increase: 1 + instructions: 1978888007 + heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_stable_neurons_with_heap_index: total: - instructions: 2913853879 - heap_increase: 1 + instructions: 1956237656 + heap_increase: 0 stable_memory_increase: 0 scopes: {} centralized_following_all_stable: total: - instructions: 2169878594 + instructions: 2057421983 heap_increase: 0 stable_memory_increase: 0 scopes: {} compute_ballots_for_new_proposal_with_stable_neurons: total: - instructions: 1555655 + instructions: 1685116 heap_increase: 0 stable_memory_increase: 0 scopes: {} neuron_metrics_calculation_heap: total: - instructions: 845228 + instructions: 840837 heap_increase: 0 stable_memory_increase: 0 scopes: {} neuron_metrics_calculation_stable: total: - instructions: 1898294 + instructions: 1834053 heap_increase: 0 stable_memory_increase: 0 scopes: {} range_neurons_performance: total: - instructions: 47389870 + instructions: 48615662 heap_increase: 0 stable_memory_increase: 0 scopes: {} single_vote_all_stable: total: - instructions: 13047584 + instructions: 13224969 heap_increase: 0 stable_memory_increase: 0 scopes: {} update_recent_ballots: total: - instructions: 16205133 + instructions: 16275553 heap_increase: 0 stable_memory_increase: 0 scopes: {} diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index 9275952d06e..b7abd290081 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.rs @@ -135,7 +135,7 @@ mod benches; pub mod tla_macros; #[cfg(feature = "tla")] pub mod tla; -use crate::voting::cast_vote_and_cascade_follow; + #[cfg(feature = "tla")] pub use tla::{ claim_neuron_desc, split_neuron_desc, tla_update_method, InstrumentationState, ToTla, @@ -5073,19 +5073,6 @@ impl Governance { .expect("NetworkEconomics not present") } - /// Inserts a proposals that has already been validated in the state. - /// - /// This is a low-level function that makes no verification whatsoever. - fn insert_proposal(&mut self, pid: u64, data: ProposalData) { - let voting_period_seconds = self.voting_period_seconds()(data.topic()); - self.closest_proposal_deadline_timestamp_seconds = std::cmp::min( - data.proposal_timestamp_seconds + voting_period_seconds, - self.closest_proposal_deadline_timestamp_seconds, - ); - self.heap_data.proposals.insert(pid, data); - self.process_proposal(pid); - } - /// The proposal id of the next proposal. fn next_proposal_id(&self) -> u64 { // Correctness is based on the following observations: @@ -5575,7 +5562,7 @@ impl Governance { }; // Create the proposal. - let mut proposal_data = ProposalData { + let proposal_data = ProposalData { id: Some(proposal_id), proposer: Some(*proposer_id), reject_cost_e8s, @@ -5597,17 +5584,17 @@ impl Governance { }) .expect("Proposer not found."); - // Cast self-vote, including following. - cast_vote_and_cascade_follow( - &proposal_id, - &mut proposal_data.ballots, - proposer_id, - Vote::Yes, - topic, - &mut self.neuron_store, - ); // Finally, add this proposal as an open proposal. - self.insert_proposal(proposal_num, proposal_data); + let voting_period_seconds = self.voting_period_seconds()(proposal_data.topic()); + self.closest_proposal_deadline_timestamp_seconds = std::cmp::min( + proposal_data.proposal_timestamp_seconds + voting_period_seconds, + self.closest_proposal_deadline_timestamp_seconds, + ); + self.heap_data.proposals.insert(proposal_num, proposal_data); + + self.cast_vote_and_cascade_follow(proposal_id, *proposer_id, Vote::Yes, topic); + + self.process_proposal(proposal_num); self.refresh_voting_power(proposer_id); @@ -5823,14 +5810,12 @@ impl Governance { )); } - cast_vote_and_cascade_follow( + self.cast_vote_and_cascade_follow( // Actually update the ballot, including following. - proposal_id, - &mut proposal.ballots, - neuron_id, + *proposal_id, + *neuron_id, vote, topic, - &mut self.neuron_store, ); self.process_proposal(proposal_id.id); diff --git a/rs/nns/governance/src/governance/benches.rs b/rs/nns/governance/src/governance/benches.rs index 03160bb350f..17de7f2d3e5 100644 --- a/rs/nns/governance/src/governance/benches.rs +++ b/rs/nns/governance/src/governance/benches.rs @@ -4,14 +4,13 @@ use crate::{ neuron_store::NeuronStore, pb::v1::{ neuron::Followees, proposal::Action, Ballot, BallotInfo, Governance as GovernanceProto, - KnownNeuron, Neuron as NeuronProto, Topic, Vote, + KnownNeuron, Neuron as NeuronProto, ProposalData, Topic, Vote, }, temporarily_disable_active_neurons_in_stable_memory, temporarily_disable_stable_memory_following_index, temporarily_enable_active_neurons_in_stable_memory, temporarily_enable_stable_memory_following_index, test_utils::{MockEnvironment, StubCMC, StubIcpLedger}, - voting::cast_vote_and_cascade_follow, }; use canbench_rs::{bench, bench_fn, BenchResult}; use ic_base_types::PrincipalId; @@ -20,7 +19,7 @@ use ic_nns_common::{ types::NeuronId, }; use icp_ledger::Subaccount; -use maplit::{btreemap, hashmap}; +use maplit::hashmap; use rand::{Rng, SeedableRng}; use rand_chacha::ChaCha20Rng; use std::collections::{BTreeMap, HashMap}; @@ -47,10 +46,19 @@ enum SetUpStrategy { fn set_up( strategy: SetUpStrategy, rng: &mut R, - neuron_store: &mut NeuronStore, - ballots: &mut HashMap, + governance: &mut Governance, topic: Topic, ) -> NeuronId { + let neuron_store = &mut governance.neuron_store; + governance.heap_data.proposals.insert( + 1, + ProposalData { + id: Some(ProposalId { id: 1 }), + ..Default::default() + }, + ); + let ballots = &mut governance.heap_data.proposals.get_mut(&1).unwrap().ballots; + match strategy { SetUpStrategy::Centralized { num_neurons } => { set_up_centralized(num_neurons, rng, neuron_store, ballots, topic) @@ -301,21 +309,23 @@ fn set_up_chain( } fn cast_vote_cascade_helper(strategy: SetUpStrategy, topic: Topic) -> BenchResult { - let mut ballots = HashMap::new(); let mut rng = ChaCha20Rng::seed_from_u64(0); - let mut neuron_store = NeuronStore::new(btreemap! {}); - let neuron_id = set_up(strategy, &mut rng, &mut neuron_store, &mut ballots, topic); + + let governance_proto = GovernanceProto { + ..Default::default() + }; + let mut governance = Governance::new( + governance_proto, + Box::new(MockEnvironment::new(Default::default(), 0)), + Box::new(StubIcpLedger {}), + Box::new(StubCMC {}), + ); + + let neuron_id = set_up(strategy, &mut rng, &mut governance, topic); let proposal_id = ProposalId { id: 1 }; bench_fn(|| { - cast_vote_and_cascade_follow( - &proposal_id, - &mut ballots, - &neuron_id.into(), - Vote::Yes, - topic, - &mut neuron_store, - ); + governance.cast_vote_and_cascade_follow(proposal_id, neuron_id.into(), Vote::Yes, topic); // let yes_votes = ballots // .iter() // .filter(|(id, ballot)| ballot.vote == Vote::Yes as i32) diff --git a/rs/nns/governance/src/governance/tests/mod.rs b/rs/nns/governance/src/governance/tests/mod.rs index d843d90d411..930a5caedb5 100644 --- a/rs/nns/governance/src/governance/tests/mod.rs +++ b/rs/nns/governance/src/governance/tests/mod.rs @@ -1136,16 +1136,15 @@ mod neuron_archiving_tests { mod cast_vote_and_cascade_follow { use crate::{ - governance::MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS, + governance::{Governance, MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS}, neuron::{DissolveStateAndAge, Neuron, NeuronBuilder}, - neuron_store::NeuronStore, - pb::v1::{neuron::Followees, Ballot, Topic, Vote}, - voting::cast_vote_and_cascade_follow, + pb::v1::{neuron::Followees, Ballot, ProposalData, Topic, Vote}, + test_utils::{MockEnvironment, StubCMC, StubIcpLedger}, }; use ic_base_types::PrincipalId; use ic_nns_common::pb::v1::{NeuronId, ProposalId}; use icp_ledger::Subaccount; - use maplit::hashmap; + use maplit::{btreemap, hashmap}; use std::collections::{BTreeMap, HashMap}; fn make_ballot(voting_power: u64, vote: Vote) -> Ballot { @@ -1231,24 +1230,42 @@ mod cast_vote_and_cascade_follow { // Add a neuron without a ballot for neuron 6 to follow. add_neuron_without_ballot(&mut heap_neurons, 7, vec![1]); - let mut neuron_store = NeuronStore::new(heap_neurons); + let governance_proto = crate::pb::v1::Governance { + neurons: heap_neurons + .into_iter() + .map(|(id, neuron)| (id, neuron.into())) + .collect(), + proposals: btreemap! { + 1 => ProposalData { + id: Some(ProposalId {id: 1}), + ballots, + ..Default::default() + } + }, + ..Default::default() + }; + let mut governance = Governance::new( + governance_proto, + Box::new(MockEnvironment::new(Default::default(), 0)), + Box::new(StubIcpLedger {}), + Box::new(StubCMC {}), + ); - cast_vote_and_cascade_follow( - &ProposalId { id: 1 }, - &mut ballots, - &NeuronId { id: 1 }, + governance.cast_vote_and_cascade_follow( + ProposalId { id: 1 }, + NeuronId { id: 1 }, Vote::Yes, topic, - &mut neuron_store, ); let deciding_voting_power = |neuron_id| { - neuron_store + governance + .neuron_store .with_neuron(&neuron_id, |n| n.deciding_voting_power(now)) .unwrap() }; assert_eq!( - ballots, + governance.heap_data.proposals.get(&1).unwrap().ballots, hashmap! { 1 => make_ballot(deciding_voting_power(NeuronId { id: 1}), Vote::Yes), 2 => make_ballot(deciding_voting_power(NeuronId { id: 2}), Vote::Unspecified), @@ -1300,24 +1317,42 @@ mod cast_vote_and_cascade_follow { // Add a neuron without a ballot for neuron 6 to follow. add_neuron_without_ballot(&mut neurons, 7, vec![1]); - let mut neuron_store = NeuronStore::new(neurons); + let governance_proto = crate::pb::v1::Governance { + neurons: neurons + .into_iter() + .map(|(id, neuron)| (id, neuron.into())) + .collect(), + proposals: btreemap! { + 1 => ProposalData { + id: Some(ProposalId {id: 1}), + ballots, + ..Default::default() + } + }, + ..Default::default() + }; + let mut governance = Governance::new( + governance_proto, + Box::new(MockEnvironment::new(Default::default(), 0)), + Box::new(StubIcpLedger {}), + Box::new(StubCMC {}), + ); - cast_vote_and_cascade_follow( - &ProposalId { id: 1 }, - &mut ballots, - &NeuronId { id: 1 }, + governance.cast_vote_and_cascade_follow( + ProposalId { id: 1 }, + NeuronId { id: 1 }, Vote::Yes, topic, - &mut neuron_store, ); let deciding_voting_power = |neuron_id| { - neuron_store + governance + .neuron_store .with_neuron(&neuron_id, |n| n.deciding_voting_power(now)) .unwrap() }; assert_eq!( - ballots, + governance.heap_data.proposals.get(&1).unwrap().ballots, hashmap! { 1 => make_ballot(deciding_voting_power(NeuronId { id: 1 }), Vote::Yes), 2 => make_ballot(deciding_voting_power(NeuronId { id: 2 }), Vote::Yes), diff --git a/rs/nns/governance/src/voting.rs b/rs/nns/governance/src/voting.rs index 60e95a2ceb3..01680f2297f 100644 --- a/rs/nns/governance/src/voting.rs +++ b/rs/nns/governance/src/voting.rs @@ -1,4 +1,5 @@ use crate::{ + governance::Governance, neuron_store::NeuronStore, pb::v1::{Ballot, Topic, Topic::NeuronManagement, Vote}, }; @@ -11,6 +12,40 @@ use std::{ thread_local! { static VOTING_STATE_MACHINES: RefCell = RefCell::new(VotingStateMachines::new()); } +impl Governance { + pub fn cast_vote_and_cascade_follow( + &mut self, + proposal_id: ProposalId, + voting_neuron_id: NeuronId, + vote_of_neuron: Vote, + topic: Topic, + ) { + let neuron_store = &mut self.neuron_store; + let ballots = &mut self + .heap_data + .proposals + .get_mut(&proposal_id.id) + .unwrap() + .ballots; + // 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); + + proposal_voting_machine.cast_vote(ballots, voting_neuron_id, vote_of_neuron); + + while !proposal_voting_machine.is_done() { + proposal_voting_machine.continue_processing(neuron_store, ballots); + } + + voting_state_machines.remove_if_done(&proposal_id); + }); + } +} struct VotingStateMachines { // Up to one machine per proposal, to avoid having to do unnecessary checks for followers that @@ -131,6 +166,7 @@ impl ProposalVotingStateMachine { self.add_followers_to_check(neuron_store, neuron_id, self.topic); } + // 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() { @@ -183,33 +219,6 @@ fn retain_neurons_with_castable_ballots( }); } -pub(crate) fn cast_vote_and_cascade_follow( - proposal_id: &ProposalId, - ballots: &mut HashMap, - voting_neuron_id: &NeuronId, - vote_of_neuron: Vote, - topic: Topic, - neuron_store: &mut NeuronStore, -) { - // 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); - - proposal_voting_machine.cast_vote(ballots, *voting_neuron_id, vote_of_neuron); - - while !proposal_voting_machine.is_done() { - proposal_voting_machine.continue_processing(neuron_store, ballots); - } - - voting_state_machines.remove_if_done(proposal_id); - }); -} - #[cfg(test)] mod test {