refactor(flexible-outcalls): remove obsolete subnet_size param from context's generate_from_args (#9037)

Removes the obsolete `subnet_size` param from
`CanisterHttpRequestContext::generate_from_args` and use
`node_ids.len()` instead, while rejecting empty node_ids with
`NoNodesAvailableForDelegation`, so as to make the API less confusing.

This is done because in production, `subnet_size` and `node_ids.len()`
are the same, except for the special (critical error) case when the
subnet record's `membership` is empty:
* `node_ids` is derived from the registry's subnet record's membership
[here](https://github.com/dfinity/ic/blob/4bb874fc5cc90e25ef1b003f932a48da40eb34a6/rs/messaging/src/message_routing.rs#L862-L869)
(de-duplicated via `BTreeSet`)
* `subnet_size` is also calculated from the registry's subnet record's
membership
[here](https://github.com/dfinity/ic/blob/master/rs/messaging/src/message_routing.rs#L905-L919)
(de-duplicated via `BTreeSet`), **BUT** there is a fallback when the
membership is empty, namely to `SMALL_APP_SUBNET_MAX_SIZE` (13) with a
warn! log (while the `node_ids` remain empty).
* Also in some tests, the two [can
diverge](https://github.com/dfinity/ic/blob/4bb874fc5cc90e25ef1b003f932a48da40eb34a6/rs/test_utilities/execution_environment/src/lib.rs#L206-L215),
which is irrelevant here.

Author: fspreiss

rs/execution_environment/src/execution_environment.rs

@@ -1104,7 +1104,6 @@ impl ExecutionEnvironment {
                                         request.as_ref(),
                                         args,
                                         &registry_settings.node_ids,
-                                        registry_settings.subnet_size,
                                         rng,
                                     ) {
                                         Err(err) => ExecuteSubnetMessageResult::Finished {

rs/test_utilities/execution_environment/src/lib.rs

@@ -216,7 +216,9 @@ pub fn test_registry_settings() -> RegistryExecutionSettings {
         provisional_whitelist: ProvisionalWhitelist::Set(BTreeSet::new()),
         chain_key_settings: BTreeMap::new(),
         subnet_size: SMALL_APP_SUBNET_MAX_SIZE,
-        node_ids: BTreeSet::new(),
+        node_ids: (0..SMALL_APP_SUBNET_MAX_SIZE)
+            .map(|i| node_test_id(i as u64))
+            .collect(),
         registry_version: RegistryVersion::default(),
     }
 }

rs/tests/networking/canister_http_correctness_test.rs

@@ -25,7 +25,7 @@ use ic_agent::{
     Agent, AgentError,
     agent::{RejectCode, RejectResponse},
 };
-use ic_base_types::{CanisterId, NumBytes};
+use ic_base_types::{CanisterId, NumBytes, PrincipalId};
 use ic_cdk::api::call::RejectionCode;
 use ic_management_canister_types_private::{
     HttpHeader, HttpMethod, TransformContext, TransformFunc,
@@ -2642,8 +2642,7 @@ fn expected_cycle_cost(
             .sender(proxy_canister)
             .build(),
         request.into(),
-        &BTreeSet::new(),
-        0,
+        &BTreeSet::from([PrincipalId::new_node_test_id(0).into()]),
         &mut rand::thread_rng(),
     )
     .unwrap();

rs/types/types/src/canister_http.rs

@@ -507,12 +507,14 @@ impl CanisterHttpRequestContext {
         request: &Request,
         args: CanisterHttpRequestArgs,
         node_ids: &BTreeSet<NodeId>,
-        subnet_size: usize,
         rng: &mut dyn RngCore,
     ) -> Result<Self, CanisterHttpRequestContextError> {
         validate_transform_principal(&args.transform, request.sender.get())?;
         validate_url_length(&args.url)?;
         validate_http_headers_and_body(args.headers.get(), args.body.as_ref().unwrap_or(&vec![]))?;
+        if node_ids.is_empty() {
+            return Err(CanisterHttpRequestContextError::NoNodesAvailableForDelegation);
+        }
 
         let max_response_bytes = match args.max_response_bytes {
             Some(max_response_bytes) => {
@@ -579,7 +581,7 @@ impl CanisterHttpRequestContext {
             refund_status: RefundStatus {
                 //TODO(IC-1937): subtract the base fee from the refundable amount.
                 refundable_cycles: request.payment,
-                per_replica_allowance: request.payment / subnet_size.max(1),
+                per_replica_allowance: request.payment / node_ids.len(),
                 refunded_cycles: Cycles::new(0),
                 refunding_nodes: BTreeSet::new(),
             },
@@ -1247,7 +1249,7 @@ mod tests {
         // PUT with is_replicated None -> rejected
         let args = dummy_args(HttpMethod::PUT, None);
         let result = CanisterHttpRequestContext::generate_from_args(
-            UNIX_EPOCH, &request, args, &node_ids, 1, rng,
+            UNIX_EPOCH, &request, args, &node_ids, rng,
         );
         assert_matches!(
             result,
@@ -1257,7 +1259,7 @@ mod tests {
         // PUT with is_replicated Some(true) -> rejected
         let args = dummy_args(HttpMethod::PUT, Some(true));
         let result = CanisterHttpRequestContext::generate_from_args(
-            UNIX_EPOCH, &request, args, &node_ids, 1, rng,
+            UNIX_EPOCH, &request, args, &node_ids, rng,
         );
         assert_matches!(
             result,
@@ -1267,7 +1269,7 @@ mod tests {
         // PUT with is_replicated Some(false) -> accepted
         let args = dummy_args(HttpMethod::PUT, Some(false));
         let result = CanisterHttpRequestContext::generate_from_args(
-            UNIX_EPOCH, &request, args, &node_ids, 1, rng,
+            UNIX_EPOCH, &request, args, &node_ids, rng,
         );
         assert_matches!(
             result,
@@ -1287,7 +1289,7 @@ mod tests {
         // DELETE with is_replicated None -> rejected
         let args = dummy_args(HttpMethod::DELETE, None);
         let result = CanisterHttpRequestContext::generate_from_args(
-            UNIX_EPOCH, &request, args, &node_ids, 1, rng,
+            UNIX_EPOCH, &request, args, &node_ids, rng,
         );
         assert_matches!(
             result,
@@ -1297,7 +1299,7 @@ mod tests {
         // DELETE with is_replicated Some(true) -> rejected
         let args = dummy_args(HttpMethod::DELETE, Some(true));
         let result = CanisterHttpRequestContext::generate_from_args(
-            UNIX_EPOCH, &request, args, &node_ids, 1, rng,
+            UNIX_EPOCH, &request, args, &node_ids, rng,
         );
         assert_matches!(
             result,
@@ -1307,7 +1309,7 @@ mod tests {
         // DELETE with is_replicated Some(false) -> accepted
         let args = dummy_args(HttpMethod::DELETE, Some(false));
         let result = CanisterHttpRequestContext::generate_from_args(
-            UNIX_EPOCH, &request, args, &node_ids, 1, rng,
+            UNIX_EPOCH, &request, args, &node_ids, rng,
         );
         assert_matches!(
             result,
@@ -1335,7 +1337,7 @@ mod tests {
         });
 
         let result = CanisterHttpRequestContext::generate_from_args(
-            UNIX_EPOCH, &request, args, &node_ids, 1, rng,
+            UNIX_EPOCH, &request, args, &node_ids, rng,
         );
 
         assert_matches!(
@@ -1344,6 +1346,23 @@ mod tests {
         );
     }
 
+    #[test]
+    fn rejects_empty_node_ids() {
+        let rng = &mut ReproducibleRng::new();
+        let node_ids = BTreeSet::new();
+        let request = dummy_request();
+        let args = dummy_args(HttpMethod::GET, None);
+
+        let result = CanisterHttpRequestContext::generate_from_args(
+            UNIX_EPOCH, &request, args, &node_ids, rng,
+        );
+
+        assert_matches!(
+            result,
+            Err(CanisterHttpRequestContextError::NoNodesAvailableForDelegation)
+        );
+    }
+
     #[test]
     fn flexible_rejects_invalid_transform_principal() {
         let rng = &mut ReproducibleRng::new();