diff --git a/rs/nervous_system/integration_tests/src/pocket_ic_helpers.rs b/rs/nervous_system/integration_tests/src/pocket_ic_helpers.rs index 41b16553571..8229f576993 100644 --- a/rs/nervous_system/integration_tests/src/pocket_ic_helpers.rs +++ b/rs/nervous_system/integration_tests/src/pocket_ic_helpers.rs @@ -1,12 +1,13 @@ use candid::{Decode, Encode, Nat, Principal}; use canister_test::Wasm; -use futures::stream; -use futures::StreamExt; +use futures::{stream, StreamExt}; use ic_base_types::{CanisterId, PrincipalId, SubnetId}; use ic_ledger_core::Tokens; -use ic_nervous_system_agent::pocketic_impl::{PocketIcAgent, PocketIcCallError}; -use ic_nervous_system_agent::sns::Sns; -use ic_nervous_system_agent::CallCanisters; +use ic_nervous_system_agent::{ + pocketic_impl::{PocketIcAgent, PocketIcCallError}, + sns::Sns, + CallCanisters, +}; use ic_nervous_system_common::{E8, ONE_DAY_SECONDS}; use ic_nervous_system_common_test_keys::{TEST_NEURON_1_ID, TEST_NEURON_1_OWNER_PRINCIPAL}; use ic_nns_common::pb::v1::{NeuronId, ProposalId}; @@ -60,8 +61,7 @@ use icrc_ledger_types::icrc1::{ account::Account, transfer::{TransferArg, TransferError}, }; -use itertools::EitherOrBoth; -use itertools::Itertools; +use itertools::{EitherOrBoth, Itertools}; use maplit::btreemap; use pocket_ic::{ management_canister::CanisterSettings, nonblocking::PocketIc, ErrorCode, PocketIcBuilder, @@ -69,8 +69,7 @@ use pocket_ic::{ }; use prost::Message; use rust_decimal::prelude::ToPrimitive; -use std::ops::Range; -use std::{collections::BTreeMap, fmt::Write, time::Duration}; +use std::{collections::BTreeMap, fmt::Write, ops::Range, time::Duration}; pub const STARTING_CYCLES_PER_CANISTER: u128 = 2_000_000_000_000_000; @@ -833,6 +832,8 @@ pub mod nns { include_neurons_readable_by_caller: true, include_empty_neurons_readable_by_caller: None, include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None }) .unwrap(), ) @@ -1366,8 +1367,9 @@ pub mod sns { use assert_matches::assert_matches; use ic_crypto_sha2::Sha256; use ic_nervous_system_agent::sns::governance::{GovernanceCanister, SubmitProposalError}; - use ic_sns_governance::governance::UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS; - use ic_sns_governance::pb::v1::get_neuron_response; + use ic_sns_governance::{ + governance::UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS, pb::v1::get_neuron_response, + }; use pocket_ic::ErrorCode; use sns_pb::UpgradeSnsControlledCanister; diff --git a/rs/nns/governance/api/src/ic_nns_governance.pb.v1.rs b/rs/nns/governance/api/src/ic_nns_governance.pb.v1.rs index ee7f9705904..7102f5ab5fe 100644 --- a/rs/nns/governance/api/src/ic_nns_governance.pb.v1.rs +++ b/rs/nns/governance/api/src/ic_nns_governance.pb.v1.rs @@ -3320,6 +3320,20 @@ pub struct ListProposalInfoResponse { /// of neurons listed in `neuron_ids` and, if `caller_neurons` is true, /// the list of neuron IDs of neurons for which the caller is the /// controller or one of the hot keys. +/// +/// Paging is available if the result set is larger than `MAX_LIST_NEURONS_RESULTS`, +/// which is currently 500 neurons. If you are unsure of the number of results in a set, +/// you can use the `total_pages_available` field in the response to determine how many +/// additional pages need to be queried. It will be based on your `page_size` parameter. +/// When paging through results, it is good to keep in mind that newly inserted neurons +/// could be missed if they are inserted between calls to pages, and this could result in missing +/// a neuron in the combined responses. +/// +/// If a user provides neuron_ids that do not exist in the request, there is no guarantee that +/// each page will contain the exactly the page size, even if it is not the final request. This is +/// because neurons are retrieved by their neuron_id, and no additional checks are made on the +/// validity of the neuron_ids provided by the user before deciding which sets of neuron_ids +/// will be returned in the current page. #[derive(candid::CandidType, candid::Deserialize, serde::Serialize, comparable::Comparable)] #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] @@ -3350,6 +3364,15 @@ pub struct ListNeurons { /// existing (unmigrated) callers. #[prost(bool, optional, tag = "4")] pub include_public_neurons_in_full_neurons: Option, + /// If this is set, we return the batch of neurons at a given page, using the `page_size` to + /// determine how many neurons are returned in each page. + #[prost(uint64, optional, tag = "5")] + pub page_number: Option, + /// If this is set, we use the page limit provided to determine how large pages will be. + /// This cannot be greater than MAX_LIST_NEURONS_RESULTS, which is set to 500. + /// If not set, this defaults to MAX_LIST_NEURONS_RESULTS. + #[prost(uint64, optional, tag = "6")] + pub page_size: Option, } /// A response to a `ListNeurons` request. /// @@ -3368,6 +3391,10 @@ pub struct ListNeuronsResponse { /// `ManageNeuron` topic). #[prost(message, repeated, tag = "2")] pub full_neurons: Vec, + /// This is returned to tell the caller how many pages of results are available to query. + /// If there are fewer than the page_size neurons, this will equal 1. + #[prost(uint64, optional, tag = "3")] + pub total_pages_available: Option, } /// A response to "ListKnownNeurons" #[derive(candid::CandidType, candid::Deserialize, serde::Serialize, comparable::Comparable)] diff --git a/rs/nns/governance/canbench/canbench_results.yml b/rs/nns/governance/canbench/canbench_results.yml index 7fc2d61ada9..905d055297d 100644 --- a/rs/nns/governance/canbench/canbench_results.yml +++ b/rs/nns/governance/canbench/canbench_results.yml @@ -1,55 +1,55 @@ benches: add_neuron_active_maximum: total: - instructions: 42752805 + instructions: 42749561 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_active_typical: total: - instructions: 2170667 + instructions: 2170531 heap_increase: 0 stable_memory_increase: 0 scopes: {} add_neuron_inactive_maximum: total: - instructions: 112624643 + instructions: 112621399 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_inactive_typical: total: - instructions: 8497304 + instructions: 8497168 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_all_heap: total: - instructions: 35002536 + instructions: 34977262 heap_increase: 0 stable_memory_increase: 128 scopes: {} cascading_vote_heap_neurons_stable_index: total: - instructions: 61137575 + instructions: 61112301 heap_increase: 0 stable_memory_increase: 128 scopes: {} cascading_vote_stable_everything: total: - instructions: 188621982 + instructions: 188615092 heap_increase: 0 stable_memory_increase: 128 scopes: {} cascading_vote_stable_neurons_with_heap_index: total: - instructions: 162353547 + instructions: 162346657 heap_increase: 0 stable_memory_increase: 128 scopes: {} centralized_following_all_stable: total: - instructions: 78268135 + instructions: 78266673 heap_increase: 0 stable_memory_increase: 128 scopes: {} @@ -61,7 +61,7 @@ benches: scopes: {} draw_maturity_from_neurons_fund_heap: total: - instructions: 7598030 + instructions: 7584430 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -85,7 +85,7 @@ benches: scopes: {} list_neurons_heap: total: - instructions: 4763239 + instructions: 4802543 heap_increase: 9 stable_memory_increase: 0 scopes: {} @@ -103,13 +103,13 @@ benches: scopes: {} list_neurons_stable: total: - instructions: 113374022 + instructions: 113411672 heap_increase: 5 stable_memory_increase: 0 scopes: {} list_proposals: total: - instructions: 168095717 + instructions: 168095740 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -127,13 +127,13 @@ benches: scopes: {} neuron_data_validation_heap: total: - instructions: 406864991 + instructions: 406848584 heap_increase: 0 stable_memory_increase: 0 scopes: {} neuron_data_validation_stable: total: - instructions: 362661286 + instructions: 362648972 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -151,13 +151,13 @@ benches: scopes: {} range_neurons_performance: total: - instructions: 56448740 + instructions: 56447340 heap_increase: 0 stable_memory_increase: 0 scopes: {} single_vote_all_stable: total: - instructions: 2805838 + instructions: 2803168 heap_increase: 0 stable_memory_increase: 128 scopes: {} diff --git a/rs/nns/governance/canister/canister.rs b/rs/nns/governance/canister/canister.rs index 2f54dda2027..db92ba3acf7 100644 --- a/rs/nns/governance/canister/canister.rs +++ b/rs/nns/governance/canister/canister.rs @@ -888,7 +888,10 @@ fn get_latest_reward_event() -> RewardEvent { #[query] fn get_neuron_ids() -> Vec { debug_log("get_neuron_ids"); - let votable = governance().get_neuron_ids_by_principal(&caller()); + let votable = governance() + .get_neuron_ids_by_principal(&caller()) + .into_iter() + .collect(); governance() .get_managed_neuron_ids_for(votable) diff --git a/rs/nns/governance/canister/governance.did b/rs/nns/governance/canister/governance.did index ea1acbcdec5..6b0b5a3f7ed 100644 --- a/rs/nns/governance/canister/governance.did +++ b/rs/nns/governance/canister/governance.did @@ -439,11 +439,14 @@ type ListNeurons = record { neuron_ids : vec nat64; include_empty_neurons_readable_by_caller : opt bool; include_neurons_readable_by_caller : bool; + page_number: opt nat64; + page_size: opt nat64; }; type ListNeuronsResponse = record { neuron_infos : vec record { nat64; NeuronInfo }; full_neurons : vec Neuron; + total_pages_available: opt nat64; }; type ListNodeProviderRewardsRequest = record { diff --git a/rs/nns/governance/canister/governance_test.did b/rs/nns/governance/canister/governance_test.did index 691764b9a11..4a46fb58f15 100644 --- a/rs/nns/governance/canister/governance_test.did +++ b/rs/nns/governance/canister/governance_test.did @@ -438,11 +438,14 @@ type ListNeurons = record { neuron_ids : vec nat64; include_empty_neurons_readable_by_caller : opt bool; include_neurons_readable_by_caller : bool; + page_number: opt nat64; + page_size: opt nat64; }; type ListNeuronsResponse = record { neuron_infos : vec record { nat64; NeuronInfo }; full_neurons : vec Neuron; + total_pages_available: opt nat64; }; type ListNodeProviderRewardsRequest = record { diff --git a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto index 7db94271fc0..2ca1d2dff1c 100644 --- a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto +++ b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto @@ -2635,6 +2635,20 @@ message ListProposalInfoResponse { // of neurons listed in `neuron_ids` and, if `caller_neurons` is true, // the list of neuron IDs of neurons for which the caller is the // controller or one of the hot keys. +// +// Paging is available if the result set is larger than `MAX_LIST_NEURONS_RESULTS`, +// which is currently 500 neurons. If you are unsure of the number of results in a set, +// you can use the `total_pages_available` field in the response to determine how many +// additional pages need to be queried. It will be based on your `page_size` parameter. +// When paging through results, it is good to keep in mind that newly inserted neurons +// could be missed if they are inserted between calls to pages, and this could result in missing +// a neuron in the combined responses. +// +// If a user provides neuron_ids that do not exist in the request, there is no guarantee that +// each page will contain the exactly the page size, even if it is not the final request. This is +// because neurons are retrieved by their neuron_id, and no additional checks are made on the +// validity of the neuron_ids provided by the user before deciding which sets of neuron_ids +// will be returned in the current page. message ListNeurons { option (ic_base_types.pb.v1.tui_signed_message) = true; // The neurons to get information about. The "requested list" @@ -2659,6 +2673,13 @@ message ListNeurons { // since this feature was added later, it is opt in to avoid confusing // existing (unmigrated) callers. optional bool include_public_neurons_in_full_neurons = 4; + // If this is set, we return the batch of neurons at a given page, using the `page_size` to + // determine how many neurons are returned in each page. + optional uint64 page_number = 5; + // If this is set, we use the page limit provided to determine how large pages will be. + // This cannot be greater than MAX_LIST_NEURONS_RESULTS, which is set to 500. + // If not set, this defaults to MAX_LIST_NEURONS_RESULTS. + optional uint64 page_size = 6; } // A response to a `ListNeurons` request. @@ -2673,6 +2694,9 @@ message ListNeuronsResponse { // hot key, or controller or hot key of some followee on the // `ManageNeuron` topic). repeated Neuron full_neurons = 2; + // This is returned to tell the caller how many pages of results are available to query. + // If there are fewer than the page_size neurons, this will equal 1. + optional fixed64 total_pages_available = 3; } // A response to "ListKnownNeurons" diff --git a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs index bfc6f5d5de3..9a71ac990ac 100644 --- a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs +++ b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs @@ -3932,6 +3932,20 @@ pub struct ListProposalInfoResponse { /// of neurons listed in `neuron_ids` and, if `caller_neurons` is true, /// the list of neuron IDs of neurons for which the caller is the /// controller or one of the hot keys. +/// +/// Paging is available if the result set is larger than `MAX_LIST_NEURONS_RESULTS`, +/// which is currently 500 neurons. If you are unsure of the number of results in a set, +/// you can use the `total_pages_available` field in the response to determine how many +/// additional pages need to be queried. It will be based on your `page_size` parameter. +/// When paging through results, it is good to keep in mind that newly inserted neurons +/// could be missed if they are inserted between calls to pages, and this could result in missing +/// a neuron in the combined responses. +/// +/// If a user provides neuron_ids that do not exist in the request, there is no guarantee that +/// each page will contain the exactly the page size, even if it is not the final request. This is +/// because neurons are retrieved by their neuron_id, and no additional checks are made on the +/// validity of the neuron_ids provided by the user before deciding which sets of neuron_ids +/// will be returned in the current page. #[derive( candid::CandidType, candid::Deserialize, @@ -3968,6 +3982,15 @@ pub struct ListNeurons { /// existing (unmigrated) callers. #[prost(bool, optional, tag = "4")] pub include_public_neurons_in_full_neurons: ::core::option::Option, + /// If this is set, we return the batch of neurons at a given page, using the `page_size` to + /// determine how many neurons are returned in each page. + #[prost(uint64, optional, tag = "5")] + pub page_number: ::core::option::Option, + /// If this is set, we use the page limit provided to determine how large pages will be. + /// This cannot be greater than MAX_LIST_NEURONS_RESULTS, which is set to 500. + /// If not set, this defaults to MAX_LIST_NEURONS_RESULTS. + #[prost(uint64, optional, tag = "6")] + pub page_size: ::core::option::Option, } /// A response to a `ListNeurons` request. /// @@ -3992,6 +4015,10 @@ pub struct ListNeuronsResponse { /// `ManageNeuron` topic). #[prost(message, repeated, tag = "2")] pub full_neurons: ::prost::alloc::vec::Vec, + /// This is returned to tell the caller how many pages of results are available to query. + /// If there are fewer than the page_size neurons, this will equal 1. + #[prost(fixed64, optional, tag = "3")] + pub total_pages_available: ::core::option::Option, } /// A response to "ListKnownNeurons" #[derive( diff --git a/rs/nns/governance/src/governance.rs b/rs/nns/governance/src/governance.rs index 52c74392d00..84f8d92d531 100644 --- a/rs/nns/governance/src/governance.rs +++ b/rs/nns/governance/src/governance.rs @@ -117,7 +117,7 @@ use rust_decimal_macros::dec; use std::{ borrow::Cow, cmp::{max, Ordering}, - collections::{BTreeMap, HashMap, HashSet}, + collections::{BTreeMap, BTreeSet, HashMap, HashSet}, convert::{TryFrom, TryInto}, fmt, future::Future, @@ -143,8 +143,6 @@ pub mod tla; use crate::storage::with_voting_state_machines_mut; #[cfg(feature = "tla")] -use std::collections::BTreeSet; -#[cfg(feature = "tla")] pub use tla::{ tla_update_method, InstrumentationState, ToTla, CLAIM_NEURON_DESC, DISBURSE_NEURON_DESC, DISBURSE_TO_NEURON_DESC, MERGE_NEURONS_DESC, SPAWN_NEURONS_DESC, SPAWN_NEURON_DESC, @@ -212,6 +210,9 @@ 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; +/// The maximum number of neurons returned by `list_neurons` +pub const MAX_LIST_NEURONS_RESULTS: usize = 500; + const MAX_LIST_NODE_PROVIDER_REWARDS_RESULTS: usize = 24; /// The number of e8s per ICP; @@ -2387,11 +2388,9 @@ impl Governance { /// TODO(NNS1-2499): inline this. /// Return the Neuron IDs of all Neurons that have `principal` as their /// controller or as one of their hot keys. - pub fn get_neuron_ids_by_principal(&self, principal_id: &PrincipalId) -> Vec { + pub fn get_neuron_ids_by_principal(&self, principal_id: &PrincipalId) -> BTreeSet { self.neuron_store .get_neuron_ids_readable_by_caller(*principal_id) - .into_iter() - .collect() } /// Return the union of `followees` with the set of Neuron IDs of all @@ -2423,8 +2422,15 @@ impl Governance { include_neurons_readable_by_caller, include_empty_neurons_readable_by_caller, include_public_neurons_in_full_neurons, + page_number, + page_size, } = list_neurons; + let page_number = page_number.unwrap_or(0); + let page_size = page_size + .unwrap_or(MAX_LIST_NEURONS_RESULTS as u64) + .min(MAX_LIST_NEURONS_RESULTS as u64); + let include_empty_neurons_readable_by_caller = include_empty_neurons_readable_by_caller // This default is to maintain the previous behavior. (Unlike // protobuf, we do not have a convention that says "the default @@ -2451,11 +2457,11 @@ impl Governance { .get_non_empty_neuron_ids_readable_by_caller(caller) } } else { - Vec::new() + BTreeSet::new() }; // Concatenate (explicit and implicit)-ly included neurons. - let mut requested_neuron_ids: Vec = + let mut requested_neuron_ids: BTreeSet = neuron_ids.iter().map(|id| NeuronId { id: *id }).collect(); requested_neuron_ids.append(&mut implicitly_requested_neuron_ids); @@ -2463,12 +2469,24 @@ impl Governance { let mut neuron_infos = hashmap![]; let mut full_neurons = vec![]; + let chunks: Vec> = requested_neuron_ids + .into_iter() + .chunks(page_size as usize) + .into_iter() + .map(|chunk| chunk.collect()) + .collect(); + + let total_pages_available = Some(chunks.len() as u64); + + let empty = Vec::new(); + let current_page = chunks.get(page_number as usize).unwrap_or(&empty); + // Populate the above two neuron collections. - for neuron_id in requested_neuron_ids { + for neuron_id in current_page { // Ignore when a neuron is not found. It is not guaranteed that a // neuron will be found, because some of the elements in // requested_neuron_ids are supplied by the caller. - let _ignore_when_neuron_not_found = self.with_neuron(&neuron_id, |neuron| { + let _ignore_when_neuron_not_found = self.with_neuron(neuron_id, |neuron| { // Populate neuron_infos. neuron_infos.insert(neuron_id.id, neuron.get_neuron_info(self.voting_power_economics(), now, caller)); @@ -2498,6 +2516,7 @@ impl Governance { ListNeuronsResponse { neuron_infos, full_neurons, + total_pages_available, } } @@ -3898,7 +3917,7 @@ impl Governance { match proposal_data { None => None, Some(pd) => { - let caller_neurons: HashSet = + let caller_neurons: BTreeSet = self.neuron_store.get_neuron_ids_readable_by_caller(*caller); let now = self.env.now(); Some(self.proposal_data_to_info(pd, &caller_neurons, now, false)) @@ -3984,7 +4003,7 @@ impl Governance { /// retrieve dropped payloads by calling `get_proposal_info` for /// each proposal of interest. pub fn get_pending_proposals(&self, caller: &PrincipalId) -> Vec { - let caller_neurons: HashSet = + let caller_neurons: BTreeSet = self.neuron_store.get_neuron_ids_readable_by_caller(*caller); let now = self.env.now(); self.get_pending_proposals_data() @@ -4022,7 +4041,7 @@ impl Governance { fn proposal_data_to_info( &self, data: &ProposalData, - caller_neurons: &HashSet, + caller_neurons: &BTreeSet, now_seconds: u64, multi_query: bool, ) -> ProposalInfo { @@ -4054,7 +4073,7 @@ impl Governance { /// in `except_from`. fn remove_ballots_not_cast_by( all_ballots: &HashMap, - except_from: &HashSet, + except_from: &BTreeSet, ) -> HashMap { let mut ballots = HashMap::new(); for neuron_id in except_from.iter() { @@ -4094,7 +4113,7 @@ impl Governance { fn proposal_is_visible_to_neurons( &self, info: &ProposalData, - caller_neurons: &HashSet, + caller_neurons: &BTreeSet, ) -> bool { // Is 'info' a manage neuron proposal? if let Some(ref managed_id) = info.proposal.as_ref().and_then(|x| x.managed_neuron()) { @@ -4164,7 +4183,7 @@ impl Governance { caller: &PrincipalId, req: &ListProposalInfo, ) -> ListProposalInfoResponse { - let caller_neurons: HashSet = + let caller_neurons: BTreeSet = self.neuron_store.get_neuron_ids_readable_by_caller(*caller); let exclude_topic: HashSet = req.exclude_topic.iter().cloned().collect(); let include_reward_status: HashSet = diff --git a/rs/nns/governance/src/governance/benches.rs b/rs/nns/governance/src/governance/benches.rs index 9952e9fee1a..592987c9ef5 100644 --- a/rs/nns/governance/src/governance/benches.rs +++ b/rs/nns/governance/src/governance/benches.rs @@ -563,6 +563,8 @@ fn list_neurons_benchmark() -> BenchResult { include_neurons_readable_by_caller: true, include_empty_neurons_readable_by_caller: Some(false), include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }; bench_fn(|| { diff --git a/rs/nns/governance/src/governance/tests/list_neurons.rs b/rs/nns/governance/src/governance/tests/list_neurons.rs new file mode 100644 index 00000000000..49644f98646 --- /dev/null +++ b/rs/nns/governance/src/governance/tests/list_neurons.rs @@ -0,0 +1,92 @@ +use crate::{ + governance::Governance, + pb::v1::{neuron::DissolveState, ListNeurons, NetworkEconomics, Neuron}, + test_utils::{MockEnvironment, StubCMC, StubIcpLedger}, +}; +use ic_base_types::PrincipalId; +use ic_nns_common::pb::v1::NeuronId; + +#[test] +fn test_list_neurons_with_paging() { + let user_id = PrincipalId::new_user_test_id(100); + + let neurons = (1..1000u64) + .map(|id| { + let dissolve_state = DissolveState::DissolveDelaySeconds(100); + let account = crate::test_utils::test_subaccount_for_neuron_id(id); + ( + id, + Neuron { + id: Some(NeuronId::from_u64(id)), + controller: Some(user_id), + account, + dissolve_state: Some(dissolve_state), + // Fill in the rest as needed (stake, maturity, etc.) + ..Default::default() + }, + ) + }) + .collect(); + + let governance = Governance::new( + crate::pb::v1::Governance { + neurons, + economics: Some(NetworkEconomics { + voting_power_economics: Some(Default::default()), + ..Default::default() + }), + ..crate::pb::v1::Governance::default() + }, + Box::new(MockEnvironment::new(Default::default(), 0)), + Box::new(StubIcpLedger {}), + Box::new(StubCMC {}), + ); + + let mut request = ListNeurons { + neuron_ids: vec![], + include_neurons_readable_by_caller: true, + include_empty_neurons_readable_by_caller: None, + include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, + }; + + let response_with_no_page_number = governance.list_neurons(&request, user_id); + request.page_number = Some(0); + let response_with_0_page_number = governance.list_neurons(&request, user_id); + + assert_eq!(response_with_0_page_number, response_with_no_page_number); + assert_eq!(response_with_0_page_number.full_neurons.len(), 500); + assert_eq!(response_with_0_page_number.total_pages_available, Some(2)); + + let response = governance.list_neurons( + &ListNeurons { + neuron_ids: vec![], + include_neurons_readable_by_caller: true, + include_empty_neurons_readable_by_caller: None, + include_public_neurons_in_full_neurons: None, + page_number: Some(1), + page_size: None, + }, + user_id, + ); + + assert_eq!(response.full_neurons.len(), 499); + assert_eq!(response.total_pages_available, Some(2)); + + // Assert maximum page size cannot be exceeded + let response = governance.list_neurons( + &ListNeurons { + neuron_ids: vec![], + include_neurons_readable_by_caller: true, + include_empty_neurons_readable_by_caller: None, + include_public_neurons_in_full_neurons: None, + page_number: Some(0), + page_size: Some(501), + }, + user_id, + ); + + assert_eq!(response.full_neurons.len(), 500); + assert_eq!(response.total_pages_available, Some(2)); +} diff --git a/rs/nns/governance/src/governance/tests/mod.rs b/rs/nns/governance/src/governance/tests/mod.rs index 0e04e9c710e..4bb54b0a32b 100644 --- a/rs/nns/governance/src/governance/tests/mod.rs +++ b/rs/nns/governance/src/governance/tests/mod.rs @@ -24,6 +24,7 @@ use lazy_static::lazy_static; use maplit::{btreemap, hashmap}; use std::convert::TryFrom; +mod list_neurons; mod neurons_fund; mod node_provider_rewards; mod stake_maturity; diff --git a/rs/nns/governance/src/neuron_store.rs b/rs/nns/governance/src/neuron_store.rs index dfbb49d773e..1a4929d3133 100644 --- a/rs/nns/governance/src/neuron_store.rs +++ b/rs/nns/governance/src/neuron_store.rs @@ -31,7 +31,7 @@ use ic_nns_common::pb::v1::{NeuronId, ProposalId}; use icp_ledger::{AccountIdentifier, Subaccount}; use std::{ borrow::Cow, - collections::{BTreeMap, HashMap, HashSet}, + collections::{BTreeMap, BTreeSet, HashMap}, fmt::{Debug, Display, Formatter}, ops::{Bound, Deref, RangeBounds}, }; @@ -1241,7 +1241,7 @@ impl NeuronStore { pub fn get_neuron_ids_readable_by_caller( &self, principal_id: PrincipalId, - ) -> HashSet { + ) -> BTreeSet { with_stable_neuron_indexes(|indexes| { indexes .principal() @@ -1256,7 +1256,7 @@ impl NeuronStore { pub fn get_non_empty_neuron_ids_readable_by_caller( &self, caller: PrincipalId, - ) -> Vec { + ) -> BTreeSet { let is_non_empty = |neuron_id: &NeuronId| { self.with_neuron_sections(neuron_id, NeuronSections::NONE, |neuron| neuron.is_funded()) .unwrap_or(false) diff --git a/rs/nns/governance/src/neuron_store/neuron_store_tests.rs b/rs/nns/governance/src/neuron_store/neuron_store_tests.rs index 879c1019e2d..e626d6c366f 100644 --- a/rs/nns/governance/src/neuron_store/neuron_store_tests.rs +++ b/rs/nns/governance/src/neuron_store/neuron_store_tests.rs @@ -8,7 +8,7 @@ use crate::{ }; use ic_nervous_system_common::{ONE_DAY_SECONDS, ONE_MONTH_SECONDS}; use ic_nns_constants::GOVERNANCE_CANISTER_ID; -use maplit::{btreemap, hashmap, hashset}; +use maplit::{btreemap, btreeset, hashmap, hashset}; use num_traits::bounds::LowerBounded; use pretty_assertions::assert_eq; use std::cell::Cell; @@ -638,12 +638,12 @@ fn test_get_neuron_ids_readable_by_caller() { for i in 1..=3 { assert_eq!( neuron_store.get_neuron_ids_readable_by_caller(PrincipalId::new_user_test_id(i)), - hashset! { NeuronId { id: 1 } } + btreeset! { NeuronId { id: 1 } } ); } assert_eq!( neuron_store.get_neuron_ids_readable_by_caller(PrincipalId::new_user_test_id(4)), - hashset! {} + btreeset! {} ); } @@ -883,33 +883,23 @@ fn test_get_non_empty_neuron_ids_readable_by_caller() { 5 => neuron_with_staked_maturity, }); - // Verify that the non-empty neurons readable by the controller and hot key are neurons 3, 4 and - // 5, while a principal that's not controller or hot key can't read any. - let neuron_id_vec_to_u64_hash_set = |neuron_ids: Vec| -> HashSet { - neuron_ids + assert_eq!( + neuron_store.get_non_empty_neuron_ids_readable_by_caller(controller), + btreeset! { 3, 4, 5 } .into_iter() - .map(|neuron_id| neuron_id.id) + .map(NeuronId::from_u64) .collect() - }; - - assert_eq!( - neuron_id_vec_to_u64_hash_set( - neuron_store.get_non_empty_neuron_ids_readable_by_caller(controller) - ), - hashset! { 3, 4, 5 } ); assert_eq!( - neuron_id_vec_to_u64_hash_set( - neuron_store.get_non_empty_neuron_ids_readable_by_caller(hot_key) - ), - hashset! { 3, 4, 5 } + neuron_store.get_non_empty_neuron_ids_readable_by_caller(hot_key), + btreeset! { 3, 4, 5 } + .into_iter() + .map(NeuronId::from_u64) + .collect() ); assert_eq!( - neuron_id_vec_to_u64_hash_set( - neuron_store - .get_non_empty_neuron_ids_readable_by_caller(PrincipalId::new_user_test_id(3)) - ), - hashset! {} + neuron_store.get_non_empty_neuron_ids_readable_by_caller(PrincipalId::new_user_test_id(3)), + btreeset! {} ); } diff --git a/rs/nns/governance/src/pb/conversions.rs b/rs/nns/governance/src/pb/conversions.rs index e25e507ab52..eb6aa16d2d1 100644 --- a/rs/nns/governance/src/pb/conversions.rs +++ b/rs/nns/governance/src/pb/conversions.rs @@ -3766,6 +3766,8 @@ impl From for pb_api::ListNeurons { include_neurons_readable_by_caller: item.include_neurons_readable_by_caller, include_empty_neurons_readable_by_caller: item.include_empty_neurons_readable_by_caller, include_public_neurons_in_full_neurons: item.include_public_neurons_in_full_neurons, + page_number: item.page_number, + page_size: item.page_size, } } } @@ -3776,6 +3778,8 @@ impl From for pb::ListNeurons { include_neurons_readable_by_caller: item.include_neurons_readable_by_caller, include_empty_neurons_readable_by_caller: item.include_empty_neurons_readable_by_caller, include_public_neurons_in_full_neurons: item.include_public_neurons_in_full_neurons, + page_number: item.page_number, + page_size: item.page_size, } } } @@ -3789,6 +3793,7 @@ impl From for pb_api::ListNeuronsResponse { .map(|(k, v)| (k, v.into())) .collect(), full_neurons: item.full_neurons.into_iter().map(|x| x.into()).collect(), + total_pages_available: item.total_pages_available, } } } @@ -3801,6 +3806,7 @@ impl From for pb::ListNeuronsResponse { .map(|(k, v)| (k, v.into())) .collect(), full_neurons: item.full_neurons.into_iter().map(|x| x.into()).collect(), + total_pages_available: item.total_pages_available, } } } diff --git a/rs/nns/governance/src/test_utils.rs b/rs/nns/governance/src/test_utils.rs index 7764f856b3a..8dbc0f33ebf 100644 --- a/rs/nns/governance/src/test_utils.rs +++ b/rs/nns/governance/src/test_utils.rs @@ -314,3 +314,9 @@ impl Environment for MockEnvironment { result } } + +/// Useful for avoiding errors related to index corruption that happens when neurons +/// share subaccounts. +pub fn test_subaccount_for_neuron_id(neuron_id: u64) -> Vec { + [vec![0; 24], neuron_id.to_be_bytes().to_vec()].concat() +} diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs index 534115d2941..8309277fd98 100644 --- a/rs/nns/governance/tests/governance.rs +++ b/rs/nns/governance/tests/governance.rs @@ -118,7 +118,7 @@ use ic_sns_wasm::pb::v1::{ }; use icp_ledger::{protobuf, AccountIdentifier, Memo, Subaccount, Tokens}; use lazy_static::lazy_static; -use maplit::{btreemap, hashmap}; +use maplit::{btreemap, btreeset, hashmap}; use pretty_assertions::{assert_eq, assert_ne}; use proptest::prelude::{proptest, ProptestConfig}; use rand::{prelude::IteratorRandom, rngs::StdRng, Rng, SeedableRng}; @@ -126,7 +126,7 @@ use registry_canister::mutations::do_add_node_operator::AddNodeOperatorPayload; use rust_decimal_macros::dec; use std::{ cmp::Ordering, - collections::{BTreeMap, HashSet, VecDeque}, + collections::{BTreeMap, BTreeSet, HashSet, VecDeque}, convert::{TryFrom, TryInto}, iter::{self, once}, ops::{Deref, Div}, @@ -2336,6 +2336,7 @@ fn test_get_neuron_when_private_neuron_enforcement_disabled() { 1 => neuron_info, }, full_neurons: vec![full_neuron], + total_pages_available: Some(1), }, ); } @@ -2391,6 +2392,7 @@ fn test_get_neuron_when_private_neuron_enforcement_enabled() { 1 => neuron_info, }, full_neurons: vec![full_neuron], + total_pages_available: Some(1), }, ); } @@ -4466,24 +4468,21 @@ fn test_get_neuron_ids_by_principal() { driver.get_fake_cmc(), ); - let mut principal2_neuron_ids = gov.get_neuron_ids_by_principal(&principal2); - principal2_neuron_ids.sort_unstable(); - assert_eq!( gov.get_neuron_ids_by_principal(&principal1), - vec![NeuronId { id: 1 }] + btreeset![NeuronId { id: 1 }] ); assert_eq!( - principal2_neuron_ids, - vec![NeuronId { id: 2 }, NeuronId { id: 3 }, NeuronId { id: 4 }] + gov.get_neuron_ids_by_principal(&principal2), + btreeset![NeuronId { id: 2 }, NeuronId { id: 3 }, NeuronId { id: 4 }] ); assert_eq!( gov.get_neuron_ids_by_principal(&principal3), - Vec::::new() + BTreeSet::::new() ); assert_eq!( gov.get_neuron_ids_by_principal(&principal4), - vec![NeuronId { id: 4 }] + btreeset![NeuronId { id: 4 }] ); } @@ -4645,7 +4644,7 @@ fn create_mature_neuron(dissolved: bool) -> (fake::FakeDriver, Governance, Neuro ..Default::default() } ); - assert_eq!(gov.get_neuron_ids_by_principal(&from), vec![id]); + assert_eq!(gov.get_neuron_ids_by_principal(&from), btreeset![id]); // Dissolve the neuron if `dissolved` is true if dissolved { @@ -5788,10 +5787,8 @@ fn test_neuron_split() { parent_neuron.voting_power_refreshed_timestamp_seconds, ); - let mut neuron_ids = governance.get_neuron_ids_by_principal(&from); - neuron_ids.sort_unstable(); - let mut expected_neuron_ids = vec![id, child_nid]; - expected_neuron_ids.sort_unstable(); + let neuron_ids = governance.get_neuron_ids_by_principal(&from); + let expected_neuron_ids = btreeset![id, child_nid]; assert_eq!(neuron_ids, expected_neuron_ids); } @@ -10789,6 +10786,9 @@ fn test_include_public_neurons_in_full_neurons() { // This should have no effect. include_empty_neurons_readable_by_caller: Some(true), + + page_number: None, + page_size: None, }, caller, ); @@ -14852,6 +14852,8 @@ fn test_neuron_info_private_enforcement() { include_neurons_readable_by_caller: false, include_empty_neurons_readable_by_caller: None, include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }, requester, ) diff --git a/rs/nns/governance/unreleased_changelog.md b/rs/nns/governance/unreleased_changelog.md index aacc80ecf2c..724841babeb 100644 --- a/rs/nns/governance/unreleased_changelog.md +++ b/rs/nns/governance/unreleased_changelog.md @@ -9,6 +9,22 @@ on the process that this file is part of, see ## Added +### List Neurons Paging + +Two new fields are added to the request, and one to the response. + +The request now supports `page_size` and `page_number`. If `page_size` is greater than +`MAX_LIST_NEURONS_RESULTS` (currently 500), the API will treat it as `MAX_LIST_NEURONS_RESULTS`, and +continue procesisng the request. If `page_number` is None, the API will treat it as Some(0) + +In the response, a field `total_pages_available` is available to tell the user how many +additional requests need to be made. + +This will only affect neuron holders with more than 500 neurons, which is a small minority. + +This allows neuron holders with many neurons to list all of their neurons, whereas before, +responses could be too large to be sent by the protocol. + ### Periodic Confirmation Enabled voting power adjustment and follow pruning. diff --git a/rs/nns/integration_tests/src/governance_neurons.rs b/rs/nns/integration_tests/src/governance_neurons.rs index 6d6fb4cbc7d..ab6772deff2 100644 --- a/rs/nns/integration_tests/src/governance_neurons.rs +++ b/rs/nns/integration_tests/src/governance_neurons.rs @@ -551,6 +551,8 @@ fn test_list_neurons() { include_neurons_readable_by_caller: false, include_empty_neurons_readable_by_caller: Some(false), include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }, ); assert_eq!(list_neurons_response.neuron_infos.len(), 3); @@ -565,6 +567,8 @@ fn test_list_neurons() { include_neurons_readable_by_caller: true, include_empty_neurons_readable_by_caller: Some(true), include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }, ); assert_eq!(list_neurons_response.neuron_infos.len(), 2); @@ -579,6 +583,8 @@ fn test_list_neurons() { include_neurons_readable_by_caller: true, include_empty_neurons_readable_by_caller: Some(false), include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }, ); assert_eq!(list_neurons_response.neuron_infos.len(), 1); @@ -594,6 +600,8 @@ fn test_list_neurons() { include_neurons_readable_by_caller: true, include_empty_neurons_readable_by_caller: None, include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }, ); assert_eq!(list_neurons_response.neuron_infos.len(), 3); diff --git a/rs/nns/integration_tests/src/neuron_voting.rs b/rs/nns/integration_tests/src/neuron_voting.rs index d5b6fb3f37e..0edc130fe31 100644 --- a/rs/nns/integration_tests/src/neuron_voting.rs +++ b/rs/nns/integration_tests/src/neuron_voting.rs @@ -18,8 +18,9 @@ use ic_nns_test_utils::{ get_unauthorized_neuron, submit_proposal, }, state_test_helpers::{ - get_pending_proposals, list_neurons, nns_cast_vote, nns_governance_get_full_neuron, - nns_governance_make_proposal, setup_nns_canisters, state_machine_builder_for_nns_tests, + get_pending_proposals, list_all_neurons_and_combine_responses, nns_cast_vote, + nns_governance_get_full_neuron, nns_governance_make_proposal, setup_nns_canisters, + state_machine_builder_for_nns_tests, }, }; use ic_state_machine_tests::StateMachine; @@ -394,14 +395,16 @@ fn test_voting_can_span_multiple_rounds() { assert_matches!(response.command, Some(Command::MakeProposal(_))); - let listed_neurons = list_neurons( + let listed_neurons = list_all_neurons_and_combine_responses( &state_machine, *TEST_NEURON_1_OWNER_PRINCIPAL, ListNeurons { - neuron_ids: (0..1000u64).collect(), + neuron_ids: (1..1000u64).collect(), include_neurons_readable_by_caller: false, include_empty_neurons_readable_by_caller: None, include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }, ); @@ -419,7 +422,7 @@ fn test_voting_can_span_multiple_rounds() { state_machine.tick(); } - let listed_neurons = list_neurons( + let listed_neurons = list_all_neurons_and_combine_responses( &state_machine, *TEST_NEURON_1_OWNER_PRINCIPAL, ListNeurons { @@ -427,6 +430,8 @@ fn test_voting_can_span_multiple_rounds() { include_neurons_readable_by_caller: false, include_empty_neurons_readable_by_caller: None, include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }, ); diff --git a/rs/nns/test_utils/src/state_test_helpers.rs b/rs/nns/test_utils/src/state_test_helpers.rs index 599e05bbdfe..91fb87a37f7 100644 --- a/rs/nns/test_utils/src/state_test_helpers.rs +++ b/rs/nns/test_utils/src/state_test_helpers.rs @@ -1573,6 +1573,38 @@ pub fn list_neurons( Decode!(&result, ListNeuronsResponse).unwrap() } +/// This function is intended to ensure all neurons are paged through. It +/// recursively calls `list_neurons`. This method will panic if more than 20 requests are made +/// this method could be adjusted. +pub fn list_all_neurons_and_combine_responses( + state_machine: &StateMachine, + sender: PrincipalId, + request: ListNeurons, +) -> ListNeuronsResponse { + assert_eq!( + request.page_number.unwrap_or_default(), + 0, + "This method is intended to ensure all neurons \ + are paged through. `page_number` should be None or Some(0)" + ); + + let mut response = list_neurons(state_machine, sender, request.clone()); + + let pages_needed = response.total_pages_available.unwrap(); + + for page in 1..=pages_needed { + let mut new_request = request.clone(); + new_request.page_number = Some(page); + let mut new_response = list_neurons(state_machine, sender, new_request); + response.full_neurons.append(&mut new_response.full_neurons); + response + .neuron_infos + .extend(new_response.neuron_infos.into_iter()); + } + + response +} + pub fn list_neurons_by_principal( state_machine: &StateMachine, sender: PrincipalId, @@ -1585,6 +1617,8 @@ pub fn list_neurons_by_principal( include_neurons_readable_by_caller: true, include_empty_neurons_readable_by_caller: None, include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }, ) } diff --git a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs index eb596881af6..adbbc337072 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs @@ -9,7 +9,6 @@ use on_wire::IntoWire; use rand::Rng; use std::{collections::HashMap, sync::Arc, time::Duration}; -use crate::request_types::RefreshVotingPower; use crate::{ convert, convert::{make_read_state_from_update, to_arg, to_model_account_identifier}, @@ -25,8 +24,9 @@ use crate::{ request_handler::{make_sig_data, verify_network_id, RosettaRequestHandler}, request_types::{ AddHotKey, ChangeAutoStakeMaturity, Disburse, Follow, ListNeurons, MergeMaturity, - NeuronInfo, PublicKeyOrPrincipal, RegisterVote, RemoveHotKey, RequestType, - SetDissolveTimestamp, Spawn, Stake, StakeMaturity, StartDissolve, StopDissolve, + NeuronInfo, PublicKeyOrPrincipal, RefreshVotingPower, RegisterVote, RemoveHotKey, + RequestType, SetDissolveTimestamp, Spawn, Stake, StakeMaturity, StartDissolve, + StopDissolve, }, }; use ic_nns_governance_api::pb::v1::{ @@ -435,6 +435,8 @@ fn handle_list_neurons( include_neurons_readable_by_caller: true, include_empty_neurons_readable_by_caller: None, include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }; let update = HttpCanisterUpdate { canister_id: Blob(ic_nns_constants::GOVERNANCE_CANISTER_ID.get().to_vec()), diff --git a/rs/rosetta-api/icp/tests/system_tests/common/utils.rs b/rs/rosetta-api/icp/tests/system_tests/common/utils.rs index 4dfcd549f52..3a702ee8401 100644 --- a/rs/rosetta-api/icp/tests/system_tests/common/utils.rs +++ b/rs/rosetta-api/icp/tests/system_tests/common/utils.rs @@ -1,18 +1,13 @@ use crate::common::constants::MAX_ROSETTA_SYNC_ATTEMPTS; use candid::{Decode, Encode}; -use ic_agent::identity::BasicIdentity; -use ic_agent::Agent; -use ic_agent::Identity; +use ic_agent::{identity::BasicIdentity, Agent, Identity}; use ic_icp_rosetta_client::RosettaClient; use ic_ledger_core::block::BlockType; -use ic_nns_constants::GOVERNANCE_CANISTER_ID; -use ic_nns_constants::LEDGER_CANISTER_ID; -use ic_nns_governance::pb::v1::ListNeurons; -use ic_nns_governance::pb::v1::ListNeuronsResponse; +use ic_nns_constants::{GOVERNANCE_CANISTER_ID, LEDGER_CANISTER_ID}; +use ic_nns_governance::pb::v1::{ListNeurons, ListNeuronsResponse}; use ic_nns_governance_api::pb::v1::GovernanceError; use ic_rosetta_api::convert::to_hash; -use icp_ledger::GetBlocksArgs; -use icp_ledger::QueryEncodedBlocksResponse; +use icp_ledger::{GetBlocksArgs, QueryEncodedBlocksResponse}; use rosetta_core::identifiers::NetworkIdentifier; use std::sync::Arc; use url::Url; @@ -180,6 +175,8 @@ pub async fn list_neurons(agent: &Agent) -> ListNeuronsResponse { include_neurons_readable_by_caller: true, include_empty_neurons_readable_by_caller: Some(true), include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }) .unwrap() ) diff --git a/rs/tests/driver/src/canister_api.rs b/rs/tests/driver/src/canister_api.rs index 1dd732f65ea..d575641426a 100644 --- a/rs/tests/driver/src/canister_api.rs +++ b/rs/tests/driver/src/canister_api.rs @@ -738,6 +738,8 @@ impl ListNnsNeuronsRequest { include_neurons_readable_by_caller, include_empty_neurons_readable_by_caller: None, include_public_neurons_in_full_neurons: None, + page_number: None, + page_size: None, }, } }