Skip to content

Commit

Permalink
refactor(nns): Move recompute_tally to inside of cast_vote_and_cascad…
Browse files Browse the repository at this point in the history
…e_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.
  • Loading branch information
max-dfinity authored Dec 4, 2024
1 parent bed178d commit 6e5283d
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 36 deletions.
36 changes: 18 additions & 18 deletions rs/nns/governance/canbench/canbench_results.yml
Original file line number Diff line number Diff line change
@@ -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: {}
Expand All @@ -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: {}
Expand All @@ -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: {}
Expand All @@ -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: {}
Expand All @@ -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
10 changes: 0 additions & 10 deletions rs/nns/governance/src/governance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
61 changes: 53 additions & 8 deletions rs/nns/governance/src/voting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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));
}
}

Expand Down Expand Up @@ -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,
};
Expand Down Expand Up @@ -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 {}),
);
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit 6e5283d

Please sign in to comment.