From a52f966827af0f1ceee8d2dc395b4236799a8053 Mon Sep 17 00:00:00 2001 From: Daniel Wong <97631336+daniel-wong-dfinity-org@users.noreply.github.com> Date: Thu, 31 Oct 2024 19:20:18 +0100 Subject: [PATCH] feat(nns): Refresh voting power. (#2320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This occurs when a neuron is modified in the following ways: 1. vote (directly, not indirectly) 2. change follow # Background This is part of the "periodic confirmation" feature that was recently approved in proposal [132411][prop]. [prop]: https://dashboard.internetcomputer.org/proposal/132411 # Future Work This will be used to reduce the voting power in ballots when a neuron has not refreshed for a long time. # References Closes https://dfinity.atlassian.net/browse/NNS1-3405. [👈 Previous PR][prev] | [Next PR 👉][next] [prev]: https://github.com/dfinity/ic/pull/2268 [next]: https://github.com/dfinity/ic/pull/2338 --- rs/nns/governance/src/governance.rs | 27 +++++- rs/nns/governance/src/lib.rs | 12 +++ rs/nns/governance/src/neuron/types.rs | 17 +--- rs/nns/governance/tests/governance.rs | 129 ++++++++++++++++++++------ 4 files changed, 141 insertions(+), 44 deletions(-) diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index ab589846a17..91cc84e62fa 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.rs @@ -5584,6 +5584,8 @@ impl Governance { // Finally, add this proposal as an open proposal. self.insert_proposal(proposal_num, proposal_data); + self.refresh_voting_power(proposer_id); + Ok(proposal_id) } @@ -5966,9 +5968,27 @@ impl Governance { self.process_proposal(proposal_id.id); + self.refresh_voting_power(neuron_id); + Ok(()) } + fn refresh_voting_power(&mut self, neuron_id: &NeuronId) { + let now_seconds = self.env.now(); + + let result = self.with_neuron_mut(neuron_id, |neuron| { + neuron.refresh_voting_power(now_seconds); + }); + + if let Err(err) = result { + println!( + "{}WARNING: Tried to refresh the voting power of neuron {}, \ + but was unable to find it: {}", + LOG_PREFIX, neuron_id.id, err, + ); + } + } + /// Add or remove followees for this neuron for a specified topic. /// /// If the list of followees is empty, remove the followees for @@ -6031,17 +6051,20 @@ impl Governance { ) })?; + let now_seconds = self.env.now(); self.with_neuron_mut(id, |neuron| { if follow_request.followees.is_empty() { - neuron.followees.remove(&(topic as i32)) + neuron.followees.remove(&(topic as i32)); } else { neuron.followees.insert( topic as i32, Followees { followees: follow_request.followees.clone(), }, - ) + ); } + + neuron.refresh_voting_power(now_seconds); })?; Ok(()) diff --git a/rs/nns/governance/src/lib.rs b/rs/nns/governance/src/lib.rs index c9a469d39f5..03f80ceaa48 100644 --- a/rs/nns/governance/src/lib.rs +++ b/rs/nns/governance/src/lib.rs @@ -175,6 +175,18 @@ mod subaccount_index; /// The value of 10_000 follows the Candid recommendation. const DEFAULT_SKIPPING_QUOTA: usize = 10_000; +/// Value: one second after midnight, 2024-11-05 (UTC). +/// +/// How this value was chosen: This is around the earliest time when +/// "refreshing" a neuron's voting power might be released, (assuming the usual +/// NNS release cycle). Significantly different values could also work, but this +/// seems like a nice "neutral" value. +/// +/// How this value is used: when a neuron does not have a value in the +/// voting_power_refreshed_timestamp_seconds field (because it was created before +/// this feature), we pretend as though this value is in that field. +pub const DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS: u64 = 1731628801; + // TODO(NNS1-3248): Delete this once the feature has made it through the // probation period. At that point, we will not need this "kill switch". We can // leave this here indefinitely, but it will just be clutter after a modest diff --git a/rs/nns/governance/src/neuron/types.rs b/rs/nns/governance/src/neuron/types.rs index cae41e540f5..c6211e5d2b3 100644 --- a/rs/nns/governance/src/neuron/types.rs +++ b/rs/nns/governance/src/neuron/types.rs @@ -15,6 +15,7 @@ use crate::{ Neuron as NeuronProto, NeuronInfo, NeuronStakeTransfer, NeuronState, NeuronType, Topic, Visibility, Vote, }, + DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS, }; use ic_base_types::PrincipalId; use ic_cdk::println; @@ -23,18 +24,6 @@ use ic_nns_common::pb::v1::{NeuronId, ProposalId}; use icp_ledger::Subaccount; use std::collections::{BTreeSet, HashMap}; -/// Value: one second after midnight, 2024-11-05 (UTC). -/// -/// How this value was chosen: This is around the earliest time when -/// "refreshing" a neuron's voting power might be released, (assuming the usual -/// NNS release cycle). Significantly different values could also work, but this -/// seems like a nice "neutral" value. -/// -/// How this value is used: when a neuron does not have a value in the -/// voting_power_refreshed_timestamp_seconds field (because it was created before -/// this feature), we pretend as though this value is in that field. -const DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS: u64 = 1731628801; - /// A neuron type internal to the governance crate. Currently, this type is identical to the /// prost-generated Neuron type (except for derivations for prost). Gradually, this type will evolve /// towards having all private fields while exposing methods for mutations, which allows it to hold @@ -480,6 +469,10 @@ impl Neuron { } } + pub(crate) fn refresh_voting_power(&mut self, now_seconds: u64) { + self.voting_power_refreshed_timestamp_seconds = now_seconds; + } + pub(crate) fn ready_to_unstake_maturity(&self, now_seconds: u64) -> bool { self.state(now_seconds) == NeuronState::Dissolved && self.staked_maturity_e8s_equivalent.unwrap_or(0) > 0 diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs index b00a9f5aee0..1401eb5ded3 100644 --- a/rs/nns/governance/tests/governance.rs +++ b/rs/nns/governance/tests/governance.rs @@ -95,6 +95,7 @@ use ic_nns_governance::{ }, temporarily_disable_private_neuron_enforcement, temporarily_disable_set_visibility_proposals, temporarily_enable_private_neuron_enforcement, temporarily_enable_set_visibility_proposals, + DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS, }; use ic_nns_governance_api::{ pb::v1::CreateServiceNervousSystem as ApiCreateServiceNervousSystem, @@ -265,16 +266,30 @@ fn test_single_neuron_proposal_new() { NNSStateChange::GovernanceProto(vec![ GovernanceChange::Neurons(vec![MapChange::Changed( 0, - vec![NeuronChange::RecentBallots(vec![VecChange::Added( - 0, - vec![ - BallotInfoChange::ProposalId(OptionChange::Different( - None, - Some(ProposalId { id: 1 }), - )), - BallotInfoChange::Vote(I32Change(0, 1)), - ], - )])], + vec![ + NeuronChange::RecentBallots(vec![VecChange::Added( + 0, + vec![ + BallotInfoChange::ProposalId(OptionChange::Different( + None, + Some(ProposalId { id: 1 }), + )), + BallotInfoChange::Vote(I32Change(0, 1)), + ], + )]), + // Neuron's voting power was refreshed, because it directly votes (i.e. not + // as a result of following), but implicitly voted (by virtue of being the + // proposer). (Normally, the amount would increase, but for testing + // purposes, it is enough to show that the value changed.) + NeuronChange::VotingPowerRefreshedTimestampSeconds( + OptionChange::BothSome( + U64Change( + DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS, + DEFAULT_TEST_START_TIMESTAMP_SECONDS, + ), + ), + ), + ], )]), GovernanceChange::Proposals(vec![MapChange::Added( 1, @@ -932,6 +947,13 @@ async fn test_cascade_following_new() { BallotInfoChange::Vote(I32Change(0, 1)), ], )]), + // As in an earlier test, the proposer's voting power gets refreshed. + NeuronChange::VotingPowerRefreshedTimestampSeconds(OptionChange::BothSome( + U64Change( + DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS, + DEFAULT_TEST_START_TIMESTAMP_SECONDS, + ), + ),), ], )]), GovernanceChange::Proposals(vec![MapChange::Added( @@ -1064,16 +1086,25 @@ async fn test_cascade_following_new() { Changed::Changed(vec![NNSStateChange::GovernanceProto(vec![ GovernanceChange::Neurons(vec![MapChange::Changed( 5, - vec![NeuronChange::RecentBallots(vec![VecChange::Added( - 0, - vec![ - BallotInfoChange::ProposalId(OptionChange::Different( - None, - Some(ProposalId { id: 1 }), - )), - BallotInfoChange::Vote(I32Change(0, 1)), - ], - )])], + vec![ + NeuronChange::RecentBallots(vec![VecChange::Added( + 0, + vec![ + BallotInfoChange::ProposalId(OptionChange::Different( + None, + Some(ProposalId { id: 1 }), + )), + BallotInfoChange::Vote(I32Change(0, 1)), + ], + )]), + // Neuron's voting power was refreshed, because it voted directly. + NeuronChange::VotingPowerRefreshedTimestampSeconds(OptionChange::BothSome( + U64Change( + DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS, + DEFAULT_TEST_START_TIMESTAMP_SECONDS, + ), + ),), + ], )]), GovernanceChange::Proposals(vec![MapChange::Changed( 1, @@ -1167,16 +1198,26 @@ async fn test_cascade_following_new() { ), MapChange::Changed( 6, - vec![NeuronChange::RecentBallots(vec![VecChange::Added( - 0, - vec![ - BallotInfoChange::ProposalId(OptionChange::Different( - None, - Some(ProposalId { id: 1 }), - )), - BallotInfoChange::Vote(I32Change(0, 1)), - ], - )])], + vec![ + NeuronChange::RecentBallots(vec![VecChange::Added( + 0, + vec![ + BallotInfoChange::ProposalId(OptionChange::Different( + None, + Some(ProposalId { id: 1 }), + )), + BallotInfoChange::Vote(I32Change(0, 1)), + ], + )]), + // Only this neuron voted directly; therefore, even though other neurons + // voted (via following) only this neuron's voting power was refreshed. + NeuronChange::VotingPowerRefreshedTimestampSeconds(OptionChange::BothSome( + U64Change( + DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS, + DEFAULT_TEST_START_TIMESTAMP_SECONDS, + ), + ),), + ], ), ]), GovernanceChange::Proposals(vec![MapChange::Changed( @@ -1699,6 +1740,18 @@ async fn test_all_follow_proposer() { driver.get_fake_ledger(), driver.get_fake_cmc(), ); + + // Later, we'll inspect the same field again to verify that voting power + // refresh actually took place. This is just to make sure that the later + // value is different from this earlier one. + assert_eq!( + gov.with_neuron(&NeuronId { id: 5 }, |neuron| { + neuron.voting_power_refreshed_timestamp_seconds() + }) + .unwrap(), + DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS, + ); + // Add following for 5 and 6 for 1. gov.manage_neuron( // Must match neuron 5's serialized_id. @@ -1716,6 +1769,22 @@ async fn test_all_follow_proposer() { .unwrap() .panic_if_error("Manage neuron failed"); + // Assert that neuron 5's voting power was refreshed. More concretely, + // verifying that neuron 5's voting_power_refreshed_timestamp_seconds + // changed from before. (Earlier, we saw that this field had a different + // value, to wit, DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS) + assert_ne!( + DEFAULT_TEST_START_TIMESTAMP_SECONDS, + DEFAULT_VOTING_POWER_REFRESHED_TIMESTAMP_SECONDS, + ); + assert_eq!( + gov.with_neuron(&NeuronId { id: 5 }, |neuron| { + neuron.voting_power_refreshed_timestamp_seconds() + }) + .unwrap(), + DEFAULT_TEST_START_TIMESTAMP_SECONDS, + ); + gov.manage_neuron( // Must match neuron 6's serialized_id. &principal(6),