diff --git a/rs/https_outcalls/consensus/src/payload_builder.rs b/rs/https_outcalls/consensus/src/payload_builder.rs index 61d5b4bed932..d6e2ca3223f3 100644 --- a/rs/https_outcalls/consensus/src/payload_builder.rs +++ b/rs/https_outcalls/consensus/src/payload_builder.rs @@ -658,6 +658,18 @@ impl CanisterHttpPayloadBuilderImpl { ); } + // Rejects are not allowed in flexible ok-responses + if matches!( + entry.response.content, + CanisterHttpResponseContent::Reject(_) + ) { + return invalid_artifact( + InvalidCanisterHttpPayloadReason::FlexibleRejectNotAllowedInOkResponses { + callback_id, + }, + ); + } + // No duplicate signers let signer = entry.proof.signature.signer; if !seen_signers.insert(signer) { diff --git a/rs/https_outcalls/consensus/src/payload_builder/tests.rs b/rs/https_outcalls/consensus/src/payload_builder/tests.rs index 7ca409997211..786576661506 100644 --- a/rs/https_outcalls/consensus/src/payload_builder/tests.rs +++ b/rs/https_outcalls/consensus/src/payload_builder/tests.rs @@ -1809,6 +1809,54 @@ fn flexible_build_respects_max_responses_per_block() { }); } +#[test] +fn flexible_build_filters_rejects_in_ok_responses() { + let num_nodes = 4; + let committee: BTreeSet<_> = (0..num_nodes as u64).map(node_test_id).collect(); + let callback_id = CallbackId::from(42); + + setup_test_with_flexible_context(num_nodes, callback_id, committee, 2, 4, |pb, pool| { + { + use ic_types::canister_http::CanisterHttpReject; + + let mut pool_access = pool.write().unwrap(); + // Nodes 0 and 1 produce Reject responses + for node_idx in 0..2_u64 { + let (response, metadata) = test_response_and_metadata_with_content( + callback_id.get(), + CanisterHttpResponseContent::Reject(CanisterHttpReject { + reject_code: RejectCode::SysTransient, + message: format!("error_{node_idx}"), + }), + ); + let share = metadata_to_share(node_idx, &metadata); + add_own_share_to_pool(pool_access.deref_mut(), &share, &response); + } + // Nodes 2 and 3 produce Success responses + for node_idx in 2..4_u64 { + let (response, metadata) = test_response_and_metadata_with_content( + callback_id.get(), + CanisterHttpResponseContent::Success(format!("resp_{node_idx}").into_bytes()), + ); + let share = metadata_to_share(node_idx, &metadata); + add_own_share_to_pool(pool_access.deref_mut(), &share, &response); + } + } + + let parsed = build_and_validate_and_parse_payload(&pb); + + assert_eq!(parsed.flexible_responses.len(), 1); + let group = &parsed.flexible_responses[0]; + assert_eq!(group.responses.len(), 2); + for entry in &group.responses { + assert!(matches!( + entry.response.content, + CanisterHttpResponseContent::Success(_) + )); + } + }); +} + #[test] fn flexible_build_and_validate_round_trip() { let num_nodes = 4; @@ -2361,6 +2409,40 @@ fn flexible_invalid_unknown_callback_id() { }); } +#[test] +fn flexible_invalid_rejects_in_ok_responses() { + let committee: BTreeSet<_> = (0..4).map(node_test_id).collect(); + let callback_id = CallbackId::from(42); + + setup_test_with_flexible_context(4, callback_id, committee, 2, 4, |payload_builder, _pool| { + let payload = flexible_payload(vec![FlexibleCanisterHttpResponses { + callback_id, + responses: vec![ + flexible_response(42, 0, b"good_response"), + flexible_reject_response(42, 1), + flexible_response(42, 2, b"another_good"), + ], + }]); + + let result = payload_builder.validate_payload( + Height::from(1), + &test_proposal_context(&default_validation_context()), + &payload_to_bytes_max_4mb(payload), + &[], + ); + assert_matches!( + result, + Err(ValidationError::InvalidArtifact( + InvalidPayloadReason::InvalidCanisterHttpPayload( + InvalidCanisterHttpPayloadReason::FlexibleRejectNotAllowedInOkResponses { + callback_id: id, + }, + ), + )) if id == CallbackId::from(42) + ); + }); +} + #[test] fn flexible_invalid_callback_id_mismatch_in_response() { let committee: BTreeSet<_> = (0..4).map(node_test_id).collect(); @@ -2683,6 +2765,25 @@ fn flexible_response( } } +fn flexible_reject_response( + callback_id: u64, + signer_node: u64, +) -> FlexibleCanisterHttpResponseWithProof { + use ic_types::canister_http::CanisterHttpReject; + + let (response, metadata) = test_response_and_metadata_with_content( + callback_id, + CanisterHttpResponseContent::Reject(CanisterHttpReject { + reject_code: RejectCode::SysTransient, + message: "could not connect".to_string(), + }), + ); + FlexibleCanisterHttpResponseWithProof { + response, + proof: metadata_to_share(signer_node, &metadata), + } +} + fn flexible_payload(groups: Vec) -> CanisterHttpPayload { CanisterHttpPayload { responses: vec![], diff --git a/rs/https_outcalls/consensus/src/payload_builder/utils.rs b/rs/https_outcalls/consensus/src/payload_builder/utils.rs index f4efbc7df31c..dd6a5d1bf849 100644 --- a/rs/https_outcalls/consensus/src/payload_builder/utils.rs +++ b/rs/https_outcalls/consensus/src/payload_builder/utils.rs @@ -5,8 +5,8 @@ use ic_types::{ FlexibleCanisterHttpResponseWithProof, FlexibleCanisterHttpResponses, ValidationContext, }, canister_http::{ - CanisterHttpResponse, CanisterHttpResponseMetadata, CanisterHttpResponseShare, - CanisterHttpResponseWithConsensus, + CanisterHttpResponse, CanisterHttpResponseContent, CanisterHttpResponseMetadata, + CanisterHttpResponseShare, CanisterHttpResponseWithConsensus, }, crypto::crypto_hash, messages::CallbackId, @@ -217,10 +217,11 @@ pub(crate) fn find_non_replicated_response( }) } -/// Collects distinct HTTP outcall responses from flexible committee members. +/// Collects distinct HTTP outcall OK-responses from flexible committee members. /// -/// Gathers up to `max_responses` individually-signed `(response, share)` pairs -/// from unique committee members, skipping any that would exceed `max_payload_size`. +/// Gathers up to `max_responses` individually-signed `(ok-response, share)` pairs +/// from unique committee members while disregarding rejects, and skipping any +/// that would exceed `max_payload_size`. /// Returns the group and its accumulated byte size if at least `min_responses` /// were collected. pub(crate) fn find_flexible_responses( @@ -250,6 +251,13 @@ pub(crate) fn find_flexible_responses( if let Some(http_response) = pool_access.get_response_content_by_hash(&metadata.content_hash) { + if matches!( + http_response.content, + CanisterHttpResponseContent::Reject(_) + ) { + // Disregard rejects, as we are collecting ok-responses. + continue; + } let response = FlexibleCanisterHttpResponseWithProof { response: http_response, proof: (*share).clone(), diff --git a/rs/interfaces/src/canister_http.rs b/rs/interfaces/src/canister_http.rs index a8026d6eadfb..9ab2b16efe5d 100644 --- a/rs/interfaces/src/canister_http.rs +++ b/rs/interfaces/src/canister_http.rs @@ -92,6 +92,10 @@ pub enum InvalidCanisterHttpPayloadReason { callback_id: CallbackId, signer: NodeId, }, + /// A flexible ok-response group contains a Reject response. + FlexibleRejectNotAllowedInOkResponses { + callback_id: CallbackId, + }, /// The payload is not in its designated section. /// For example, a non-flexible response is not in the responses section /// or a flexible response is not in the flexible_responses section.