Merge branch 'ulan/run-1001' into 'master'

fix: RUN-1001 Properly handle updating of reserved cycles limit This fixes a bug in update settings validation for setting the reserved cycles limit below the existing reserved cycles. Now an attempt to set the limit below reserved cycles returns an error explaining that it is not allowed. Closes RUN-1001 See merge request dfinity-lab/public/ic!20022
dfinity · Jun 27, 2024 · 023e03c · massimoalbarello · Jul 5, 2024 · AleDema
2 parents 9d7d9af + 9c7d1cc
commit 023e03c
Show file tree

Hide file tree

Showing 9 changed files with 166 additions and 37 deletions.
diff --git a/rs/canonical_state/tests/hash_tree.rs b/rs/canonical_state/tests/hash_tree.rs
@@ -197,8 +197,8 @@ fn error_code_change_guard() {
     //    the name.
     assert_eq!(
         [
-            168, 93, 103, 26, 25, 115, 47, 212, 112, 195, 250, 97, 212, 98, 80, 98, 182, 207, 148,
-            106, 249, 63, 178, 91, 255, 255, 186, 84, 195, 222, 206, 80
+            49, 160, 44, 253, 61, 181, 131, 134, 148, 115, 119, 247, 83, 108, 123, 1, 163, 212,
+            137, 11, 99, 122, 199, 16, 89, 105, 87, 234, 200, 211, 76, 96
         ],
         hasher.finish()
     );

diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs
@@ -2249,6 +2249,13 @@ pub(crate) enum CanisterManagerError {
         requested: Cycles,
         limit: Cycles,
     },
+    // TODO(RUN-1001): Use this error type after successful rollout of the next
+    // replica version.
+    #[allow(dead_code)]
+    ReservedCyclesLimitIsTooLow {
+        cycles: Cycles,
+        limit: Cycles,
+    },
     WasmChunkStoreError {
         message: String,
     },
@@ -2306,6 +2313,7 @@ impl AsErrorHelp for CanisterManagerError {
             | CanisterManagerError::InsufficientCyclesInMemoryGrow { .. }
             | CanisterManagerError::ReservedCyclesLimitExceededInMemoryAllocation { .. }
             | CanisterManagerError::ReservedCyclesLimitExceededInMemoryGrow { .. }
+            | CanisterManagerError::ReservedCyclesLimitIsTooLow { .. }
             | CanisterManagerError::WasmChunkStoreError { .. }
             | CanisterManagerError::CanisterSnapshotNotFound { .. }
             | CanisterManagerError::CanisterHeapDeltaRateLimited { .. }
@@ -2553,6 +2561,16 @@ impl From<CanisterManagerError> for UserError {
                     ),
                 )
             }
+            ReservedCyclesLimitIsTooLow { cycles, limit } => {
+                Self::new(
+                    ErrorCode::ReservedCyclesLimitIsTooLow,
+                    format!(
+                        "Cannot set the reserved cycles limit {} below the reserved cycles balance of \
+                        the canister {}.{additional_help}",
+                        limit, cycles,
+                    ),
+                )
+            }
             WasmChunkStoreError { message } => {
                 Self::new(
                     ErrorCode::CanisterContractViolation,

diff --git a/rs/execution_environment/src/canister_settings.rs b/rs/execution_environment/src/canister_settings.rs
@@ -519,43 +519,47 @@ pub(crate) fn validate_canister_settings(
         }
     }
 
-    let reservation_cycles = if new_memory_bytes <= old_memory_bytes {
-        Cycles::zero()
+    let allocated_bytes = if new_memory_bytes > old_memory_bytes {
+        new_memory_bytes - old_memory_bytes
     } else {
-        let allocated_bytes = new_memory_bytes - old_memory_bytes;
-        let reservation_cycles = cycles_account_manager.storage_reservation_cycles(
-            allocated_bytes,
-            subnet_memory_saturation,
-            subnet_size,
-        );
-        let reserved_balance_limit = settings
-            .reserved_cycles_limit()
-            .or(canister_reserved_balance_limit);
-        if let Some(limit) = reserved_balance_limit {
-            if canister_reserved_balance + reservation_cycles > limit {
-                return Err(
-                    CanisterManagerError::ReservedCyclesLimitExceededInMemoryAllocation {
-                        memory_allocation: new_memory_allocation,
-                        requested: canister_reserved_balance + reservation_cycles,
-                        limit,
-                    },
-                );
-            }
-        }
-        // Note that this check does not include the freezing threshold to be
-        // consistent with the `reserve_cycles()` function, which moves
-        // cycles between the main and reserved balances without checking
-        // the freezing threshold.
-        if canister_cycles_balance < reservation_cycles {
-            return Err(CanisterManagerError::InsufficientCyclesInMemoryAllocation {
-                memory_allocation: new_memory_allocation,
-                available: canister_cycles_balance,
-                threshold: reservation_cycles,
-            });
-        }
-        reservation_cycles
+        NumBytes::new(0)
     };
 
+    let reservation_cycles = cycles_account_manager.storage_reservation_cycles(
+        allocated_bytes,
+        subnet_memory_saturation,
+        subnet_size,
+    );
+    let reserved_balance_limit = settings
+        .reserved_cycles_limit()
+        .or(canister_reserved_balance_limit);
+
+    if let Some(limit) = reserved_balance_limit {
+        // TODO(RUN-1001): return `ReservedCyclesLimitIsTooLow` once
+        // the replica with that error type rolls out successfully.
+        if canister_reserved_balance + reservation_cycles > limit {
+            return Err(
+                CanisterManagerError::ReservedCyclesLimitExceededInMemoryAllocation {
+                    memory_allocation: new_memory_allocation,
+                    requested: canister_reserved_balance + reservation_cycles,
+                    limit,
+                },
+            );
+        }
+    }
+
+    // Note that this check does not include the freezing threshold to be
+    // consistent with the `reserve_cycles()` function, which moves
+    // cycles between the main and reserved balances without checking
+    // the freezing threshold.
+    if canister_cycles_balance < reservation_cycles {
+        return Err(CanisterManagerError::InsufficientCyclesInMemoryAllocation {
+            memory_allocation: new_memory_allocation,
+            available: canister_cycles_balance,
+            threshold: reservation_cycles,
+        });
+    }
+
     Ok(ValidatedCanisterSettings {
         controller: settings.controller(),
         controllers: settings.controllers(),

diff --git a/rs/execution_environment/src/history.rs b/rs/execution_environment/src/history.rs
@@ -330,6 +330,9 @@ fn dashboard_label_value_from(code: ErrorCode) -> &'static str {
         ReservedCyclesLimitExceededInMemoryGrow => {
             "Canister cannot grow memory due to its reserved cycles limit"
         }
+        ReservedCyclesLimitIsTooLow => {
+            "Canister cannot set the reserved cycles limit below the reserved cycles balance"
+        }
         InsufficientCyclesInMessageMemoryGrow => {
             "Canister does not have enough cycles to grow message memory"
         }

diff --git a/rs/execution_environment/src/hypervisor/tests.rs b/rs/execution_environment/src/hypervisor/tests.rs
@@ -6670,6 +6670,92 @@ fn wasm_memory_grow_reserves_cycles() {
     assert!(balance_before - balance_after > reserved_cycles);
 }
 
+#[test]
+fn set_reserved_cycles_limit_below_existing_fails() {
+    const CYCLES: Cycles = Cycles::new(20_000_000_000_000);
+    const CAPACITY: u64 = 1_000_000_000;
+    const THRESHOLD: u64 = 500_000_000;
+
+    let mut test = ExecutionTestBuilder::new()
+        .with_subnet_execution_memory(CAPACITY as i64)
+        .with_subnet_memory_threshold(THRESHOLD as i64)
+        .with_subnet_memory_reservation(0)
+        .build();
+
+    let wat = r#"
+        (module
+            (import "ic0" "msg_reply" (func $msg_reply))
+            (import "ic0" "msg_reply_data_append"
+                (func $msg_reply_data_append (param i32 i32)))
+            (func $update
+                ;; 7500 Wasm pages is close to 500MB.
+                (if (i32.eq (memory.grow (i32.const 7500)) (i32.const -1))
+                  (then (unreachable))
+                )
+                (call $msg_reply)
+            )
+            (memory $memory 1)
+            (export "canister_update update" (func $update))
+        )"#;
+
+    let wasm = wat::parse_str(wat).unwrap();
+
+    let canister_id = test.canister_from_cycles_and_binary(CYCLES, wasm).unwrap();
+
+    test.update_freezing_threshold(canister_id, NumSeconds::new(0))
+        .unwrap();
+    test.canister_update_reserved_cycles_limit(canister_id, CYCLES)
+        .unwrap();
+
+    let balance_before = test.canister_state(canister_id).system_state.balance();
+    let result = test.ingress(canister_id, "update", vec![]).unwrap();
+    assert_eq!(result, WasmResult::Reply(vec![]));
+    let balance_after = test.canister_state(canister_id).system_state.balance();
+
+    assert_eq!(
+        test.canister_state(canister_id)
+            .system_state
+            .reserved_balance(),
+        Cycles::zero()
+    );
+    // Message execution fee is an order of a few million cycles.
+    assert!(balance_before - balance_after < Cycles::new(1_000_000_000));
+
+    let subnet_memory_usage =
+        CAPACITY - test.subnet_available_memory().get_execution_memory() as u64;
+    let memory_usage_before = test.canister_state(canister_id).execution_memory_usage();
+    let balance_before = test.canister_state(canister_id).system_state.balance();
+    let result = test.ingress(canister_id, "update", vec![]).unwrap();
+    assert_eq!(result, WasmResult::Reply(vec![]));
+    let balance_after = test.canister_state(canister_id).system_state.balance();
+    let memory_usage_after = test.canister_state(canister_id).execution_memory_usage();
+
+    let reserved_cycles = test
+        .canister_state(canister_id)
+        .system_state
+        .reserved_balance();
+
+    assert_eq!(
+        reserved_cycles,
+        test.cycles_account_manager().storage_reservation_cycles(
+            memory_usage_after - memory_usage_before,
+            &ResourceSaturation::new(subnet_memory_usage, THRESHOLD, CAPACITY),
+            test.subnet_size(),
+        )
+    );
+
+    assert!(balance_before - balance_after > reserved_cycles);
+
+    let err = test
+        .canister_update_reserved_cycles_limit(canister_id, Cycles::from(reserved_cycles.get() - 1))
+        .unwrap_err();
+
+    assert_eq!(
+        err.code(),
+        ErrorCode::ReservedCyclesLimitExceededInMemoryAllocation
+    );
+}
+
 #[test]
 fn upgrade_with_skip_pre_upgrade_preserves_stable_memory() {
     let mut test: ExecutionTest = ExecutionTestBuilder::new().build();

diff --git a/rs/protobuf/def/state/ingress/v1/ingress.proto b/rs/protobuf/def/state/ingress/v1/ingress.proto
@@ -93,6 +93,7 @@ enum ErrorCode {
   ERROR_CODE_CANISTER_WASM_MODULE_NOT_FOUND = 537;
   ERROR_CODE_CANISTER_ALREADY_INSTALLED = 538;
   ERROR_CODE_CANISTER_WASM_MEMORY_LIMIT_EXCEEDED = 539;
+  ERROR_CODE_RESERVED_CYCLES_LIMIT_IS_TOO_LOW = 540;
 }
 
 message IngressStatusFailed {

diff --git a/rs/protobuf/src/gen/state/state.ingress.v1.rs b/rs/protobuf/src/gen/state/state.ingress.v1.rs
@@ -203,6 +203,7 @@ pub enum ErrorCode {
     CanisterWasmModuleNotFound = 537,
     CanisterAlreadyInstalled = 538,
     CanisterWasmMemoryLimitExceeded = 539,
+    ReservedCyclesLimitIsTooLow = 540,
 }
 impl ErrorCode {
     /// String value of the enum field names used in the ProtoBuf definition.
@@ -294,6 +295,7 @@ impl ErrorCode {
             ErrorCode::CanisterWasmMemoryLimitExceeded => {
                 "ERROR_CODE_CANISTER_WASM_MEMORY_LIMIT_EXCEEDED"
             }
+            ErrorCode::ReservedCyclesLimitIsTooLow => "ERROR_CODE_RESERVED_CYCLES_LIMIT_IS_TOO_LOW",
         }
     }
     /// Creates an enum from field names used in the ProtoBuf definition.
@@ -382,6 +384,9 @@ impl ErrorCode {
             "ERROR_CODE_CANISTER_WASM_MEMORY_LIMIT_EXCEEDED" => {
                 Some(Self::CanisterWasmMemoryLimitExceeded)
             }
+            "ERROR_CODE_RESERVED_CYCLES_LIMIT_IS_TOO_LOW" => {
+                Some(Self::ReservedCyclesLimitIsTooLow)
+            }
             _ => None,
         }
     }

diff --git a/rs/protobuf/src/gen/types/state.ingress.v1.rs b/rs/protobuf/src/gen/types/state.ingress.v1.rs
@@ -203,6 +203,7 @@ pub enum ErrorCode {
     CanisterWasmModuleNotFound = 537,
     CanisterAlreadyInstalled = 538,
     CanisterWasmMemoryLimitExceeded = 539,
+    ReservedCyclesLimitIsTooLow = 540,
 }
 impl ErrorCode {
     /// String value of the enum field names used in the ProtoBuf definition.
@@ -294,6 +295,7 @@ impl ErrorCode {
             ErrorCode::CanisterWasmMemoryLimitExceeded => {
                 "ERROR_CODE_CANISTER_WASM_MEMORY_LIMIT_EXCEEDED"
             }
+            ErrorCode::ReservedCyclesLimitIsTooLow => "ERROR_CODE_RESERVED_CYCLES_LIMIT_IS_TOO_LOW",
         }
     }
     /// Creates an enum from field names used in the ProtoBuf definition.
@@ -382,6 +384,9 @@ impl ErrorCode {
             "ERROR_CODE_CANISTER_WASM_MEMORY_LIMIT_EXCEEDED" => {
                 Some(Self::CanisterWasmMemoryLimitExceeded)
             }
+            "ERROR_CODE_RESERVED_CYCLES_LIMIT_IS_TOO_LOW" => {
+                Some(Self::ReservedCyclesLimitIsTooLow)
+            }
             _ => None,
         }
     }

diff --git a/rs/types/error_types/src/lib.rs b/rs/types/error_types/src/lib.rs
@@ -150,6 +150,7 @@ impl From<ErrorCode> for RejectCode {
             InsufficientCyclesInMemoryGrow => CanisterError,
             ReservedCyclesLimitExceededInMemoryAllocation => CanisterError,
             ReservedCyclesLimitExceededInMemoryGrow => CanisterError,
+            ReservedCyclesLimitIsTooLow => CanisterError,
             InsufficientCyclesInMessageMemoryGrow => CanisterError,
             CanisterMethodNotFound => CanisterError,
             CanisterWasmModuleNotFound => CanisterError,
@@ -229,6 +230,7 @@ pub enum ErrorCode {
     CanisterWasmModuleNotFound = 537,
     CanisterAlreadyInstalled = 538,
     CanisterWasmMemoryLimitExceeded = 539,
+    ReservedCyclesLimitIsTooLow = 540,
 }
 
 impl TryFrom<ErrorCodeProto> for ErrorCode {
@@ -325,6 +327,9 @@ impl TryFrom<ErrorCodeProto> for ErrorCode {
             ErrorCodeProto::CanisterWasmMemoryLimitExceeded => {
                 Ok(ErrorCode::CanisterWasmMemoryLimitExceeded)
             }
+            ErrorCodeProto::ReservedCyclesLimitIsTooLow => {
+                Ok(ErrorCode::ReservedCyclesLimitIsTooLow)
+            }
         }
     }
 }
@@ -412,6 +417,7 @@ impl From<ErrorCode> for ErrorCodeProto {
             ErrorCode::CanisterWasmMemoryLimitExceeded => {
                 ErrorCodeProto::CanisterWasmMemoryLimitExceeded
             }
+            ErrorCode::ReservedCyclesLimitIsTooLow => ErrorCodeProto::ReservedCyclesLimitIsTooLow,
         }
     }
 }
@@ -528,6 +534,7 @@ impl UserError {
             | ErrorCode::InsufficientCyclesInMemoryGrow
             | ErrorCode::ReservedCyclesLimitExceededInMemoryAllocation
             | ErrorCode::ReservedCyclesLimitExceededInMemoryGrow
+            | ErrorCode::ReservedCyclesLimitIsTooLow
             | ErrorCode::InsufficientCyclesInMessageMemoryGrow
             | ErrorCode::CanisterSnapshotNotFound
             | ErrorCode::CanisterHeapDeltaRateLimited
@@ -589,7 +596,7 @@ mod tests {
                 402, 403, 404, 405, 406, 407, 408,
                 502, 503, 504, 505, 506, 507, 508, 509, 510, 511, 512, 513, 514,
                 517, 520, 521, 522, 524, 525, 526, 527, 528, 529, 530, 531, 532,
-                533, 534, 535, 536, 537, 538, 539
+                533, 534, 535, 536, 537, 538, 539, 540,
             ]
         );
     }