Merge branch 'master' into eero/upgrade-setupos

rs/nns/governance/canister/canister.rs

@@ -860,7 +860,7 @@ fn canister_heartbeat() {
 fn manage_neuron_pb() {
     debug_log("manage_neuron_pb");
     panic_with_probability(
-        0.3,
+        0.7,
         "manage_neuron_pb is deprecated. Please use manage_neuron instead.",
     );
     over_async(protobuf, manage_neuron_)
@@ -882,7 +882,7 @@ fn list_proposals_pb() {
 fn list_neurons_pb() {
     debug_log("list_neurons_pb");
     panic_with_probability(
-        0.3,
+        0.7,
         "list_neurons_pb is deprecated. Please use list_neurons instead.",
     );
     over(protobuf, list_neurons_)

rs/nns/governance/src/governance.rs

@@ -397,14 +397,20 @@ impl NnsFunction {
     fn allowed_when_resources_are_low(&self) -> bool {
         matches!(
             self,
-            NnsFunction::ReviseElectedGuestosVersions | NnsFunction::DeployGuestosToAllSubnetNodes
+            NnsFunction::NnsRootUpgrade
+                | NnsFunction::NnsCanisterUpgrade
+                | NnsFunction::HardResetNnsRootToVersion
+                | NnsFunction::ReviseElectedGuestosVersions
+                | NnsFunction::DeployGuestosToAllSubnetNodes
         )
     }
 
     fn can_have_large_payload(&self) -> bool {
         matches!(
             self,
-            NnsFunction::NnsCanisterInstall
+            NnsFunction::NnsCanisterUpgrade
+                | NnsFunction::NnsCanisterInstall
+                | NnsFunction::NnsRootUpgrade
                 | NnsFunction::HardResetNnsRootToVersion
                 | NnsFunction::AddSnsWasm
         )
@@ -420,8 +426,6 @@ impl NnsFunction {
                 | NnsFunction::UpdateNodesHostosVersion
                 | NnsFunction::BlessReplicaVersion
                 | NnsFunction::RetireReplicaVersion
-                | NnsFunction::NnsCanisterUpgrade
-                | NnsFunction::NnsRootUpgrade
         )
     }
 }

rs/replicated_state/src/canister_state/queues.rs

@@ -359,29 +359,36 @@ impl CanisterQueues {
         self.ingress_queue.filter_messages(filter)
     }
 
-    /// Pushes a canister-to-canister message into the induction pool.
+    /// Enqueues a canister-to-canister message into the induction pool.
     ///
-    /// If the message is a `Request` this will also reserve a slot in the
-    /// corresponding output queue for the eventual response.
+    /// If the message is a `Request` and is enqueued successfully, this will also
+    /// reserve a slot in the corresponding output queue for the eventual response.
     ///
-    /// If the message is a `Response` the protocol will have already reserved a
-    /// slot for it, so the push should not fail due to the input queue being full
-    /// (although an error may be returned in case of a bug in the upper layers).
+    /// If the message is a `Response`, `SystemState` will have already checked for
+    /// a matching callback:
     ///
-    /// Adds the sender to the appropriate input schedule (local or remote), if not
-    /// already there.
+    ///  * If this is a guaranteed `Response`, the protocol should have reserved a
+    ///    slot for it, so the push should not fail for lack of one (although an
+    ///    error may be returned in case of a bug in the upper layers).
+    ///  * If this is a best-effort `Response`, a slot is available and no duplicate
+    ///    (time out) response is already enqueued, it is enqueued.
+    ///  * If this is a best-effort `Response` and a duplicate (time out) response
+    ///    is already enqueued (which is implicitly true when no slot is available),
+    ///    the response is silently dropped and `Ok(())` is returned.
+    ///
+    /// If the message was enqueued, adds the sender to the appropriate input
+    /// schedule (local or remote), if not already there.
     ///
     /// # Errors
     ///
-    /// If pushing fails, returns the provided message along with a
-    /// `StateError`:
+    /// If pushing fails, returns the provided message along with a `StateError`:
     ///
-    ///  * `QueueFull` if pushing a `Request` and the corresponding input or
-    ///    output queues are full.
+    ///  * `QueueFull` if pushing a `Request` and the corresponding input or output
+    ///    queues are full.
     ///
-    ///  * `NonMatchingResponse` if pushing a `Response` and the corresponding input
-    ///    queue does not have a reserved slot; or this is a duplicate guaranteed
-    ///    response.
+    ///  * `NonMatchingResponse` if pushing a guaranteed `Response` and the
+    ///    corresponding input queue does not have a reserved slot; or it is a
+    ///    duplicate.
     pub(super) fn push_input(
         &mut self,
         msg: RequestOrResponse,
@@ -411,8 +418,8 @@ impl CanisterQueues {
                             .insert(response.originator_reply_callback)
                         {
                             debug_assert_eq!(Ok(()), self.test_invariants());
-                            // This is a critical error for guaranteed responses.
                             if response.deadline == NO_DEADLINE {
+                                // This is a critical error for a guaranteed response.
                                 return Err((
                                     StateError::non_matching_response(
                                         "Duplicate response",
@@ -421,8 +428,7 @@ impl CanisterQueues {
                                     msg,
                                 ));
                             } else {
-                                // But is OK for best-effort responses (if we already generated a timeout response).
-                                // Silently ignore the response.
+                                // But it's OK for a best-effort response. Silently drop it.
                                 return Ok(());
                             }
                         }
@@ -431,13 +437,23 @@ impl CanisterQueues {
 
                     // Queue does not exist or has no reserved slot for this response.
                     _ => {
-                        return Err((
-                            StateError::non_matching_response(
-                                "No reserved response slot",
-                                response,
-                            ),
-                            msg,
-                        ));
+                        if response.deadline == NO_DEADLINE {
+                            // Critical error for a guaranteed response.
+                            return Err((
+                                StateError::non_matching_response(
+                                    "No reserved response slot",
+                                    response,
+                                ),
+                                msg,
+                            ));
+                        } else {
+                            // This must be a duplicate best-effort response (since `SystemState` has
+                            // aleady checked for a matching callback). Silently drop it.
+                            debug_assert!(self
+                                .callbacks_with_enqueued_response
+                                .contains(&response.originator_reply_callback));
+                            return Ok(());
+                        }
                     }
                 }
             }

rs/replicated_state/src/canister_state/system_state.rs

@@ -1148,9 +1148,13 @@ impl SystemState {
                 },
             ) => {
                 if let RequestOrResponse::Response(response) = &msg {
-                    call_context_manager
-                        .validate_response(response)
-                        .map_err(|err| (err, msg.clone()))?;
+                    if !call_context_manager
+                        .should_enqueue(response)
+                        .map_err(|err| (err, msg.clone()))?
+                    {
+                        // Best effort response whose callback is gone. Silently drop it.
+                        return Ok(());
+                    }
                 }
                 push_input(
                     &mut self.queues,

rs/replicated_state/src/canister_state/system_state/call_context_manager.rs

@@ -594,13 +594,21 @@ impl CallContextManager {
         self.callbacks.get(&callback_id).map(AsRef::as_ref)
     }
 
-    /// Validates the given response before inducting it into the queue.
+    /// Tests whether the given response should be inducted or silently dropped.
     /// Verifies that the stored respondent and originator associated with the
     /// `callback_id`, as well as its deadline match those of the response.
     ///
-    /// Returns a `StateError::NonMatchingResponse` if the `callback_id` was not found
-    /// or if the response is not valid.
-    pub(crate) fn validate_response(&self, response: &Response) -> Result<(), StateError> {
+    /// Returns:
+    ///
+    ///  * `Ok(true)` if the response can be safely inducted.
+    ///  * `Ok(false)` (drop silently) when a matching `callback_id` was not found
+    ///    for a best-effort response (because the callback might have expired and
+    ///    been closed).
+    ///  * `Err(StateError::NonMatchingResponse)` when a matching `callback_id` was
+    ///    not found for a guaranteed response.
+    ///  * `Err(StateError::NonMatchingResponse)` if the response details do not
+    ///    match those of the callback.
+    pub(crate) fn should_enqueue(&self, response: &Response) -> Result<bool, StateError> {
         match self.callback(response.originator_reply_callback) {
             Some(callback) if response.respondent != callback.respondent
                     || response.originator != callback.originator
@@ -610,10 +618,17 @@ impl CallContextManager {
                         callback.originator, callback.respondent, Time::from(callback.deadline)
                     ), response))
             }
-            Some(_) => Ok(()),
+            Some(_) => Ok(true),
             None => {
                 // Received an unknown callback ID.
-                Err(StateError::non_matching_response("unknown callback ID", response))
+                if response.deadline == NO_DEADLINE {
+                    // This is an error for a guaranteed response.
+                    Err(StateError::non_matching_response("unknown callback ID", response))
+                } else {
+                    // But should be ignored in the case of a best-effort response (as the callback
+                    // may have expired and been dropped in the meantime).
+                    Ok(false)
+                }
             }
         }
     }