From 6e5283ddc7be94517e15598788a85e886682c4fd Mon Sep 17 00:00:00 2001 From: max-dfinity <100170574+max-dfinity@users.noreply.github.com> Date: Wed, 4 Dec 2024 14:48:39 -0800 Subject: [PATCH] refactor(nns): Move recompute_tally to inside of cast_vote_and_cascade_follow (#2977) This removes a difficult edge case of trying to determine when to re-tally. We know that any votes cast in cast_vote_and_cascade_follow are either when a proposal is created, or when register_vote is called, which makes sure the vote cast is cast while the proposal voting is still open. This helps later when votes can be processed across multiple messages. --- .../governance/canbench/canbench_results.yml | 36 +++++------ rs/nns/governance/src/governance.rs | 10 --- rs/nns/governance/src/voting.rs | 61 ++++++++++++++++--- 3 files changed, 71 insertions(+), 36 deletions(-) diff --git a/rs/nns/governance/canbench/canbench_results.yml b/rs/nns/governance/canbench/canbench_results.yml index e15d386e650..414e9b32895 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: 36059483 + instructions: 36059517 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_active_typical: total: - instructions: 1830111 + instructions: 1830145 heap_increase: 0 stable_memory_increase: 0 scopes: {} add_neuron_inactive_maximum: total: - instructions: 96070090 + instructions: 96070124 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_inactive_typical: total: - instructions: 7372887 + instructions: 7372921 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_all_heap: total: - instructions: 31756954 + instructions: 34047728 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_heap_neurons_stable_index: total: - instructions: 54261919 + instructions: 56554267 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_stable_everything: total: - instructions: 160631204 + instructions: 162950875 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_stable_neurons_with_heap_index: total: - instructions: 138235474 + instructions: 140555145 heap_increase: 0 stable_memory_increase: 0 scopes: {} centralized_following_all_stable: total: - instructions: 66109851 + instructions: 68428123 heap_increase: 0 stable_memory_increase: 0 scopes: {} compute_ballots_for_new_proposal_with_stable_neurons: total: - instructions: 1826966 + instructions: 1830966 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -67,7 +67,7 @@ benches: scopes: {} draw_maturity_from_neurons_fund_stable: total: - instructions: 10256384 + instructions: 10264384 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -79,7 +79,7 @@ benches: scopes: {} list_neurons_ready_to_unstake_maturity_stable: total: - instructions: 37619197 + instructions: 37739237 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -91,7 +91,7 @@ benches: scopes: {} list_ready_to_spawn_neuron_ids_stable: total: - instructions: 37607898 + instructions: 37727938 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -103,26 +103,26 @@ benches: scopes: {} neuron_metrics_calculation_stable: total: - instructions: 1861928 + instructions: 1865928 heap_increase: 0 stable_memory_increase: 0 scopes: {} range_neurons_performance: total: - instructions: 48539417 + instructions: 48547417 heap_increase: 0 stable_memory_increase: 0 scopes: {} single_vote_all_stable: total: - instructions: 364631 + instructions: 2690730 heap_increase: 0 stable_memory_increase: 0 scopes: {} update_recent_ballots_stable_memory: total: - instructions: 13454595 + instructions: 13454675 heap_increase: 0 stable_memory_increase: 0 scopes: {} -version: 0.1.7 +version: 0.1.8 diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index 809f901bf08..c4b1c7645d2 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.rs @@ -4107,16 +4107,6 @@ impl Governance { let topic = proposal.topic(); let voting_period_seconds = voting_period_seconds_fn(topic); - // Recompute the tally here. It should correctly reflect all votes, - // even the ones after the proposal has been decided. It's possible - // to have Open status while it does not accept votes anymore, since - // the status change happens below this point. - if proposal.status() == ProposalStatus::Open - || proposal.accepts_vote(now_seconds, voting_period_seconds) - { - proposal.recompute_tally(now_seconds, voting_period_seconds); - } - if proposal.status() != ProposalStatus::Open { return; } diff --git a/rs/nns/governance/src/voting.rs b/rs/nns/governance/src/voting.rs index a00579c3a49..3cc52fa90ae 100644 --- a/rs/nns/governance/src/voting.rs +++ b/rs/nns/governance/src/voting.rs @@ -20,13 +20,19 @@ impl Governance { vote_of_neuron: Vote, topic: Topic, ) { + let voting_started = self.env.now(); + let neuron_store = &mut self.neuron_store; - let ballots = &mut self - .heap_data - .proposals - .get_mut(&proposal_id.id) - .unwrap() - .ballots; + 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; + } + }; + // 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 @@ -44,6 +50,29 @@ impl Governance { voting_state_machines.remove_if_done(&proposal_id); }); + // 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); + } + + /// Recompute the tally for a proposal, using the time provided as the current time. + fn recompute_proposal_tally(&mut self, proposal_id: ProposalId, now: u64) { + let voting_period_seconds_fn = self.voting_period_seconds(); + + let proposal = match self.heap_data.proposals.get_mut(&proposal_id.id) { + 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 recompute_proposal_tally: Proposal not found: {}", + proposal_id.id + ); + return; + } + Some(proposal) => &mut *proposal, + }; + let topic = proposal.topic(); + proposal.recompute_tally(now, voting_period_seconds_fn(topic)); } } @@ -229,7 +258,7 @@ mod test { governance::{Governance, MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS}, neuron::{DissolveStateAndAge, Neuron, NeuronBuilder}, neuron_store::NeuronStore, - pb::v1::{neuron::Followees, Ballot, ProposalData, Topic, Vote}, + pb::v1::{neuron::Followees, Ballot, ProposalData, Tally, Topic, Vote}, test_utils::{MockEnvironment, StubCMC, StubIcpLedger}, voting::ProposalVotingStateMachine, }; @@ -446,7 +475,7 @@ mod test { }; let mut governance = Governance::new( governance_proto, - Box::new(MockEnvironment::new(Default::default(), 0)), + Box::new(MockEnvironment::new(Default::default(), 234)), Box::new(StubIcpLedger {}), Box::new(StubCMC {}), ); @@ -478,6 +507,22 @@ mod test { 6 => make_ballot(deciding_voting_power(NeuronId { id: 6 }), Vote::Unspecified), } ); + let expected_tally = Tally { + timestamp_seconds: 234, + yes: 530, + no: 0, + total: 636, + }; + assert_eq!( + governance + .heap_data + .proposals + .get(&1) + .unwrap() + .latest_tally + .unwrap(), + expected_tally + ); } fn add_neuron_with_ballot(