From 025822ef89bc6c00772f4031b721ca43518d23d7 Mon Sep 17 00:00:00 2001 From: Carly Gundy Date: Wed, 18 Sep 2024 12:49:40 +0200 Subject: [PATCH] Revert "feat(sns): Only set the `wasm_memory_limit` for SNS Governance when deploying an SNS (#1427)" This reverts commit a4b9ab2ec1eea243d96887e13f3c6cb79f6aefd5. --- .../integration_tests/tests/sns_lifecycle.rs | 43 ------- rs/nns/constants/src/lib.rs | 7 +- rs/nns/governance/tests/governance.rs | 5 +- rs/nns/sns-wasm/canister/canister.rs | 13 +- rs/nns/sns-wasm/src/canister_api.rs | 1 - rs/nns/sns-wasm/src/sns_wasm.rs | 117 ++++++++---------- 6 files changed, 65 insertions(+), 121 deletions(-) diff --git a/rs/nervous_system/integration_tests/tests/sns_lifecycle.rs b/rs/nervous_system/integration_tests/tests/sns_lifecycle.rs index ed1ec7db838..12dc4152b9e 100644 --- a/rs/nervous_system/integration_tests/tests/sns_lifecycle.rs +++ b/rs/nervous_system/integration_tests/tests/sns_lifecycle.rs @@ -1,4 +1,3 @@ -use crate::sns::root::get_sns_canisters_summary; use assert_matches::assert_matches; use candid::{Nat, Principal}; use canister_test::Wasm; @@ -28,7 +27,6 @@ use ic_sns_governance::{ pb::v1::{self as sns_pb, NeuronPermissionType}, }; use ic_sns_init::distributions::MAX_DEVELOPER_DISTRIBUTION_COUNT; -use ic_sns_root::CanisterSummary; use ic_sns_swap::{ pb::v1::{ new_sale_ticket_response, set_dapp_controllers_call_result, set_mode_call_result, @@ -1880,47 +1878,6 @@ fn test_sns_lifecycle( "No archives found from get_sns_canisters_summary response: {:#?}", response ); - - // Check that the SNS framework canister settings are as expected - { - // get SNS canisters summary - let sns_canisters_summary = get_sns_canisters_summary(&pocket_ic, sns_root_canister_id); - fn get_wasm_memory_limit(summary: Option) -> u64 { - u64::try_from( - summary - .unwrap() - .status - .unwrap() - .settings - .wasm_memory_limit - .unwrap() - .0, - ) - .unwrap() - } - // Governance should have a higher memory limit - assert_eq!( - get_wasm_memory_limit(sns_canisters_summary.governance), - 4 * 1024 * 1024 * 1024, - ); - // Other canisters should have a lower memory limit - assert_eq!( - get_wasm_memory_limit(sns_canisters_summary.root), - 3 * 1024 * 1024 * 1024, - ); - assert_eq!( - get_wasm_memory_limit(sns_canisters_summary.swap), - 3 * 1024 * 1024 * 1024, - ); - assert_eq!( - get_wasm_memory_limit(sns_canisters_summary.ledger), - 3 * 1024 * 1024 * 1024, - ); - assert_eq!( - get_wasm_memory_limit(sns_canisters_summary.index), - 3 * 1024 * 1024 * 1024, - ); - } } #[test] diff --git a/rs/nns/constants/src/lib.rs b/rs/nns/constants/src/lib.rs index 89cdd6d7f28..1ae9eb39f58 100644 --- a/rs/nns/constants/src/lib.rs +++ b/rs/nns/constants/src/lib.rs @@ -163,13 +163,10 @@ const NNS_GOVERNANCE_CANISTER_MEMORY_ALLOCATION_IN_BYTES: u64 = 10 * 1024 * 1024 // The default memory allocation to set for the remaining NNS canister (1GiB) const NNS_DEFAULT_CANISTER_MEMORY_ALLOCATION_IN_BYTES: u64 = 1024 * 1024 * 1024; -/// The current value is 4 GiB, s.t. the SNS governance canister never hits the soft memory limit. +/// The current value is 4 GiB, s.t. the SNS framework canisters never hit the soft memory limit. /// This mitigates the risk that an SNS Governance canister runs out of memory and proposals cannot /// be passed anymore. -pub const DEFAULT_SNS_GOVERNANCE_CANISTER_WASM_MEMORY_LIMIT: u64 = 1 << 32; - -/// This value is 3GiB, which will leave a comfortable buffer in the situation when a canister runs out of memory -pub const DEFAULT_SNS_NON_GOVERNANCE_CANISTER_WASM_MEMORY_LIMIT: u64 = 3 * (1 << 30); +pub const DEFAULT_SNS_FRAMEWORK_CANISTER_WASM_MEMORY_LIMIT: u64 = 1 << 32; /// Returns the memory allocation of the given nns canister. pub fn memory_allocation_of(canister_id: CanisterId) -> u64 { diff --git a/rs/nns/governance/tests/governance.rs b/rs/nns/governance/tests/governance.rs index ba533b6efa3..96f1a6030fe 100644 --- a/rs/nns/governance/tests/governance.rs +++ b/rs/nns/governance/tests/governance.rs @@ -39,7 +39,8 @@ use ic_nns_common::{ types::UpdateIcpXdrConversionRatePayload, }; use ic_nns_constants::{ - GOVERNANCE_CANISTER_ID, LEDGER_CANISTER_ID as ICP_LEDGER_CANISTER_ID, SNS_WASM_CANISTER_ID, + DEFAULT_SNS_FRAMEWORK_CANISTER_WASM_MEMORY_LIMIT, GOVERNANCE_CANISTER_ID, + LEDGER_CANISTER_ID as ICP_LEDGER_CANISTER_ID, SNS_WASM_CANISTER_ID, }; use ic_nns_governance::{ governance::{ @@ -11774,7 +11775,7 @@ lazy_static! { Some(517576), // memory_allocation 448076, // freezing_threshold 268693, // idle_cycles_burned_per_day - (3.5 * (1 << 30) as f32) as u64, // wasm_memory_limit (3.5gb) + DEFAULT_SNS_FRAMEWORK_CANISTER_WASM_MEMORY_LIMIT, // wasm_memory_limit )), }), governance: Some(ic_sns_root::CanisterSummary { diff --git a/rs/nns/sns-wasm/canister/canister.rs b/rs/nns/sns-wasm/canister/canister.rs index 715ba896d82..83241603e08 100644 --- a/rs/nns/sns-wasm/canister/canister.rs +++ b/rs/nns/sns-wasm/canister/canister.rs @@ -15,7 +15,7 @@ use ic_nervous_system_clients::{ canister_status::{canister_status, CanisterStatusResultV2, CanisterStatusType}, }; use ic_nervous_system_runtime::DfnRuntime; -use ic_nns_constants::GOVERNANCE_CANISTER_ID; +use ic_nns_constants::{DEFAULT_SNS_FRAMEWORK_CANISTER_WASM_MEMORY_LIMIT, GOVERNANCE_CANISTER_ID}; use ic_nns_handler_root_interface::client::NnsRootCanisterClientImpl; use ic_sns_wasm::{ canister_api::CanisterApi, @@ -69,17 +69,18 @@ impl CanisterApi for CanisterApiImpl { target_subnet: SubnetId, controller_id: PrincipalId, cycles: Cycles, - wasm_memory_limit: u64, ) -> Result { - let settings = CanisterSettingsArgsBuilder::new() - .with_controllers(vec![controller_id]) - .with_wasm_memory_limit(wasm_memory_limit); let result: Result = dfn_core::api::call_with_funds_and_cleanup( target_subnet.into(), &Method::CreateCanister.to_string(), candid_one, CreateCanisterArgs { - settings: Some(settings.build()), + settings: Some( + CanisterSettingsArgsBuilder::new() + .with_controllers(vec![controller_id]) + .with_wasm_memory_limit(DEFAULT_SNS_FRAMEWORK_CANISTER_WASM_MEMORY_LIMIT) + .build(), + ), sender_canister_version: Some(dfn_core::api::canister_version()), }, Funds::new(cycles.get().try_into().unwrap()), diff --git a/rs/nns/sns-wasm/src/canister_api.rs b/rs/nns/sns-wasm/src/canister_api.rs index f5efb7ccad7..1e7d07d036c 100644 --- a/rs/nns/sns-wasm/src/canister_api.rs +++ b/rs/nns/sns-wasm/src/canister_api.rs @@ -13,7 +13,6 @@ pub trait CanisterApi { target_subnet: SubnetId, controller_id: PrincipalId, cycles: Cycles, - wasm_memory_limit: u64, ) -> Result; /// Delete a canister that has been created diff --git a/rs/nns/sns-wasm/src/sns_wasm.rs b/rs/nns/sns-wasm/src/sns_wasm.rs index dc422bfb1a1..e86a97fd132 100644 --- a/rs/nns/sns-wasm/src/sns_wasm.rs +++ b/rs/nns/sns-wasm/src/sns_wasm.rs @@ -27,10 +27,6 @@ use ic_cdk::api::stable::StableMemory; use ic_nervous_system_clients::canister_id_record::CanisterIdRecord; use ic_nervous_system_common::{ONE_TRILLION, SNS_CREATION_FEE}; use ic_nervous_system_proto::pb::v1::Canister; -use ic_nns_constants::{ - DEFAULT_SNS_GOVERNANCE_CANISTER_WASM_MEMORY_LIMIT, - DEFAULT_SNS_NON_GOVERNANCE_CANISTER_WASM_MEMORY_LIMIT, -}; use ic_nns_constants::{GOVERNANCE_CANISTER_ID, ROOT_CANISTER_ID}; use ic_nns_handler_root_interface::{ client::NnsRootCanisterClient, ChangeCanisterControllersRequest, @@ -1175,68 +1171,61 @@ where initial_cycles_per_canister: u64, ) -> Result)> { let this_canister_id = canister_api.local_canister_id().get(); - let new_canister = |canister_type: SnsCanisterType| { + let new_canister = || { canister_api.create_canister( subnet_id, this_canister_id, Cycles::new(initial_cycles_per_canister.into()), - if canister_type == SnsCanisterType::Governance { - DEFAULT_SNS_GOVERNANCE_CANISTER_WASM_MEMORY_LIMIT - } else { - DEFAULT_SNS_NON_GOVERNANCE_CANISTER_WASM_MEMORY_LIMIT - }, ) }; // Create these in order instead of join_all to get deterministic ordering for tests - let root = new_canister(SnsCanisterType::Root).await; - let governance = new_canister(SnsCanisterType::Governance).await; - let ledger = new_canister(SnsCanisterType::Ledger).await; - let swap = new_canister(SnsCanisterType::Swap).await; - let index = new_canister(SnsCanisterType::Index).await; + let canisters_attempted = vec![ + new_canister().await, + new_canister().await, + new_canister().await, + new_canister().await, + new_canister().await, + ]; + let canisters_attempted_count = canisters_attempted.len(); - let (root, governance, ledger, swap, index) = match (root, governance, ledger, swap, index) - { - (Ok(root), Ok(governance), Ok(ledger), Ok(swap), Ok(index)) => { - (root, governance, ledger, swap, index) - } - (root, governance, ledger, swap, index) => { - let canisters_to_delete = SnsCanisterIds { - root: root.ok().map(|canister_id| canister_id.get()), - governance: governance.ok().map(|canister_id| canister_id.get()), - ledger: ledger.ok().map(|canister_id| canister_id.get()), - swap: swap.ok().map(|canister_id| canister_id.get()), - index: index.ok().map(|canister_id| canister_id.get()), - }; - let problem_canisters = vec![ - canisters_to_delete.root.is_none().then_some("Root"), - canisters_to_delete - .governance - .is_none() - .then_some("Governance"), - canisters_to_delete.ledger.is_none().then_some("Ledger"), - canisters_to_delete.swap.is_none().then_some("Swap"), - canisters_to_delete.index.is_none().then_some("Index"), - ] - .into_iter() - .flatten() - .collect::>(); - return Err(( - format!( - "Could not create some canisters: {}", - problem_canisters.join(", ") - ), - Some(canisters_to_delete), - )); - } - }; + let mut canisters_created = canisters_attempted + .into_iter() + .flatten() + .collect::>(); + + let canisters_created_count = canisters_created.len(); + + if canisters_created_count < canisters_attempted_count { + let next = |c: &mut Vec| { + if !c.is_empty() { + Some(c.remove(0).get()) + } else { + None + } + }; + let canisters_to_delete = SnsCanisterIds { + root: next(&mut canisters_created), + governance: next(&mut canisters_created), + ledger: next(&mut canisters_created), + swap: next(&mut canisters_created), + index: next(&mut canisters_created), + }; + return Err(( + format!( + "Could not create needed canisters. Only created {} but 5 needed.", + canisters_created_count + ), + Some(canisters_to_delete), + )); + } Ok(SnsCanisterIds { - root: Some(root.get()), - governance: Some(governance.get()), - ledger: Some(ledger.get()), - swap: Some(swap.get()), - index: Some(index.get()), + root: Some(canisters_created.remove(0).get()), + governance: Some(canisters_created.remove(0).get()), + ledger: Some(canisters_created.remove(0).get()), + swap: Some(canisters_created.remove(0).get()), + index: Some(canisters_created.remove(0).get()), }) } @@ -2042,7 +2031,6 @@ mod test { _target_subnet: SubnetId, _controller_id: PrincipalId, _cycles: Cycles, - _wasm_memory_limit: u64, ) -> Result { let mut errors = self.errors_on_create_canister.lock().unwrap(); if errors.len() > 0 { @@ -3278,10 +3266,10 @@ mod test { let subnet_id = subnet_test_id(1); - let governance_id = canister_test_id(1); - let ledger_id = canister_test_id(2); - let swap_id = canister_test_id(3); - let index_id = canister_test_id(4); + let root_id = canister_test_id(1); + let governance_id = canister_test_id(2); + let ledger_id = canister_test_id(3); + let swap_id = canister_test_id(4); let sns_init_payload = SnsInitPayload { dapp_canisters: None, @@ -3296,21 +3284,22 @@ mod test { true, vec![], vec![], - vec![governance_id, ledger_id, swap_id, index_id], + vec![root_id, governance_id, ledger_id, swap_id], vec![], vec![], vec![], DeployNewSnsResponse { canisters: Some(SnsCanisterIds { - root: None, + root: Some(root_id.get()), ledger: Some(ledger_id.get()), governance: Some(governance_id.get()), swap: Some(swap_id.get()), - index: Some(index_id.get()), + index: None, }), subnet_id: Some(subnet_id.get()), error: Some(SnsWasmError { - message: "Could not create some canisters: Root".to_string(), + message: "Could not create needed canisters. Only created 4 but 5 needed." + .to_string(), }), dapp_canisters_transfer_result: Some(DappCanistersTransferResult { restored_dapp_canisters: vec![],