refactor: Hold the whole consumed-cycles total in SubnetMetrics

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",