refactor: Hold the whole consumed-cycles total in SubnetMetrics (#11335)

`SubnetMetrics::consumed_cycles_by_canisters` holds only the canisters'
part of
the total; every consumer adds the subnet-level aggregate on top,
recomputing
that part each time it reads. That is fine for a consumer that only ever
looks
at a committed state -- the certified tree and the gauge -- but it makes
the
combined value ill-defined mid-round: the canisters' part is as of the
last
commit, while the subnet-level accumulators move as the round proceeds.
A
`delete_canister` adds the deleted canister's consumption to
`consumed_cycles_by_deleted_canisters` at once, so between that and the
next
commit the two parts count the same canister twice.

Hold the whole total instead. `consumed_cycles_by_canisters` becomes
`consumed_cycles_total_including_canisters`, still transient, now
covering the
subnet-level aggregate as well. The field is private, with a getter of
the same
name and a single writer, `SubnetMetrics::refresh_consumed_cycles`,
which takes
the canisters' part and adds `consumed_cycles_total()` on top: callers
hand over
that part only, so the sum lives in one place and none of them can get
it wrong.
The certified state tree at `/subnet/<subnet_id>/metrics` (from
certification
version `V29`) and the
`replicated_state_consumed_cycles_since_replica_started`
gauge both read the getter, so they cannot drift and any future consumer
gets a
self-consistent value whenever it reads.

`ReplicatedState::refresh_consumed_cycles`, renamed from
`refresh_consumed_cycles_by_canisters` now that it publishes more than
the
canisters' part, hands the canister states' sum over on every
`commit_and_certify`, and `new_from_checkpoint` does the same on load,
so a
replica restarting from a checkpoint agrees with one that keeps running
--
which `consumed_cycles_total_is_the_same_across_a_restart` pins,
comparing a
live state against one reloaded from the same canisters and subnet
metrics.

The certified encoding and the state hash are unchanged: the byte-exact
expectations in `encoding/tests/compatibility.rs` and the `V29` hash in
`state_manager/src/tree_hash.rs` still hold, with their fixtures now
handing
their canisters' part to `refresh_consumed_cycles` rather than writing
the
aggregate themselves.
`test_traverse_subnet_metrics_includes_canister_consumed_cycles_at_v29`
now also consumes subnet-level cycles, so the total it pins covers both
parts
rather than only the canisters' one. The gauge does now depend on the
refresh
having run, which in production it has, as `commit_and_certify` enqueues
the
observation after it; the scheduler metrics tests, which never commit a
state,
refresh explicitly through the new `observe_state_metrics` helper.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: mraszyk

rs/canonical_state/src/encoding/tests/compatibility.rs

@@ -307,8 +307,9 @@ fn canonical_encoding_stream_header_v26() {
 /// Starting with `V29`, the `Instructions` (80B) and
 /// `RequestAndResponseTransmission` (20B) use case entries are no longer added
 /// on top of the deleted canisters scalar (fixing the double counting), while
-/// the cycles consumed by non-deleted canisters (50B, passed to
-/// `encode_subnet_metrics`) are added instead. Hence the expected value becomes
+/// the cycles consumed by non-deleted canisters (50B, folded into the stored
+/// aggregate by `SubnetMetrics::refresh_consumed_cycles`) are added instead.
+/// Hence the expected value becomes
 /// 0 (deleted) + 50B (HTTP) + 100B (ECDSA) + 50B (canisters) = 200B
 /// (`1B 0000002E90EDD000`).
 ///
@@ -342,8 +343,7 @@ fn canonical_encoding_subnet_metrics() {
         metrics.threshold_signature_agreements =
             BTreeMap::from([(schnorr_key_id, 15), (ecdsa_key_id, 16)]);
 
-        // The canister-consumed part of the reported total, included from `V29` on.
-        metrics.consumed_cycles_by_canisters = NominalCycles::new(50_000_000_000);
+        metrics.refresh_consumed_cycles(NominalCycles::new(50_000_000_000));
 
         let expected = if certification_version >= CertificationVersion::V29 {
             "A4 00 05 01 1A 00 50 00 00 02 A2 00 1B 00 00 00 2E 90 ED D0 00 01 00 03 19 10 68"

rs/canonical_state/src/encoding/types.rs

@@ -737,9 +737,10 @@ impl
         // `consumed_cycles_total_v28`, which double counts the cycles consumed
         // by deleted canisters and does not account for non-deleted canisters.
         //
-        // Starting with `V29`, the reported total uses the fixed
-        // `consumed_cycles_total` (which no longer double counts deleted
-        // canisters) plus `SubnetMetrics::consumed_cycles_by_canisters`.
+        // Starting with `V29`, the reported total is the stored
+        // `SubnetMetrics::consumed_cycles_total_including_canisters`, which no
+        // longer double counts deleted canisters and does account for the existing
+        // ones.
         let consumed_cycles_total = if certification_version >= CertificationVersion::V29 {
             metrics.consumed_cycles_total_including_canisters()
         } else {

rs/canonical_state/src/lazy_tree_conversion.rs

@@ -1144,10 +1144,7 @@ fn subnets_as_tree<'a>(
                         subnet_id == &own_subnet_id,
                         "metrics",
                         // Starting with `V29`, the reported total also includes
-                        // the cycles consumed by all non-deleted canisters, read
-                        // from `SubnetMetrics::consumed_cycles_by_canisters`
-                        // (refreshed by
-                        // `ReplicatedState::refresh_consumed_cycles_by_canisters`).
+                        // the cycles consumed by all non-deleted canisters.
                         blob(move || encode_subnet_metrics(metrics, certification_version)),
                     )
                     .with_tree_if(

rs/canonical_state/src/traversal.rs

@@ -1199,7 +1199,9 @@ mod tests {
     #[test]
     fn test_traverse_subnet_metrics_includes_canister_consumed_cycles_at_v29() {
         use crate::encoding::encode_subnet_metrics;
-        use ic_types_cycles::{CompoundCycles, Instructions, NominalCycles};
+        use ic_types_cycles::{
+            CompoundCycles, CyclesUseCase, Instructions, NominalCycles, NominalCyclesTesting,
+        };
 
         let own_subnet_id = subnet_test_id(1);
         let mut state = ReplicatedState::new(own_subnet_id, SubnetType::Application);
@@ -1227,6 +1229,13 @@ mod tests {
             );
         });
 
+        // Non-zero subnet-level consumption, so that the reported total covers both
+        // parts: the subnet-level aggregate and the canisters' part below.
+        let subnet_metrics = &mut state.metadata.subnet_metrics;
+        subnet_metrics.observe_consumed_cycles_by_deleted_canisters(NominalCycles::new(1_000));
+        subnet_metrics
+            .observe_consumed_cycles_with_use_case(CyclesUseCase::VetKd, NominalCycles::new(4));
+
         // Add a non-deleted canister that has consumed some cycles.
         let mut canister_state = new_canister_state(
             canister_test_id(2),
@@ -1249,15 +1258,22 @@ mod tests {
 
         // The tree reads the stored aggregate, which is zero until refreshed.
         assert_eq!(
-            state.metadata.subnet_metrics.consumed_cycles_by_canisters,
+            state
+                .metadata
+                .subnet_metrics
+                .consumed_cycles_total_including_canisters(),
             NominalCycles::zero()
         );
 
-        // The refresh publishes the fold into `SubnetMetrics`.
-        state.refresh_consumed_cycles_by_canisters();
+        let subnet_level = state.metadata.subnet_metrics.consumed_cycles_total();
+        assert!(subnet_level > NominalCycles::zero());
+        state.refresh_consumed_cycles();
         assert_eq!(
-            state.metadata.subnet_metrics.consumed_cycles_by_canisters,
-            consumed_by_canisters
+            state
+                .metadata
+                .subnet_metrics
+                .consumed_cycles_total_including_canisters(),
+            subnet_level + consumed_by_canisters
         );
 
         for certification_version in all_supported_versions() {
@@ -1285,7 +1301,7 @@ mod tests {
 
             // The canister's consumed cycles are included only starting with V29.
             let mut metrics_without_canisters = state.metadata.subnet_metrics.clone();
-            metrics_without_canisters.consumed_cycles_by_canisters = NominalCycles::zero();
+            metrics_without_canisters.refresh_consumed_cycles(NominalCycles::zero());
             let without_canisters =
                 encode_subnet_metrics(&metrics_without_canisters, certification_version);
             if certification_version >= CertificationVersion::V29 {

rs/execution_environment/src/scheduler/tests/metrics.rs

@@ -1,7 +1,8 @@
 //! Tests for scheduler metrics.
 
 use super::super::test_utilities::{
-    SchedulerTestBuilder, TestInstallCode, ingress, instructions, on_response, other_side,
+    SchedulerTest, SchedulerTestBuilder, TestInstallCode, ingress, instructions, on_response,
+    other_side,
 };
 use super::super::*;
 use super::{
@@ -44,6 +45,20 @@ use ic_types_test_utils::ids::{canister_test_id, message_test_id, subnet_test_id
 use more_asserts::assert_ge;
 use std::time::Duration;
 
+/// Observes the state metrics at `height`, having first refreshed the derived
+/// consumed-cycles total that the `replicated_state_consumed_cycles_since_replica_started`
+/// gauge reads. Production refreshes it on every `commit_and_certify`, which this
+/// harness never does.
+fn observe_state_metrics(test: &mut SchedulerTest, height: u64) {
+    test.state_mut().refresh_consumed_cycles();
+    test.state_metrics().observe(
+        test.state().metadata.own_subnet_id,
+        test.state(),
+        height.into(),
+        &no_op_logger(),
+    );
+}
+
 #[test]
 fn validate_consumed_instructions_metric() {
     let mut test = SchedulerTestBuilder::new()
@@ -892,12 +907,7 @@ fn threshold_signature_agreements_metric_is_updated() {
         ])
         .build();
 
-    test.state_metrics().observe(
-        test.state().metadata.own_subnet_id,
-        test.state(),
-        1.into(),
-        &no_op_logger(),
-    );
+    observe_state_metrics(&mut test, 1);
 
     let canister_id = test.create_canister();
 
@@ -1056,12 +1066,7 @@ fn threshold_signature_agreements_metric_is_updated() {
 
     test.execute_round(ExecutionRoundType::OrdinaryRound);
 
-    test.state_metrics().observe(
-        test.state().metadata.own_subnet_id,
-        test.state(),
-        2.into(),
-        &no_op_logger(),
-    );
+    observe_state_metrics(&mut test, 2);
 
     let threshold_signature_agreements_after = &test
         .state()
@@ -1116,12 +1121,7 @@ fn consumed_cycles_ecdsa_outcalls_are_added_to_consumed_cycles_total() {
 
         let canister_id = test.create_canister();
 
-        test.state_metrics().observe(
-            test.state().metadata.own_subnet_id,
-            test.state(),
-            0.into(),
-            &no_op_logger(),
-        );
+        observe_state_metrics(&mut test, 0);
 
         let consumed_cycles_before = NominalCycles::new(
             fetch_gauge(
@@ -1153,12 +1153,7 @@ fn consumed_cycles_ecdsa_outcalls_are_added_to_consumed_cycles_total() {
             .sign_with_ecdsa_contexts();
         assert_eq!(sign_with_ecdsa_contexts.len(), 1);
 
-        test.state_metrics().observe(
-            test.state().metadata.own_subnet_id,
-            test.state(),
-            0.into(),
-            &no_op_logger(),
-        );
+        observe_state_metrics(&mut test, 0);
         let consumed_cycles_after = NominalCycles::new(
             fetch_gauge(
                 test.metrics_registry(),
@@ -1197,12 +1192,7 @@ fn consumed_cycles_http_outcalls_are_added_to_consumed_cycles_total() {
             .subnet_features
             .http_requests = true;
 
-        test.state_metrics().observe(
-            test.state().metadata.own_subnet_id,
-            test.state(),
-            0.into(),
-            &no_op_logger(),
-        );
+        observe_state_metrics(&mut test, 0);
 
         let consumed_cycles_before = NominalCycles::new(
             fetch_gauge(
@@ -1263,12 +1253,7 @@ fn consumed_cycles_http_outcalls_are_added_to_consumed_cycles_total() {
             Some(NumBytes::from(response_size_limit)),
         );
 
-        test.state_metrics().observe(
-            test.state().metadata.own_subnet_id,
-            test.state(),
-            0.into(),
-            &no_op_logger(),
-        );
+        observe_state_metrics(&mut test, 0);
         let consumed_cycles_after = NominalCycles::new(
             fetch_gauge(
                 test.metrics_registry(),
@@ -1434,12 +1419,7 @@ fn consumed_cycles_for_instructions_are_updated_from_valid_canisters() {
             .system_state
             .consume_cycles(removed_cycles);
 
-        test.state_metrics().observe(
-            test.state().metadata.own_subnet_id,
-            test.state(),
-            0.into(),
-            &no_op_logger(),
-        );
+        observe_state_metrics(&mut test, 0);
 
         assert_eq!(
             fetch_gauge_vec(
@@ -1480,12 +1460,7 @@ fn consumed_cycles_for_resource_allocations_are_updated_from_valid_canisters() {
         test.advance_time(duration);
         test.charge_for_resource_allocations();
 
-        test.state_metrics().observe(
-            test.state().metadata.own_subnet_id,
-            test.state(),
-            0.into(),
-            &no_op_logger(),
-        );
+        observe_state_metrics(&mut test, 0);
 
         let expected_memory_cycles = (test.memory_cost(memory_allocation, duration)
             + test.canister_base_cost(memory_allocation, duration))
@@ -1558,12 +1533,7 @@ fn consumed_cycles_are_updated_from_deleted_canisters() {
         );
         test.execute_round(ExecutionRoundType::OrdinaryRound);
 
-        test.state_metrics().observe(
-            test.state().metadata.own_subnet_id,
-            test.state(),
-            0.into(),
-            &no_op_logger(),
-        );
+        observe_state_metrics(&mut test, 0);
 
         assert_eq!(
             fetch_gauge_vec(

rs/replicated_state/src/metadata_state.rs

@@ -440,15 +440,10 @@ pub struct SubnetMetrics {
     /// Transactions here refer to all messages processed in replicated mode.
     pub update_transactions_total: u64,
 
-    /// The total cycles consumed by the canisters that currently exist on this
-    /// subnet, i.e. the sum of `CanisterMetrics::consumed_cycles()` over all of
-    /// them.
-    ///
-    /// Derived, not persisted: refreshed by
-    /// `ReplicatedState::refresh_consumed_cycles_by_canisters` when a state is
-    /// committed, and re-derived by `ReplicatedState::new_from_checkpoint` on load.
+    /// Backing store of [`Self::consumed_cycles_total_including_canisters()`]; zero
+    /// until [`Self::refresh_consumed_cycles`] derives it.
     #[validate_eq(Ignore)]
-    pub consumed_cycles_by_canisters: NominalCycles,
+    consumed_cycles_total_including_canisters: NominalCycles,
 }
 
 impl SubnetMetrics {
@@ -586,14 +581,14 @@ impl SubnetMetrics {
         &self.consumed_cycles_by_use_case_as_counters
     }
 
-    /// Computes the total consumed cycles on the subnet.
+    /// Computes the subnet-level aggregate of the consumed cycles, i.e. the part
+    /// of the total that is not held by the canisters that still exist.
     ///
     /// This is the current computation, which avoids double counting the cycles
-    /// consumed by deleted canisters. The canonical state consumer uses it
-    /// starting with certification version `V29`, adding on top the cycles
-    /// consumed by all non-deleted canisters; for earlier certification
-    /// versions the consumer uses the legacy [`Self::consumed_cycles_total_v28`]
-    /// instead.
+    /// consumed by deleted canisters, as the legacy
+    /// [`Self::consumed_cycles_total_v28`] does. It is one of the two summands of
+    /// [`Self::consumed_cycles_total_including_canisters`], which is what the
+    /// canonical state consumer reports from certification version `V29` on.
     pub fn consumed_cycles_total(&self) -> NominalCycles {
         let mut total = NominalCycles::zero();
 
@@ -647,16 +642,30 @@ 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 [`Self::consumed_cycles_by_canisters`].
+    /// All cycles removed from circulation on this subnet, by both deleted and
+    /// still-existing canisters: [`Self::consumed_cycles_total`] plus the sum of
+    /// `CanisterMetrics::consumed_cycles()` over the canisters that currently
+    /// exist, as of the end of the last committed round.
     ///
-    /// Both the certified state tree at `/subnet/<subnet_id>/metrics` (from
-    /// certification version `V29`) and the
-    /// `replicated_state_consumed_cycles_since_replica_started` gauge report this
-    /// same definition, so the two cannot drift apart.
+    /// Every consumer of the full total reads it here -- the certified state tree at
+    /// `/subnet/<subnet_id>/metrics` (from certification version `V29`) and the
+    /// `replicated_state_consumed_cycles_since_replica_started` gauge -- so they
+    /// cannot drift apart.
     pub fn consumed_cycles_total_including_canisters(&self) -> NominalCycles {
-        self.consumed_cycles_total() + self.consumed_cycles_by_canisters
+        self.consumed_cycles_total_including_canisters
+    }
+
+    /// Recomputes [`Self::consumed_cycles_total_including_canisters`] from the
+    /// subnet-level aggregate and `consumed_by_canisters`, the sum of
+    /// `CanisterMetrics::consumed_cycles()` over the canisters that currently exist.
+    ///
+    /// Callers pass the canisters' part only; adding the subnet-level part happens
+    /// here, so no caller can get it wrong. The total is derived, not
+    /// persisted: `ReplicatedState::refresh_consumed_cycles` calls this whenever a
+    /// state is committed and `ReplicatedState::new_from_checkpoint` on load.
+    pub fn refresh_consumed_cycles(&mut self, consumed_by_canisters: NominalCycles) {
+        self.consumed_cycles_total_including_canisters =
+            self.consumed_cycles_total() + consumed_by_canisters;
     }
 
     /// Legacy computation of the total consumed cycles, used by the canonical
@@ -667,8 +676,9 @@ impl SubnetMetrics {
     /// `consumed_cycles_by_deleted_canisters` and to the
     /// `consumed_cycles_by_use_case` map, and both are summed here. It is kept
     /// unchanged to preserve the certified state for certification versions up
-    /// to and including `V28`; [`Self::consumed_cycles_total`] fixes the double
-    /// counting starting with certification version `V29`.
+    /// to and including `V28`; from `V29` on the consumer reports
+    /// [`Self::consumed_cycles_total_including_canisters`], which does not
+    /// double count.
     pub fn consumed_cycles_total_v28(&self) -> NominalCycles {
         let mut total = NominalCycles::zero();
 

rs/replicated_state/src/metadata_state/proto.rs

@@ -353,7 +353,7 @@ impl TryFrom<pb_metadata::SubnetMetrics> for SubnetMetrics {
             // Transient, with no corresponding proto field:
             // `ReplicatedState::new_from_checkpoint` derives it from the canisters
             // it loads.
-            consumed_cycles_by_canisters: NominalCycles::zero(),
+            consumed_cycles_total_including_canisters: NominalCycles::zero(),
             num_canisters: try_from_option_field(
                 item.num_canisters,
                 "SubnetMetrics::num_canisters",