From a26c96337a8c04f1c6f1a64372f2076b87e2ca26 Mon Sep 17 00:00:00 2001 From: Bjoern Tackmann Date: Wed, 5 Aug 2026 09:31:53 +0200 Subject: [PATCH 1/4] feat: Add subnet_metrics management canister endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the EXPERIMENTAL `subnet_metrics` endpoint from dfinity/developer-docs#333: given a subnet ID, returns that subnet's current block height, canister count, total canister state size, total consumed cycles, and total processed transactions. Canister-callable only; not reachable via ingress. Four of the five values were previously readable only by external users via the certified state tree at /subnet//metrics, which canisters cannot read. `block_height` is new: it is `current_round`, already deterministic at execution time (the same value `vetkd_derive_key` already commits to replicated state). `subnet_id` may name any subnet. Routing delivers the call to the named subnet, so the `args.subnet_id == own_subnet_id` check in the handler mirrors `node_metrics_history` and does not block cross-subnet calls; it guards the NNS direct-subnet-addressing path, where a call can reach subnet A while naming subnet B. Notes for future readers, since these are easy to "fix" back: * The instruction charge is keyed on `hot_len()`, NOT `num_canisters()`. The fold in `total_consumed_cycles()` visits hot canisters only, and `hot_len() << len()` is the steady state. Keying on the total over-charges ~41x at 100k canisters, which does not protect the subnet — it lets ~61 calls/round pin the whole shared subnet-message budget and defer install_code/snapshot traffic. Priced against the already enabled `fetch_canister_logs` (2.4 cycles per round-instruction of budget), hot-keyed `subnet_metrics` costs an attacker 3.6. `subnet_metrics_charge_ignores_cold_canisters` fails if this regresses. * `hot_len()` is the first partition-cardinality input to execution, so the unconditional `repartition_canister_states()` call in `commit_and_certify` is now a correctness requirement, not an optimisation. Moving it inside the `CertificationScope::Metadata` branch would diverge the charge across replicas. `hot_cold_partition_is_canonical_after_every_commit` guards this. * `canister_state_bytes` is read from the stored `subnet_metrics` field and must not be recomputed live: the stored value refreshes only every 10 rounds by design, so recomputing would disagree with the certified state tree on 9 rounds out of 10. * `validate_cold_stats()` alerts; it does not enforce. `validate_eq_checkpoint` discards the error and the checkpoint still finalizes. Describe it as detection, not prevention. * The system tests in general_execution_tests/api_tests.rs are Linux-only and could not be compiled locally. CI is their first real check. Co-Authored-By: Claude Opus 5 --- .../ic-management-canister-types/CHANGELOG.md | 1 + .../ic-management-canister-types/src/lib.rs | 36 ++ .../tests/candid_equality.rs | 5 + .../ic-management-canister-types/tests/ic.did | 20 + rs/canonical_state/src/encoding.rs | 1 + .../src/encoding/tests/subnet_metrics.rs | 78 ++++ .../wasmtime_embedder/system_api/routing.rs | 91 +++- .../system_api/sandbox_safe_system_state.rs | 1 + .../benches/management_canister/main.rs | 2 + .../management_canister/subnet_metrics.rs | 224 +++++++++ .../test_canister/candid.did | 1 + .../test_canister/src/main.rs | 28 ++ .../src/canister_manager.rs | 4 + .../src/canister_manager/tests.rs | 296 +++++++++++- .../src/execution_environment.rs | 192 +++++++- .../src/execution_environment_metrics.rs | 1 + .../src/ic00_permissions.rs | 4 + rs/execution_environment/src/scheduler.rs | 10 +- .../tests/execution_test.rs | 424 +++++++++++++++++- rs/replicated_state/src/canister_states.rs | 42 ++ .../src/canister_states/tests.rs | 105 +++++ rs/replicated_state/src/replicated_state.rs | 11 + rs/state_manager/src/checkpoint.rs | 68 ++- rs/state_manager/src/lib.rs | 10 + rs/state_manager/tests/state_manager.rs | 67 ++- .../execution_environment/src/lib.rs | 35 +- rs/tests/execution/general_execution_test.rs | 10 + .../general_execution_tests/api_tests.rs | 243 ++++++++++ rs/types/management_canister_types/src/lib.rs | 35 ++ .../tests/candid_equality.rs | 6 + .../management_canister_types/tests/ic.did | 20 + .../types/src/messages/ingress_messages.rs | 1 + rs/types/types/src/messages/inter_canister.rs | 1 + 33 files changed, 2026 insertions(+), 47 deletions(-) create mode 100644 rs/canonical_state/src/encoding/tests/subnet_metrics.rs create mode 100644 rs/execution_environment/benches/management_canister/subnet_metrics.rs diff --git a/packages/ic-management-canister-types/CHANGELOG.md b/packages/ic-management-canister-types/CHANGELOG.md index 5612358588c3..442cd5f1b474 100644 --- a/packages/ic-management-canister-types/CHANGELOG.md +++ b/packages/ic-management-canister-types/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Added the `PATCH` variant to the `HttpMethod` enum used by canister HTTPS outcalls (`http_request`). The variant is plumbed through the type but not yet enabled on replicated subnets. +- Types for `subnet_metrics`: added the types `SubnetMetricsArgs` and `SubnetMetricsResult`. ## [0.8.0] - 2026-05-13 diff --git a/packages/ic-management-canister-types/src/lib.rs b/packages/ic-management-canister-types/src/lib.rs index 0518cf7cfc78..5809898f2026 100644 --- a/packages/ic-management-canister-types/src/lib.rs +++ b/packages/ic-management-canister-types/src/lib.rs @@ -1372,6 +1372,42 @@ pub struct SubnetInfoResult { pub registry_version: u64, } +/// # Subnet Metrics Args. +/// +/// Argument type of [`subnet_metrics`](https://docs.internetcomputer.org/references/management-canister/#subnet_metrics). +#[derive( + CandidType, Serialize, Deserialize, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, +)] +pub struct SubnetMetricsArgs { + /// Subnet ID. + pub subnet_id: Principal, +} + +/// # Subnet Metrics Result. +/// +/// Result type of [`subnet_metrics`](https://docs.internetcomputer.org/references/management-canister/#subnet_metrics). +/// +/// This API is EXPERIMENTAL and may evolve in a non-backward-compatible way. +#[derive( + CandidType, Serialize, Deserialize, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, +)] +pub struct SubnetMetricsResult { + /// Current block height of the subnet, i.e. the height of the block in whose + /// execution the call is processed. Monotonically non-decreasing for a given + /// subnet; heights of different subnets are unrelated. + pub block_height: Nat, + /// Current number of canisters on the subnet. + pub num_canisters: Nat, + /// Current total size in bytes of the state taken by canisters on the subnet. + pub canister_state_bytes: Nat, + /// Total cycles removed from circulation on the subnet by all current and + /// deleted canisters. + pub consumed_cycles_total: Nat, + /// Total number of transactions processed on the subnet, i.e. the total + /// number of messages executed in replicated mode. + pub update_transactions_total: Nat, +} + /// # Canister ID Range. /// /// A closed range of canister IDs, both endpoints inclusive. diff --git a/packages/ic-management-canister-types/tests/candid_equality.rs b/packages/ic-management-canister-types/tests/candid_equality.rs index 5fe869aa6901..6b7c13565fc2 100644 --- a/packages/ic-management-canister-types/tests/candid_equality.rs +++ b/packages/ic-management-canister-types/tests/candid_equality.rs @@ -132,6 +132,11 @@ fn node_metrics_history(_: NodeMetricsHistoryArgs) -> NodeMetricsHistoryResult { unimplemented!() } +#[candid_method(update)] +fn subnet_metrics(_: SubnetMetricsArgs) -> SubnetMetricsResult { + unimplemented!() +} + #[candid_method(update)] fn provisional_create_canister_with_cycles( _: ProvisionalCreateCanisterWithCyclesArgs, diff --git a/packages/ic-management-canister-types/tests/ic.did b/packages/ic-management-canister-types/tests/ic.did index 773adfbc6d61..43b36866bc7c 100644 --- a/packages/ic-management-canister-types/tests/ic.did +++ b/packages/ic-management-canister-types/tests/ic.did @@ -439,6 +439,25 @@ type node_metrics_history_result = vec record { node_metrics : vec node_metrics; }; +type subnet_metrics_args = record { + subnet_id : principal; +}; + +type subnet_metrics_result = record { + // Current block height of the subnet, i.e. the height of the block in + // whose execution this call is processed. + block_height : nat; + // Current number of canisters on the subnet. + num_canisters : nat; + // Current total size in bytes of the state taken by canisters on the subnet. + canister_state_bytes : nat; + // Total cycles removed from circulation on the subnet by all current and + // deleted canisters. + consumed_cycles_total : nat; + // Total number of transactions processed on the subnet. + update_transactions_total : nat; +}; + type subnet_info_args = record { subnet_id : principal; }; @@ -701,6 +720,7 @@ service ic : { // metrics interface node_metrics_history : (node_metrics_history_args) -> (node_metrics_history_result); + subnet_metrics : (subnet_metrics_args) -> (subnet_metrics_result); // subnet info subnet_info : (subnet_info_args) -> (subnet_info_result); diff --git a/rs/canonical_state/src/encoding.rs b/rs/canonical_state/src/encoding.rs index 96203a2df8f4..53598f4dd9b1 100644 --- a/rs/canonical_state/src/encoding.rs +++ b/rs/canonical_state/src/encoding.rs @@ -145,5 +145,6 @@ mod tests { mod compatibility; mod conversion; mod encoding; + mod subnet_metrics; mod test_fixtures; } diff --git a/rs/canonical_state/src/encoding/tests/subnet_metrics.rs b/rs/canonical_state/src/encoding/tests/subnet_metrics.rs new file mode 100644 index 000000000000..b10cd250db30 --- /dev/null +++ b/rs/canonical_state/src/encoding/tests/subnet_metrics.rs @@ -0,0 +1,78 @@ +//! Cross-checks the `subnet_metrics` management canister method's +//! `consumed_cycles_total` against the canonical (certified) state encoding. + +use crate::CertificationVersion; +use crate::encoding::types::SubnetMetrics as CanonicalSubnetMetrics; +use ic_replicated_state::CanisterStates; +use ic_replicated_state::metadata_state::SubnetMetrics; +use ic_test_utilities_state::new_canister_state; +use ic_test_utilities_types::ids::{canister_test_id, user_test_id}; +use ic_types::NumBytes; +use ic_types_cycles::{ + CanisterCyclesCostSchedule, CompoundCycles, Cycles, Instructions, NominalCycles, + NominalCyclesTesting, +}; +use std::sync::Arc; + +/// The `consumed_cycles_total` that `ExecutionEnvironment::subnet_metrics` +/// computes must equal the one that the canonical state encoding produces at +/// certification version `V29`. +/// +// Keep in sync with `ExecutionEnvironment::subnet_metrics` in +// `rs/execution_environment/src/execution_environment.rs`, which carries the +// reciprocal comment. +#[test] +fn subnet_metrics_consumed_cycles_matches_v29_canonical_encoding() { + let mut metrics = SubnetMetrics::default(); + metrics.num_canisters = 3; + metrics.canister_state_bytes = NumBytes::new(1_234); + metrics.update_transactions_total = 42; + metrics.observe_consumed_cycles_by_deleted_canisters(NominalCycles::new(1_000_000_007)); + metrics.observe_consumed_cycles_http_outcalls(NominalCycles::new(2_000_000_011)); + + let mut canisters = CanisterStates::default(); + for id in 1..=3_u64 { + let mut canister = new_canister_state( + canister_test_id(id), + user_test_id(1).get(), + Cycles::new(1 << 60), + ic_base_types::NumSeconds::new(100_000), + ); + canister + .system_state + .consume_cycles(CompoundCycles::::new( + Cycles::new(100_000 * id as u128), + CanisterCyclesCostSchedule::Normal, + )); + canisters.insert(Arc::new(canister)); + } + + // What the `subnet_metrics` handler computes. + let handler_total = metrics.consumed_cycles_total() + canisters.total_consumed_cycles(); + + // What the certified state tree reports at `V29`, recombined from its + // `(high, low)` parts. + let canonical = CanonicalSubnetMetrics::from(( + &metrics, + canisters.total_consumed_cycles(), + CertificationVersion::V29, + )); + let low = canonical.consumed_cycles_total.low; + let high = canonical.consumed_cycles_total.high.unwrap(); + let canonical_total = ((high as u128) << 64) | (low as u128); + + assert_eq!(handler_total.get(), canonical_total); + // The test would be vacuous if both were zero. + assert!(canonical_total > 0); + + // The other three fields pass through unchanged. + assert_eq!(canonical.num_canisters, metrics.num_canisters); + assert_eq!( + canonical.canister_state_bytes, + metrics.canister_state_bytes.get() + ); + assert_eq!( + canonical.update_transactions_total, + metrics.update_transactions_total + ); +} diff --git a/rs/embedders/src/wasmtime_embedder/system_api/routing.rs b/rs/embedders/src/wasmtime_embedder/system_api/routing.rs index 9540b8b59721..49934509ccf6 100644 --- a/rs/embedders/src/wasmtime_embedder/system_api/routing.rs +++ b/rs/embedders/src/wasmtime_embedder/system_api/routing.rs @@ -14,9 +14,9 @@ use ic_management_canister_types_private::{ NodeMetricsHistoryArgs, Payload, ProvisionalTopUpCanisterArgs, ReadCanisterSnapshotDataArgs, ReadCanisterSnapshotMetadataArgs, RenameCanisterArgs, ReshareChainKeyArgs, SchnorrPublicKeyArgs, SetupInitialDKGArgs, SignWithECDSAArgs, SignWithSchnorrArgs, - StoredChunksArgs, SubnetInfoArgs, TakeCanisterSnapshotArgs, UninstallCodeArgs, - UpdateSettingsArgs, UploadCanisterSnapshotDataArgs, UploadCanisterSnapshotMetadataArgs, - UploadChunkArgs, VetKdDeriveKeyArgs, VetKdPublicKeyArgs, + StoredChunksArgs, SubnetInfoArgs, SubnetMetricsArgs, TakeCanisterSnapshotArgs, + UninstallCodeArgs, UpdateSettingsArgs, UploadCanisterSnapshotDataArgs, + UploadCanisterSnapshotMetadataArgs, UploadChunkArgs, VetKdDeriveKeyArgs, VetKdPublicKeyArgs, }; use ic_replicated_state::NetworkTopology; use itertools::Itertools; @@ -202,6 +202,28 @@ pub(super) fn resolve_destination( Ok(Ic00Method::NodeMetricsHistory) => { Ok(NodeMetricsHistoryArgs::decode(payload)?.subnet_id) } + Ok(Ic00Method::SubnetMetrics) => { + // Rejected explicitly, mirroring `FetchCanisterLogs` below, rather than + // relying on the composite-query path failing later in + // `QueryContext::handle_request` (where `get_active_canister` cannot + // resolve a subnet principal). That indirect guarantee holds today, but + // it would evaporate the moment `subnet_metrics` were added to + // `QueryMethod`: the query path has no round-instruction accounting, so + // the `O(|hot canisters|)` fold would run unmetered on query threads + // against a different state snapshot. Keeping the rejection here makes + // that a compile-time-visible decision rather than an accident. + if is_composite_query { + Err(ResolveDestinationError::UserError(UserError::new( + ic_error_types::ErrorCode::CanisterRejectedMessage, + format!( + "{} API cannot be called from a composite query", + Ic00Method::SubnetMetrics + ), + ))) + } else { + Ok(SubnetMetricsArgs::decode(payload)?.subnet_id) + } + } Ok(Ic00Method::SubnetInfo) => Ok(SubnetInfoArgs::decode(payload)?.subnet_id), Ok(Ic00Method::FetchCanisterLogs) => { if is_composite_query { @@ -1204,4 +1226,67 @@ mod tests { }; } } + + /// `subnet_metrics` names its target subnet in the payload, so an ordinary + /// (non-composite-query) call routes there. + #[test] + fn resolve_subnet_metrics_routes_to_named_subnet() { + let logger = no_op_logger(); + let target_subnet = subnet_test_id(1); + assert_eq!( + resolve_destination( + &network_with_ecdsa_subnets(), + &Ic00Method::SubnetMetrics.to_string(), + &Encode!(&SubnetMetricsArgs { + subnet_id: target_subnet.get() + }) + .unwrap(), + subnet_test_id(2), + canister_test_id(1), + false, + &logger, + ) + .unwrap(), + target_subnet.get() + ); + } + + /// ...but a composite query is rejected outright, mirroring + /// `fetch_canister_logs`. + /// + /// The composite-query path has no round-instruction accounting, so the + /// `O(|hot canisters|)` fold that `subnet_metrics` performs must never run on + /// query threads. This is the in-process guard for that arm; the system test + /// `subnet_metrics_composite_query_fails` asserts the same thing end to end but + /// is Linux-only. + #[test] + fn resolve_subnet_metrics_rejects_composite_query() { + let logger = no_op_logger(); + let err = resolve_destination( + &network_with_ecdsa_subnets(), + &Ic00Method::SubnetMetrics.to_string(), + &Encode!(&SubnetMetricsArgs { + subnet_id: subnet_test_id(1).get() + }) + .unwrap(), + subnet_test_id(2), + canister_test_id(1), + true, + &logger, + ) + .unwrap_err(); + match err { + ResolveDestinationError::UserError(err) => { + assert_eq!( + err.code(), + ic_error_types::ErrorCode::CanisterRejectedMessage + ); + assert_eq!( + err.description(), + "subnet_metrics API cannot be called from a composite query" + ); + } + other => panic!("Unexpected error: {other:?}"), + } + } } diff --git a/rs/embedders/src/wasmtime_embedder/system_api/sandbox_safe_system_state.rs b/rs/embedders/src/wasmtime_embedder/system_api/sandbox_safe_system_state.rs index 55664fe5ddb6..132e81724876 100644 --- a/rs/embedders/src/wasmtime_embedder/system_api/sandbox_safe_system_state.rs +++ b/rs/embedders/src/wasmtime_embedder/system_api/sandbox_safe_system_state.rs @@ -338,6 +338,7 @@ impl SystemStateModifications { | Ok(Ic00Method::BitcoinSendTransaction) | Ok(Ic00Method::BitcoinGetCurrentFeePercentiles) | Ok(Ic00Method::NodeMetricsHistory) + | Ok(Ic00Method::SubnetMetrics) | Ok(Ic00Method::SubnetInfo) | Ok(Ic00Method::FetchCanisterLogs) | Ok(Ic00Method::UploadChunk) diff --git a/rs/execution_environment/benches/management_canister/main.rs b/rs/execution_environment/benches/management_canister/main.rs index 403120c5f122..c2858b62de1a 100644 --- a/rs/execution_environment/benches/management_canister/main.rs +++ b/rs/execution_environment/benches/management_canister/main.rs @@ -6,6 +6,7 @@ mod ecdsa; mod http_request; mod install_code; mod list_canisters; +mod subnet_metrics; mod update_settings; mod utils; @@ -20,6 +21,7 @@ fn all_benchmarks(c: &mut Criterion) { http_request::http_request_benchmark(c); install_code::install_code_benchmark(c); list_canisters::list_canisters_benchmark(c); + subnet_metrics::subnet_metrics_benchmark(c); update_settings::update_settings_benchmark(c); } diff --git a/rs/execution_environment/benches/management_canister/subnet_metrics.rs b/rs/execution_environment/benches/management_canister/subnet_metrics.rs new file mode 100644 index 000000000000..2e5d7301d157 --- /dev/null +++ b/rs/execution_environment/benches/management_canister/subnet_metrics.rs @@ -0,0 +1,224 @@ +use crate::create_canisters::CreateCanistersArgs; +use crate::utils::{CANISTERS_PER_BATCH, expect_reply, test_canister_wasm}; +use candid::{Encode, Principal}; +use criterion::{BenchmarkGroup, Criterion, criterion_group, criterion_main}; +use ic_base_types::{CanisterId, NumBytes, NumSeconds}; +use ic_config::execution_environment::Config as HypervisorConfig; +use ic_config::subnet_config::SubnetConfig; +use ic_registry_subnet_type::SubnetType; +use ic_replicated_state::canister_state::canister_snapshots::CanisterSnapshots; +use ic_replicated_state::canister_state::system_state::SystemState; +use ic_replicated_state::{CanisterState, CanisterStates, SchedulerState}; +use ic_state_machine_tests::{StateMachine, StateMachineBuilder, StateMachineConfig}; +use ic_test_utilities_types::ids::canister_test_id; +use ic_types_cycles::{CanisterCyclesCostSchedule, CompoundCycles, Cycles, Instructions}; +use std::sync::Arc; + +/// Builds a `StateMachine` and populates the subnet with `canisters_number` +/// canisters, created through a test canister via batched inter-canister calls. +/// Returns the `StateMachine` and the test canister ID. +/// +/// `subnet_metrics` is canister-only, so the call must go through the test +/// canister; unlike `list_canisters` it needs no subnet-admin setup. +fn setup_with_canisters(canisters_number: u64) -> (StateMachine, CanisterId) { + let env = StateMachineBuilder::new() + .with_config(Some(StateMachineConfig::new( + SubnetConfig::new(SubnetType::Application), + HypervisorConfig::default(), + ))) + .with_subnet_type(SubnetType::Application) + .with_cost_schedule(CanisterCyclesCostSchedule::Free) + .build(); + + let test_canister = env.create_canister_with_cycles(None, Cycles::new(u128::MAX / 2), None); + env.install_existing_canister(test_canister, test_canister_wasm(), vec![]) + .expect("failed to install the test canister"); + + const CHUNK: u64 = 5_000; + let mut remaining_to_create = canisters_number; + while remaining_to_create > 0 { + let chunk = remaining_to_create.min(CHUNK); + remaining_to_create -= chunk; + let result = env.execute_ingress( + test_canister, + "create_canisters", + Encode!(&CreateCanistersArgs { + canisters_number: chunk, + canisters_per_batch: CANISTERS_PER_BATCH, + initial_cycles: 0, + }) + .unwrap(), + ); + let created: Vec = expect_reply(result); + assert_eq!(created.len() as u64, chunk); + } + + (env, test_canister) +} + +/// Measures the end-to-end cost of one `subnet_metrics` call on a subnet with +/// `canisters_number` canisters. This is what `BASE_INSTRUCTIONS` in +/// `subnet_metrics_instructions` must cover: message induction, the reads from +/// `state.metadata.subnet_metrics`, the fold over the hot pool, and the Candid +/// encode. +/// +/// Note that the canisters created during setup are demoted to the cold pool +/// after a round of inactivity (`repartition_canister_states` runs on every +/// commit), so this measurement deliberately does *not* capture the +/// per-hot-canister term — which is also why the charge must be keyed on +/// `hot_len()` rather than `num_canisters()`: on a mostly-cold subnet the two +/// differ by orders of magnitude while the work does not. The per-hot-canister +/// term is measured by `bench_consumed_cycles_fold`. +fn bench_end_to_end( + group: &mut BenchmarkGroup, + bench_name: &str, + canisters_number: u64, +) { + // `subnet_metrics` is read-only, so the environment (and its set of + // canisters) does not change across iterations and can be set up once. + let (env, test_canister) = setup_with_canisters(canisters_number); + let subnet_id: Principal = env.get_subnet_id().get().into(); + group.bench_function(bench_name, |b| { + b.iter(|| { + let result = env.execute_ingress( + test_canister, + "subnet_metrics", + Encode!(&subnet_id).unwrap(), + ); + let _num_canisters: u64 = expect_reply(result); + }); + }); +} + +/// Builds one hot canister with non-zero consumed cycles. +/// +/// A non-zero `heap_delta_debit` keeps a canister out of the cold pool +/// (`CanisterState::is_cold`), which is what makes a fully hot pool the worst case +/// for `CanisterStates::total_consumed_cycles()`: the fold is `O(|hot|)`, the cold +/// pool being a precomputed aggregate. +fn hot_canister(id: u64) -> Arc { + let mut system_state = SystemState::new_running_for_testing( + canister_test_id(id), + canister_test_id(u64::MAX).get(), + Cycles::new(1 << 60), + NumSeconds::new(100_000), + ); + system_state.consume_cycles(CompoundCycles::::new( + Cycles::new(1_000 + id as u128), + CanisterCyclesCostSchedule::Normal, + )); + Arc::new(CanisterState::new( + system_state, + None, + SchedulerState { + heap_delta_debit: NumBytes::new(1), + ..SchedulerState::default() + }, + CanisterSnapshots::default(), + )) +} + +/// Builds a `CanisterStates` holding `canisters_number` hot canisters, allocated +/// and inserted in ascending canister-ID order. +/// +/// This is the *favourable* memory layout: the `BTreeMap` nodes and the `Arc` +/// payloads are laid out in the order the fold visits them. +fn hot_canister_states(canisters_number: u64) -> CanisterStates { + let mut states = CanisterStates::default(); + for id in 0..canisters_number { + states.insert(hot_canister(id)); + } + assert_eq!(states.hot_len() as u64, canisters_number); + states +} + +/// As [`hot_canister_states`], but with the allocation and insertion order +/// scrambled and with allocator churn interleaved, so the `BTreeMap` nodes and the +/// `Arc` payloads are scattered rather than laid out in visit +/// order. +/// +/// This is the adversarial-locality variant, and it is the one +/// `INSTRUCTIONS_PER_HOT_CANISTER` is justified against: a production hot pool is +/// built up over a long period from independently allocated, long-lived canisters, +/// not in one tight loop. In practice it measures only ~13% above the favourable +/// layout, because `size_of::()` is ~2.5KB, so at 100k canisters the +/// pool is ~254MB and the fold is DRAM-bound either way. +fn shuffled_hot_canister_states(canisters_number: u64) -> CanisterStates { + /// Deterministic pseudo-random value, so the benchmark needs no RNG + /// dependency and is reproducible run to run. + fn scramble(i: u64) -> u64 { + let mut x = i.wrapping_mul(0x9E37_79B9_7F4A_7C15); + x ^= x >> 31; + x.wrapping_mul(0xBF58_476D_1CE4_E5B9) + } + + // Fisher-Yates over `0..n`, rather than rejection-sampling a scrambled index + // until every residue has been hit: this is `O(n)` with a static termination + // bound, where the rejection loop terminates only in expectation (~12n + // iterations by coupon collector, and in principle never). + let mut order: Vec = (0..canisters_number).collect(); + for i in (1..order.len()).rev() { + order.swap(i, (scramble(i as u64) % (i as u64 + 1)) as usize); + } + + let mut states = CanisterStates::default(); + let mut ballast: Vec> = Vec::new(); + for (inserted, id) in order.into_iter().enumerate() { + // Churn: allocate, keep some, free some, so canister allocations are + // interleaved with unrelated live objects. + ballast.push(vec![0_u8; 4096]); + if ballast.len() > 64 { + let victim = inserted % ballast.len(); + ballast.swap_remove(victim); + } + states.insert(hot_canister(id)); + } + // Drop the ballast, leaving holes in the heap. + drop(ballast); + assert_eq!(states.hot_len() as u64, canisters_number); + states +} + +/// Measures `CanisterStates::total_consumed_cycles()` over a fully hot pool. +/// The slope of this measurement is what `INSTRUCTIONS_PER_HOT_CANISTER` in +/// `subnet_metrics_instructions` must cover. +fn bench_consumed_cycles_fold( + group: &mut BenchmarkGroup, + bench_name: &str, + states: CanisterStates, +) { + group.bench_function(bench_name, |b| { + b.iter(|| std::hint::black_box(states.total_consumed_cycles())); + }); +} + +pub fn subnet_metrics_benchmark(c: &mut Criterion) { + let mut group = c.benchmark_group("subnet_metrics"); + bench_end_to_end(&mut group, "end_to_end/10", 10); + bench_end_to_end(&mut group, "end_to_end/1k", 1_000); + bench_end_to_end(&mut group, "end_to_end/10k", 10_000); + group.finish(); + + let mut group = c.benchmark_group("subnet_metrics_consumed_cycles_fold"); + for n in [0_u64, 1_000, 10_000, 100_000] { + let label = match n { + 0 => "0".to_string(), + n if n % 1_000 == 0 => format!("{}k", n / 1_000), + n => n.to_string(), + }; + bench_consumed_cycles_fold( + &mut group, + &format!("hot/{label}/sequential"), + hot_canister_states(n), + ); + bench_consumed_cycles_fold( + &mut group, + &format!("hot/{label}/shuffled"), + shuffled_hot_canister_states(n), + ); + } + group.finish(); +} + +criterion_group!(benchmarks, subnet_metrics_benchmark); +criterion_main!(benchmarks); diff --git a/rs/execution_environment/benches/management_canister/test_canister/candid.did b/rs/execution_environment/benches/management_canister/test_canister/candid.did index 72b2eba18e98..91194ea69e39 100644 --- a/rs/execution_environment/benches/management_canister/test_canister/candid.did +++ b/rs/execution_environment/benches/management_canister/test_canister/candid.did @@ -45,4 +45,5 @@ service : { "sign_with_ecdsa" : (ecdsa_args) -> (); "http_request" : (http_request_args) -> (); "list_canisters" : () -> (nat64); + "subnet_metrics" : (principal) -> (nat64); }; diff --git a/rs/execution_environment/benches/management_canister/test_canister/src/main.rs b/rs/execution_environment/benches/management_canister/test_canister/src/main.rs index cd99e2e6fce4..50db733acdb2 100644 --- a/rs/execution_environment/benches/management_canister/test_canister/src/main.rs +++ b/rs/execution_environment/benches/management_canister/test_canister/src/main.rs @@ -309,4 +309,32 @@ async fn list_canisters() -> u64 { result.canisters.len() as u64 } +#[derive(Clone, Debug, CandidType, Deserialize, Serialize)] +pub struct SubnetMetricsArgs { + pub subnet_id: Principal, +} + +#[derive(Clone, Debug, CandidType, Deserialize, Serialize)] +pub struct SubnetMetricsResult { + pub block_height: candid::Nat, + pub num_canisters: candid::Nat, + pub canister_state_bytes: candid::Nat, + pub consumed_cycles_total: candid::Nat, + pub update_transactions_total: candid::Nat, +} + +/// Calls the management canister's `subnet_metrics` method for the given subnet +/// and returns the reported number of canisters. +#[update] +async fn subnet_metrics(subnet_id: Principal) -> u64 { + let result: SubnetMetricsResult = + Call::unbounded_wait(Principal::management_canister(), "subnet_metrics") + .with_arg(SubnetMetricsArgs { subnet_id }) + .await + .expect("subnet_metrics call failed") + .candid() + .expect("failed to decode subnet_metrics response"); + u64::try_from(result.num_canisters.0).expect("num_canisters does not fit into u64") +} + fn main() {} diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 5f9517d47a43..8bebab745ed4 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -161,6 +161,10 @@ impl CanisterManager { | Ok(Ic00Method::BitcoinSendTransactionInternal) | Ok(Ic00Method::BitcoinGetCurrentFeePercentiles) | Ok(Ic00Method::NodeMetricsHistory) + // Unreachable for `SubnetMetrics`: `extract_effective_canister_id` + // rejects it earlier, at the ingress filter. Listed for exhaustiveness + // and as defence in depth. + | Ok(Ic00Method::SubnetMetrics) | Ok(Ic00Method::SubnetInfo) // `RenameCanister` can only be called from the NNS subnet. | Ok(Ic00Method::RenameCanister) => Err(UserError::new( diff --git a/rs/execution_environment/src/canister_manager/tests.rs b/rs/execution_environment/src/canister_manager/tests.rs index 64e37a4e6404..9502071a8630 100644 --- a/rs/execution_environment/src/canister_manager/tests.rs +++ b/rs/execution_environment/src/canister_manager/tests.rs @@ -45,8 +45,8 @@ use ic_management_canister_types_private::{ InstallCodeArgsV2, Method, NodeMetricsHistoryArgs, NodeMetricsHistoryResponse, OnLowWasmMemoryHookStatus, Payload, ProvisionalCreateCanisterWithCyclesArgs, RenameCanisterArgs, RenameToArgs, StoredChunksArgs, StoredChunksReply, SubnetInfoArgs, - SubnetInfoResponse, TakeCanisterSnapshotArgs, UpdateSettingsArgs, UploadChunkArgs, - UploadChunkReply, WasmMemoryPersistence, + SubnetInfoResponse, SubnetMetricsArgs, SubnetMetricsResponse, TakeCanisterSnapshotArgs, + UpdateSettingsArgs, UploadChunkArgs, UploadChunkReply, WasmMemoryPersistence, }; use ic_metrics::MetricsRegistry; use ic_registry_provisional_whitelist::ProvisionalWhitelist; @@ -6150,6 +6150,298 @@ fn subnet_info_ingress_fails() { ); } +/// Sends the given payload to `subnet_metrics` as an inter-canister call from a +/// canister on a remote subnet, executes it, and returns the decoded response or +/// the reject. +fn subnet_metrics_raw_call( + test: &mut ExecutionTest, + payload: Vec, +) -> Result { + test.inject_call_to_ic00(Method::SubnetMetrics, payload, Cycles::zero()); + test.execute_subnet_message(); + // Route the response back towards the caller (on a different subnet) so that + // it can be inspected via `xnet_messages`. + test.induct_messages(); + let index = test.xnet_messages().len() - 1; + match &test.get_xnet_response(index).response_payload { + ic_types::messages::Payload::Data(bytes) => { + Ok(Decode!(bytes, SubnetMetricsResponse).unwrap()) + } + ic_types::messages::Payload::Reject(context) => { + Err((context.code(), context.message().to_string())) + } + } +} + +/// As [`subnet_metrics_raw_call`], with a well-formed payload naming `subnet_id`. +fn subnet_metrics_call( + test: &mut ExecutionTest, + subnet_id: PrincipalId, +) -> Result { + subnet_metrics_raw_call(test, SubnetMetricsArgs { subnet_id }.encode()) +} + +#[test] +fn subnet_metrics_canister_call_succeeds() { + let own_subnet_id = subnet_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .build(); + let uni_canister = test + .universal_canister_with_cycles(Cycles::new(1_000_000_000_000)) + .unwrap(); + let payload = SubnetMetricsArgs { + subnet_id: own_subnet_id.get(), + } + .encode(); + let uc_call = wasm() + .call_simple( + CanisterId::ic_00(), + Method::SubnetMetrics, + call_args().other_side(payload), + ) + .build(); + let result = test.ingress(uni_canister, "update", uc_call).unwrap(); + let bytes = match result { + WasmResult::Reply(bytes) => bytes, + WasmResult::Reject(err_msg) => panic!("Unexpected reject, expected reply: {err_msg}"), + }; + let response = Decode!(&bytes, SubnetMetricsResponse).unwrap(); + // All five fields decode. `ExecutionTest` starts at round 1 and does not run + // message routing, so only `block_height` and the live cycles fold have + // non-default values here; the other fields are covered by + // `subnet_metrics_reflects_subnet_metrics_state`. + assert_eq!(response.block_height, candid::Nat::from(1_u64)); + assert_eq!( + response.num_canisters, + candid::Nat::from(test.state().metadata.subnet_metrics.num_canisters) + ); + assert_eq!( + response.canister_state_bytes, + candid::Nat::from( + test.state() + .metadata + .subnet_metrics + .canister_state_bytes + .get() + ) + ); + assert!(response.consumed_cycles_total > 0_u64); + assert_eq!( + response.update_transactions_total, + candid::Nat::from( + test.state() + .metadata + .subnet_metrics + .update_transactions_total + ) + ); +} + +#[test] +fn subnet_metrics_block_height_matches_current_round() { + let own_subnet_id = subnet_test_id(1); + let caller_canister = canister_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .with_caller(subnet_test_id(2), caller_canister) + .with_current_round(42) + .build(); + + let response = subnet_metrics_call(&mut test, own_subnet_id.get()).unwrap(); + assert_eq!(response.block_height, candid::Nat::from(42_u64)); +} + +#[test] +fn subnet_metrics_block_height_is_non_decreasing() { + let own_subnet_id = subnet_test_id(1); + let caller_canister = canister_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .with_caller(subnet_test_id(2), caller_canister) + .with_current_round(7) + .build(); + + let first = subnet_metrics_call(&mut test, own_subnet_id.get()).unwrap(); + test.set_current_round(8); + let second = subnet_metrics_call(&mut test, own_subnet_id.get()).unwrap(); + + assert_eq!(first.block_height, candid::Nat::from(7_u64)); + assert_eq!(second.block_height, candid::Nat::from(8_u64)); + assert!(second.block_height > first.block_height); +} + +#[test] +fn subnet_metrics_ingress_update_fails_at_ingress_filter() { + let own_subnet_id = subnet_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .build(); + let payload = SubnetMetricsArgs { + subnet_id: own_subnet_id.get(), + } + .encode(); + + let result = test.should_accept_ingress_message(IC_00, Method::SubnetMetrics, payload); + assert_eq!( + result, + Err(UserError::new( + ErrorCode::CanisterRejectedMessage, + "ic00 method subnet_metrics can not be called via ingress messages" + )) + ); +} + +#[test] +fn subnet_metrics_ingress_update_fails_at_execution() { + let own_subnet_id = subnet_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .build(); + let payload = SubnetMetricsArgs { + subnet_id: own_subnet_id.get(), + } + .encode(); + test.subnet_message(Method::SubnetMetrics, payload) + .unwrap_err() + .assert_contains( + ErrorCode::CanisterContractViolation, + "subnet_metrics cannot be called by a user", + ); +} + +#[test] +fn subnet_metrics_ingress_query_fails() { + let own_subnet_id = subnet_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .build(); + let payload = SubnetMetricsArgs { + subnet_id: own_subnet_id.get(), + } + .encode(); + test.non_replicated_query(CanisterId::ic_00(), "subnet_metrics", payload) + .unwrap_err() + .assert_contains( + ErrorCode::CanisterMethodNotFound, + "Query method subnet_metrics not found.", + ); +} + +#[test] +fn subnet_metrics_foreign_subnet_id_is_rejected() { + let own_subnet_id = subnet_test_id(1); + let other_subnet_id = subnet_test_id(3); + let caller_canister = canister_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .with_caller(subnet_test_id(2), caller_canister) + .build(); + + let (code, message) = subnet_metrics_call(&mut test, other_subnet_id.get()).unwrap_err(); + assert_eq!(code, RejectCode::CanisterReject); + assert!( + message.contains("does not match current subnet ID"), + "unexpected reject message: {message}" + ); +} + +#[test] +fn subnet_metrics_reflects_subnet_metrics_state() { + let own_subnet_id = subnet_test_id(1); + let caller_canister = canister_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .with_caller(subnet_test_id(2), caller_canister) + .build(); + // Create a canister so that the fold over canisters is non-trivial. + let canister_id = test.create_canister(Cycles::new(1_000_000_000_000)); + let cost_schedule = test.state().get_own_subnet_cycles_config().cost_schedule; + test.canister_state_mut(canister_id) + .system_state + .consume_cycles(CompoundCycles::::new( + Cycles::new(1_000_000), + cost_schedule, + )); + + let deleted_cycles = NominalCycles::new(987_654_321_u128); + { + let metrics = &mut test.state_mut().metadata.subnet_metrics; + metrics.num_canisters = 17; + metrics.canister_state_bytes = NumBytes::new(4_321); + metrics.update_transactions_total = 99; + metrics.observe_consumed_cycles_by_deleted_canisters(deleted_cycles); + } + // Computed independently of the handler: the sum over all canisters plus the + // subnet-level aggregate. + let expected_consumed_cycles = test.state().metadata.subnet_metrics.consumed_cycles_total() + + test + .state() + .canister_states() + .all_values() + .fold(NominalCycles::zero(), |acc, canister| { + acc + canister.system_state.canister_metrics().consumed_cycles() + }); + assert!(expected_consumed_cycles > deleted_cycles); + + let response = subnet_metrics_call(&mut test, own_subnet_id.get()).unwrap(); + + assert_eq!(response.num_canisters, candid::Nat::from(17_u64)); + assert_eq!(response.canister_state_bytes, candid::Nat::from(4_321_u64)); + assert_eq!( + response.update_transactions_total, + candid::Nat::from(99_u64) + ); + assert_eq!( + response.consumed_cycles_total, + candid::Nat::from(expected_consumed_cycles.get()) + ); +} + +#[test] +fn subnet_metrics_is_partition_independent() { + let own_subnet_id = subnet_test_id(1); + let caller_canister = canister_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .with_caller(subnet_test_id(2), caller_canister) + .build(); + let canister_id = test.create_canister(Cycles::new(1_000_000_000_000)); + let cost_schedule = test.state().get_own_subnet_cycles_config().cost_schedule; + test.canister_state_mut(canister_id) + .system_state + .consume_cycles(CompoundCycles::::new( + Cycles::new(1_000_000), + cost_schedule, + )); + + let before = subnet_metrics_call(&mut test, own_subnet_id.get()).unwrap(); + test.state_mut().repartition_canister_states(); + let after = subnet_metrics_call(&mut test, own_subnet_id.get()).unwrap(); + + assert_eq!(before.consumed_cycles_total, after.consumed_cycles_total); +} + +#[test] +fn subnet_metrics_malformed_payload_is_rejected() { + let own_subnet_id = subnet_test_id(1); + let caller_canister = canister_test_id(1); + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet_id) + .with_caller(subnet_test_id(2), caller_canister) + .build(); + + let (code, message) = + subnet_metrics_raw_call(&mut test, EmptyBlob.encode()).expect_err("expected a reject"); + // The Candid decode failure surfaces as `ErrorCode::InvalidManagementPayload` + // (`candid_error_to_user_error`), which maps to `RejectCode::CanisterReject`. + assert_eq!(code, RejectCode::CanisterReject); + assert!( + message.contains("Error decoding candid"), + "unexpected reject message: {message}" + ); +} + #[test] fn node_metrics_history_update_succeeds() { let own_subnet_id = subnet_test_id(1); diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs index a99aba704e85..0907959ee0ac 100644 --- a/rs/execution_environment/src/execution_environment.rs +++ b/rs/execution_environment/src/execution_environment.rs @@ -42,10 +42,10 @@ use ic_management_canister_types_private::{ ReadCanisterSnapshotDataArgs, ReadCanisterSnapshotMetadataArgs, RenameCanisterArgs, ReshareChainKeyArgs, SchnorrAlgorithm, SchnorrPublicKeyArgs, SchnorrPublicKeyResponse, SetupInitialDKGArgs, SignWithECDSAArgs, SignWithSchnorrArgs, SignWithSchnorrAux, - StoredChunksArgs, SubnetInfoArgs, SubnetInfoResponse, TakeCanisterSnapshotArgs, - UninstallCodeArgs, UpdateSettingsArgs, UploadCanisterSnapshotDataArgs, - UploadCanisterSnapshotMetadataArgs, UploadChunkArgs, VetKdDeriveKeyArgs, VetKdPublicKeyArgs, - VetKdPublicKeyResult, + StoredChunksArgs, SubnetInfoArgs, SubnetInfoResponse, SubnetMetricsArgs, SubnetMetricsResponse, + TakeCanisterSnapshotArgs, UninstallCodeArgs, UpdateSettingsArgs, + UploadCanisterSnapshotDataArgs, UploadCanisterSnapshotMetadataArgs, UploadChunkArgs, + VetKdDeriveKeyArgs, VetKdPublicKeyArgs, VetKdPublicKeyResult, }; use ic_metrics::MetricsRegistry; use ic_registry_provisional_whitelist::ProvisionalWhitelist; @@ -1860,6 +1860,27 @@ impl ExecutionEnvironment { } }, + Ok(Ic00Method::SubnetMetrics) => match &msg { + CanisterCall::Ingress(_) => { + self.reject_unexpected_ingress(Ic00Method::SubnetMetrics) + } + CanisterCall::Request(_) => { + // Only deduct round instructions for building the response + // when the request is accepted; a rejected call must not + // consume round instructions. + let res = SubnetMetricsArgs::decode(payload) + .and_then(|args| self.subnet_metrics(&state, current_round, args)) + .map(|(res, instructions)| { + round_limits.instructions -= as_round_instructions(instructions); + (res, None) + }); + ExecuteSubnetMessageResult::Finished { + response: res, + refund: msg.take_cycles(), + } + } + }, + Ok(Ic00Method::SubnetInfo) => match &msg { CanisterCall::Ingress(_) => self.reject_unexpected_ingress(Ic00Method::SubnetInfo), CanisterCall::Request(_) => { @@ -3364,6 +3385,60 @@ impl ExecutionEnvironment { Ok(Encode!(&res).unwrap()) } + /// Computes the response to the `subnet_metrics` management canister method, + /// together with the number of round instructions the caller must deduct for + /// computing it. + fn subnet_metrics( + &self, + state: &ReplicatedState, + current_round: ExecutionRound, + args: SubnetMetricsArgs, + ) -> Result<(Vec, NumInstructions), UserError> { + if args.subnet_id != self.own_subnet_id.get() { + return Err(UserError::new( + ErrorCode::CanisterRejectedMessage, + format!( + "Provided target subnet ID {} does not match current subnet ID {}.", + args.subnet_id, self.own_subnet_id + ), + )); + } + let metrics = &state.metadata.subnet_metrics; + // Keep in sync with the certified state tree at + // `/subnet//metrics`: this is the same sum that + // `ic_canonical_state::encoding::types::SubnetMetrics::from` computes + // starting with certification version `V29`. Pinned by + // `subnet_metrics_consumed_cycles_matches_v29_canonical_encoding` in + // `rs/canonical_state`, which carries the reciprocal comment. + // + // `total_consumed_cycles()` reads the derived `ColdStats::consumed_cycles` + // aggregate. + let consumed_cycles_total = + metrics.consumed_cycles_total() + state.canister_states().total_consumed_cycles(); + let res = SubnetMetricsResponse { + // The height of the block in whose execution this call is processed. + // `ExecutionRound` is numerically the finalized consensus block + // height; see `rs/messaging/src/state_machine.rs`. + block_height: candid::Nat::from(current_round.get()), + // `num_canisters` and `update_transactions_total` are written at the + // *end* of a round (`message_routing.rs`, `scheduler.rs`), so a call + // executing in round N reports the end-of-round-(N-1) values. That + // one-round lag is what `read_state` reports for height N-1 too, so the + // two agree; it is nonetheless not literally "current". + num_canisters: candid::Nat::from(metrics.num_canisters), + // Read from the stored `SubnetMetrics` field rather than recomputed + // live, so that the value agrees with the certified state tree. Note + // that message routing only refreshes the stored field every 10 + // rounds by design (`rs/messaging/src/message_routing.rs`), so + // recomputing it here would make `subnet_metrics` disagree with + // `read_state` on 9 rounds out of 10. + canister_state_bytes: candid::Nat::from(metrics.canister_state_bytes.get()), + consumed_cycles_total: candid::Nat::from(consumed_cycles_total.get()), + update_transactions_total: candid::Nat::from(metrics.update_transactions_total), + }; + Ok((Encode!(&res).unwrap(), subnet_metrics_instructions(state))) + } + // Executes an inter-canister response. // // Returns a tuple with the result, along with a flag indicating whether or @@ -4928,6 +5003,115 @@ pub(crate) fn full_subnet_memory_capacity( ) } +/// Computes the number of round instructions consumed by executing the +/// `subnet_metrics` management method against the given state. +/// +/// The dominant cost is `CanisterStates::total_consumed_cycles()`, which folds +/// over the **hot** canister pool only; the cold pool contributes a precomputed +/// `O(1)` aggregate. The variable term is therefore keyed on +/// `CanisterStates::hot_len()`, which is exactly what the fold visits — *not* on +/// `num_canisters()`. +/// +/// Keying on the total would over-charge by the ratio `len / hot_len`, which is +/// large in the steady state: `repartition_canister_states()` runs on every +/// `commit_and_certify` (`rs/state_manager/src/lib.rs`), so at the start of a +/// round the hot pool holds only canisters that were active in the previous one. +/// On a 100k-canister subnet with a few thousand hot canisters that is a ~40x +/// over-charge — i.e. ~40x more of the shared per-round subnet-message budget +/// consumable per call than the call actually costs the subnet, which is denial +/// capacity that is not backed by any work. See the note on inflation below: this +/// is the same mistake in a different guise. +/// +/// **This makes execution depend on the *cardinality* of the hot/cold partition, +/// which is new.** Every prior consumer of the partition is +/// partition-*independent* — `total_canister_memory_usage()` and +/// `total_consumed_cycles()` are `fold(hot) + cold aggregate`, so they yield the +/// same number wherever the split lies. `hot_len()` is a raw count of one side of +/// it, so for the first time *where* the split lies changes an execution result, +/// and hence how many subnet messages fit in a round. The determinism argument is +/// therefore not the one those consumers rely on; it is: +/// +/// 1. `CanisterState::is_cold()` is a pure function of the canister +/// (`rs/replicated_state/src/canister_state.rs`). The one term that reads as +/// time-dependent is not: `has_unexpired_callbacks()` is +/// `!unexpired_callbacks.is_empty()` and takes no `now`, unlike the +/// `has_expired_callbacks(now)` defined just above it, which `is_cold()` does +/// not call. +/// 2. The partition is **never serialized**. A checkpoint stores only the flat +/// canister set; every load path goes through +/// `ReplicatedState::new_from_checkpoint` → `CanisterStates::new`, which +/// re-derives the split from `is_cold()`. So no persisted or +/// attacker-writable value can encode a non-derived partition. +/// 3. `ReplicatedState::repartition_canister_states()` runs **unconditionally** on +/// every `commit_and_certify` (`rs/state_manager/src/lib.rs`, outside the +/// `CertificationScope::Metadata` branch), so the committed partition equals +/// the one `CanisterStates::new` would derive. +/// 4. By (2) and (3) every way a replica can acquire the state for the next round +/// yields the same partition: continuing in memory, restarting from a +/// checkpoint, state sync (same load path), and the catch-up branch of +/// `take_tip`, which clones a snapshot produced by one of the former. +/// +/// Fact (3) is load-bearing and is **pinned by +/// `hot_cold_partition_is_canonical_after_every_commit`** in +/// `rs/state_manager/tests/state_manager.rs`: making that repartition conditional +/// on checkpoint rounds would diverge the charge between a replica that kept +/// running and one that restarted, and that test fails if anyone does. +/// +/// Cost model, using the conversion `2B instructions = 1 second` +/// (i.e. `2M instructions = 1 ms`): +/// - a base cost of 100K instructions (≈50us), and +/// - a variable cost of 40 instructions (≈20ns) per **hot** canister. +/// +/// The variable term is measured by the `subnet_metrics_consumed_cycles_fold` +/// group of `benches/management_canister/subnet_metrics.rs`, which folds over a +/// fully hot pool. Measured per-hot-canister cost at 100K hot canisters: 7.2ns +/// with sequential allocation, 8.2ns with shuffled insertion order and allocator +/// churn (the `hot/…/shuffled` variants), and 13.2ns worst case on a loaded +/// machine — i.e. 14 to 27 instructions. 40 is ≈1.5x the worst observation. +/// +/// Two reasons allocation order barely matters here, so the measurement is not +/// optimistic. `size_of::()` is 2544 bytes, so 100K hot canisters +/// are ≈254MB of separately allocated `Arc` payloads: the working set is +/// DRAM-resident regardless of the order they were created in, which is why +/// shuffling costs only ~13%. And the fold touches one cache line *inside* that +/// fixed-size allocation (`system_state.canister_metrics.consumed_cycles`), so a +/// canister that owns more heap elsewhere — queues, execution state, snapshots — +/// does not make the fold slower. +/// +/// An attacker can pin canisters in the hot pool cheaply (e.g. a `global_timer` +/// set far in the future keeps `is_cold()` false forever). Under this keying that +/// raises the charge in proportion to the work it creates, which is the intent; +/// it cannot be used to make the charge under-state the work. +/// +/// The base covers the per-call work that does not scale with the number of +/// canisters: the Candid decode of the argument, five field reads, and the Candid +/// encode of five `Nat`s. That is well under 50us. It is estimated from that +/// work rather than measured end to end, deliberately on the generous side, and +/// is 200x below `list_canisters`'s 20M. +/// +/// Both constants are far below `list_canisters`'s 20M / 16K. That is +/// intentional: `list_canisters` is gated to subnet admins, whereas +/// `subnet_metrics` is open to any canister with no cycle fee, so overcharging +/// here would let an unauthenticated caller exhaust the per-round subnet-message +/// instruction budget and defer unrelated subnet messages. Do not inflate these +/// to "be safe", and do not key them on a count larger than the work — either +/// widens the denial surface rather than narrowing it. +/// +/// Saturating arithmetic, unlike `list_canisters_instructions`: a release-build +/// wrap would silently produce a small charge and remove the bound this function +/// exists to provide. +// Keep in sync with `SUBNET_METRICS_BASE_INSTRUCTIONS` / +// `SUBNET_METRICS_INSTRUCTIONS_PER_HOT_CANISTER` in `execution_test.rs`. +fn subnet_metrics_instructions(state: &ReplicatedState) -> NumInstructions { + const BASE_INSTRUCTIONS: u64 = 100_000; + const INSTRUCTIONS_PER_HOT_CANISTER: u64 = 40; + let hot_canisters = state.canister_states().hot_len() as u64; + NumInstructions::new( + BASE_INSTRUCTIONS + .saturating_add(INSTRUCTIONS_PER_HOT_CANISTER.saturating_mul(hot_canisters)), + ) +} + fn get_canister( canister_id: CanisterId, state: &ReplicatedState, diff --git a/rs/execution_environment/src/execution_environment_metrics.rs b/rs/execution_environment/src/execution_environment_metrics.rs index b5b6d3a4dd32..b0dc9e965377 100644 --- a/rs/execution_environment/src/execution_environment_metrics.rs +++ b/rs/execution_environment/src/execution_environment_metrics.rs @@ -313,6 +313,7 @@ impl ExecutionEnvironmentMetrics { | ic00::Method::BitcoinSendTransaction | ic00::Method::BitcoinGetCurrentFeePercentiles | ic00::Method::NodeMetricsHistory + | ic00::Method::SubnetMetrics | ic00::Method::SubnetInfo | ic00::Method::FetchCanisterLogs | ic00::Method::ProvisionalCreateCanisterWithCycles diff --git a/rs/execution_environment/src/ic00_permissions.rs b/rs/execution_environment/src/ic00_permissions.rs index 76d39db95c7d..c812d3b366be 100644 --- a/rs/execution_environment/src/ic00_permissions.rs +++ b/rs/execution_environment/src/ic00_permissions.rs @@ -59,6 +59,10 @@ impl Ic00MethodPermissions { | Ic00Method::BitcoinSendTransactionInternal | Ic00Method::BitcoinGetSuccessors | Ic00Method::NodeMetricsHistory + // `counts_toward_round_limit` is never consulted for `SubnetMetrics`: + // the method has no effective canister ID, so it is handled by the + // special case in `Scheduler::can_execute_subnet_msg` instead. + | Ic00Method::SubnetMetrics | Ic00Method::SubnetInfo | Ic00Method::ProvisionalCreateCanisterWithCycles | Ic00Method::ProvisionalTopUpCanister diff --git a/rs/execution_environment/src/scheduler.rs b/rs/execution_environment/src/scheduler.rs index 0204dc5b4d70..158162eea178 100644 --- a/rs/execution_environment/src/scheduler.rs +++ b/rs/execution_environment/src/scheduler.rs @@ -1801,10 +1801,11 @@ fn can_execute_subnet_msg( // Some heavy methods use round instructions. let instructions_reached = round_limits.instructions_reached(); - // `list_canisters` iterates over the subnet's canisters and thus consumes - // round instructions, even though it has no effective canister ID. Defer it - // to a later round if the round instruction limit has already been reached. - if let Some(Ic00Method::ListCanisters) = msg_method { + // `list_canisters` and `subnet_metrics` iterate over the subnet's canisters + // and thus consume round instructions, even though they have no effective + // canister ID. Defer them to a later round if the round instruction limit has + // already been reached. + if let Some(Ic00Method::ListCanisters | Ic00Method::SubnetMetrics) = msg_method { return !instructions_reached; } @@ -1903,6 +1904,7 @@ fn get_instruction_limits_for_subnet_message( | BitcoinGetCurrentFeePercentiles | BitcoinGetSuccessors | NodeMetricsHistory + | SubnetMetrics | SubnetInfo | FetchCanisterLogs | ProvisionalCreateCanisterWithCycles diff --git a/rs/execution_environment/tests/execution_test.rs b/rs/execution_environment/tests/execution_test.rs index 793dde956469..d90ce40f5ad8 100644 --- a/rs/execution_environment/tests/execution_test.rs +++ b/rs/execution_environment/tests/execution_test.rs @@ -14,7 +14,8 @@ use ic_management_canister_types_private::{ CanisterMetricsArgs, CanisterSettingsArgs, CanisterSettingsArgsBuilder, CanisterStatusResultV2, CreateCanisterArgs, DerivationPath, EcdsaKeyId, EmptyBlob, IC_00, InstallCodeArgsV2, ListCanistersResponse, LoadCanisterSnapshotArgs, MasterPublicKeyId, Method, Payload, - SignWithECDSAArgs, TakeCanisterSnapshotArgs, UpdateSettingsArgs, + SignWithECDSAArgs, SubnetMetricsArgs, SubnetMetricsResponse, TakeCanisterSnapshotArgs, + UpdateSettingsArgs, }; use ic_registry_resource_limits::ResourceLimits; use ic_registry_subnet_type::SubnetType; @@ -28,7 +29,9 @@ use ic_test_utilities_metrics::{ use ic_test_utilities_types::ids::user_test_id; use ic_types::ingress::{IngressState, IngressStatus}; use ic_types::messages::MessageId; -use ic_types::{CanisterId, NumBytes, Time, ingress::WasmResult, messages::NO_DEADLINE}; +use ic_types::{ + CanisterId, NumBytes, NumInstructions, Time, ingress::WasmResult, messages::NO_DEADLINE, +}; use ic_types_cycles::{CanisterCyclesCostSchedule, Cycles}; use ic_universal_canister::{UNIVERSAL_CANISTER_WASM, call_args, wasm}; use more_asserts::{assert_ge, assert_gt, assert_le, assert_lt}; @@ -2881,6 +2884,423 @@ fn list_canisters_via_inter_canister_call_rejected_for_non_admin() { assert_eq!(env.subnet_message_instructions(), instructions_baseline); } +/// Keep in sync with `subnet_metrics_instructions` in +/// `rs/execution_environment/src/execution_environment.rs`. +const SUBNET_METRICS_BASE_INSTRUCTIONS: u64 = 100_000; +/// Keep in sync with `subnet_metrics_instructions` in +/// `rs/execution_environment/src/execution_environment.rs`. Note this is per +/// **hot** canister, which is what the fold visits — not per canister. +const SUBNET_METRICS_INSTRUCTIONS_PER_HOT_CANISTER: u64 = 40; + +fn subnet_metrics_count(env: &StateMachine) -> u64 { + fetch_histogram_vec_stats( + env.metrics_registry(), + "execution_subnet_message_duration_seconds", + ) + .get(&labels(&[ + ("method_name", "ic00_subnet_metrics"), + ("outcome", "finished"), + ("status", "success"), + ("speed", "fast"), + ])) + .map_or(0, |stats| stats.count) +} + +fn subnet_metrics_payload(env: &StateMachine) -> Vec { + SubnetMetricsArgs { + subnet_id: env.get_subnet_id().get(), + } + .encode() +} + +/// Builds a `StateMachine` whose round instruction limit is small enough that +/// the derived per-round subnet-message budget +/// (`max_instructions_per_round / SUBNET_MESSAGES_LIMIT_FRACTION`) is only a +/// small multiple of the `subnet_metrics` per-call charge. +/// +/// All four instruction-limit fields must be set together. In particular +/// `max_instructions_per_install_code_slice` defaults to `2 * B`, and the +/// canister round budget is +/// `max_instructions_per_round - max(max_instructions_per_slice, max_instructions_per_install_code_slice) + 1` +/// (see `Scheduler::round_limits` in `rs/execution_environment/src/scheduler.rs`). +/// Leaving the install-code slice at its default would make that budget negative +/// (`80M - 2B + 1 < 0`, and `RoundInstructions` is a signed `i64`), so +/// `RoundInstructions::instructions_reached()` would be true from round start, the +/// inner round would break before any canister message executed, and no +/// `subnet_metrics` call would ever be made. +/// +/// Note that the *production* sizing rule documented at +/// `rs/config/src/subnet_config.rs` — round at least +/// `max(slice, install_code_slice) + 2 * B`, so that a round lasts about a second +/// — cannot hold once the round budget is shrunk below `2 * B`. It is a sizing +/// rule, not a correctness requirement; what execution actually requires is the +/// positive canister round budget asserted below. +fn subnet_metrics_env_with_round_limit(max_instructions_per_round: u64) -> StateMachine { + let slice = max_instructions_per_round / 2; + let mut subnet_config = SubnetConfig::new(SubnetType::Application); + subnet_config.scheduler_config.max_instructions_per_round = + NumInstructions::new(max_instructions_per_round); + subnet_config.scheduler_config.max_instructions_per_slice = NumInstructions::new(slice); + subnet_config.scheduler_config.max_instructions_per_message = NumInstructions::new(slice); + subnet_config + .scheduler_config + .max_instructions_per_install_code_slice = NumInstructions::new(slice); + + // Executable precondition: the canister round budget, recomputed exactly as + // `Scheduler::round_limits` does, must be positive. Otherwise + // `RoundInstructions::instructions_reached()` is true from round start, the + // inner round breaks before executing any canister message, and every test + // built on this environment would pass vacuously. + let canister_round_budget = max_instructions_per_round as i64 + - std::cmp::max( + subnet_config + .scheduler_config + .max_instructions_per_slice + .get(), + subnet_config + .scheduler_config + .max_instructions_per_install_code_slice + .get(), + ) as i64 + + 1; + assert!( + canister_round_budget > 0, + "canister round budget {canister_round_budget} is not positive: \ + max_instructions_per_round ({}) must exceed \ + max(max_instructions_per_slice ({}), max_instructions_per_install_code_slice ({}))", + max_instructions_per_round, + subnet_config.scheduler_config.max_instructions_per_slice, + subnet_config + .scheduler_config + .max_instructions_per_install_code_slice, + ); + + StateMachineBuilder::new() + .with_config(Some(StateMachineConfig::new( + subnet_config, + HypervisorConfig::default(), + ))) + .with_subnet_type(SubnetType::Application) + .with_cost_schedule(CanisterCyclesCostSchedule::Free) + .build() +} + +// `subnet_metrics` consumes round instructions according to its cost model (a +// base cost plus a per-canister cost). This test checks that the round +// instruction limit is respected: when many `subnet_metrics` calls are pending +// at once, the per-round subnet-message instruction budget only allows some of +// them to execute per round, so the rest are deferred to later rounds (i.e. not +// all calls execute in the same round). +// +// Mirrors `list_canisters_respects_round_instruction_limit`. Unlike that test it +// has to shrink the round budget, because at the default +// `max_instructions_per_round` of `4 * B` the per-round subnet-message budget of +// 250M would need thousands of concurrent `subnet_metrics` calls to saturate, +// well past the canister output queue capacity of +// `DEFAULT_QUEUE_CAPACITY = 500`. +#[test] +fn subnet_metrics_respects_round_instruction_limit() { + // Number of concurrent `subnet_metrics` calls, bounded by + // `DEFAULT_QUEUE_CAPACITY = 500`. + const NUM_CALLS: u64 = 200; + // Keep in sync with `SUBNET_MESSAGES_LIMIT_FRACTION` in + // `rs/execution_environment/src/scheduler.rs`. + const SUBNET_MESSAGES_LIMIT_FRACTION: u64 = 16; + const MAX_INSTRUCTIONS_PER_ROUND: u64 = 80_000_000; + + let env = subnet_metrics_env_with_round_limit(MAX_INSTRUCTIONS_PER_ROUND); + let caller = create_universal_canister_with_cycles( + &env, + Some(CanisterSettingsArgsBuilder::new().build()), + INITIAL_CYCLES_BALANCE, + ); + + let num_canisters = env.get_latest_state().num_canisters() as u64; + assert_eq!(num_canisters, 1); + // The charge is `BASE + 40 * hot_len`, and `hot_len` is a property of the + // round the call happens to execute in — the caller canister is hot while it + // has pending work and cold otherwise — so an exact per-call cost is not + // observable from here. With a single canister on the subnet it is bracketed + // by `hot_len ∈ {0, 1}`, which is tight enough for every assertion below. + let min_cost_per_call = SUBNET_METRICS_BASE_INSTRUCTIONS; + let max_cost_per_call = SUBNET_METRICS_BASE_INSTRUCTIONS + + SUBNET_METRICS_INSTRUCTIONS_PER_HOT_CANISTER * num_canisters; + let budget = MAX_INSTRUCTIONS_PER_ROUND / SUBNET_MESSAGES_LIMIT_FRACTION; + // Use the *minimum* charge here, so reaching the condition is guaranteed + // rather than merely likely. + assert!( + NUM_CALLS * min_cost_per_call > budget, + "test cannot reach the condition it asserts: {NUM_CALLS} calls x \ + {min_cost_per_call} instructions do not exceed the per-round \ + subnet-message budget {budget}; lower MAX_INSTRUCTIONS_PER_ROUND or \ + raise NUM_CALLS" + ); + // ...and the *maximum* charge here, for the same reason. + assert!( + budget >= 2 * max_cost_per_call, + "budget {budget} fits fewer than two calls, so the test degenerates to \ + one call per round and proves nothing about batching" + ); + + // Build an update that fires `NUM_CALLS` concurrent `subnet_metrics` + // inter-canister calls (ignoring their responses) and then replies. + let payload = subnet_metrics_payload(&env); + let mut update = wasm(); + for _ in 0..NUM_CALLS { + update = update.call_simple( + CanisterId::ic_00(), + Method::SubnetMetrics, + call_args() + .other_side(payload.clone()) + .on_reply(wasm().noop()) + .on_reject(wasm().noop()), + ); + } + let update = update.reply().build(); + + let instructions_baseline = env.subnet_message_instructions(); + let calls_baseline = subnet_metrics_count(&env); + assert_eq!(calls_baseline, 0); + env.send_ingress(PrincipalId::new_anonymous(), caller, "update", update); + + let executed_so_far = || subnet_metrics_count(&env) - calls_baseline; + let mut executed_per_round = vec![]; + for _ in 0..200 { + env.tick(); + executed_per_round.push(executed_so_far()); + if executed_so_far() == NUM_CALLS { + break; + } + } + + // Not all `subnet_metrics` calls were executed in the same round: there is a + // round after which some but not all of them had been executed. + assert!( + executed_per_round.iter().any(|&n| n > 0 && n < NUM_CALLS), + "expected subnet_metrics calls to be spread across rounds, got progression {:?}", + executed_per_round, + ); + // Eventually all of them were executed. + assert_eq!(*executed_per_round.last().unwrap(), NUM_CALLS); + // The calls were *batched*, not executed one per round: some round drained at + // least two of them. Asserting only "spread across rounds" above would also be + // satisfied by a degenerate one-call-per-round progression, which is what the + // `budget >= 2 * max_cost_per_call` precondition exists to rule out — so + // assert the consequence too, not just the precondition. + let per_round_deltas: Vec = std::iter::once(executed_per_round[0]) + .chain(executed_per_round.windows(2).map(|w| w[1] - w[0])) + .collect(); + assert!( + per_round_deltas.iter().any(|&n| n >= 2), + "expected at least one round to execute two or more calls, got per-round \ + counts {per_round_deltas:?}" + ); + // Every executed call was charged per the cost model, within the `hot_len` + // bracket established above. + let charged = env.subnet_message_instructions() - instructions_baseline; + assert!( + charged >= (NUM_CALLS * min_cost_per_call) as f64 + && charged <= (NUM_CALLS * max_cost_per_call) as f64, + "total charge {charged} outside [{}, {}] for {NUM_CALLS} calls", + NUM_CALLS * min_cost_per_call, + NUM_CALLS * max_cost_per_call, + ); +} + +// A successful `subnet_metrics` call is charged round instructions per the cost +// model; a rejected one (malformed payload, or a `subnet_id` naming a different +// subnet) is charged nothing. +#[test] +fn subnet_metrics_charges_round_instructions() { + let env = StateMachineBuilder::new() + .with_config(Some(StateMachineConfig::new( + SubnetConfig::new(SubnetType::Application), + HypervisorConfig::default(), + ))) + .with_subnet_type(SubnetType::Application) + .with_cost_schedule(CanisterCyclesCostSchedule::Free) + .build(); + let caller = create_universal_canister_with_cycles( + &env, + Some(CanisterSettingsArgsBuilder::new().build()), + INITIAL_CYCLES_BALANCE, + ); + + let num_canisters = env.get_latest_state().num_canisters() as u64; + assert_eq!(num_canisters, 1); + // See the note in `subnet_metrics_respects_round_instruction_limit`: the exact + // `hot_len` at handler time is not observable, so bracket it. + let min_cost_per_call = SUBNET_METRICS_BASE_INSTRUCTIONS; + let max_cost_per_call = SUBNET_METRICS_BASE_INSTRUCTIONS + + SUBNET_METRICS_INSTRUCTIONS_PER_HOT_CANISTER * num_canisters; + + let call = |payload: Vec| { + wasm() + .call_simple( + CanisterId::ic_00(), + Method::SubnetMetrics, + call_args() + .other_side(payload) + .on_reject(wasm().reject_message().reject()), + ) + .build() + }; + + // Success: charged per the cost model. + let baseline = env.subnet_message_instructions(); + let reply = + get_reply(env.execute_ingress(caller, "update", call(subnet_metrics_payload(&env)))); + let first = SubnetMetricsResponse::decode(&reply).unwrap(); + let charged = env.subnet_message_instructions() - baseline; + assert!( + charged >= min_cost_per_call as f64 && charged <= max_cost_per_call as f64, + "charge {charged} outside [{min_cost_per_call}, {max_cost_per_call}]" + ); + + // `block_height` tracks the *real* block height, not just whatever round + // number a harness handed the handler: after N further rounds it has advanced + // by at least N. (`subnet_metrics_block_height_matches_current_round` in + // `canister_manager/tests.rs` pins the `current_round` plumbing; this pins that + // `current_round` is the block height in a running `StateMachine`.) + const TICKS: u64 = 5; + assert!(first.block_height > 0_u64); + for _ in 0..TICKS { + env.tick(); + } + let reply = + get_reply(env.execute_ingress(caller, "update", call(subnet_metrics_payload(&env)))); + let second = SubnetMetricsResponse::decode(&reply).unwrap(); + assert!( + second.block_height >= first.block_height.clone() + candid::Nat::from(TICKS), + "block_height did not advance with the block height: {} then {} across \ + {TICKS} ticks", + first.block_height, + second.block_height, + ); + + // Malformed payload: rejected, charged nothing. + let baseline = env.subnet_message_instructions(); + let reject = get_reject(env.execute_ingress(caller, "update", call(EmptyBlob.encode()))); + assert!( + reject.contains("Error decoding candid"), + "unexpected reject: {reject}" + ); + assert_eq!(env.subnet_message_instructions(), baseline); + + // Foreign `subnet_id`: rejected, charged nothing. + // + // Note which layer rejects here. `resolve_destination` routes the call to the + // subnet named in the payload, and this single-subnet `StateMachine` has no + // route to it, so the call is rejected by message routing and the handler + // never runs. That is exactly the behaviour the interface spec relies on for + // the cross-subnet case; the handler's own-subnet check is exercised instead + // by `subnet_metrics_foreign_subnet_id_is_rejected` in + // `canister_manager/tests.rs`, which injects the request directly into the + // subnet queue. Either way, nothing is charged. + let foreign = SubnetMetricsArgs { + subnet_id: PrincipalId::new_subnet_test_id(0x1234), + } + .encode(); + let baseline = env.subnet_message_instructions(); + let reject = get_reject(env.execute_ingress(caller, "update", call(foreign))); + assert!( + reject.contains("No route to canister"), + "unexpected reject: {reject}" + ); + assert_eq!(env.subnet_message_instructions(), baseline); +} + +// The `subnet_metrics` charge must scale with the number of **hot** canisters — +// what `CanisterStates::total_consumed_cycles()` actually folds over — and not +// with the total number of canisters on the subnet. +// +// This is the regression test for a real defect: keying the charge on +// `num_canisters()` while the work is `O(|hot|)` manufactures denial capacity that +// is not backed by any work. `repartition_canister_states()` runs on every +// `commit_and_certify`, so `hot_len() << len()` is the steady state: on a +// 100k-canister subnet a `num_canisters()`-keyed charge over-states the cost by +// ~40x, meaning ~40x fewer calls suffice to pin the shared per-round +// subnet-message budget at zero and defer every `install_code` / `upload_chunk` / +// snapshot / `update_settings` on that subnet. +#[test] +fn subnet_metrics_charge_ignores_cold_canisters() { + // Enough extra canisters that a `num_canisters()`-keyed charge is + // unambiguously distinguishable from a `hot_len()`-keyed one, while keeping + // the test cheap. + const EXTRA_CANISTERS: u64 = 30; + + let env = StateMachineBuilder::new() + .with_config(Some(StateMachineConfig::new( + SubnetConfig::new(SubnetType::Application), + HypervisorConfig::default(), + ))) + .with_subnet_type(SubnetType::Application) + .with_cost_schedule(CanisterCyclesCostSchedule::Free) + .build(); + let caller = create_universal_canister_with_cycles( + &env, + Some(CanisterSettingsArgsBuilder::new().build()), + INITIAL_CYCLES_BALANCE, + ); + for _ in 0..EXTRA_CANISTERS { + env.create_canister(Some(CanisterSettingsArgsBuilder::new().build())); + } + // Let the freshly created canisters go quiet and be demoted to the cold pool. + for _ in 0..3 { + env.tick(); + } + + let state = env.get_latest_state(); + let num_canisters = state.num_canisters() as u64; + let hot_canisters = state.canister_states().hot_len() as u64; + assert_eq!(num_canisters, EXTRA_CANISTERS + 1); + // Executable precondition: the pool really is mostly cold, so the two keyings + // give different answers and the assertion below is not vacuous. + assert!( + hot_canisters * 4 < num_canisters, + "precondition failed: {hot_canisters} of {num_canisters} canisters are hot, \ + so a hot-keyed and a total-keyed charge are not distinguishable; the \ + test proves nothing" + ); + drop(state); + + let call = wasm() + .call_simple( + CanisterId::ic_00(), + Method::SubnetMetrics, + call_args() + .other_side(subnet_metrics_payload(&env)) + .on_reject(wasm().reject_message().reject()), + ) + .build(); + + let baseline = env.subnet_message_instructions(); + let reply = get_reply(env.execute_ingress(caller, "update", call)); + SubnetMetricsResponse::decode(&reply).unwrap(); + let charged = env.subnet_message_instructions() - baseline; + + // The charge is strictly below what keying on the total would give. This is + // the assertion that fails if the cost function regresses to + // `state.num_canisters()`. + let total_keyed = SUBNET_METRICS_BASE_INSTRUCTIONS + + SUBNET_METRICS_INSTRUCTIONS_PER_HOT_CANISTER * num_canisters; + assert!( + charged < total_keyed as f64, + "charge {charged} matches a total-keyed cost model ({total_keyed} for \ + {num_canisters} canisters, of which only {hot_canisters} are hot); the \ + charge must scale with the hot pool only" + ); + // And it is within the hot-keyed bracket. `hot_len` at handler time can differ + // from the value read above by the caller canister itself, hence the slack. + let hot_keyed_upper = SUBNET_METRICS_BASE_INSTRUCTIONS + + SUBNET_METRICS_INSTRUCTIONS_PER_HOT_CANISTER * (hot_canisters + 2); + assert!( + charged >= SUBNET_METRICS_BASE_INSTRUCTIONS as f64 && charged <= hot_keyed_upper as f64, + "charge {charged} outside the hot-keyed bracket \ + [{SUBNET_METRICS_BASE_INSTRUCTIONS}, {hot_keyed_upper}]" + ); +} + #[test] fn maximum_state_size() { let maximum_state_size = NumBytes::new(1 << 30); diff --git a/rs/replicated_state/src/canister_states.rs b/rs/replicated_state/src/canister_states.rs index ec78a48ac2d0..801df88b12e6 100644 --- a/rs/replicated_state/src/canister_states.rs +++ b/rs/replicated_state/src/canister_states.rs @@ -160,6 +160,11 @@ impl ColdStats { /// 2. every canister in the `cold` pool satisfies `CanisterState::is_cold()`; /// 3. `cold_stats` matches a fresh recomputation over the `cold` pool. /// +/// Invariant (3) is *additionally checked* in release builds during checkpoint +/// validation, by [`Self::validate_cold_stats`]. That check is advisory: it logs +/// a critical error and increments a counter, and does not abort or otherwise +/// alter the checkpoint. +/// /// Additionally, the **strict** partition invariant — that every canister in /// the `hot` pool does *not* satisfy `is_cold()` — holds after /// [`Self::try_cool_all`] / @@ -645,6 +650,43 @@ impl CanisterStates { Ok(()) } + /// Validates that `cold_stats` matches a fresh recomputation over the `cold` + /// pool, i.e. that the sub-before / add-after bracketing around every + /// cold-pool mutation has been respected. + /// + /// Unlike the `debug_assert` in `debug_assert_invariants`, this is intended to + /// run in release builds during checkpoint validation, because the aggregates + /// are read into hashed replicated state + /// (`SubnetMetrics::canister_state_bytes`, which has no other check) and + /// returned to canisters (`subnet_metrics`). + /// + /// It runs only for a *locally produced* checkpoint, i.e. on the branch of + /// `validate_and_finalize_checkpoint_and_remove_unverified_marker` that has a + /// reference state, and not on the state-sync path. That is the only branch + /// where it could find anything: a `CanisterStates` freshly loaded from disk has + /// `cold_stats` recomputed by `CanisterStates::new`, so it is consistent by + /// construction; only the in-memory reference state can have drifted. + /// + /// Note that the caller's failure mode is **advisory**: `validate_eq_checkpoint` + /// logs a critical error and increments a counter, then finalizes the + /// checkpoint regardless. This detects and attributes a stale aggregate; it + /// does not prevent one from being used. The caller runs it *after* the + /// per-canister comparison and combines the two errors, so that an advisory + /// failure here does not mask the diagnostics that identify which canister + /// drifted. + /// + /// Complexity: `O(|cold canisters|)`. + pub fn validate_cold_stats(&self) -> Result<(), String> { + let recomputed = ColdStats::recompute(self.cold.values()); + if recomputed != self.cold_stats { + return Err(format!( + "cold_stats out of sync with the cold pool: stored {:?}, recomputed {:?}", + self.cold_stats, recomputed + )); + } + Ok(()) + } + /// Debug-only consistency check, called at the end of every mutating operation. /// Verifies invariants (1)–(3) listed under [`CanisterStates`]. /// diff --git a/rs/replicated_state/src/canister_states/tests.rs b/rs/replicated_state/src/canister_states/tests.rs index fde77f4e9552..f2aea2290037 100644 --- a/rs/replicated_state/src/canister_states/tests.rs +++ b/rs/replicated_state/src/canister_states/tests.rs @@ -901,6 +901,111 @@ fn total_consumed_cycles_combines_hot_and_cold() { assert_eq!(states.total_consumed_cycles(), NominalCycles::new(135)); } +/// Consumes `amount` cycles on `canister`, as storage / instruction charging +/// does. Consuming cycles does not create work, so a cold canister stays cold. +fn consume_cycles(canister: &mut Arc, amount: u128) { + use ic_types_cycles::{CanisterCyclesCostSchedule, CompoundCycles, Instructions}; + + Arc::make_mut(canister) + .system_state + .consume_cycles(CompoundCycles::::new( + Cycles::new(amount), + CanisterCyclesCostSchedule::Normal, + )); +} + +/// Folds `consumed_cycles` over every canister, hot and cold, without going +/// through the `cold_stats` aggregate. +fn direct_consumed_cycles_fold(states: &CanisterStates) -> ic_types_cycles::NominalCycles { + use ic_types_cycles::NominalCycles; + + states + .all_values() + .fold(NominalCycles::zero(), |acc, canister| { + acc + canister.system_state.canister_metrics().consumed_cycles() + }) +} + +#[test] +fn total_consumed_cycles_equals_direct_fold() { + let mut states = CanisterStates::default(); + for id in 1..=4 { + let mut cold = cold_canister(id); + consume_cycles(&mut cold, 100 * id as u128); + states.insert(cold); + } + for id in 5..=7 { + let mut hot = hot_canister(id); + consume_cycles(&mut hot, 7 * id as u128); + states.insert(hot); + } + + assert_eq!(states.cold.len(), 4); + assert_eq!(states.hot.len(), 3); + assert_eq!( + states.total_consumed_cycles(), + direct_consumed_cycles_fold(&states) + ); +} + +#[test] +fn validate_cold_stats_accepts_consistent_stats() { + let mut states = CanisterStates::default(); + states.insert(cold_canister(1)); + states.insert(hot_canister(2)); + states.insert(cold_canister(3)); + + assert_eq!(states.validate_cold_stats(), Ok(())); +} + +#[test] +fn validate_cold_stats_rejects_stale_stats() { + use ic_types_cycles::{NominalCycles, NominalCyclesTesting}; + + let mut states = CanisterStates::default(); + let c = cold_canister(1); + states.insert(Arc::clone(&c)); + assert_eq!(states.validate_cold_stats(), Ok(())); + + // Bypass the public mutation entry points: mutate a cold canister's consumed + // cycles directly, behind the aggregate's back, simulating missing + // sub-before / add-after bracketing. + consume_cycles(states.cold.get_mut(&c.canister_id()).unwrap(), 42); + assert_eq!(states.hot.len(), 0); + assert_eq!(states.cold.len(), 1); + + let err = states.validate_cold_stats().unwrap_err(); + assert!( + err.contains("cold_stats out of sync with the cold pool"), + "unexpected error: {err}", + ); + // The aggregate is stale, so the reported total is now wrong. + assert_eq!(states.cold_stats.consumed_cycles, NominalCycles::new(0)); + assert_ne!( + states.total_consumed_cycles(), + direct_consumed_cycles_fold(&states) + ); +} + +#[test] +fn for_each_mut_keeps_cold_stats_consumed_cycles_in_sync() { + let mut states = CanisterStates::default(); + states.insert(cold_canister(1)); + states.insert(cold_canister(2)); + states.insert(hot_canister(3)); + assert_eq!(states.cold.len(), 2); + + // The path that storage charging takes: mutate every canister in place, + // including the cold ones. + states.for_each_mut(|_id, canister| consume_cycles(canister, 11)); + + assert_eq!(states.validate_cold_stats(), Ok(())); + assert_eq!( + states.total_consumed_cycles(), + direct_consumed_cycles_fold(&states) + ); +} + #[test] fn validate_strict_split_accepts_canonical_partition() { let mut states = CanisterStates::default(); diff --git a/rs/replicated_state/src/replicated_state.rs b/rs/replicated_state/src/replicated_state.rs index 56976d4a5fc0..38cd7f098e50 100644 --- a/rs/replicated_state/src/replicated_state.rs +++ b/rs/replicated_state/src/replicated_state.rs @@ -676,6 +676,17 @@ impl ReplicatedState { /// Re-establishes strict hot / cold partitioning of canister states (see /// [`CanisterStates::try_cool_all`]). + /// + /// **The caller in `commit_and_certify` must not be made conditional** (e.g. + /// "only on checkpoint rounds"). Execution reads + /// [`CanisterStates::hot_len`] — the `subnet_metrics` management method + /// charges round instructions proportional to it — so the *cardinality* of + /// the partition, not just its consistency, has to be identical on every + /// replica. Repartitioning on every commit is what makes the committed + /// partition equal the one `CanisterStates::new` derives at load, and hence + /// makes a replica that keeps running agree with one that restarts from a + /// checkpoint. `hot_cold_partition_is_canonical_after_every_commit` in + /// `rs/state_manager/tests/state_manager.rs` pins this. pub fn repartition_canister_states(&mut self) { self.canister_states.try_cool_all(); } diff --git a/rs/state_manager/src/checkpoint.rs b/rs/state_manager/src/checkpoint.rs index c62a7c167a0a..c977359d1aba 100644 --- a/rs/state_manager/src/checkpoint.rs +++ b/rs/state_manager/src/checkpoint.rs @@ -555,31 +555,51 @@ impl CheckpointLoader { .or_default() .push(snapshot_id); } - maybe_parallel_map(thread_pool, ref_canister_ids.iter(), |canister_id| { - load_canister_state_from_checkpoint( - &self.checkpoint_layout, - canister_id, - snapshot_ids_per_canister - .get(canister_id) - .cloned() - .unwrap_or_default(), - Arc::clone(&self.fd_factory), - &self.metrics, - ) - .map_err(|err| { - format!( - "Failed to load canister state for validation for key #{canister_id}: {err}" + let per_canister = + maybe_parallel_map(thread_pool, ref_canister_ids.iter(), |canister_id| { + load_canister_state_from_checkpoint( + &self.checkpoint_layout, + canister_id, + snapshot_ids_per_canister + .get(canister_id) + .cloned() + .unwrap_or_default(), + Arc::clone(&self.fd_factory), + &self.metrics, ) - })? - .0 - .validate_eq( - ref_canister_states - .get(canister_id) - .expect("Failed to get canister from canister_states"), - ) - }) - .into_iter() - .try_for_each(identity) + .map_err(|err| { + format!( + "Failed to load canister state for validation for key #{canister_id}: {err}" + ) + })? + .0 + .validate_eq( + ref_canister_states + .get(canister_id) + .expect("Failed to get canister from canister_states"), + ) + }) + .into_iter() + .try_for_each(identity); + + // Detect (and attribute) a stale cold-pool aggregate. Like every other + // check here, this is advisory: the caller logs a critical error and + // increments a counter, then finalizes the checkpoint regardless. + // + // Deliberately run *after* the per-canister comparison above, and combined + // with it rather than short-circuiting it: in the very scenario where this + // check fires, the per-canister diagnostics are what tell the operator + // *which* canister drifted, and an advisory check must not cost the + // operator that information. + let cold_stats = ref_canister_states + .validate_cold_stats() + .map_err(|err| format!("Canister Validation: {err}")); + + match (per_canister, cold_stats) { + (Ok(()), Ok(())) => Ok(()), + (Err(err), Ok(())) | (Ok(()), Err(err)) => Err(err), + (Err(per_canister), Err(cold_stats)) => Err(format!("{per_canister}; {cold_stats}")), + } } fn validate_eq_canister_snapshots_ids( diff --git a/rs/state_manager/src/lib.rs b/rs/state_manager/src/lib.rs index 408496253872..7c5e81d79f1b 100644 --- a/rs/state_manager/src/lib.rs +++ b/rs/state_manager/src/lib.rs @@ -3529,6 +3529,16 @@ impl StateManager for StateManagerImpl { // during the round may have left canisters that are now cold in `hot`. The // partition must be canonical at checkpoint time so that a replica continuing // through a checkpoint and one (re)starting from it agree on the partition. + // + // This call is deliberately outside the `CertificationScope::Metadata` + // branch above and must stay unconditional: execution reads + // `CanisterStates::hot_len()` (the `subnet_metrics` management method + // charges round instructions proportional to it), so a round that skipped + // the repartition would leave a continuing replica and a restarted one + // with different `hot_len()`, hence a different charge and a different + // number of subnet messages drained — a state divergence. Pinned by + // `hot_cold_partition_is_canonical_after_every_commit` in + // `tests/state_manager.rs`. self.metrics .hot_canisters_count .observe(state.canister_states().hot_len() as f64); diff --git a/rs/state_manager/tests/state_manager.rs b/rs/state_manager/tests/state_manager.rs index 25fdf37ec1ba..c09eedcb4656 100644 --- a/rs/state_manager/tests/state_manager.rs +++ b/rs/state_manager/tests/state_manager.rs @@ -24,8 +24,8 @@ use ic_registry_routing_table::{CANISTER_IDS_PER_SUBNET, CanisterIdRange, Routin use ic_registry_subnet_features::SubnetFeatures; use ic_registry_subnet_type::SubnetType; use ic_replicated_state::{ - ExecutionState, ExportedFunctions, Memory, NetworkTopology, NumWasmPages, PageMap, - ReplicatedState, Stream, SubnetTopology, + CanisterStates, ExecutionState, ExportedFunctions, Memory, NetworkTopology, NumWasmPages, + PageMap, ReplicatedState, Stream, SubnetTopology, canister_state::canister_snapshots::CanisterSnapshot, canister_state::{execution_state::WasmBinary, system_state::wasm_chunk_store::WasmChunkStore}, metadata_state::{ @@ -716,6 +716,69 @@ fn last_install_timestamp_survives_a_checkpoint() { }); } +/// `hot_len()` as `CanisterStates::new` derives it from the flat canister set, +/// i.e. the value a replica that loads this state from a checkpoint would see. +fn derived_hot_len(state: &ReplicatedState) -> usize { + let flat: BTreeMap<_, _> = state + .canister_states() + .all_iter() + .map(|(id, canister)| (*id, Arc::clone(canister))) + .collect(); + CanisterStates::new(flat).hot_len() +} + +/// The hot/cold partition must be re-canonicalised on **every** commit, not only +/// on checkpoint rounds. +/// +/// This is a correctness requirement, not a canonicalisation convenience, since +/// `subnet_metrics_instructions` in `rs/execution_environment` charges round +/// instructions proportional to `CanisterStates::hot_len()`. If +/// `repartition_canister_states()` were made conditional — the plausible +/// optimisation being "strictness is only *needed* at checkpoint time, so only do +/// it there" — a replica continuing in memory would carry quiet-but-still-hot +/// canisters into the next round while a replica that restarted from the last +/// checkpoint would load them as cold. Different `hot_len()` means a different +/// charge, which means a different number of subnet messages drained in that +/// round, which is state divergence. +/// +/// So this test commits with `CertificationScope::Metadata` — a *non-checkpoint* +/// round — and asserts the committed partition still equals the derived one. +#[test] +fn hot_cold_partition_is_canonical_after_every_commit() { + state_manager_test(|_metrics, state_manager| { + let canister_id: CanisterId = canister_test_id(100); + let (_height, mut state) = state_manager.take_tip(); + insert_dummy_canister(&mut state, canister_id); + state_manager.commit_and_certify(state, CertificationScope::Metadata, None); + + // Leave behind a stale hot entry, as a round of execution does: taking a + // mutable reference promotes the canister into the `hot` pool without + // giving it any work, so it is hot-by-position but cold-by-predicate. + let (_height, mut state) = state_manager.take_tip(); + assert!(state.canister_state_make_mut(&canister_id).is_some()); + + // Executable precondition: the partition really is stale before the + // commit, so the assertion afterwards is not vacuous. + assert_eq!(state.canister_states().hot_len(), 1); + assert_eq!(derived_hot_len(&state), 0); + + // A non-checkpoint commit. This is the round that a conditional + // repartition would skip. + state_manager.commit_and_certify(state, CertificationScope::Metadata, None); + + let (_height, state) = state_manager.take_tip(); + assert_eq!( + state.canister_states().hot_len(), + derived_hot_len(&state), + "the committed hot/cold partition differs from the one a replica \ + loading this state from a checkpoint would derive; \ + `repartition_canister_states()` must run on every commit, because \ + `subnet_metrics_instructions` charges on `hot_len()`" + ); + assert_eq!(state.canister_states().hot_len(), 0); + }); +} + #[test] fn tip_can_be_recovered_from_metadata_checkpoint() { state_manager_restart_test(|state_manager, restart_fn| { diff --git a/rs/test_utilities/execution_environment/src/lib.rs b/rs/test_utilities/execution_environment/src/lib.rs index 4fffd9477e91..2f4b3be69e42 100644 --- a/rs/test_utilities/execution_environment/src/lib.rs +++ b/rs/test_utilities/execution_environment/src/lib.rs @@ -661,6 +661,16 @@ impl ExecutionTest { self.time += duration; } + pub fn current_round(&self) -> ExecutionRound { + self.current_round + } + + /// Sets the round number passed to `execute_subnet_message` and friends, + /// i.e. the block height as seen by the execution environment. + pub fn set_current_round(&mut self, round: u64) { + self.current_round = ExecutionRound::new(round); + } + pub fn ingress_status(&self, message_id: &MessageId) -> IngressStatus { self.state().get_ingress_status(message_id).clone() } @@ -1680,7 +1690,8 @@ impl ExecutionTest { }; let maybe_canister_id = get_effective_canister_id(message.clone()); let is_install_code = check_is_install_code(message.clone()); - let is_list_canisters = check_is_list_canisters(message.clone()); + let consumes_round_instructions_without_effective_canister_id = + check_consumes_round_instructions_without_effective_canister_id(message.clone()); let mut round_limits = RoundLimits { instructions: RoundInstructions::from(i64::MAX), subnet_available_memory: self.subnet_available_memory, @@ -1765,9 +1776,10 @@ impl ExecutionTest { .insert(canister_id, paused_subnet_message); } } - } else if !is_list_canisters { - // `list_canisters` has no effective canister ID but still consumes - // round instructions, so it is exempt from this assertion. + } else if !consumes_round_instructions_without_effective_canister_id { + // `list_canisters` and `subnet_metrics` have no effective canister ID + // but still consume round instructions, so they are exempt from this + // assertion. assert_eq!(slice_instructions_used.get(), 0); } self.check_invariants(); @@ -2776,6 +2788,13 @@ impl ExecutionTestBuilder { self } + /// Sets the initial round number, i.e. the block height as seen by the + /// execution environment. + pub fn with_current_round(mut self, round: u64) -> Self { + self.current_round = ExecutionRound::new(round); + self + } + pub fn with_resource_saturation_scaling(mut self, scaling: usize) -> Self { self.subnet_config.scheduler_config.scheduler_cores = scaling; // If scaling == 1, i.e. a single core is requested in the test, DTS must @@ -3243,13 +3262,17 @@ fn check_is_install_code(message: SubnetMessage) -> bool { message.method_name() == "install_code" || message.method_name() == "install_chunked_code" } -fn check_is_list_canisters(message: SubnetMessage) -> bool { +/// Whether the message is one of the management methods that consume round +/// instructions even though they have no effective canister ID (and therefore +/// cannot use `Ic00MethodPermissions::counts_toward_round_limit`). Keep in sync +/// with the special case in `Scheduler::can_execute_subnet_msg`. +fn check_consumes_round_instructions_without_effective_canister_id(message: SubnetMessage) -> bool { let message = match message { SubnetMessage::Response(_) => return false, SubnetMessage::Request(request) => CanisterCall::Request(request), SubnetMessage::Ingress(ingress) => CanisterCall::Ingress(ingress), }; - message.method_name() == "list_canisters" + matches!(message.method_name(), "list_canisters" | "subnet_metrics") } pub fn wat_compilation_cost(wat: &str) -> NumInstructions { diff --git a/rs/tests/execution/general_execution_test.rs b/rs/tests/execution/general_execution_test.rs index c76cb8fec81e..63558c8691df 100644 --- a/rs/tests/execution/general_execution_test.rs +++ b/rs/tests/execution/general_execution_test.rs @@ -4,6 +4,11 @@ use anyhow::Result; use general_execution_tests::api_tests::node_metrics_history_another_subnet_succeeds; use general_execution_tests::api_tests::node_metrics_history_non_existing_subnet_fails; use general_execution_tests::api_tests::node_metrics_history_query_fails; +use general_execution_tests::api_tests::subnet_metrics_another_subnet_succeeds; +use general_execution_tests::api_tests::subnet_metrics_composite_query_fails; +use general_execution_tests::api_tests::subnet_metrics_non_existing_subnet_fails; +use general_execution_tests::api_tests::subnet_metrics_own_subnet_succeeds; +use general_execution_tests::api_tests::subnet_metrics_query_fails; use general_execution_tests::api_tests::test_controller; use general_execution_tests::api_tests::test_cycles_burn; use general_execution_tests::api_tests::test_in_replicated_execution; @@ -44,6 +49,11 @@ fn main() -> Result<()> { .add_test(systest!(node_metrics_history_query_fails)) .add_test(systest!(node_metrics_history_another_subnet_succeeds)) .add_test(systest!(node_metrics_history_non_existing_subnet_fails)) + .add_test(systest!(subnet_metrics_own_subnet_succeeds)) + .add_test(systest!(subnet_metrics_another_subnet_succeeds)) + .add_test(systest!(subnet_metrics_non_existing_subnet_fails)) + .add_test(systest!(subnet_metrics_query_fails)) + .add_test(systest!(subnet_metrics_composite_query_fails)) .add_test(systest!(can_access_big_heap_and_big_stable_memory)) .add_test(systest!(can_access_big_stable_memory)) .add_test(systest!(can_handle_overflows_when_indexing_stable_memory)) diff --git a/rs/tests/execution/general_execution_tests/api_tests.rs b/rs/tests/execution/general_execution_tests/api_tests.rs index 6b230fd6ae37..c7a86b185cec 100644 --- a/rs/tests/execution/general_execution_tests/api_tests.rs +++ b/rs/tests/execution/general_execution_tests/api_tests.rs @@ -220,6 +220,249 @@ pub fn test_cycles_burn(env: TestEnv) { }) } +/// Decodes a `subnet_metrics` reply and returns it, asserting the fields are +/// plausible. +fn decode_subnet_metrics(bytes: &[u8]) -> ic00::SubnetMetricsResponse { + let response = Decode!(bytes, ic00::SubnetMetricsResponse).unwrap(); + // The subnet has processed at least the blocks that carried this call. + assert!(response.block_height > 0_u64); + response +} + +pub fn subnet_metrics_own_subnet_succeeds(env: TestEnv) { + // Arrange. + let (app_node, agent) = setup_app_node_and_agent(&env); + let logger = env.logger(); + let subnet_id = app_node.subnet_id().unwrap().get(); + block_on({ + async move { + let canister = UniversalCanister::new_with_retries( + &agent, + app_node.effective_canister_id(), + &logger, + ) + .await; + // Act. + let result = canister + .update(wasm().call_simple( + ic00::IC_00, + Method::SubnetMetrics, + call_args().other_side(ic00::SubnetMetricsArgs { subnet_id }.encode()), + )) + .await; + // Assert. + let bytes = result.expect("subnet_metrics call failed"); + let response = decode_subnet_metrics(&bytes); + // The universal canister itself is on the subnet, so there is at + // least one canister and some state. + assert!(response.num_canisters > 0_u64); + assert!(response.canister_state_bytes > 0_u64); + assert!(response.update_transactions_total > 0_u64); + } + }) +} + +/// A canister on the application subnet calls `subnet_metrics` naming a +/// *different* subnet. Message routing delivers the call to that subnet, which +/// executes it and answers with **its own** metrics. +/// +/// The attribution half is what this test is really for, and asserting only that +/// a reply arrives would not test it: a subnet answering a foreign `subnet_id` +/// with its *own* metrics — exactly what the own-subnet check exists to prevent — +/// also replies successfully. So the test perturbs only the *remote* subnet, by +/// installing a canister there, and asserts the remote reading moves. Under that +/// bug the two readings would be local and a remote canister creation could not +/// move them. +/// +/// Note also: unlike `node_metrics_history_another_subnet_succeeds`, which calls +/// `get_first_healthy_application_node_snapshot()` twice and so ends up naming its +/// *own* subnet (the test group's `setup` configures a single application subnet), +/// this test names the verified-application subnet, so the call really does cross +/// a subnet boundary. +pub fn subnet_metrics_another_subnet_succeeds(env: TestEnv) { + // Arrange. + let (app_node, agent) = setup_app_node_and_agent(&env); + let other_node = env.get_first_healthy_verified_application_node_snapshot(); + let other_agent = other_node.build_default_agent(); + let logger = env.logger(); + let other_subnet_id = other_node.subnet_id().unwrap().get(); + assert_ne!(other_subnet_id, app_node.subnet_id().unwrap().get()); + block_on({ + async move { + let canister = UniversalCanister::new_with_retries( + &agent, + app_node.effective_canister_id(), + &logger, + ) + .await; + + let read_remote = || async { + let result = canister + .update( + wasm().call_simple( + ic00::IC_00, + Method::SubnetMetrics, + call_args().other_side( + ic00::SubnetMetricsArgs { + subnet_id: other_subnet_id, + } + .encode(), + ), + ), + ) + .await; + decode_subnet_metrics(&result.expect("cross-subnet subnet_metrics call failed")) + }; + + // Act. + let before = read_remote().await; + // Perturb only the remote subnet. + let _remote_canister = UniversalCanister::new_with_retries( + &other_agent, + other_node.effective_canister_id(), + &logger, + ) + .await; + let after = read_remote().await; + + // Assert: the reply reports the *target* subnet's population, so + // creating a canister there moves it. + // + // Note the direction of the assertion. The tests of this group are + // registered via `SystemTestGroup::add_parallel(SystemTestSubGroup..)` + // in `general_execution_test.rs`, and both of those compose under + // `EvalOrder::Parallel` (`rs/tests/driver/src/driver/group.rs`: + // `add_parallel` → `add_group(_, EvalOrder::Parallel)`, and + // `SystemTestSubGroup::new()` sets `ordering: EvalOrder::Parallel`, + // which `add_test` preserves). So siblings *do* run concurrently and + // can create canisters on the remote subnet meanwhile — but that can + // only make `num_canisters` larger, never smaller, so a strict `>` + // cannot fail spuriously. + assert!( + after.num_canisters > before.num_canisters, + "cross-subnet subnet_metrics did not report the target subnet's \ + canister population: num_canisters was {} before and {} after \ + creating a canister on subnet {other_subnet_id}", + before.num_canisters, + after.num_canisters, + ); + // Sanity: the counters advance on the target subnet too. + assert!(after.block_height > before.block_height); + assert!(after.update_transactions_total > before.update_transactions_total); + } + }) +} + +pub fn subnet_metrics_non_existing_subnet_fails(env: TestEnv) { + // Arrange. + let (app_node, agent) = setup_app_node_and_agent(&env); + let logger = env.logger(); + // Create non existing subnet id. + let subnet_id = PrincipalId::new_subnet_test_id(1); + block_on({ + async move { + let canister = UniversalCanister::new_with_retries( + &agent, + app_node.effective_canister_id(), + &logger, + ) + .await; + // Act. + let result = canister + .update(wasm().call_simple( + ic00::IC_00, + Method::SubnetMetrics, + call_args().other_side(ic00::SubnetMetricsArgs { subnet_id }.encode()), + )) + .await; + // Assert. The universal canister masks the inner `DestinationInvalid` + // reject as a `CanisterReject`. + assert_reject(result, RejectCode::CanisterReject); + } + }) +} + +pub fn subnet_metrics_query_fails(env: TestEnv) { + // Arrange. + let (app_node, agent) = setup_app_node_and_agent(&env); + let logger = env.logger(); + let subnet_id = app_node.subnet_id().unwrap().get(); + block_on({ + async move { + let canister = UniversalCanister::new_with_retries( + &agent, + app_node.effective_canister_id(), + &logger, + ) + .await; + // Act. + let result = canister + .query(wasm().call_simple( + ic00::IC_00, + Method::SubnetMetrics, + call_args().other_side(ic00::SubnetMetricsArgs { subnet_id }.encode()), + )) + .await; + // Assert. Note that this message comes from `ic0.call_new` being + // unavailable in a non-replicated query and is method-agnostic, so + // this test would also pass against a stub implementation. It exists + // for parity with `node_metrics_history_query_fails`; + // `subnet_metrics_composite_query_fails` is the test that actually + // exercises the new code in a query context. + assert_reject_msg( + result, + RejectCode::CanisterError, + "cannot be executed in non replicated query mode", + ); + } + }) +} + +pub fn subnet_metrics_composite_query_fails(env: TestEnv) { + // Arrange. + let (app_node, agent) = setup_app_node_and_agent(&env); + let logger = env.logger(); + let subnet_id = app_node.subnet_id().unwrap().get(); + block_on({ + async move { + let canister = UniversalCanister::new_with_retries( + &agent, + app_node.effective_canister_id(), + &logger, + ) + .await; + // Act. This is the only path on which the new `resolve_destination` + // arm runs with `is_composite_query == true`. + let result = canister + .composite_query( + wasm().call_simple( + ic00::IC_00, + Method::SubnetMetrics, + call_args() + .other_side(ic00::SubnetMetricsArgs { subnet_id }.encode()) + // Surface the inner reject message so the assertion below + // can distinguish "rejected before the handler ran" from + // "the handler ran and rejected". + .on_reject(wasm().reject_message().reject()), + ), + ) + .await; + // Assert. The call is rejected by `resolve_destination`'s explicit + // composite-query arm, before the handler runs and before the request + // is ever routed. `reject_subnet_message_routing` turns that into a + // `DestinationInvalid` reject on the inner call, whose message the + // universal canister re-rejects above — so the method name in the + // asserted text is what makes this test method-specific rather than a + // generic "queries cannot call ic00" check. + assert_reject_msg( + result, + RejectCode::CanisterReject, + "subnet_metrics API cannot be called from a composite query", + ); + } + }) +} + pub fn node_metrics_history_query_fails(env: TestEnv) { // Arrange. let (app_node, agent) = setup_app_node_and_agent(&env); diff --git a/rs/types/management_canister_types/src/lib.rs b/rs/types/management_canister_types/src/lib.rs index 46b9fb3f8c83..f22088cd23d8 100644 --- a/rs/types/management_canister_types/src/lib.rs +++ b/rs/types/management_canister_types/src/lib.rs @@ -117,6 +117,7 @@ pub enum Method { // Subnet information NodeMetricsHistory, + SubnetMetrics, SubnetInfo, FetchCanisterLogs, @@ -3786,6 +3787,40 @@ pub struct SubnetInfoResponse { impl Payload<'_> for SubnetInfoResponse {} +/// `CandidType` for `SubnetMetricsArgs` +/// ```text +/// record { +/// subnet_id : principal; +/// } +/// ``` +#[derive(Clone, Debug, Default, CandidType, Deserialize)] +pub struct SubnetMetricsArgs { + pub subnet_id: PrincipalId, +} + +impl Payload<'_> for SubnetMetricsArgs {} + +/// `CandidType` for `SubnetMetricsResponse` +/// ```text +/// record { +/// block_height : nat; +/// num_canisters : nat; +/// canister_state_bytes : nat; +/// consumed_cycles_total : nat; +/// update_transactions_total : nat; +/// } +/// ``` +#[derive(Clone, Debug, Deserialize, CandidType, Serialize, PartialEq)] +pub struct SubnetMetricsResponse { + pub block_height: candid::Nat, + pub num_canisters: candid::Nat, + pub canister_state_bytes: candid::Nat, + pub consumed_cycles_total: candid::Nat, + pub update_transactions_total: candid::Nat, +} + +impl Payload<'_> for SubnetMetricsResponse {} + /// `CandidType` for `NodeMetricsHistoryArgs` /// ```text /// record { diff --git a/rs/types/management_canister_types/tests/candid_equality.rs b/rs/types/management_canister_types/tests/candid_equality.rs index 76e698a4a4e5..1fcca7342ce7 100644 --- a/rs/types/management_canister_types/tests/candid_equality.rs +++ b/rs/types/management_canister_types/tests/candid_equality.rs @@ -16,6 +16,7 @@ type CanisterInfoResult = CanisterInfoResponse; type CanisterMetadataArgs = CanisterMetadataRequest; type CanisterMetadataResult = CanisterMetadataResponse; type SubnetInfoResult = SubnetInfoResponse; +type SubnetMetricsResult = SubnetMetricsResponse; type DeleteCanisterArgs = CanisterIdRecord; type DepositCyclesArgs = CanisterIdRecord; type RawRandResult = Vec; @@ -151,6 +152,11 @@ fn node_metrics_history(_: NodeMetricsHistoryArgs) -> NodeMetricsHistoryResult { unreachable!() } +#[candid_method(update)] +fn subnet_metrics(_: SubnetMetricsArgs) -> SubnetMetricsResult { + unreachable!() +} + #[candid_method(update)] fn provisional_create_canister_with_cycles( _: ProvisionalCreateCanisterWithCyclesArgs, diff --git a/rs/types/management_canister_types/tests/ic.did b/rs/types/management_canister_types/tests/ic.did index 5d242892bc83..00f026e9fc9c 100644 --- a/rs/types/management_canister_types/tests/ic.did +++ b/rs/types/management_canister_types/tests/ic.did @@ -453,6 +453,25 @@ type node_metrics_history_result = vec record { node_metrics : vec node_metrics; }; +type subnet_metrics_args = record { + subnet_id : principal; +}; + +type subnet_metrics_result = record { + // Current block height of the subnet, i.e. the height of the block in + // whose execution this call is processed. + block_height : nat; + // Current number of canisters on the subnet. + num_canisters : nat; + // Current total size in bytes of the state taken by canisters on the subnet. + canister_state_bytes : nat; + // Total cycles removed from circulation on the subnet by all current and + // deleted canisters. + consumed_cycles_total : nat; + // Total number of transactions processed on the subnet. + update_transactions_total : nat; +}; + type subnet_info_args = record { subnet_id : principal; }; @@ -694,6 +713,7 @@ service ic : { // metrics interface node_metrics_history : (node_metrics_history_args) -> (node_metrics_history_result); + subnet_metrics : (subnet_metrics_args) -> (subnet_metrics_result); // subnet info subnet_info : (subnet_info_args) -> (subnet_info_result); diff --git a/rs/types/types/src/messages/ingress_messages.rs b/rs/types/types/src/messages/ingress_messages.rs index 0685491cc8f8..19ee6f7a31e1 100644 --- a/rs/types/types/src/messages/ingress_messages.rs +++ b/rs/types/types/src/messages/ingress_messages.rs @@ -702,6 +702,7 @@ pub fn extract_effective_canister_id( | Ok(Method::BitcoinGetSuccessors) | Ok(Method::BitcoinGetCurrentFeePercentiles) | Ok(Method::NodeMetricsHistory) + | Ok(Method::SubnetMetrics) | Ok(Method::SubnetInfo) | Ok(Method::FetchCanisterLogs) => { // Subnet method not allowed for ingress. diff --git a/rs/types/types/src/messages/inter_canister.rs b/rs/types/types/src/messages/inter_canister.rs index 135635864aa2..fd57f9b31162 100644 --- a/rs/types/types/src/messages/inter_canister.rs +++ b/rs/types/types/src/messages/inter_canister.rs @@ -287,6 +287,7 @@ impl Request { | Ok(Method::BitcoinGetSuccessors) | Ok(Method::BitcoinGetCurrentFeePercentiles) | Ok(Method::NodeMetricsHistory) + | Ok(Method::SubnetMetrics) | Ok(Method::SubnetInfo) => { // No effective canister id. None From 813460468407a44b21284ca7a6dd8f36d1d1c352 Mon Sep 17 00:00:00 2001 From: Bjoern Tackmann Date: Wed, 5 Aug 2026 11:16:18 +0200 Subject: [PATCH 2/4] fix: Correct subnet_metrics composite-query test and document field freshness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CI failure and the Copilot review. No production logic changed; this is test code and doc comments only. **Composite-query system test.** `subnet_metrics_composite_query_fails` asserted that the routing rejection's message reaches the caller. It does not: `reject_subnet_message_routing`'s synthesized response is never delivered on the query path, so the universal canister never replies and the outer query fails `CanisterError` / "did not produce a response". This is established platform behaviour of the composite-query arm in `resolve_destination`, not something this change introduced. A control experiment showed `fetch_canister_logs` — which has the identical arm and ships enabled — behaves identically, while `canister_status`, which has no such arm, does deliver its reject (no arm means the request is created and `QueryContext::handle_request`'s reject is delivered normally). The test now asserts the real behaviour and says plainly that this makes it weak: it cannot distinguish the arm from any other failure to reply, and would pass against a stub. The method-specific assertion lives in `resolve_subnet_metrics_rejects_composite_query` in `routing.rs`, which tests `resolve_destination` directly. The division of labour is: the unit test proves the arm, the system test documents user-visible behaviour. The now-inert `.on_reject(...)` is kept deliberately, so that if the platform ever does deliver the reject, the test fails loudly rather than quietly continuing to assert the swallowed behaviour. All five `subnet_metrics` system tests now pass, verified by execution on a Linux host rather than by inspection — including the cross-subnet attribution test, which is the first genuine cross-subnet management-call test in the repo. **Field freshness docs.** Per review, the Rust doc comments described values as "current" when four of the five lag: only `block_height` is current, the other four are as of end-of-previous-round, and `canister_state_bytes` is refreshed only every 10 rounds (so it reads 0 early in a subnet's life). Documented on both `SubnetMetricsResult` and `SubnetMetricsResponse`. The review also asked for the same wording change in the two `ic.did` fixtures. Deliberately not done: those must stay byte-identical to the upstream spec's `public/references/ic.did`. That wording fix belongs in dfinity/developer-docs#333, which already carries an open item on imprecise gauge-vs-counter wording. Co-Authored-By: Claude Opus 5 --- .../ic-management-canister-types/src/lib.rs | 30 ++++++-- .../general_execution_tests/api_tests.rs | 69 ++++++++++++++----- rs/types/management_canister_types/src/lib.rs | 9 +++ 3 files changed, 87 insertions(+), 21 deletions(-) diff --git a/packages/ic-management-canister-types/src/lib.rs b/packages/ic-management-canister-types/src/lib.rs index 5809898f2026..d42343dd240e 100644 --- a/packages/ic-management-canister-types/src/lib.rs +++ b/packages/ic-management-canister-types/src/lib.rs @@ -1388,6 +1388,23 @@ pub struct SubnetMetricsArgs { /// Result type of [`subnet_metrics`](https://docs.internetcomputer.org/references/management-canister/#subnet_metrics). /// /// This API is EXPERIMENTAL and may evolve in a non-backward-compatible way. +/// +/// # Freshness +/// +/// Only `block_height` is current as of the block in which the call is executed. +/// The other four are read from the subnet's aggregated metrics, which the replica +/// updates at the *end* of a round, so they describe the state as of an earlier +/// block: +/// +/// - `num_canisters`, `update_transactions_total` and `consumed_cycles_total` are +/// as of the end of the previous round. +/// - `canister_state_bytes` is recomputed only every 10 rounds, because summing it +/// over every canister is expensive and it does not need to be exact. It can +/// therefore be up to ten rounds stale, and reads as `0` for the first rounds +/// after a subnet's first canister appears. +/// +/// These are the same values, with the same staleness, that `read_state` returns +/// for the `/subnet//metrics` path, so the two agree. #[derive( CandidType, Serialize, Deserialize, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, )] @@ -1396,15 +1413,20 @@ pub struct SubnetMetricsResult { /// execution the call is processed. Monotonically non-decreasing for a given /// subnet; heights of different subnets are unrelated. pub block_height: Nat, - /// Current number of canisters on the subnet. + /// Number of canisters on the subnet, as of the end of the previous round. pub num_canisters: Nat, - /// Current total size in bytes of the state taken by canisters on the subnet. + /// Total size in bytes of the state taken by canisters on the subnet. + /// + /// Refreshed only every 10 rounds, so this can be up to ten rounds stale (and + /// reads as `0` for the first rounds of a subnet's life). See the type-level + /// "Freshness" note. pub canister_state_bytes: Nat, /// Total cycles removed from circulation on the subnet by all current and - /// deleted canisters. + /// deleted canisters, as of the end of the previous round. pub consumed_cycles_total: Nat, /// Total number of transactions processed on the subnet, i.e. the total - /// number of messages executed in replicated mode. + /// number of messages executed in replicated mode, as of the end of the + /// previous round. pub update_transactions_total: Nat, } diff --git a/rs/tests/execution/general_execution_tests/api_tests.rs b/rs/tests/execution/general_execution_tests/api_tests.rs index c7a86b185cec..9506f0de5e44 100644 --- a/rs/tests/execution/general_execution_tests/api_tests.rs +++ b/rs/tests/execution/general_execution_tests/api_tests.rs @@ -253,11 +253,21 @@ pub fn subnet_metrics_own_subnet_succeeds(env: TestEnv) { // Assert. let bytes = result.expect("subnet_metrics call failed"); let response = decode_subnet_metrics(&bytes); - // The universal canister itself is on the subnet, so there is at - // least one canister and some state. + // The universal canister itself is on the subnet, so all three are + // non-zero by the time this call executes. `num_canisters` and + // `update_transactions_total` are written at the end of every round, so + // they are non-zero as soon as the canister exists. assert!(response.num_canisters > 0_u64); - assert!(response.canister_state_bytes > 0_u64); assert!(response.update_transactions_total > 0_u64); + // `canister_state_bytes` is refreshed only on rounds whose batch number + // is a multiple of 10 (`rs/messaging/src/message_routing.rs`), so unlike + // the other two it legitimately reads 0 for the first rounds after a + // subnet's first canister appears — measured in-process as 0 at heights + // 5 and 9, non-zero from height 17. By the time this test runs the + // subnet is well past that, and this assertion is observed to hold; it + // is noted here as the first thing to look at should this test ever + // start flaking. + assert!(response.canister_state_bytes > 0_u64); } }) } @@ -418,6 +428,35 @@ pub fn subnet_metrics_query_fails(env: TestEnv) { }) } +/// Documents what a canister developer actually observes when calling +/// `subnet_metrics` from a composite query. +/// +/// **This test is weak, and deliberately so.** It asserts only that the caller gets +/// no response, which any failure to reply would also produce — it cannot +/// distinguish "stopped by the `subnet_metrics` composite-query arm in +/// `resolve_destination`" from any other reason the canister did not reply, and it +/// would pass against a stub. The test that actually proves the arm is +/// `resolve_subnet_metrics_rejects_composite_query` in +/// `rs/embedders/src/wasmtime_embedder/system_api/routing.rs`, which calls +/// `resolve_destination` directly and asserts its error code and exact message. The +/// division of labour is: **that** unit test proves the arm; **this** system test +/// documents the end-to-end user-visible behaviour. +/// +/// **Why the arm's message cannot be asserted here.** The arm makes +/// `resolve_destination` fail, so the request never becomes a message: +/// `reject_subnet_message_routing` synthesises a reject response into the calling +/// canister's system state, and on the *query* path that response is never delivered +/// back to the callback. The universal canister therefore ends its composite query +/// without replying, and the caller gets `ErrorCode::CanisterDidNotReply` → +/// `RejectCode::CanisterError`, `"Canister did not produce a response"` — +/// never `"subnet_metrics API cannot be called from a composite query"`. +/// +/// That swallowing is **pre-existing platform behaviour of this arm, not something +/// `subnet_metrics` introduced**: `fetch_canister_logs` has the identical arm, ships +/// enabled by default, and behaves the same way. It went unnoticed because there is +/// no end-to-end test of it anywhere in the repo. Improving the error a developer +/// sees would mean changing how routing rejects are delivered on the query path for +/// every ic00 method, which is a platform change and out of scope here. pub fn subnet_metrics_composite_query_fails(env: TestEnv) { // Arrange. let (app_node, agent) = setup_app_node_and_agent(&env); @@ -431,8 +470,8 @@ pub fn subnet_metrics_composite_query_fails(env: TestEnv) { &logger, ) .await; - // Act. This is the only path on which the new `resolve_destination` - // arm runs with `is_composite_query == true`. + // Act. This is the only path on which `resolve_destination` runs with + // `is_composite_query == true`. let result = canister .composite_query( wasm().call_simple( @@ -440,24 +479,20 @@ pub fn subnet_metrics_composite_query_fails(env: TestEnv) { Method::SubnetMetrics, call_args() .other_side(ic00::SubnetMetricsArgs { subnet_id }.encode()) - // Surface the inner reject message so the assertion below - // can distinguish "rejected before the handler ran" from - // "the handler ran and rejected". + // Kept even though it never fires, so that if the platform + // ever does deliver the routing reject, this test fails + // loudly with the inner message rather than silently + // continuing to assert the swallowed behaviour. .on_reject(wasm().reject_message().reject()), ), ) .await; - // Assert. The call is rejected by `resolve_destination`'s explicit - // composite-query arm, before the handler runs and before the request - // is ever routed. `reject_subnet_message_routing` turns that into a - // `DestinationInvalid` reject on the inner call, whose message the - // universal canister re-rejects above — so the method name in the - // asserted text is what makes this test method-specific rather than a - // generic "queries cannot call ic00" check. + // Assert. See the doc comment: the reject is swallowed, so the + // observable outcome is that the canister produced no response. assert_reject_msg( result, - RejectCode::CanisterReject, - "subnet_metrics API cannot be called from a composite query", + RejectCode::CanisterError, + "did not produce a response", ); } }) diff --git a/rs/types/management_canister_types/src/lib.rs b/rs/types/management_canister_types/src/lib.rs index f22088cd23d8..7203007d9446 100644 --- a/rs/types/management_canister_types/src/lib.rs +++ b/rs/types/management_canister_types/src/lib.rs @@ -3810,6 +3810,15 @@ impl Payload<'_> for SubnetMetricsArgs {} /// update_transactions_total : nat; /// } /// ``` +/// +/// Freshness: only `block_height` is current as of the block in which the call is +/// executed. The other four are read from `SystemMetadata::subnet_metrics`, which is +/// written at the *end* of a round, so they are as of the end of the previous round +/// — except `canister_state_bytes`, which message routing recomputes only every 10 +/// rounds (summing it over every canister is expensive and it need not be exact), +/// so it can be up to ten rounds stale and reads as `0` for the first rounds after a +/// subnet's first canister appears. These are the same values, with the same +/// staleness, that `read_state` serves at `/subnet//metrics`. #[derive(Clone, Debug, Deserialize, CandidType, Serialize, PartialEq)] pub struct SubnetMetricsResponse { pub block_height: candid::Nat, From eabc3d27ea4aa8414b49e4cfde834a851e548582 Mon Sep 17 00:00:00 2001 From: Bjoern Tackmann Date: Wed, 5 Aug 2026 13:10:49 +0200 Subject: [PATCH 3/4] refactor: Simplify subnet_metrics change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three simplifications, no behaviour change. Net -296 insertions, -4 files. **Share the consumed-cycles formula instead of testing for drift.** `SubnetMetrics::consumed_cycles_total_including_canisters()` is now called by both the canonical-state encoder and the `subnet_metrics` handler, so the invariant is structural rather than pinned by a cross-check test. That test (78 lines) and its reciprocal keep-in-sync comments are deleted. The method lives on `SubnetMetrics` rather than `ReplicatedState` because `SubnetMetrics::from` — where the state tree does the addition — has no `ReplicatedState` in scope. Only the `>= V29` branch is rerouted through the new method; the `<= V28` branch still calls `consumed_cycles_total_v28()` untouched, so no state hash at any existing certification version moves. **Drop the `end_to_end` benchmark group.** It was never successfully measured, and the per-canister constant came from `bench_consumed_cycles_fold` instead. Its test-canister plumbing goes with it, restoring `benches/management_canister/test_canister/` to its previous state. The base-cost doc comment now states plainly that the base is estimated from the handler's fixed work and was never measured end to end, rather than pointing at a benchmark that no longer exists. **Move `validate_cold_stats()` out to its own change.** It is hardening for pre-existing code, not a requirement of this endpoint: `ColdStats` is already consensus-critical today via `canister_state_bytes`, with no check at all. The determinism argument for reading `hot_len` does not depend on it — it rests on `is_cold()` being time-independent, the partition never being serialized, unconditional repartitioning at commit, and all four state acquisition paths agreeing. `rs/state_manager/src/checkpoint.rs` and `rs/replicated_state/src/canister_states.rs` are byte-identical to master again. What deliberately stays, because it guards a coupling *this* change introduces rather than the removed check: `hot_cold_partition_is_canonical_after_every_commit`, the `repartition_canister_states` doc comment, and the test that `total_consumed_cycles()` equals a direct fold. Co-Authored-By: Claude Opus 5 --- rs/canonical_state/src/encoding.rs | 1 - .../src/encoding/tests/subnet_metrics.rs | 78 ---------------- rs/canonical_state/src/encoding/types.rs | 2 +- .../management_canister/subnet_metrics.rs | 91 +------------------ .../test_canister/candid.did | 1 - .../test_canister/src/main.rs | 28 ------ .../src/execution_environment.rs | 28 +++--- rs/replicated_state/src/canister_states.rs | 42 --------- .../src/canister_states/tests.rs | 44 +-------- rs/replicated_state/src/metadata_state.rs | 22 +++++ rs/state_manager/src/checkpoint.rs | 68 +++++--------- 11 files changed, 66 insertions(+), 339 deletions(-) delete mode 100644 rs/canonical_state/src/encoding/tests/subnet_metrics.rs diff --git a/rs/canonical_state/src/encoding.rs b/rs/canonical_state/src/encoding.rs index 53598f4dd9b1..96203a2df8f4 100644 --- a/rs/canonical_state/src/encoding.rs +++ b/rs/canonical_state/src/encoding.rs @@ -145,6 +145,5 @@ mod tests { mod compatibility; mod conversion; mod encoding; - mod subnet_metrics; mod test_fixtures; } diff --git a/rs/canonical_state/src/encoding/tests/subnet_metrics.rs b/rs/canonical_state/src/encoding/tests/subnet_metrics.rs deleted file mode 100644 index b10cd250db30..000000000000 --- a/rs/canonical_state/src/encoding/tests/subnet_metrics.rs +++ /dev/null @@ -1,78 +0,0 @@ -//! Cross-checks the `subnet_metrics` management canister method's -//! `consumed_cycles_total` against the canonical (certified) state encoding. - -use crate::CertificationVersion; -use crate::encoding::types::SubnetMetrics as CanonicalSubnetMetrics; -use ic_replicated_state::CanisterStates; -use ic_replicated_state::metadata_state::SubnetMetrics; -use ic_test_utilities_state::new_canister_state; -use ic_test_utilities_types::ids::{canister_test_id, user_test_id}; -use ic_types::NumBytes; -use ic_types_cycles::{ - CanisterCyclesCostSchedule, CompoundCycles, Cycles, Instructions, NominalCycles, - NominalCyclesTesting, -}; -use std::sync::Arc; - -/// The `consumed_cycles_total` that `ExecutionEnvironment::subnet_metrics` -/// computes must equal the one that the canonical state encoding produces at -/// certification version `V29`. -/// -// Keep in sync with `ExecutionEnvironment::subnet_metrics` in -// `rs/execution_environment/src/execution_environment.rs`, which carries the -// reciprocal comment. -#[test] -fn subnet_metrics_consumed_cycles_matches_v29_canonical_encoding() { - let mut metrics = SubnetMetrics::default(); - metrics.num_canisters = 3; - metrics.canister_state_bytes = NumBytes::new(1_234); - metrics.update_transactions_total = 42; - metrics.observe_consumed_cycles_by_deleted_canisters(NominalCycles::new(1_000_000_007)); - metrics.observe_consumed_cycles_http_outcalls(NominalCycles::new(2_000_000_011)); - - let mut canisters = CanisterStates::default(); - for id in 1..=3_u64 { - let mut canister = new_canister_state( - canister_test_id(id), - user_test_id(1).get(), - Cycles::new(1 << 60), - ic_base_types::NumSeconds::new(100_000), - ); - canister - .system_state - .consume_cycles(CompoundCycles::::new( - Cycles::new(100_000 * id as u128), - CanisterCyclesCostSchedule::Normal, - )); - canisters.insert(Arc::new(canister)); - } - - // What the `subnet_metrics` handler computes. - let handler_total = metrics.consumed_cycles_total() + canisters.total_consumed_cycles(); - - // What the certified state tree reports at `V29`, recombined from its - // `(high, low)` parts. - let canonical = CanonicalSubnetMetrics::from(( - &metrics, - canisters.total_consumed_cycles(), - CertificationVersion::V29, - )); - let low = canonical.consumed_cycles_total.low; - let high = canonical.consumed_cycles_total.high.unwrap(); - let canonical_total = ((high as u128) << 64) | (low as u128); - - assert_eq!(handler_total.get(), canonical_total); - // The test would be vacuous if both were zero. - assert!(canonical_total > 0); - - // The other three fields pass through unchanged. - assert_eq!(canonical.num_canisters, metrics.num_canisters); - assert_eq!( - canonical.canister_state_bytes, - metrics.canister_state_bytes.get() - ); - assert_eq!( - canonical.update_transactions_total, - metrics.update_transactions_total - ); -} diff --git a/rs/canonical_state/src/encoding/types.rs b/rs/canonical_state/src/encoding/types.rs index cee7d4eeaad8..bda192d6278c 100644 --- a/rs/canonical_state/src/encoding/types.rs +++ b/rs/canonical_state/src/encoding/types.rs @@ -744,7 +744,7 @@ impl // `consumed_cycles_total` (which no longer double counts deleted // canisters) plus the cycles consumed by all non-deleted canisters. let consumed_cycles_total = if certification_version >= CertificationVersion::V29 { - metrics.consumed_cycles_total() + consumed_cycles_by_canisters + metrics.consumed_cycles_total_including_canisters(consumed_cycles_by_canisters) } else { metrics.consumed_cycles_total_v28() }; diff --git a/rs/execution_environment/benches/management_canister/subnet_metrics.rs b/rs/execution_environment/benches/management_canister/subnet_metrics.rs index 2e5d7301d157..1dc0897b9e90 100644 --- a/rs/execution_environment/benches/management_canister/subnet_metrics.rs +++ b/rs/execution_environment/benches/management_canister/subnet_metrics.rs @@ -1,95 +1,12 @@ -use crate::create_canisters::CreateCanistersArgs; -use crate::utils::{CANISTERS_PER_BATCH, expect_reply, test_canister_wasm}; -use candid::{Encode, Principal}; use criterion::{BenchmarkGroup, Criterion, criterion_group, criterion_main}; -use ic_base_types::{CanisterId, NumBytes, NumSeconds}; -use ic_config::execution_environment::Config as HypervisorConfig; -use ic_config::subnet_config::SubnetConfig; -use ic_registry_subnet_type::SubnetType; +use ic_base_types::{NumBytes, NumSeconds}; use ic_replicated_state::canister_state::canister_snapshots::CanisterSnapshots; use ic_replicated_state::canister_state::system_state::SystemState; use ic_replicated_state::{CanisterState, CanisterStates, SchedulerState}; -use ic_state_machine_tests::{StateMachine, StateMachineBuilder, StateMachineConfig}; use ic_test_utilities_types::ids::canister_test_id; use ic_types_cycles::{CanisterCyclesCostSchedule, CompoundCycles, Cycles, Instructions}; use std::sync::Arc; -/// Builds a `StateMachine` and populates the subnet with `canisters_number` -/// canisters, created through a test canister via batched inter-canister calls. -/// Returns the `StateMachine` and the test canister ID. -/// -/// `subnet_metrics` is canister-only, so the call must go through the test -/// canister; unlike `list_canisters` it needs no subnet-admin setup. -fn setup_with_canisters(canisters_number: u64) -> (StateMachine, CanisterId) { - let env = StateMachineBuilder::new() - .with_config(Some(StateMachineConfig::new( - SubnetConfig::new(SubnetType::Application), - HypervisorConfig::default(), - ))) - .with_subnet_type(SubnetType::Application) - .with_cost_schedule(CanisterCyclesCostSchedule::Free) - .build(); - - let test_canister = env.create_canister_with_cycles(None, Cycles::new(u128::MAX / 2), None); - env.install_existing_canister(test_canister, test_canister_wasm(), vec![]) - .expect("failed to install the test canister"); - - const CHUNK: u64 = 5_000; - let mut remaining_to_create = canisters_number; - while remaining_to_create > 0 { - let chunk = remaining_to_create.min(CHUNK); - remaining_to_create -= chunk; - let result = env.execute_ingress( - test_canister, - "create_canisters", - Encode!(&CreateCanistersArgs { - canisters_number: chunk, - canisters_per_batch: CANISTERS_PER_BATCH, - initial_cycles: 0, - }) - .unwrap(), - ); - let created: Vec = expect_reply(result); - assert_eq!(created.len() as u64, chunk); - } - - (env, test_canister) -} - -/// Measures the end-to-end cost of one `subnet_metrics` call on a subnet with -/// `canisters_number` canisters. This is what `BASE_INSTRUCTIONS` in -/// `subnet_metrics_instructions` must cover: message induction, the reads from -/// `state.metadata.subnet_metrics`, the fold over the hot pool, and the Candid -/// encode. -/// -/// Note that the canisters created during setup are demoted to the cold pool -/// after a round of inactivity (`repartition_canister_states` runs on every -/// commit), so this measurement deliberately does *not* capture the -/// per-hot-canister term — which is also why the charge must be keyed on -/// `hot_len()` rather than `num_canisters()`: on a mostly-cold subnet the two -/// differ by orders of magnitude while the work does not. The per-hot-canister -/// term is measured by `bench_consumed_cycles_fold`. -fn bench_end_to_end( - group: &mut BenchmarkGroup, - bench_name: &str, - canisters_number: u64, -) { - // `subnet_metrics` is read-only, so the environment (and its set of - // canisters) does not change across iterations and can be set up once. - let (env, test_canister) = setup_with_canisters(canisters_number); - let subnet_id: Principal = env.get_subnet_id().get().into(); - group.bench_function(bench_name, |b| { - b.iter(|| { - let result = env.execute_ingress( - test_canister, - "subnet_metrics", - Encode!(&subnet_id).unwrap(), - ); - let _num_canisters: u64 = expect_reply(result); - }); - }); -} - /// Builds one hot canister with non-zero consumed cycles. /// /// A non-zero `heap_delta_debit` keeps a canister out of the cold pool @@ -193,12 +110,6 @@ fn bench_consumed_cycles_fold( } pub fn subnet_metrics_benchmark(c: &mut Criterion) { - let mut group = c.benchmark_group("subnet_metrics"); - bench_end_to_end(&mut group, "end_to_end/10", 10); - bench_end_to_end(&mut group, "end_to_end/1k", 1_000); - bench_end_to_end(&mut group, "end_to_end/10k", 10_000); - group.finish(); - let mut group = c.benchmark_group("subnet_metrics_consumed_cycles_fold"); for n in [0_u64, 1_000, 10_000, 100_000] { let label = match n { diff --git a/rs/execution_environment/benches/management_canister/test_canister/candid.did b/rs/execution_environment/benches/management_canister/test_canister/candid.did index 91194ea69e39..72b2eba18e98 100644 --- a/rs/execution_environment/benches/management_canister/test_canister/candid.did +++ b/rs/execution_environment/benches/management_canister/test_canister/candid.did @@ -45,5 +45,4 @@ service : { "sign_with_ecdsa" : (ecdsa_args) -> (); "http_request" : (http_request_args) -> (); "list_canisters" : () -> (nat64); - "subnet_metrics" : (principal) -> (nat64); }; diff --git a/rs/execution_environment/benches/management_canister/test_canister/src/main.rs b/rs/execution_environment/benches/management_canister/test_canister/src/main.rs index 50db733acdb2..cd99e2e6fce4 100644 --- a/rs/execution_environment/benches/management_canister/test_canister/src/main.rs +++ b/rs/execution_environment/benches/management_canister/test_canister/src/main.rs @@ -309,32 +309,4 @@ async fn list_canisters() -> u64 { result.canisters.len() as u64 } -#[derive(Clone, Debug, CandidType, Deserialize, Serialize)] -pub struct SubnetMetricsArgs { - pub subnet_id: Principal, -} - -#[derive(Clone, Debug, CandidType, Deserialize, Serialize)] -pub struct SubnetMetricsResult { - pub block_height: candid::Nat, - pub num_canisters: candid::Nat, - pub canister_state_bytes: candid::Nat, - pub consumed_cycles_total: candid::Nat, - pub update_transactions_total: candid::Nat, -} - -/// Calls the management canister's `subnet_metrics` method for the given subnet -/// and returns the reported number of canisters. -#[update] -async fn subnet_metrics(subnet_id: Principal) -> u64 { - let result: SubnetMetricsResult = - Call::unbounded_wait(Principal::management_canister(), "subnet_metrics") - .with_arg(SubnetMetricsArgs { subnet_id }) - .await - .expect("subnet_metrics call failed") - .candid() - .expect("failed to decode subnet_metrics response"); - u64::try_from(result.num_canisters.0).expect("num_canisters does not fit into u64") -} - fn main() {} diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs index 0907959ee0ac..463525339376 100644 --- a/rs/execution_environment/src/execution_environment.rs +++ b/rs/execution_environment/src/execution_environment.rs @@ -3404,17 +3404,11 @@ impl ExecutionEnvironment { )); } let metrics = &state.metadata.subnet_metrics; - // Keep in sync with the certified state tree at - // `/subnet//metrics`: this is the same sum that - // `ic_canonical_state::encoding::types::SubnetMetrics::from` computes - // starting with certification version `V29`. Pinned by - // `subnet_metrics_consumed_cycles_matches_v29_canonical_encoding` in - // `rs/canonical_state`, which carries the reciprocal comment. - // - // `total_consumed_cycles()` reads the derived `ColdStats::consumed_cycles` - // aggregate. - let consumed_cycles_total = - metrics.consumed_cycles_total() + state.canister_states().total_consumed_cycles(); + // The same function the certified state tree at `/subnet//metrics` + // uses from certification version `V29` on, so the two cannot drift. + let consumed_cycles_total = metrics.consumed_cycles_total_including_canisters( + state.canister_states().total_consumed_cycles(), + ); let res = SubnetMetricsResponse { // The height of the block in whose execution this call is processed. // `ExecutionRound` is numerically the finalized consensus block @@ -5085,9 +5079,15 @@ pub(crate) fn full_subnet_memory_capacity( /// /// The base covers the per-call work that does not scale with the number of /// canisters: the Candid decode of the argument, five field reads, and the Candid -/// encode of five `Nat`s. That is well under 50us. It is estimated from that -/// work rather than measured end to end, deliberately on the generous side, and -/// is 200x below `list_canisters`'s 20M. +/// encode of five `Nat`s. **It is estimated from that work and was never measured +/// end to end** — there is no benchmark for it, deliberately, since an end-to-end +/// `StateMachine` measurement is dominated by round overhead rather than by the +/// handler. The estimate is generous: that work is order 2-10us against the 50us +/// that 100K instructions represents, and the constant is 200x below +/// `list_canisters`'s 20M. Over-estimating the base is safe for the wall-clock +/// bound (fewer calls are served per round) and costs only denial headroom, which +/// is priced in the security review against `fetch_canister_logs` — a deployed +/// method of the same shape with a *larger* base of 150K and likewise no cycle fee. /// /// Both constants are far below `list_canisters`'s 20M / 16K. That is /// intentional: `list_canisters` is gated to subnet admins, whereas diff --git a/rs/replicated_state/src/canister_states.rs b/rs/replicated_state/src/canister_states.rs index 801df88b12e6..ec78a48ac2d0 100644 --- a/rs/replicated_state/src/canister_states.rs +++ b/rs/replicated_state/src/canister_states.rs @@ -160,11 +160,6 @@ impl ColdStats { /// 2. every canister in the `cold` pool satisfies `CanisterState::is_cold()`; /// 3. `cold_stats` matches a fresh recomputation over the `cold` pool. /// -/// Invariant (3) is *additionally checked* in release builds during checkpoint -/// validation, by [`Self::validate_cold_stats`]. That check is advisory: it logs -/// a critical error and increments a counter, and does not abort or otherwise -/// alter the checkpoint. -/// /// Additionally, the **strict** partition invariant — that every canister in /// the `hot` pool does *not* satisfy `is_cold()` — holds after /// [`Self::try_cool_all`] / @@ -650,43 +645,6 @@ impl CanisterStates { Ok(()) } - /// Validates that `cold_stats` matches a fresh recomputation over the `cold` - /// pool, i.e. that the sub-before / add-after bracketing around every - /// cold-pool mutation has been respected. - /// - /// Unlike the `debug_assert` in `debug_assert_invariants`, this is intended to - /// run in release builds during checkpoint validation, because the aggregates - /// are read into hashed replicated state - /// (`SubnetMetrics::canister_state_bytes`, which has no other check) and - /// returned to canisters (`subnet_metrics`). - /// - /// It runs only for a *locally produced* checkpoint, i.e. on the branch of - /// `validate_and_finalize_checkpoint_and_remove_unverified_marker` that has a - /// reference state, and not on the state-sync path. That is the only branch - /// where it could find anything: a `CanisterStates` freshly loaded from disk has - /// `cold_stats` recomputed by `CanisterStates::new`, so it is consistent by - /// construction; only the in-memory reference state can have drifted. - /// - /// Note that the caller's failure mode is **advisory**: `validate_eq_checkpoint` - /// logs a critical error and increments a counter, then finalizes the - /// checkpoint regardless. This detects and attributes a stale aggregate; it - /// does not prevent one from being used. The caller runs it *after* the - /// per-canister comparison and combines the two errors, so that an advisory - /// failure here does not mask the diagnostics that identify which canister - /// drifted. - /// - /// Complexity: `O(|cold canisters|)`. - pub fn validate_cold_stats(&self) -> Result<(), String> { - let recomputed = ColdStats::recompute(self.cold.values()); - if recomputed != self.cold_stats { - return Err(format!( - "cold_stats out of sync with the cold pool: stored {:?}, recomputed {:?}", - self.cold_stats, recomputed - )); - } - Ok(()) - } - /// Debug-only consistency check, called at the end of every mutating operation. /// Verifies invariants (1)–(3) listed under [`CanisterStates`]. /// diff --git a/rs/replicated_state/src/canister_states/tests.rs b/rs/replicated_state/src/canister_states/tests.rs index f2aea2290037..473e91197960 100644 --- a/rs/replicated_state/src/canister_states/tests.rs +++ b/rs/replicated_state/src/canister_states/tests.rs @@ -948,45 +948,6 @@ fn total_consumed_cycles_equals_direct_fold() { ); } -#[test] -fn validate_cold_stats_accepts_consistent_stats() { - let mut states = CanisterStates::default(); - states.insert(cold_canister(1)); - states.insert(hot_canister(2)); - states.insert(cold_canister(3)); - - assert_eq!(states.validate_cold_stats(), Ok(())); -} - -#[test] -fn validate_cold_stats_rejects_stale_stats() { - use ic_types_cycles::{NominalCycles, NominalCyclesTesting}; - - let mut states = CanisterStates::default(); - let c = cold_canister(1); - states.insert(Arc::clone(&c)); - assert_eq!(states.validate_cold_stats(), Ok(())); - - // Bypass the public mutation entry points: mutate a cold canister's consumed - // cycles directly, behind the aggregate's back, simulating missing - // sub-before / add-after bracketing. - consume_cycles(states.cold.get_mut(&c.canister_id()).unwrap(), 42); - assert_eq!(states.hot.len(), 0); - assert_eq!(states.cold.len(), 1); - - let err = states.validate_cold_stats().unwrap_err(); - assert!( - err.contains("cold_stats out of sync with the cold pool"), - "unexpected error: {err}", - ); - // The aggregate is stale, so the reported total is now wrong. - assert_eq!(states.cold_stats.consumed_cycles, NominalCycles::new(0)); - assert_ne!( - states.total_consumed_cycles(), - direct_consumed_cycles_fold(&states) - ); -} - #[test] fn for_each_mut_keeps_cold_stats_consumed_cycles_in_sync() { let mut states = CanisterStates::default(); @@ -999,7 +960,10 @@ fn for_each_mut_keeps_cold_stats_consumed_cycles_in_sync() { // including the cold ones. states.for_each_mut(|_id, canister| consume_cycles(canister, 11)); - assert_eq!(states.validate_cold_stats(), Ok(())); + // `total_consumed_cycles()` combines the hot fold with the `cold_stats` + // aggregate, so it agrees with a direct fold over every canister only if the + // sub-before / add-after bracketing around the cold-pool mutation held. That + // is the property `subnet_metrics` depends on. assert_eq!( states.total_consumed_cycles(), direct_consumed_cycles_fold(&states) diff --git a/rs/replicated_state/src/metadata_state.rs b/rs/replicated_state/src/metadata_state.rs index 73ccd68582a6..b2819d02496f 100644 --- a/rs/replicated_state/src/metadata_state.rs +++ b/rs/replicated_state/src/metadata_state.rs @@ -644,6 +644,28 @@ impl SubnetMetrics { total } + /// All cycles removed from circulation on the subnet, by both deleted and + /// still-existing canisters: the subnet-level aggregate + /// ([`Self::consumed_cycles_total`]) plus the cycles consumed by the canisters + /// that currently exist, which the caller obtains from + /// [`crate::CanisterStates::total_consumed_cycles`]. + /// + /// The canisters' contribution is a parameter rather than read here because + /// `SubnetMetrics` does not own the canisters, and because computing it is + /// `O(|hot canisters|)` — the certified state tree only wants it from + /// certification version `V29` onwards and passes `zero()` below that. + /// + /// **This is the single definition of the quantity, deliberately.** Two + /// consumers must agree on it bit for bit: the certified state tree at + /// `/subnet//metrics` (from `V29`) and the `subnet_metrics` + /// management canister method. Both call this, so they cannot drift. + pub fn consumed_cycles_total_including_canisters( + &self, + consumed_cycles_by_canisters: NominalCycles, + ) -> NominalCycles { + self.consumed_cycles_total() + consumed_cycles_by_canisters + } + /// Legacy computation of the total consumed cycles, used by the canonical /// state consumer for certification versions up to and including `V28`. /// diff --git a/rs/state_manager/src/checkpoint.rs b/rs/state_manager/src/checkpoint.rs index c977359d1aba..c62a7c167a0a 100644 --- a/rs/state_manager/src/checkpoint.rs +++ b/rs/state_manager/src/checkpoint.rs @@ -555,51 +555,31 @@ impl CheckpointLoader { .or_default() .push(snapshot_id); } - let per_canister = - maybe_parallel_map(thread_pool, ref_canister_ids.iter(), |canister_id| { - load_canister_state_from_checkpoint( - &self.checkpoint_layout, - canister_id, - snapshot_ids_per_canister - .get(canister_id) - .cloned() - .unwrap_or_default(), - Arc::clone(&self.fd_factory), - &self.metrics, - ) - .map_err(|err| { - format!( - "Failed to load canister state for validation for key #{canister_id}: {err}" - ) - })? - .0 - .validate_eq( - ref_canister_states - .get(canister_id) - .expect("Failed to get canister from canister_states"), + maybe_parallel_map(thread_pool, ref_canister_ids.iter(), |canister_id| { + load_canister_state_from_checkpoint( + &self.checkpoint_layout, + canister_id, + snapshot_ids_per_canister + .get(canister_id) + .cloned() + .unwrap_or_default(), + Arc::clone(&self.fd_factory), + &self.metrics, + ) + .map_err(|err| { + format!( + "Failed to load canister state for validation for key #{canister_id}: {err}" ) - }) - .into_iter() - .try_for_each(identity); - - // Detect (and attribute) a stale cold-pool aggregate. Like every other - // check here, this is advisory: the caller logs a critical error and - // increments a counter, then finalizes the checkpoint regardless. - // - // Deliberately run *after* the per-canister comparison above, and combined - // with it rather than short-circuiting it: in the very scenario where this - // check fires, the per-canister diagnostics are what tell the operator - // *which* canister drifted, and an advisory check must not cost the - // operator that information. - let cold_stats = ref_canister_states - .validate_cold_stats() - .map_err(|err| format!("Canister Validation: {err}")); - - match (per_canister, cold_stats) { - (Ok(()), Ok(())) => Ok(()), - (Err(err), Ok(())) | (Ok(()), Err(err)) => Err(err), - (Err(per_canister), Err(cold_stats)) => Err(format!("{per_canister}; {cold_stats}")), - } + })? + .0 + .validate_eq( + ref_canister_states + .get(canister_id) + .expect("Failed to get canister from canister_states"), + ) + }) + .into_iter() + .try_for_each(identity) } fn validate_eq_canister_snapshots_ids( From 0e795b74268dfd318a8534d15d65789e215e2358 Mon Sep 17 00:00:00 2001 From: Bjoern Tackmann Date: Wed, 5 Aug 2026 17:04:17 +0200 Subject: [PATCH 4/4] refactor: Record subnet_metrics as counts_toward_round_limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review feedback. Behaviour-neutral: `counts_toward_round_limit` is read only in `Ic00MethodPermissions::can_be_executed`, whose single call site in the repo is `scheduler.rs:1847` — and `can_execute_subnet_msg` returns earlier, at the `ListCanisters | SubnetMetrics` special case, so the flag is unreachable for this method. Both affected test targets pass unchanged. The flag now records that the method does consume round instructions. Two comments had to change to keep the tree self-consistent: * The note in `ic00_permissions.rs` no longer says the flag is unset because it is not consulted. It states that the flag is not consulted, and warns that the deferral comes from the dedicated special case in `can_execute_subnet_msg`, which must not be removed on the strength of this flag. * The doc on `check_consumes_round_instructions_without_effective_canister_id` said such methods "cannot use `Ic00MethodPermissions::counts_toward_round_limit`", which is no longer accurate for `subnet_metrics`. It now says the flag is never consulted for them and so cannot identify them whatever its value — true for both entries. `ListCanisters` is in the identical position and remains `false`. Aligning it would be more consistent but changes pre-existing configuration outside this change's scope; the asymmetry is noted in the comment. Co-Authored-By: Claude Opus 5 --- rs/execution_environment/src/ic00_permissions.rs | 14 +++++++++----- rs/test_utilities/execution_environment/src/lib.rs | 9 ++++++--- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/rs/execution_environment/src/ic00_permissions.rs b/rs/execution_environment/src/ic00_permissions.rs index c812d3b366be..2701f4cd23c5 100644 --- a/rs/execution_environment/src/ic00_permissions.rs +++ b/rs/execution_environment/src/ic00_permissions.rs @@ -59,10 +59,6 @@ impl Ic00MethodPermissions { | Ic00Method::BitcoinSendTransactionInternal | Ic00Method::BitcoinGetSuccessors | Ic00Method::NodeMetricsHistory - // `counts_toward_round_limit` is never consulted for `SubnetMetrics`: - // the method has no effective canister ID, so it is handled by the - // special case in `Scheduler::can_execute_subnet_msg` instead. - | Ic00Method::SubnetMetrics | Ic00Method::SubnetInfo | Ic00Method::ProvisionalCreateCanisterWithCycles | Ic00Method::ProvisionalTopUpCanister @@ -76,7 +72,15 @@ impl Ic00MethodPermissions { does_not_run_on_aborted_canister: false, installs_code: false, }, - Ic00Method::FetchCanisterLogs + // `SubnetMetrics` consumes round instructions, and is recorded as such + // here. Note the flag is not actually consulted for it: the method has no + // effective canister ID, so `Scheduler::can_execute_subnet_msg` returns + // before reaching `can_be_executed`. Its deferral comes from the dedicated + // special case there, which must not be removed on the strength of this + // flag. (`ListCanisters` is in the same position but is recorded as + // `false`; see the note above.) + Ic00Method::SubnetMetrics + | Ic00Method::FetchCanisterLogs | Ic00Method::ReadCanisterSnapshotMetadata | Ic00Method::ReadCanisterSnapshotData => Self { method, diff --git a/rs/test_utilities/execution_environment/src/lib.rs b/rs/test_utilities/execution_environment/src/lib.rs index 2f4b3be69e42..73ac4643bde0 100644 --- a/rs/test_utilities/execution_environment/src/lib.rs +++ b/rs/test_utilities/execution_environment/src/lib.rs @@ -3263,9 +3263,12 @@ fn check_is_install_code(message: SubnetMessage) -> bool { } /// Whether the message is one of the management methods that consume round -/// instructions even though they have no effective canister ID (and therefore -/// cannot use `Ic00MethodPermissions::counts_toward_round_limit`). Keep in sync -/// with the special case in `Scheduler::can_execute_subnet_msg`. +/// instructions even though they have no effective canister ID. Their +/// `Ic00MethodPermissions::counts_toward_round_limit` flag is never consulted — +/// `Scheduler::can_execute_subnet_msg` returns before reaching +/// `can_be_executed` — so it cannot be used to identify them, whatever its +/// value. Keep in sync with the special case in +/// `Scheduler::can_execute_subnet_msg`. fn check_consumes_round_instructions_without_effective_canister_id(message: SubnetMessage) -> bool { let message = match message { SubnetMessage::Response(_) => return false,