refactor: compare a CompoundCycles on a part it names - #11515
Closed
mraszyk wants to merge 1 commit into
Closed
Conversation
`CompoundCycles` derived `Ord` and `PartialOrd` over its fields, so a comparison of two amounts was made on their real parts first and only fell back to the nominal ones for amounts whose real parts are equal. Which part a comparison is made on matters: the real part of an amount whose use case is free under the free cost schedule is zero, so a comparison of real parts cannot tell two such amounts apart, whereas the nominal part is independent of the cost schedule. The two parts coincide under the normal cost schedule, so the distinction is invisible there. Nothing derived that ordering on purpose, and the four call sites that used it all clamp a cost to a prepayment, for which the nominal part is the right one: they were correct only because the real and the nominal parts coincide under the normal cost schedule and the real parts are both zero under the free one, i.e. because the fallback happened to kick in. Reordering the two fields of the struct would have changed what they mean. Drop the two derives and add `CompoundCycles::min_nominal`, which says which part it compares, and use it at those four call sites. This is behavior-preserving: for two amounts derived under the same cost schedule, and for the use cases those call sites deal with, it picks the same amount as the derived ordering did. The tests that compared two execution costs with `assert_gt!` now compare their nominal parts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The explicit comparisons preserve existing behavior while removing ambiguous implicit ordering.
Pull request overview
Makes CompoundCycles comparisons explicit to prevent accidental ordering by the real component.
Changes:
- Removes derived ordering and adds
min_nominal. - Updates four production clamps and affected test assertions.
File summaries
| File | Description |
|---|---|
rs/types/cycles/src/compound_cycles.rs |
Adds explicit nominal minimum comparison. |
rs/cycles_account_manager/src/cycles_account_manager.rs |
Uses nominal comparisons for refunds and settlement. |
rs/execution_environment/src/canister_manager.rs |
Uses nominal comparison when recording charges. |
rs/execution_environment/src/execution/response/tests.rs |
Compares nominal execution costs explicitly. |
rs/execution_environment/tests/hypervisor.rs |
Compares nominal execution costs explicitly. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
CompoundCyclesderivedOrdandPartialOrdover its fields, so a comparison of two amounts was made on their real parts first and only fell back to the nominal ones for amounts whose real parts are equal.Which part a comparison is made on matters. The real part of an amount whose use case is free under the free cost schedule is zero (cf.
CompoundCycles::new), so a comparison of real parts cannot tell two such amounts apart; the nominal part is independent of the cost schedule. Under the normal cost schedule the two parts coincide, so the distinction is invisible there.Nothing derived that ordering on purpose, and all four call sites that used it clamp a cost to a prepayment, for which the nominal part is the right one:
CyclesAccountManager::refund_unused_execution_cyclesCyclesAccountManager::settle_prepayment_for_unexecuted_responseCyclesAccountManager::refund_for_response_transmissionCanisterManager, which recomputes the same clamp to record the net cycle chargeThey were correct only because the real and the nominal parts coincide under the normal cost schedule and the real parts are both zero under the free one, i.e. because the fallback to the nominal part happened to kick in. Reordering the two fields of the struct would have changed what they mean.
This PR drops the two derives and adds
CompoundCycles::min_nominal, which says which part it compares, and uses it at those four call sites. It is behavior-preserving: for two amounts derived under the same cost schedule, and for the use cases those call sites deal with,min_nominalpicks the same amount as the derived ordering did. The tests that compared two execution costs withassert_gt!now compare their nominal parts.Why now
Cleanup split out of #11431, which fixes a bug of exactly this shape: the cycles prepaid for a response execution were settled against the requirement by comparing their real parts, which made the adjustment a no-op under the free cost schedule and misreported the consumed cycles metrics. Removing the derived ordering means no call site can compare on a part it did not name.
🤖 Generated with Claude Code