refactor: Hold the whole consumed-cycles total in SubnetMetrics - #11335
Merged
Conversation
`SubnetMetrics::consumed_cycles_by_canisters` holds only the canisters' part of the total; every consumer adds the subnet-level aggregate on top, recomputing that half 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, and the method that used to sum the two is gone: the certified state tree at `/subnet/<subnet_id>/metrics` (from certification version `V29`) and the `replicated_state_consumed_cycles_since_replica_started` gauge read the field directly, 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, computes it on every `commit_and_certify`, and `new_from_checkpoint` re-derives it, so a replica restarting from a checkpoint agrees with one that keeps running. 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 setting the stored aggregate rather than the canisters' part. 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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ReplicatedState::new_from_checkpoint` re-derives `consumed_cycles_total_including_canisters` from the persisted subnet-level total and the loaded canisters, so that a replica restarting from a checkpoint agrees with one that keeps running. Nothing exercised that: the traversal test only refreshes a live state, the encoding and hash fixtures assign the aggregate directly, and `validate_eq` ignores the transient field. Pass non-zero subnet-level consumption -- from deleted canisters, from the scalar outcall metrics and from a subnet-only use case -- together with a canister that has consumed cycles through both paths, and assert the two `SubnetMetrics` agree field by field, which is what the certified metrics leaf encodes. Dropping the subnet-level half from `new_from_checkpoint` fails the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 0878752. Security Overview
Detected Code Changes
|
eichhorl
reviewed
Aug 27, 2026
eichhorl
approved these changes
Aug 27, 2026
`SubnetMetrics::consumed_cycles_total_including_canisters` was a public field, so every writer had to remember to add the subnet-level aggregate to the canisters' part itself -- production in `ReplicatedState::refresh_consumed_cycles` and in `new_from_checkpoint`, and each test fixture on its own. Make the field private behind two methods: a getter of the same name, and `refresh_consumed_cycles(consumed_by_canisters)`, which adds `consumed_cycles_total()` on top. Callers now hand over the canisters' part only, so the sum lives in one place and no caller can get it wrong. Also, per review: - `consumed_cycles_total`'s doc claimed the canonical state consumer adds the non-deleted canisters on top of it, which stopped being true when the total moved into `SubnetMetrics`; it now describes the subnet-level aggregate as one of the two summands of the stored total. Same for the stale mention of the canisters' part being "passed to `encode_subnet_metrics`" in `canonical_encoding_subnet_metrics`. - `test_traverse_subnet_metrics_includes_canister_consumed_cycles_at_v29` had no subnet-level consumption, so the encoded total it pinned would not have noticed that part going missing. It now consumes some, and asserts it is non-zero. - The scheduler metrics tests repeated the same refresh-then-observe block nine times; it is now the `observe_state_metrics` helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fixtures that set the consumed-cycles total each explained what the value was; `refresh_consumed_cycles(consumed_by_canisters)` now says it, so the comments only repeat the call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`consumed_cycles_total_v28` still named `consumed_cycles_total` as what fixes the double counting from `V29` on; that is the stored `consumed_cycles_total_including_canisters`, of which the former is only the subnet-level summand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The remaining mentions of the total still read as if it were a field: the canonical encoding and `ReplicatedState::refresh_consumed_cycles` now name the getter, and the private field's own doc says what it holds before the first refresh (a decoded `SubnetMetrics` starts at zero) rather than claiming the refresh is its only writer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mraszyk
enabled auto-merge
August 27, 2026 11:24
mraszyk
added a commit
that referenced
this pull request
Aug 27, 2026
Master landed #11335, the upstream version of this branch's consumed-cycles refactor, which makes `SubnetMetrics::consumed_cycles_total_including_canisters` a private field behind a getter written only by `SubnetMetrics::refresh_consumed_cycles`. Every conflict is between that and this branch's own variant of the same change, so master's version wins throughout; the `subnet_metrics` endpoint and its tests now read the total through the getter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SubnetMetrics::consumed_cycles_by_canistersholds only the canisters' part ofthe 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_canisteradds the deleted canister's consumption toconsumed_cycles_by_deleted_canistersat once, so between that and the nextcommit the two parts count the same canister twice.
Hold the whole total instead.
consumed_cycles_by_canistersbecomesconsumed_cycles_total_including_canisters, still transient, now covering thesubnet-level aggregate as well. The field is private, with a getter of the same
name and a single writer,
SubnetMetrics::refresh_consumed_cycles, which takesthe canisters' part and adds
consumed_cycles_total()on top: callers hand overthat 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 certificationversion
V29) and thereplicated_state_consumed_cycles_since_replica_startedgauge 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 fromrefresh_consumed_cycles_by_canistersnow that it publishes more than thecanisters' part, hands the canister states' sum over on every
commit_and_certify, andnew_from_checkpointdoes the same on load, so areplica restarting from a checkpoint agrees with one that keeps running --
which
consumed_cycles_total_is_the_same_across_a_restartpins, comparing alive 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.rsand theV29hash instate_manager/src/tree_hash.rsstill hold, with their fixtures now handingtheir canisters' part to
refresh_consumed_cyclesrather than writing theaggregate themselves.
test_traverse_subnet_metrics_includes_canister_consumed_cycles_at_v29now 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_certifyenqueues theobservation after it; the scheduler metrics tests, which never commit a state,
refresh explicitly through the new
observe_state_metricshelper.