Skip to content

[DO NOT MERGE] feat: Update consumed cycles metrics on free cost schedule#9758

Open
dsarlis wants to merge 3 commits intodfinity:masterfrom
dsarlis:dimitris/nominal-cycles-always-update
Open

[DO NOT MERGE] feat: Update consumed cycles metrics on free cost schedule#9758
dsarlis wants to merge 3 commits intodfinity:masterfrom
dsarlis:dimitris/nominal-cycles-always-update

Conversation

@dsarlis
Copy link
Copy Markdown
Contributor

@dsarlis dsarlis commented Apr 8, 2026

Separate the values of NominalCycles that are used to count consumed cycles from the Cycles amounts that are charged against the canister's balance. Specifically, NominalCycles are updated even on a free schedule with the amounts that correspond to the respective resource usage.

The main change in production code is in the constructor of CompoundCycles while the rest are updating tests as needed, including fixing expected values, testing both on normal and free schedule as well as adding some new tests for cases that were not covered. Additionally, a const is defined for the canister creation fee to be consistently used in production and test code where necessary.

This should be deployed after #9676 to ensure that no wrong values are used before this PR separates the two types of cycles.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates cycles accounting so that consumed-cycles metrics (nominal) are updated based on resource usage even when the subnet runs on a free cost schedule, while charged cycles (real) remain zero where appropriate. It also adapts and expands tests to validate behavior on both normal and free schedules.

Changes:

  • Update CompoundCycles::new to always preserve nominal cycles while applying cost-schedule rules only to the real (charged) portion.
  • Adjust scheduler/execution/messaging tests to use .real() for balance-affecting paths and .nominal() for consumed-metrics assertions, including running key tests under both Normal and Free schedules.
  • Centralize canister creation fee as a constant and update tests to assert nominal consumption on free schedules.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
rs/types/cycles/src/compound_cycles.rs Separates nominal vs real cycles in CompoundCycles::new, ensuring nominal updates even on free schedule.
rs/messaging/src/routing/stream_handler/tests.rs Runs relevant stream handler tests under both cost schedules; updates lost-cycles expectations.
rs/execution_environment/src/scheduler/tests/metrics.rs Updates consumed-cycles metric tests to assert nominal amounts; adds coverage for free schedule.
rs/execution_environment/src/scheduler/tests/ecdsa.rs Adjusts ECDSA fee usage to pass real cycles where a payment is required.
rs/execution_environment/src/scheduler/tests/charging.rs Updates resource charging assertions to use real cycles from CompoundCycles.
rs/execution_environment/src/scheduler/tests.rs Updates ECDSA injection to use real fee.
rs/execution_environment/src/scheduler/test_utilities.rs Changes fee/cost helpers to return CompoundCycles and adds helpers for time advancement and compute allocation cost.
rs/execution_environment/src/execution/response/tests.rs Extends refund behavior test to both schedules and validates nominal consumed-cycles updates.
rs/execution_environment/src/execution_environment/tests.rs Extends ingress charging test to validate consumed-cycles metrics alongside balance effects across schedules.
rs/execution_environment/src/canister_manager/tests.rs Updates free-schedule canister creation tests to expect nominal consumption equal to creation fee.
rs/config/src/subnet_config.rs Introduces CANISTER_CREATION_FEE constant and reuses it in cycles account manager config.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

maksymar
maksymar previously approved these changes Apr 8, 2026
Copy link
Copy Markdown
Contributor

@maksymar maksymar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @dsarlis ! please address minor comments from copilot, otherwise LGTM.

@cgundy cgundy added the security-review-passed IDX or InfraSec have concluded it's safe to run CI on the external PR. label Apr 8, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@github-actions github-actions bot dismissed maksymar’s stale review April 8, 2026 08:13

Review dismissed by automation script.

@dsarlis dsarlis changed the title feat: Update consumed cycles metrics on free cost schedule [DO NOT MERGE] feat: Update consumed cycles metrics on free cost schedule Apr 8, 2026
@dsarlis
Copy link
Copy Markdown
Contributor Author

dsarlis commented Apr 8, 2026

@cgundy or @maksymar please kick off CI again to ensure a green run so that the PR is ready to be merged when the time is right (I marked the title on purpose to avoid accidental merge before the change we need is scheduled for rollout).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

@cgundy
Copy link
Copy Markdown
Contributor

cgundy commented Apr 8, 2026

Looks like there are still some linting errors:

cargo fmt -- --check
info: syncing channel updates for 1.93.1-x86_64-unknown-linux-gnu
info: latest update on 2026-02-12 for version 1.93.1 (01f6ddf75 2026-02-11)
info: downloading 7 components
Diff in /__w/ic/ic/rs/execution_environment/src/scheduler/tests/metrics.rs:1219:
             Some(CanisterStatusType::Stopped),
         );
 
-        let removed_cycles = CompoundCycles::<Instructions>::new(
-            Cycles::from(1000_u128),
-            cost_schedule,
-        );
+        let removed_cycles =
+            CompoundCycles::<Instructions>::new(Cycles::from(1000_u128), cost_schedule);
         test.canister_state_mut(canister_id)
             .system_state
             .consume_cycles(removed_cycles);

@dsarlis
Copy link
Copy Markdown
Contributor Author

dsarlis commented Apr 8, 2026

Looks like there are still some linting errors:

cargo fmt -- --check
info: syncing channel updates for 1.93.1-x86_64-unknown-linux-gnu
info: latest update on 2026-02-12 for version 1.93.1 (01f6ddf75 2026-02-11)
info: downloading 7 components
Diff in /__w/ic/ic/rs/execution_environment/src/scheduler/tests/metrics.rs:1219:
             Some(CanisterStatusType::Stopped),
         );
 
-        let removed_cycles = CompoundCycles::<Instructions>::new(
-            Cycles::from(1000_u128),
-            cost_schedule,
-        );
+        let removed_cycles =
+            CompoundCycles::<Instructions>::new(Cycles::from(1000_u128), cost_schedule);
         test.canister_state_mut(canister_id)
             .system_state
             .consume_cycles(removed_cycles);

Yeah that's because I committed the suggestion and it happened to create a lint issue this time. I fixed it along with an update to the test that was needed. Should be fine now.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contributor feat @ic-interface-owners security-review-passed IDX or InfraSec have concluded it's safe to run CI on the external PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants