diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index fb19da0f190..9d5dfe80fe0 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.rs @@ -157,6 +157,9 @@ pub const EXECUTE_NNS_FUNCTION_PAYLOAD_LISTING_BYTES_MAX: usize = 1000; // 10 KB pub const PROPOSAL_MOTION_TEXT_BYTES_MAX: usize = 10000; +// The minimum neuron dissolve delay (set when a neuron is first claimed) +pub const INITIAL_NEURON_DISSOLVE_DELAY: u64 = 7 * ONE_DAY_SECONDS; + // The maximum dissolve delay allowed for a neuron. pub const MAX_DISSOLVE_DELAY_SECONDS: u64 = 8 * ONE_YEAR_SECONDS; @@ -186,6 +189,13 @@ pub const REWARD_DISTRIBUTION_PERIOD_SECONDS: u64 = ONE_DAY_SECONDS; /// The maximum number of neurons supported. pub const MAX_NUMBER_OF_NEURONS: usize = 350_000; +// Spawning is exempted from rate limiting, so we don't need large of a limit here. +pub const MAX_SUSTAINED_NEURONS_PER_HOUR: u64 = 15; + +pub const MINIMUM_SECONDS_BETWEEN_ALLOWANCE_INCREASE: u64 = 3600 / MAX_SUSTAINED_NEURONS_PER_HOUR; + +pub const MAX_NEURON_CREATION_SPIKE: u64 = MAX_SUSTAINED_NEURONS_PER_HOUR * 8; + /// The maximum number results returned by the method `list_proposals`. pub const MAX_LIST_PROPOSAL_RESULTS: u32 = 100; @@ -1576,6 +1586,23 @@ impl LedgerUpdateLock { } } +struct NeuronRateLimits { + /// The number of neurons that can be created. + available_allowances: u64, + /// The last time the number of neurons that can be created was increased. + last_allowance_increase: u64, +} + +impl Default for NeuronRateLimits { + fn default() -> Self { + Self { + // Post-upgrade, we reset to max neurons per hour + available_allowances: MAX_NEURON_CREATION_SPIKE, + last_allowance_increase: 0, + } + } +} + /// The `Governance` canister implements the full public interface of the /// IC's governance system. pub struct Governance { @@ -1612,6 +1639,9 @@ pub struct Governance { /// Scope guard for minting node provider rewards. minting_node_provider_rewards: bool, + + /// Current neurons available to create + neuron_rate_limits: NeuronRateLimits, } pub fn governance_minting_account() -> AccountIdentifier { @@ -1827,6 +1857,7 @@ impl Governance { latest_gc_num_proposals: 0, neuron_data_validator: NeuronDataValidator::new(), minting_node_provider_rewards: false, + neuron_rate_limits: NeuronRateLimits::default(), } } @@ -1857,6 +1888,7 @@ impl Governance { latest_gc_num_proposals: 0, neuron_data_validator: NeuronDataValidator::new(), minting_node_provider_rewards: false, + neuron_rate_limits: NeuronRateLimits::default(), } } @@ -2097,7 +2129,12 @@ impl Governance { /// - the maximum number of neurons has been reached, or /// - the given `neuron_id` already exists in `self.neuron_store.neurons`, or /// - the neuron's controller `PrincipalId` is not self-authenticating. - fn add_neuron(&mut self, neuron_id: u64, neuron: Neuron) -> Result<(), GovernanceError> { + fn add_neuron( + &mut self, + neuron_id: u64, + neuron: Neuron, + with_rate_limits: bool, + ) -> Result<(), GovernanceError> { if neuron_id == 0 { return Err(GovernanceError::new_with_message( ErrorType::PreconditionFailed, @@ -2136,8 +2173,23 @@ impl Governance { )); } + if with_rate_limits && self.neuron_rate_limits.available_allowances < 1 { + return Err(GovernanceError::new_with_message( + ErrorType::Unavailable, + "Reached maximum number of neurons that can be created in this hour. \ + Please wait and try again later.", + )); + } + self.neuron_store.add_neuron(neuron)?; + if with_rate_limits { + self.neuron_rate_limits.available_allowances = self + .neuron_rate_limits + .available_allowances + .saturating_sub(1); + } + Ok(()) } @@ -2157,6 +2209,7 @@ impl Governance { )); } self.neuron_store.remove_neuron(&neuron_id); + self.neuron_rate_limits.available_allowances += 1; Ok(()) } @@ -2723,7 +2776,7 @@ impl Governance { // acquiring the lock. Indeed, in case there is already a pending // command, we return without state rollback. If we had already created // the embryo, it would not be garbage collected. - self.add_neuron(child_nid.id, child_neuron.clone())?; + self.add_neuron(child_nid.id, child_neuron.clone(), true)?; // Do the transfer for the parent first, to avoid double spending. self.neuron_store.with_neuron_mut(id, |parent_neuron| { @@ -3124,7 +3177,7 @@ impl Governance { .build(); // `add_neuron` will verify that `child_neuron.controller` `is_self_authenticating()`, so we don't need to check it here. - self.add_neuron(child_nid.id, child_neuron)?; + self.add_neuron(child_nid.id, child_neuron, false)?; // Get the parent neuron again, but this time mutable references. self.with_neuron_mut(id, |parent_neuron| { @@ -3412,7 +3465,7 @@ impl Governance { .with_kyc_verified(parent_neuron.kyc_verified) .build(); - self.add_neuron(child_nid.id, child_neuron.clone())?; + self.add_neuron(child_nid.id, child_neuron.clone(), true)?; // Add the child neuron to the set of neurons undergoing ledger updates. let _child_lock = self.lock_neuron_for_command(child_nid.id, in_flight_command.clone())?; @@ -4172,7 +4225,7 @@ impl Governance { .with_kyc_verified(true) .build(); - self.add_neuron(nid.id, neuron) + self.add_neuron(nid.id, neuron, false) } Some(RewardMode::RewardToAccount(reward_to_account)) => { // We are not creating a neuron, just transferring funds. @@ -6170,8 +6223,9 @@ impl Governance { nid, subaccount, controller, - DissolveStateAndAge::DissolvingOrDissolved { - when_dissolved_timestamp_seconds: now, + DissolveStateAndAge::NotDissolving { + dissolve_delay_seconds: INITIAL_NEURON_DISSOLVE_DELAY, + aging_since_timestamp_seconds: now, }, now, ) @@ -6180,7 +6234,7 @@ impl Governance { .build(); // This also verifies that there are not too many neurons already. - self.add_neuron(nid.id, neuron.clone())?; + self.add_neuron(nid.id, neuron.clone(), true)?; let _neuron_lock = self.lock_neuron_for_command( nid.id, @@ -6461,6 +6515,30 @@ impl Governance { .maybe_validate(self.env.now(), &self.neuron_store); } + /// Increment neuron allowances if enough time has passed. + fn maybe_increase_neuron_allowances(&mut self) { + // We increase the allowance over the maximum per hour to account + // for natural variations in rates, but not leaving too much spare capacity. + if self.neuron_rate_limits.available_allowances < MAX_NEURON_CREATION_SPIKE { + let time_elapsed_since_last_allowance_increase = self + .env + .now() + .saturating_sub(self.neuron_rate_limits.last_allowance_increase); + + // Heartbeats should run frequently enough that the allowances increase fairly close + // to what's intended. However, some variation is acceptable. + if time_elapsed_since_last_allowance_increase + >= MINIMUM_SECONDS_BETWEEN_ALLOWANCE_INCREASE + { + self.neuron_rate_limits.available_allowances = self + .neuron_rate_limits + .available_allowances + .saturating_add(1); + self.neuron_rate_limits.last_allowance_increase = self.env.now(); + } + } + } + /// Triggers a reward distribution event if enough time has passed since /// the last one. This is intended to be called by a cron /// process. @@ -6540,6 +6618,7 @@ impl Governance { self.maybe_gc(); self.maybe_run_migrations(); self.maybe_run_validations(); + self.maybe_increase_neuron_allowances(); } fn should_update_maturity_modulation(&self) -> bool { diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs index 70f76daf86a..7e1c2feb4f6 100644 --- a/rs/nns/governance/tests/governance.rs +++ b/rs/nns/governance/tests/governance.rs @@ -22,6 +22,7 @@ use ic_crypto_sha2::Sha256; use ic_nervous_system_clients::canister_status::{CanisterStatusResultV2, CanisterStatusType}; use ic_nervous_system_common::{ cmc::CMC, + ledger, ledger::{compute_neuron_staking_subaccount_bytes, IcpLedger}, NervousSystemError, E8, ONE_DAY_SECONDS, ONE_YEAR_SECONDS, }; @@ -47,10 +48,11 @@ use ic_nns_governance::{ CREATE_SERVICE_NERVOUS_SYSTEM, CREATE_SERVICE_NERVOUS_SYSTEM_WITH_MATCHED_FUNDING, }, Environment, Governance, HeapGrowthPotential, RngError, - EXECUTE_NNS_FUNCTION_PAYLOAD_LISTING_BYTES_MAX, MAX_DISSOLVE_DELAY_SECONDS, - MAX_NEURON_AGE_FOR_AGE_BONUS, MAX_NUMBER_OF_PROPOSALS_WITH_BALLOTS, - MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS, PROPOSAL_MOTION_TEXT_BYTES_MAX, - REWARD_DISTRIBUTION_PERIOD_SECONDS, WAIT_FOR_QUIET_DEADLINE_INCREASE_SECONDS, + EXECUTE_NNS_FUNCTION_PAYLOAD_LISTING_BYTES_MAX, INITIAL_NEURON_DISSOLVE_DELAY, + MAX_DISSOLVE_DELAY_SECONDS, MAX_NEURON_AGE_FOR_AGE_BONUS, MAX_NEURON_CREATION_SPIKE, + MAX_NUMBER_OF_PROPOSALS_WITH_BALLOTS, MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS, + PROPOSAL_MOTION_TEXT_BYTES_MAX, REWARD_DISTRIBUTION_PERIOD_SECONDS, + WAIT_FOR_QUIET_DEADLINE_INCREASE_SECONDS, }, governance_proto_builder::GovernanceProtoBuilder, is_private_neuron_enforcement_enabled, @@ -123,6 +125,7 @@ use std::{ collections::{BTreeMap, HashSet, VecDeque}, convert::{TryFrom, TryInto}, iter::{self, once}, + ops::Div, path::PathBuf, sync::{Arc, Mutex}, }; @@ -4293,20 +4296,29 @@ fn governance_with_staked_neuron( assert_eq!(gov.neuron_store.len(), 1); - gov.neuron_store - .with_neuron_mut(&nid, |neuron| { - neuron.configure( - &from, - driver.now(), - &Configure { - operation: Some(Operation::IncreaseDissolveDelay(IncreaseDissolveDelay { - additional_dissolve_delay_seconds: dissolve_delay_seconds as u32, - })), - }, - ) - }) - .expect("Neuron not found") - .expect("Configure failed"); + assert!( + dissolve_delay_seconds >= INITIAL_NEURON_DISSOLVE_DELAY, + "Dissolve delay too low, as it is below the minimum threshold." + ); + + let increase_by = dissolve_delay_seconds.saturating_sub(INITIAL_NEURON_DISSOLVE_DELAY); + + if increase_by > 0 { + gov.neuron_store + .with_neuron_mut(&nid, |neuron| { + neuron.configure( + &from, + driver.now(), + &Configure { + operation: Some(Operation::IncreaseDissolveDelay(IncreaseDissolveDelay { + additional_dissolve_delay_seconds: increase_by as u32, + })), + }, + ) + }) + .expect("Neuron not found") + .expect("Configure failed"); + } (driver, gov, nid, to_subaccount) } @@ -4616,6 +4628,28 @@ fn governance_with_staked_unclaimed_neuron( (driver, gov, to_subaccount) } +fn claim_neuron_by_memo( + gov: &mut Governance, + owner: PrincipalId, + memo: u64, +) -> ManageNeuronResponse { + gov.manage_neuron( + &owner, + &ManageNeuron { + neuron_id_or_subaccount: None, + id: None, + command: Some(Command::ClaimOrRefresh(ClaimOrRefresh { + by: Some(By::MemoAndController(MemoAndController { + memo, + controller: None, + })), + })), + }, + ) + .now_or_never() + .unwrap() +} + /// Tests that the controller of a neuron (the principal whose hash was used /// to build the subaccount) can claim a neuron just with the memo. #[test] @@ -4625,22 +4659,7 @@ fn test_claim_neuron_by_memo_only() { let stake = Tokens::from_tokens(10u64).unwrap(); let (_, mut gov, _) = governance_with_staked_unclaimed_neuron(&owner, memo, stake); - let manage_neuron_response = gov - .manage_neuron( - &owner, - &ManageNeuron { - neuron_id_or_subaccount: None, - id: None, - command: Some(Command::ClaimOrRefresh(ClaimOrRefresh { - by: Some(By::MemoAndController(MemoAndController { - memo, - controller: None, - })), - })), - }, - ) - .now_or_never() - .unwrap(); + let manage_neuron_response = claim_neuron_by_memo(&mut gov, owner, memo); let nid = match manage_neuron_response.command.unwrap() { CommandResponse::ClaimOrRefresh(response) => response.refreshed_neuron_id, @@ -4665,22 +4684,7 @@ fn test_claim_neuron_without_minimum_stake_fails() { let stake = Tokens::from_e8s(50000000u64); let (_, mut gov, _) = governance_with_staked_unclaimed_neuron(&owner, memo, stake); - let manage_neuron_response = gov - .manage_neuron( - &owner, - &ManageNeuron { - neuron_id_or_subaccount: None, - id: None, - command: Some(Command::ClaimOrRefresh(ClaimOrRefresh { - by: Some(By::MemoAndController(MemoAndController { - memo, - controller: None, - })), - })), - }, - ) - .now_or_never() - .unwrap(); + let manage_neuron_response = claim_neuron_by_memo(&mut gov, owner, memo); match manage_neuron_response.command.unwrap() { CommandResponse::Error(error) => { @@ -4696,7 +4700,7 @@ fn test_claim_neuron_without_minimum_stake_fails() { tla::check_traces(); } -fn claim_neuron_by_memo_and_controller(owner: PrincipalId, caller: PrincipalId) { +fn do_test_claim_neuron_by_memo_and_controller(owner: PrincipalId, caller: PrincipalId) { let memo = 1234u64; let stake = Tokens::from_tokens(10u64).unwrap(); let (_, mut gov, _) = @@ -4740,7 +4744,7 @@ fn claim_neuron_by_memo_and_controller(owner: PrincipalId, caller: PrincipalId) #[test] fn test_claim_neuron_memo_and_controller_by_controller() { let owner = *TEST_NEURON_1_OWNER_PRINCIPAL; - claim_neuron_by_memo_and_controller(owner, owner); + do_test_claim_neuron_by_memo_and_controller(owner, owner); } /// Tests that a non-controller can claim a neuron for the controller (the @@ -4749,7 +4753,7 @@ fn test_claim_neuron_memo_and_controller_by_controller() { fn test_claim_neuron_memo_and_controller_by_proxy() { let owner = *TEST_NEURON_1_OWNER_PRINCIPAL; let caller = *TEST_NEURON_2_OWNER_PRINCIPAL; - claim_neuron_by_memo_and_controller(owner, caller); + do_test_claim_neuron_by_memo_and_controller(owner, caller); } /// Tests that a non-controller can't claim a neuron for themselves. @@ -4790,8 +4794,13 @@ fn test_non_controller_cant_claim_neuron_for_themselves() { fn refresh_neuron_by_memo(owner: PrincipalId, caller: PrincipalId) { let stake = Tokens::from_tokens(10u64).unwrap(); let memo = Memo(1234u64); - let (mut driver, mut gov, nid, subaccount) = - governance_with_staked_neuron(1, stake.get_e8s(), 0, owner, memo.0); + let (mut driver, mut gov, nid, subaccount) = governance_with_staked_neuron( + INITIAL_NEURON_DISSOLVE_DELAY, + stake.get_e8s(), + 0, + owner, + memo.0, + ); let neuron = gov.neuron_store.with_neuron(&nid, |n| n.clone()).unwrap(); assert_eq!(neuron.cached_neuron_stake_e8s, stake.get_e8s()); @@ -4862,8 +4871,13 @@ fn refresh_neuron_by_id_or_subaccount( ) { let stake = Tokens::from_tokens(10u64).unwrap(); let memo = Memo(1234u64); - let (mut driver, mut gov, nid, subaccount) = - governance_with_staked_neuron(1, stake.get_e8s(), 0, owner, memo.0); + let (mut driver, mut gov, nid, subaccount) = governance_with_staked_neuron( + INITIAL_NEURON_DISSOLVE_DELAY, + stake.get_e8s(), + 0, + owner, + memo.0, + ); let neuron = gov.neuron_store.with_neuron(&nid, |n| n.clone()).unwrap(); assert_eq!(neuron.cached_neuron_stake_e8s, stake.get_e8s()); @@ -5011,6 +5025,132 @@ fn test_claim_or_refresh_neuron_does_not_overflow() { tla::check_traces(); } +#[test] +fn test_rate_limiting_neuron_creation() { + let current_peak = MAX_NEURON_CREATION_SPIKE; + + // Some neurons with maturity and stake so we can spawn and split + let staked_neurons = (1..=(current_peak - 1)) + .map(|i| { + let controller = PrincipalId::new_user_test_id(i); + let nonce = 1234u64; + Neuron { + id: Some(NeuronId::from_u64(i)), + account: ledger::compute_neuron_staking_subaccount(controller, nonce).into(), + controller: Some(controller), + cached_neuron_stake_e8s: 10 * E8, + neuron_fees_e8s: 0, + created_timestamp_seconds: 0, + aging_since_timestamp_seconds: 0, + maturity_e8s_equivalent: 10 * E8, + staked_maturity_e8s_equivalent: Some(10 * E8), + dissolve_state: Some(DissolveState::DissolveDelaySeconds( + MIN_DISSOLVE_DELAY_FOR_VOTE_ELIGIBILITY_SECONDS, + )), + ..Default::default() + } + }) + .collect::>(); + + let (driver, mut gov) = governance_with_neurons(&staked_neurons); + + for i in 1..=(current_peak - 1) { + let neuron_id = NeuronId::from_u64(i); + // Split + let controller = gov + .neuron_store + .with_neuron(&neuron_id, |neuron| neuron.controller()) + .unwrap(); + gov.split_neuron(&neuron_id, &controller, &Split { amount_e8s: 5 * E8 }) + .now_or_never() + .unwrap() + .unwrap(); + + // spawn should not be rate limited + let controller = gov + .neuron_store + .with_neuron(&neuron_id, |neuron| neuron.controller()) + .unwrap(); + gov.spawn_neuron( + &neuron_id, + &controller, + &Spawn { + new_controller: Some(PrincipalId::new_user_test_id(i * 2)), + nonce: None, + percentage_to_spawn: Some(40), + }, + ) + .expect( + "We unexpectedly hit the rate limit, which means spawn is not exempt from the limits. \ + This is an error.", + ); + } + + // Claim our first neuron... should affect limits + let controller = PrincipalId::new_user_test_id(100); + let nonce = 0; + let amount_e8s = 10 * E8; + let new_neuron_subaccount = ledger::compute_neuron_staking_subaccount(controller, nonce); + let driver = driver.with_ledger_accounts(vec![fake::FakeAccount { + id: AccountIdentifier::new( + ic_base_types::PrincipalId::from(GOVERNANCE_CANISTER_ID), + Some(new_neuron_subaccount), + ), + amount_e8s, + }]); + + let result: ManageNeuronResponse = claim_neuron_by_memo(&mut gov, controller, nonce); + result.panic_if_error("Could not claim neuron!"); + + // Claim another neuron, which should then fail + let controller = PrincipalId::new_user_test_id(101); + let nonce = 0; + let new_neuron_subaccount = ledger::compute_neuron_staking_subaccount(controller, nonce); + let mut driver = driver.with_ledger_accounts(vec![fake::FakeAccount { + id: AccountIdentifier::new( + ic_base_types::PrincipalId::from(GOVERNANCE_CANISTER_ID), + Some(new_neuron_subaccount), + ), + amount_e8s, + }]); + + let result: ManageNeuronResponse = claim_neuron_by_memo(&mut gov, controller, nonce); + + match result.command { + Some(CommandResponse::Error(e)) => { + assert_eq!( + e, + GovernanceError::new_with_message( + ErrorType::Unavailable, + "Reached maximum number of neurons that can be created in this hour. \ + Please wait and try again later." + ) + ) + } + r => panic!("We did not get a rate limited response!, {r:?}"), + } + + // Advance time by just enough to reset the rate limit + driver.advance_time_by(3600.div(current_peak)); + gov.run_periodic_tasks().now_or_never().unwrap(); + + // Now we should be able to again claim a neuron. + let controller = PrincipalId::new_user_test_id(100); + let nonce = 0; + let amount_e8s = 10 * E8; + let new_neuron_subaccount = ledger::compute_neuron_staking_subaccount(controller, nonce); + driver.with_ledger_accounts(vec![fake::FakeAccount { + id: AccountIdentifier::new( + ic_base_types::PrincipalId::from(GOVERNANCE_CANISTER_ID), + Some(new_neuron_subaccount), + ), + amount_e8s, + }]); + + let result: ManageNeuronResponse = claim_neuron_by_memo(&mut gov, controller, nonce); + result.panic_if_error("Could not claim neuron!"); +} + #[test] fn test_cant_disburse_without_paying_fees() { let (driver, mut gov, neuron) = create_mature_neuron(true); @@ -6285,7 +6425,7 @@ fn governance_with_neurons(neurons: &[Neuron]) -> (fake::FakeDriver, Governance) driver.get_fake_ledger(), driver.get_fake_cmc(), ); - assert_eq!(gov.neuron_store.len(), 3); + assert_eq!(gov.neuron_store.len(), neurons.len()); (driver, gov) } diff --git a/rs/nns/integration_tests/src/governance_neurons.rs b/rs/nns/integration_tests/src/governance_neurons.rs index 1fa41d0fde3..461b2cb81b5 100644 --- a/rs/nns/integration_tests/src/governance_neurons.rs +++ b/rs/nns/integration_tests/src/governance_neurons.rs @@ -10,6 +10,7 @@ use ic_nervous_system_common_test_keys::{ TEST_NEURON_2_OWNER_PRINCIPAL, }; use ic_nns_common::pb::v1::NeuronId as NeuronIdProto; +use ic_nns_governance::governance::INITIAL_NEURON_DISSOLVE_DELAY; use ic_nns_governance_api::pb::v1::{ governance_error::ErrorType, manage_neuron::{Command, Merge, NeuronIdOrSubaccount, Spawn}, @@ -26,12 +27,12 @@ use ic_nns_test_utils::{ list_neurons, list_neurons_by_principal, nns_add_hot_key, nns_claim_or_refresh_neuron, nns_disburse_neuron, nns_governance_get_full_neuron, nns_governance_get_neuron_info, nns_join_community_fund, nns_leave_community_fund, nns_remove_hot_key, - nns_send_icp_to_claim_or_refresh_neuron, setup_nns_canisters, + nns_send_icp_to_claim_or_refresh_neuron, nns_start_dissolving, setup_nns_canisters, state_machine_builder_for_nns_tests, }, }; use icp_ledger::{tokens_from_proto, AccountBalanceArgs, AccountIdentifier, Tokens}; -use std::time::{SystemTime, UNIX_EPOCH}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; #[test] fn test_merge_neurons_and_simulate_merge_neurons() { @@ -453,19 +454,25 @@ fn test_claim_neuron() { assert!(created_timestamp_seconds > 0); assert_eq!( full_neuron.dissolve_state, - Some(DissolveState::WhenDissolvedTimestampSeconds( - created_timestamp_seconds + Some(DissolveState::DissolveDelaySeconds( + INITIAL_NEURON_DISSOLVE_DELAY )) ); - assert_eq!(full_neuron.aging_since_timestamp_seconds, u64::MAX); + assert_eq!( + full_neuron.aging_since_timestamp_seconds, + created_timestamp_seconds + ); assert_eq!(full_neuron.cached_neuron_stake_e8s, 1_000_000_000); // Step 3.2: Inspect the claimed neuron as neuron info. let neuron_info = nns_governance_get_neuron_info(&state_machine, PrincipalId::new_anonymous(), neuron_id.id) .unwrap(); - assert_eq!(neuron_info.state, NeuronState::Dissolved as i32); - assert_eq!(neuron_info.dissolve_delay_seconds, 0); + assert_eq!(neuron_info.state, NeuronState::NotDissolving as i32); + assert_eq!( + neuron_info.dissolve_delay_seconds, + INITIAL_NEURON_DISSOLVE_DELAY + ); assert_eq!(neuron_info.age_seconds, 0); assert_eq!(neuron_info.stake_e8s, 1_000_000_000); } @@ -498,6 +505,7 @@ fn test_list_neurons() { 1, ); let neuron_id_1 = nns_claim_or_refresh_neuron(&state_machine, principal_1, 1); + nns_send_icp_to_claim_or_refresh_neuron( &state_machine, principal_1, @@ -505,6 +513,7 @@ fn test_list_neurons() { 2, ); let neuron_id_2 = nns_claim_or_refresh_neuron(&state_machine, principal_1, 2); + nns_send_icp_to_claim_or_refresh_neuron( &state_machine, principal_2, @@ -514,7 +523,19 @@ fn test_list_neurons() { let neuron_id_3 = nns_claim_or_refresh_neuron(&state_machine, principal_2, 3); // Step 1.3: disburse neuron 2 so that it's empty. - nns_disburse_neuron(&state_machine, principal_1, neuron_id_2, 200_000_000, None); + nns_start_dissolving(&state_machine, principal_1, neuron_id_2) + .expect("Failed to start dissolving neuron"); + state_machine.advance_time(Duration::from_secs(INITIAL_NEURON_DISSOLVE_DELAY + 1)); + state_machine.tick(); + let disburse_result = + nns_disburse_neuron(&state_machine, principal_1, neuron_id_2, 200_000_000, None); + + match disburse_result { + ManageNeuronResponse { + command: Some(manage_neuron_response::Command::Disburse(_)), + } => (), + disburse_result => panic!("Failed to disburse neuron: {:#?}", disburse_result), + } // Step 2: test listing neurons by ids with an anonymous principal. let list_neurons_response = list_neurons( diff --git a/rs/nns/integration_tests/src/ledger.rs b/rs/nns/integration_tests/src/ledger.rs index 77e60c8753a..e03c19a9225 100644 --- a/rs/nns/integration_tests/src/ledger.rs +++ b/rs/nns/integration_tests/src/ledger.rs @@ -1,4 +1,5 @@ use assert_matches::assert_matches; +use canister_test::Runtime; use dfn_candid::candid_one; use dfn_protobuf::protobuf; use ic_base_types::PrincipalId; @@ -7,6 +8,7 @@ use ic_nervous_system_common::ledger; use ic_nervous_system_common_test_keys::TEST_USER1_KEYPAIR; use ic_nns_common::pb::v1::NeuronId as NeuronIdProto; use ic_nns_constants::{ALL_NNS_CANISTER_IDS, GENESIS_TOKEN_CANISTER_ID, GOVERNANCE_CANISTER_ID}; +use ic_nns_governance::governance::INITIAL_NEURON_DISSOLVE_DELAY; use ic_nns_governance_api::pb::v1::{ claim_or_refresh_neuron_from_account_response::Result as ClaimOrRefreshResult, governance_error::ErrorType, @@ -21,12 +23,13 @@ use ic_nns_governance_api::pb::v1::{ use ic_nns_test_utils::{ common::NnsInitPayloadsBuilder, itest_helpers::{state_machine_test_on_nns_subnet, NnsCanisters}, + state_test_helpers::nns_start_dissolving, }; use icp_ledger::{ tokens_from_proto, AccountBalanceArgs, AccountIdentifier, BlockIndex, LedgerCanisterInitPayload, Memo, SendArgs, Tokens, DEFAULT_TRANSFER_FEE, }; -use std::collections::HashMap; +use std::{collections::HashMap, time::Duration}; // This tests the whole neuron lifecycle in integration with the ledger. Namely // tests that the neuron can be staked from a ledger account. That the neuron @@ -35,6 +38,10 @@ use std::collections::HashMap; fn test_stake_and_disburse_neuron_with_notification() { state_machine_test_on_nns_subnet(|runtime| { async move { + let state_machine = match runtime { + Runtime::StateMachine(ref state_machine) => state_machine, + _ => panic!("This test must run on a state machine."), + }; // Initialize the ledger with an account for a user. let user = Sender::from_keypair(&TEST_USER1_KEYPAIR); @@ -145,6 +152,12 @@ fn test_stake_and_disburse_neuron_with_notification() { alloc ); + nns_start_dissolving(state_machine, user.get_principal_id(), neuron_id) + .expect("Failed to start dissolving neuron"); + + state_machine.advance_time(Duration::from_secs(INITIAL_NEURON_DISSOLVE_DELAY + 1)); + state_machine.tick(); + // Disburse the neuron. let result: ManageNeuronResponse = nns_canisters .governance @@ -202,6 +215,10 @@ fn test_stake_and_disburse_neuron_with_notification() { fn test_stake_and_disburse_neuron_with_account() { state_machine_test_on_nns_subnet(|runtime| { async move { + let state_machine = match runtime { + Runtime::StateMachine(ref state_machine) => state_machine, + _ => panic!("This test must run on a state machine."), + }; // Initialize the ledger with an account for a user. let user = Sender::from_keypair(&TEST_USER1_KEYPAIR); @@ -336,7 +353,11 @@ fn test_stake_and_disburse_neuron_with_account() { "Neuron: {:?}", full_neuron ); + nns_start_dissolving(state_machine, user.get_principal_id(), neuron_id) + .expect("Failed to start dissolving neuron"); + state_machine.advance_time(Duration::from_secs(INITIAL_NEURON_DISSOLVE_DELAY + 1)); + state_machine.tick(); // Disburse the neuron. let result: ManageNeuronResponse = nns_canisters .governance diff --git a/rs/nns/test_utils/src/state_test_helpers.rs b/rs/nns/test_utils/src/state_test_helpers.rs index cc54591258d..979a5a5076a 100644 --- a/rs/nns/test_utils/src/state_test_helpers.rs +++ b/rs/nns/test_utils/src/state_test_helpers.rs @@ -1035,6 +1035,22 @@ pub fn nns_increase_dissolve_delay( ) } +pub fn nns_start_dissolving( + state_machine: &StateMachine, + sender: PrincipalId, + neuron_id: NeuronId, +) -> Result< + nns_governance_pb::manage_neuron_response::ConfigureResponse, + nns_governance_pb::GovernanceError, +> { + nns_configure_neuron( + state_machine, + sender, + neuron_id, + Operation::StartDissolving(nns_governance_pb::manage_neuron::StartDissolving {}), + ) +} + #[must_use] pub fn nns_propose_upgrade_nns_canister( state_machine: &StateMachine, diff --git a/rs/rosetta-api/icp/tests/system_tests/test_cases/neuron_management.rs b/rs/rosetta-api/icp/tests/system_tests/test_cases/neuron_management.rs index 61761c68685..9a70adb7548 100644 --- a/rs/rosetta-api/icp/tests/system_tests/test_cases/neuron_management.rs +++ b/rs/rosetta-api/icp/tests/system_tests/test_cases/neuron_management.rs @@ -1,16 +1,17 @@ -use crate::common::system_test_environment::RosettaTestingEnvironment; -use crate::common::utils::{get_test_agent, list_neurons, test_identity}; -use ic_agent::identity::BasicIdentity; -use ic_agent::Identity; -use ic_icp_rosetta_client::RosettaCreateNeuronArgs; -use ic_icp_rosetta_client::RosettaSetNeuronDissolveDelayArgs; +use crate::common::{ + system_test_environment::RosettaTestingEnvironment, + utils::{get_test_agent, list_neurons, test_identity}, +}; +use ic_agent::{identity::BasicIdentity, Identity}; +use ic_icp_rosetta_client::{RosettaCreateNeuronArgs, RosettaSetNeuronDissolveDelayArgs}; use ic_nns_governance::pb::v1::neuron::DissolveState; use ic_types::PrincipalId; use icp_ledger::AccountIdentifier; use lazy_static::lazy_static; -use std::sync::Arc; -use std::time::SystemTime; -use std::time::UNIX_EPOCH; +use std::{ + sync::Arc, + time::{SystemTime, UNIX_EPOCH}, +}; use tokio::runtime::Runtime; lazy_static! { @@ -104,8 +105,8 @@ fn test_set_neuron_dissolve_delay_timestamp() { let neuron = list_neurons(&agent).await.full_neurons[0].to_owned(); let dissolve_delay_timestamp = match neuron.dissolve_state.unwrap() { - // When a neuron is created its dissolve delay timestamp is set to now which corresponds to the state DISSOLVED - DissolveState::WhenDissolvedTimestampSeconds(dissolve_delay_timestamp) => { + // When a neuron is created it has a one week dissolve delay + DissolveState::DissolveDelaySeconds(dissolve_delay_timestamp) => { dissolve_delay_timestamp } k => panic!( @@ -114,23 +115,15 @@ fn test_set_neuron_dissolve_delay_timestamp() { ), }; - // We can't know the exact timestamp of the dissolve delay, but we can assert that it is in the past or now - assert!( - dissolve_delay_timestamp - <= env - .pocket_ic - .get_time() - .await - .duration_since(UNIX_EPOCH) - .unwrap() - .as_secs() - ); - let additional_dissolve_delay = 1000; + let one_week = 24 * 60 * 60 * 7; + assert_eq!(dissolve_delay_timestamp, one_week); + + let new_dissolve_delay = dissolve_delay_timestamp + 1000; let new_dissolve_delay_timestamp = SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() .as_secs() - + additional_dissolve_delay; + + new_dissolve_delay; // To be able to set the dissolve delay timestamp we need to set the state machine to live again env.rosetta_client @@ -159,7 +152,9 @@ fn test_set_neuron_dissolve_delay_timestamp() { // The Dissolve Delay Timestamp should be updated // Since the state machine is live we do not know exactly how much time will be left at the time of calling the governance canister. // It should be between dissolve_delay_timestamp and dissolve_delay_timestamp - X seconds depending on how long it takes to call the governance canister - assert!(dissolve_delay_timestamp <= additional_dissolve_delay); + assert!(dissolve_delay_timestamp <= new_dissolve_delay); + assert!(dissolve_delay_timestamp > new_dissolve_delay - 10); + assert!(dissolve_delay_timestamp > 0); }); } diff --git a/rs/tests/src/rosetta_tests/tests/neuron_staking.rs b/rs/tests/src/rosetta_tests/tests/neuron_staking.rs index 01e3a1748bb..75852c685fd 100644 --- a/rs/tests/src/rosetta_tests/tests/neuron_staking.rs +++ b/rs/tests/src/rosetta_tests/tests/neuron_staking.rs @@ -1,30 +1,31 @@ -use crate::rosetta_tests::ledger_client::LedgerClient; -use crate::rosetta_tests::lib::{ - assert_canister_error, check_balance, create_ledger_client, do_multiple_txn, make_user_ed25519, - one_day_from_now_nanos, raw_construction, sign, to_public_key, +use crate::rosetta_tests::{ + ledger_client::LedgerClient, + lib::{ + assert_canister_error, check_balance, create_ledger_client, do_multiple_txn, + make_user_ed25519, one_day_from_now_nanos, raw_construction, sign, to_public_key, + }, + rosetta_client::RosettaApiClient, + setup::setup, + test_neurons::TestNeurons, }; -use crate::rosetta_tests::rosetta_client::RosettaApiClient; -use crate::rosetta_tests::setup::setup; -use crate::rosetta_tests::test_neurons::TestNeurons; use assert_json_diff::{assert_json_eq, assert_json_include}; -use ic_ledger_core::tokens::{CheckedAdd, CheckedSub}; -use ic_ledger_core::Tokens; +use ic_ledger_core::{ + tokens::{CheckedAdd, CheckedSub}, + Tokens, +}; use ic_nns_constants::GOVERNANCE_CANISTER_ID; -use ic_rosetta_api::convert::{from_model_account_identifier, neuron_account_from_public_key}; -use ic_rosetta_api::models::seconds::Seconds; -use ic_rosetta_api::models::NeuronInfoResponse; -use ic_rosetta_api::models::NeuronState; -use ic_rosetta_api::request::Request; -use ic_rosetta_api::request_types::{SetDissolveTimestamp, Stake, StartDissolve, StopDissolve}; +use ic_rosetta_api::{ + convert::{from_model_account_identifier, neuron_account_from_public_key}, + models::{seconds::Seconds, NeuronInfoResponse}, + request::Request, + request_types::{SetDissolveTimestamp, Stake, StartDissolve, StopDissolve}, +}; use ic_rosetta_test_utils::{EdKeypair, RequestInfo}; -use ic_system_test_driver::driver::test_env::TestEnv; -use ic_system_test_driver::util::block_on; +use ic_system_test_driver::{driver::test_env::TestEnv, util::block_on}; use icp_ledger::{AccountIdentifier, Operation, DEFAULT_TRANSFER_FEE}; use serde_json::{json, Value}; use slog::info; -use std::collections::HashMap; -use std::sync::Arc; -use std::time::UNIX_EPOCH; +use std::{collections::HashMap, sync::Arc, time::UNIX_EPOCH}; const PORT: u32 = 8103; const VM_NAME: &str = "neuron-staking"; @@ -126,7 +127,7 @@ async fn test_staking(client: &RosettaApiClient) -> (AccountIdentifier, Arc (AccountIdentifier, Arc (AccountIdentifier, Arc (AccountIdentifier, Arc (AccountIdentifier, Arc< .find_map(|r| r.get("metadata").and_then(|r| r.get("block_index"))); assert!(last_block_idx.is_some()); - let neuron_info: NeuronInfoResponse = client + let _neuron_info: NeuronInfoResponse = client .account_balance_neuron(neuron_account, neuron_id, None, false) .await .unwrap() @@ -557,7 +561,8 @@ async fn test_staking_raw(client: &RosettaApiClient) -> (AccountIdentifier, Arc< .metadata .try_into() .unwrap(); - assert_eq!(neuron_info.state, NeuronState::Dissolved); + // TODO(NNS1-3390) after https://github.com/dfinity/ic/pull/1982 is deployed, uncomment this line + // assert_eq!(neuron_info.state, NeuronState::NotDissolving); // Return staked account. (dst_acc, dst_acc_kp)