Merge branch 'ulan/rc--2024-06-26_23-01-reserved-limit-fix' into 'rc-…

…-2024-06-26_23-01' fix: RUN-1001 Properly handle updating of reserved cycles limit This MR backports commit `9c7d1cc` from the master branch. See merge request dfinity-lab/public/ic!20102
dfinity · Jun 27, 2024 · 2e269c7 · 2e269c7
2 parents 19a438c + bf2f0a5
commit 2e269c7
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,
             ]
         );
     }