fix: don't report the dropped ingress induction debit as consumed - #11272
Draft
mraszyk wants to merge 4 commits into
Draft
fix: don't report the dropped ingress induction debit as consumed#11272mraszyk wants to merge 4 commits into
mraszyk wants to merge 4 commits into
Conversation
`apply_ingress_induction_cycles_debit()` may be called with a cycles balance that is smaller than the pending debit: after a cleanup callback, which is allowed to burn the balance below the debit and must always be able to succeed. As documented, the part of the debit that the balance cannot cover is then dropped, making some of the postponed ingress induction charges free. The dropped part was still reported as consumed, though: the full debit was passed to `consume_cycles()`, which saturates the balance subtraction at zero but records the full nominal amount in the consumed cycles metrics. The canister (and, transitively, the subnet) therefore over-reported consumed cycles by the dropped amount. Charge only the part of the debit that the balance can cover, so that the consumed cycles metrics match the cycles actually removed from the balance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`consume_cycles()` subtracts the real amount from the balances with saturating arithmetic, but reported the full nominal amount as consumed. Whenever the balances cannot cover the request, the difference is never removed from any balance, yet it still showed up in the canister's (and, transitively, the subnet's) consumed cycles metrics. Compute the part that the balance cannot cover before draining it and report only the charged remainder, via the new `CompoundCycles::minus_uncharged()`, which reduces both the real and the nominal part. The two parts coincide under the normal cost schedule; under the free cost schedule the real part is zero, so nothing can be left uncharged and this is a no-op. This makes the fix in `apply_ingress_induction_cycles_debit()` from the previous commit hold for every use case, so update its comment accordingly: charging the covered part explicitly now only serves to keep the dropped debit visible at that call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`consume_cycles()` charges only what the balances can cover and silently caps the consumed cycles metrics accordingly. Silently is the problem: for every caller that verified the balance beforehand, a shortfall is a bug that nobody gets to see. Return the uncharged amount and mark `consume_cycles()` `#[must_use]`, so that callers holding a logger and an error counter can report it as a critical error, and the rest have to acknowledge that they are ignoring it. `apply_ingress_induction_cycles_debit()` is such a caller: it now passes the full debit and drives its existing `[EXC-BUG]` report off the returned amount instead of pre-computing the covered part, making the return value the single source of truth for what was dropped. The report stays gated on `strict`, as dropping the debit that the balance cannot cover after a cleanup callback is legitimate and must remain silent. This also removes a false positive: postponed ingress induction charges are recorded unadjusted, so under the free cost schedule the previous nominal comparison could exceed the balance and trip the strict `debug_assert` and the error log even though nothing had to be charged. The returned amount is derived from the real part, which is zero under the free cost schedule. The remaining callers cover the requested amount by construction: the uninstall burn asserts it, the others acknowledge the result where they explain why the balance suffices. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es()` Returning the uncharged amount from `consume_cycles()` left every caller with a `let _uncharged = ...` to acknowledge, and only the one caller that happened to hold a logger and an error counter could actually report a shortfall. Take the logger and the critical error counter in `consume_cycles()` instead and report there: the balances are now required to cover the requested amount, and whatever they cannot cover increments the counter and is logged as `[EXC-BUG]`, next to the cycles use case and the canister that could not pay. The consumed cycles metrics still only account for what was charged. Callers that legitimately charge only what the balances can cover must cap the amount themselves. `apply_ingress_induction_cycles_debit()` is the only such caller: it caps the debit with `CompoundCycles::minus_uncharged()`, keeping its own `[EXC-BUG]` report for the `strict` case and dropping the rest silently after a cleanup callback, as documented. Plumbing the logger and the counter to the remaining callers follows the existing convention of passing a component's critical error counter to the code that may hit it: * `CanisterManager` holds one next to its logger, registered as the `canister_manager_charging_from_balance` critical error; * `ValidSetRuleImpl` registers `mr_charging_from_balance` for the ingress induction charge; * the scheduler and the execution paths pass `execution_environment_charging_from_balance`, exposed on `ExecutionEnvironment` next to the existing `state_changes_error` accessor; * applying the system state modifications passes the state changes critical error, the counter that path already carries; * the cycles account manager forwards both to `SystemState::consume_cycles()` from the charging methods that reach it. 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
1 participant
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.
apply_ingress_induction_cycles_debit()may be called with a cycles balance that is smaller than the pending debit: after a cleanup callback, which is allowed to burn the balance below the debit and must always be able to succeed. As documented, the part of the debit that the balance cannot cover is then dropped, making some of the postponed ingress induction charges free.The dropped part was still reported as consumed, though: the full debit was passed to
consume_cycles(), which saturates the balance subtraction at zero but records the full nominal amount in the consumed cycles metrics. The canister (and, transitively, the subnet) therefore over-reported consumed cycles by the dropped amount.Charge only the part of the debit that the balance can cover, so that the consumed cycles metrics match the cycles actually removed from the balance.
Additionally, harden the implementation as follows:
Compute the part that the balance cannot cover before draining it and report
only the charged remainder, via the new
CompoundCycles::minus_uncharged(),which reduces both the real and the nominal part.
This makes the fix in
apply_ingress_induction_cycles_debit()from theprevious commit hold for every use case, so update its comment accordingly:
charging the covered part explicitly now only serves to keep the dropped debit
visible at that call site.