feat: charge cycles for log memory resize in update_settings#9118
Closed
feat: charge cycles for log memory resize in update_settings#9118
Conversation
maksymar
changed the title
…sym/logging-costs
Pass canister_log_memory_usage as a parameter so the resize cycle cost check compiles. Use a placeholder cost_per_byte=1 until benchmarks determine the real value. Also fix a missed fetch_canister_logs rename in the test helper.
Add two tests that verify cycle charging for log memory store resize: - test_canister_log_resize_deducts_cycles: fills a 256 KiB log buffer and asserts the cycle balance decreases proportionally after resize - test_canister_log_resize_rejected_insufficient_cycles: verifies that resize is rejected when the canister cannot afford the cost Both tests currently fail as expected — cycle deduction is not yet implemented. They will pass once the proper charging flow is added.
- Use div_ceil() instead of manual ceiling division in benchmark - Remove unused debug variables and dead update_settings_duration_sum fn
The removal caused new canisters to not get a default log memory store allocated when LOG_MEMORY_STORE_FEATURE_ENABLED=true, breaking tests that depend on the default log memory usage (empty_canister_memory_usage, backtraces, etc). Removing this default is a separate concern from resize charging and should not be part of this PR.
Add proper cycle charging when update_settings resizes the log memory store. The implementation follows a two-step pattern: 1. validate_canister_settings: pre-check that the canister can afford the resize cost (bytes_used * 32 instr/byte) without freezing, using management_canister_cost for proper instruction-to-cycles conversion. 2. update_settings: after do_update_settings applies the resize, deduct cycles via consume_cycles_for_management_canister_instructions and account instructions in round_limits. Charging only triggers when the user explicitly sets log_memory_limit, not when the default fallback is applied during canister creation. Add LogResizeNotEnoughCycles error variant with proper user-facing error message. Fix the insufficient-cycles rejection test to use a high freezing threshold to lock up the canister's balance.
- Remove all commented-out debug println! statements and variables - Revert unnecessary canister_logs.rs rename (old_logs/new_logs -> s) - Revert semicolon change in log_memory_store resize method - Replace LogResizeNotEnoughCycles(CanisterOutOfCyclesError) with a struct variant that doesn't require a canister_id, since validate_canister_settings doesn't have access to it
Group the default fallback, max-limit check, and resize cost check into a single coherent block with a clear top-level comment explaining the two code paths (user-set vs default). Extract user_set_log_memory_limit flag to avoid calling settings.log_memory_limit() multiple times.
maksymar
changed the title
Sync benchmark with maksym/log-resize-bench: use 0-byte messages for worst case, compute repeat count from buffer size, fill in single call.
Sync with maksym/log-store-cleanup: const fn for record size estimation, narrow visibility to pub(super), improve doc comments.
Sync canister_manager.rs, types.rs and canister_logging.rs with maksym/log-resize-charging: remove LogResizeNotEnoughCycles in favor of InsufficientCyclesInMemoryAllocation, restore debug_assert, simplify tests to use 100-byte messages and single-call fill.
Use records_to_fill() helper instead of hardcoded repeat counts.
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.
closed in favour of #9883