From f96585f3f4d988fe083cba1b85d9578317323bf1 Mon Sep 17 00:00:00 2001 From: Franz-Stefan Preiss Date: Tue, 24 Mar 2026 14:02:04 +0000 Subject: [PATCH 1/6] feat(http-outcalls): CON-1686 disregard reject responses when building payload for flexible outcall --- .../consensus/src/payload_builder.rs | 12 ++ .../consensus/src/payload_builder/tests.rs | 131 ++++++++++++++++++ .../consensus/src/payload_builder/utils.rs | 6 +- rs/interfaces/src/canister_http.rs | 2 + 4 files changed, 150 insertions(+), 1 deletion(-) diff --git a/rs/https_outcalls/consensus/src/payload_builder.rs b/rs/https_outcalls/consensus/src/payload_builder.rs index 61d5b4bed932..aef9464e0869 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 { ); } + // Reject responses are not allowed in flexible payloads + if matches!( + entry.response.content, + CanisterHttpResponseContent::Reject(_) + ) { + return invalid_artifact( + InvalidCanisterHttpPayloadReason::FlexibleRejectResponseNotAllowed { + 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..6a4f8afbdb8d 100644 --- a/rs/https_outcalls/consensus/src/payload_builder/tests.rs +++ b/rs/https_outcalls/consensus/src/payload_builder/tests.rs @@ -1809,6 +1809,84 @@ fn flexible_build_respects_max_responses_per_block() { }); } +#[test] +fn flexible_build_filters_reject_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_excludes_group_when_only_reject_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(); + // All 4 nodes produce Reject responses -- can't reach min_responses=2 + for node_idx in 0..4u64 { + 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); + } + } + + let parsed = build_and_validate_and_parse_payload(&pb); + assert!(parsed.flexible_responses.is_empty()); + }); +} + #[test] fn flexible_build_and_validate_round_trip() { let num_nodes = 4; @@ -2361,6 +2439,40 @@ fn flexible_invalid_unknown_callback_id() { }); } +#[test] +fn flexible_invalid_reject_response() { + 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::FlexibleRejectResponseNotAllowed { + 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 +2795,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..8ec6e94739ab 100644 --- a/rs/https_outcalls/consensus/src/payload_builder/utils.rs +++ b/rs/https_outcalls/consensus/src/payload_builder/utils.rs @@ -5,7 +5,8 @@ use ic_types::{ FlexibleCanisterHttpResponseWithProof, FlexibleCanisterHttpResponses, ValidationContext, }, canister_http::{ - CanisterHttpResponse, CanisterHttpResponseMetadata, CanisterHttpResponseShare, + CanisterHttpResponse, CanisterHttpResponseContent, CanisterHttpResponseMetadata, + CanisterHttpResponseShare, CanisterHttpResponseWithConsensus, }, crypto::crypto_hash, @@ -250,6 +251,9 @@ 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(_)) { + 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..f7c9b94169b2 100644 --- a/rs/interfaces/src/canister_http.rs +++ b/rs/interfaces/src/canister_http.rs @@ -92,6 +92,8 @@ pub enum InvalidCanisterHttpPayloadReason { callback_id: CallbackId, signer: NodeId, }, + /// A flexible response entry contains a Reject response, which is not allowed. + FlexibleRejectResponseNotAllowed { 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. From 0de3825397e23f2207d93253207a2e60a64cde9f Mon Sep 17 00:00:00 2001 From: IDX GitHub Automation Date: Tue, 24 Mar 2026 14:08:48 +0000 Subject: [PATCH 2/6] Automatically fixing code for linting and formatting issues --- rs/https_outcalls/consensus/src/payload_builder/utils.rs | 8 +++++--- rs/interfaces/src/canister_http.rs | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/rs/https_outcalls/consensus/src/payload_builder/utils.rs b/rs/https_outcalls/consensus/src/payload_builder/utils.rs index 8ec6e94739ab..e46da9bc334a 100644 --- a/rs/https_outcalls/consensus/src/payload_builder/utils.rs +++ b/rs/https_outcalls/consensus/src/payload_builder/utils.rs @@ -6,8 +6,7 @@ use ic_types::{ }, canister_http::{ CanisterHttpResponse, CanisterHttpResponseContent, CanisterHttpResponseMetadata, - CanisterHttpResponseShare, - CanisterHttpResponseWithConsensus, + CanisterHttpResponseShare, CanisterHttpResponseWithConsensus, }, crypto::crypto_hash, messages::CallbackId, @@ -251,7 +250,10 @@ 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(_)) { + if matches!( + http_response.content, + CanisterHttpResponseContent::Reject(_) + ) { continue; } let response = FlexibleCanisterHttpResponseWithProof { diff --git a/rs/interfaces/src/canister_http.rs b/rs/interfaces/src/canister_http.rs index f7c9b94169b2..a14766f985bd 100644 --- a/rs/interfaces/src/canister_http.rs +++ b/rs/interfaces/src/canister_http.rs @@ -93,7 +93,9 @@ pub enum InvalidCanisterHttpPayloadReason { signer: NodeId, }, /// A flexible response entry contains a Reject response, which is not allowed. - FlexibleRejectResponseNotAllowed { callback_id: CallbackId }, + FlexibleRejectResponseNotAllowed { + 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. From 44365ad142f511ac7be258d3bca41fcfa9e5dfa6 Mon Sep 17 00:00:00 2001 From: Franz-Stefan Preiss Date: Thu, 26 Mar 2026 12:58:07 +0000 Subject: [PATCH 3/6] cleanup --- .../consensus/src/payload_builder.rs | 2 +- .../consensus/src/payload_builder/tests.rs | 32 +------------------ 2 files changed, 2 insertions(+), 32 deletions(-) diff --git a/rs/https_outcalls/consensus/src/payload_builder.rs b/rs/https_outcalls/consensus/src/payload_builder.rs index aef9464e0869..c5793c6ea32f 100644 --- a/rs/https_outcalls/consensus/src/payload_builder.rs +++ b/rs/https_outcalls/consensus/src/payload_builder.rs @@ -658,7 +658,7 @@ impl CanisterHttpPayloadBuilderImpl { ); } - // Reject responses are not allowed in flexible payloads + // Rejects are not allowed in flexible ok-responses if matches!( entry.response.content, CanisterHttpResponseContent::Reject(_) diff --git a/rs/https_outcalls/consensus/src/payload_builder/tests.rs b/rs/https_outcalls/consensus/src/payload_builder/tests.rs index 6a4f8afbdb8d..868e0ca04167 100644 --- a/rs/https_outcalls/consensus/src/payload_builder/tests.rs +++ b/rs/https_outcalls/consensus/src/payload_builder/tests.rs @@ -1810,7 +1810,7 @@ fn flexible_build_respects_max_responses_per_block() { } #[test] -fn flexible_build_filters_reject_responses() { +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); @@ -1857,36 +1857,6 @@ fn flexible_build_filters_reject_responses() { }); } -#[test] -fn flexible_build_excludes_group_when_only_reject_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(); - // All 4 nodes produce Reject responses -- can't reach min_responses=2 - for node_idx in 0..4u64 { - 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); - } - } - - let parsed = build_and_validate_and_parse_payload(&pb); - assert!(parsed.flexible_responses.is_empty()); - }); -} - #[test] fn flexible_build_and_validate_round_trip() { let num_nodes = 4; From 7c5a8c1cf6fb0d2a7745f17ac08171bdd763fb1f Mon Sep 17 00:00:00 2001 From: Franz-Stefan Preiss Date: Thu, 26 Mar 2026 12:59:15 +0000 Subject: [PATCH 4/6] more cleanup --- rs/https_outcalls/consensus/src/payload_builder/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/https_outcalls/consensus/src/payload_builder/tests.rs b/rs/https_outcalls/consensus/src/payload_builder/tests.rs index 868e0ca04167..791cc9708f46 100644 --- a/rs/https_outcalls/consensus/src/payload_builder/tests.rs +++ b/rs/https_outcalls/consensus/src/payload_builder/tests.rs @@ -2410,7 +2410,7 @@ fn flexible_invalid_unknown_callback_id() { } #[test] -fn flexible_invalid_reject_response() { +fn flexible_invalid_rejects_in_ok_responses() { let committee: BTreeSet<_> = (0..4).map(node_test_id).collect(); let callback_id = CallbackId::from(42); From 2851840f1aa9234b335bd44f54051f2d17ca8551 Mon Sep 17 00:00:00 2001 From: Franz-Stefan Preiss Date: Thu, 26 Mar 2026 13:05:35 +0000 Subject: [PATCH 5/6] better error naming --- rs/https_outcalls/consensus/src/payload_builder.rs | 2 +- rs/https_outcalls/consensus/src/payload_builder/tests.rs | 2 +- rs/interfaces/src/canister_http.rs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/rs/https_outcalls/consensus/src/payload_builder.rs b/rs/https_outcalls/consensus/src/payload_builder.rs index c5793c6ea32f..d6e2ca3223f3 100644 --- a/rs/https_outcalls/consensus/src/payload_builder.rs +++ b/rs/https_outcalls/consensus/src/payload_builder.rs @@ -664,7 +664,7 @@ impl CanisterHttpPayloadBuilderImpl { CanisterHttpResponseContent::Reject(_) ) { return invalid_artifact( - InvalidCanisterHttpPayloadReason::FlexibleRejectResponseNotAllowed { + InvalidCanisterHttpPayloadReason::FlexibleRejectNotAllowedInOkResponses { callback_id, }, ); diff --git a/rs/https_outcalls/consensus/src/payload_builder/tests.rs b/rs/https_outcalls/consensus/src/payload_builder/tests.rs index 791cc9708f46..786576661506 100644 --- a/rs/https_outcalls/consensus/src/payload_builder/tests.rs +++ b/rs/https_outcalls/consensus/src/payload_builder/tests.rs @@ -2434,7 +2434,7 @@ fn flexible_invalid_rejects_in_ok_responses() { result, Err(ValidationError::InvalidArtifact( InvalidPayloadReason::InvalidCanisterHttpPayload( - InvalidCanisterHttpPayloadReason::FlexibleRejectResponseNotAllowed { + InvalidCanisterHttpPayloadReason::FlexibleRejectNotAllowedInOkResponses { callback_id: id, }, ), diff --git a/rs/interfaces/src/canister_http.rs b/rs/interfaces/src/canister_http.rs index a14766f985bd..9ab2b16efe5d 100644 --- a/rs/interfaces/src/canister_http.rs +++ b/rs/interfaces/src/canister_http.rs @@ -92,8 +92,8 @@ pub enum InvalidCanisterHttpPayloadReason { callback_id: CallbackId, signer: NodeId, }, - /// A flexible response entry contains a Reject response, which is not allowed. - FlexibleRejectResponseNotAllowed { + /// A flexible ok-response group contains a Reject response. + FlexibleRejectNotAllowedInOkResponses { callback_id: CallbackId, }, /// The payload is not in its designated section. From e2483852253b92d2fc69b8dc140fb91624e3711e Mon Sep 17 00:00:00 2001 From: Franz-Stefan Preiss Date: Thu, 26 Mar 2026 15:47:42 +0000 Subject: [PATCH 6/6] add comment --- rs/https_outcalls/consensus/src/payload_builder/utils.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/rs/https_outcalls/consensus/src/payload_builder/utils.rs b/rs/https_outcalls/consensus/src/payload_builder/utils.rs index e46da9bc334a..dd6a5d1bf849 100644 --- a/rs/https_outcalls/consensus/src/payload_builder/utils.rs +++ b/rs/https_outcalls/consensus/src/payload_builder/utils.rs @@ -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( @@ -254,6 +255,7 @@ pub(crate) fn find_flexible_responses( http_response.content, CanisterHttpResponseContent::Reject(_) ) { + // Disregard rejects, as we are collecting ok-responses. continue; } let response = FlexibleCanisterHttpResponseWithProof {