feat(CON-1253): Improve HTTP outcall divergence error message

Author: Sawchord

Cargo.lock

@@ -15,6 +15,7 @@ DEPENDENCIES = [
     "//rs/replicated_state",
     "//rs/types/error_types",
     "//rs/types/types",
+    "@crate_index//:hex",
     "@crate_index//:prometheus",
     "@crate_index//:slog",
 ]
@@ -32,6 +33,8 @@ DEV_DEPENDENCIES = [
     "//rs/test_utilities/types",
     "@crate_index//:mockall",
     "@crate_index//:proptest",
+    "@crate_index//:rand",
+    "@crate_index//:rand_chacha",
 ]
 
 rust_library(

rs/https_outcalls/consensus/BUILD.bazel

@@ -19,6 +19,7 @@ ic-protobuf = { path = "../../protobuf" }
 ic-registry-client-helpers = { path = "../../registry/helpers" }
 ic-replicated-state = { path = "../../replicated_state" }
 ic-types = { path = "../../types/types" }
+hex = { workspace = true }
 prometheus = { workspace = true }
 slog = { workspace = true }
 
@@ -35,3 +36,5 @@ ic-test-utilities-state = { path = "../../test_utilities/state" }
 ic-test-utilities-time = { path = "../../test_utilities/time" }
 ic-test-utilities-types = { path = "../../test_utilities/types" }
 mockall = { workspace = true }
+rand = { workspace = true }
+rand_chacha = { workspace = true }

rs/https_outcalls/consensus/Cargo.toml

@@ -44,7 +44,7 @@ use ic_types::{
     CountBytes, Height, NodeId, NumBytes, RegistryVersion, SubnetId,
 };
 use std::{
-    collections::{BTreeSet, HashSet},
+    collections::{BTreeMap, BTreeSet, HashSet},
     mem::size_of,
     sync::{Arc, RwLock},
 };
@@ -680,25 +680,10 @@ impl IntoMessages<(Vec<ConsensusResponse>, CanisterHttpBatchStats)>
             )
         });
 
-        let divergece_responses = messages.divergence_responses.iter().filter_map(|response| {
-            // NOTE: We skip delivering the divergence response, if it has no shares
-            // Such a divergence response should never validate, therefore this should never happen
-            // However, if it where ever to happen, we can ignore it here.
-            // This is sound, since eventually a timeout will end the outstanding callback anyway.
-            response.shares.first().map(|share| {
-                // Map divergence responses to reject response
-                stats.divergence_responses += 1;
-                ConsensusResponse::new(
-                    share.content.id,
-                    Payload::Reject(RejectContext::new(
-                        RejectCode::SysTransient,
-                        "Canister http responses were different across replicas, \
-                          and no consensus was reached"
-                            .to_string(),
-                    )),
-                )
-            })
-        });
+        let divergece_responses = messages
+            .divergence_responses
+            .iter()
+            .filter_map(divergence_response_into_reject);
 
         let responses = responses
             .chain(timeouts)
@@ -709,6 +694,69 @@ impl IntoMessages<(Vec<ConsensusResponse>, CanisterHttpBatchStats)>
     }
 }
 
+/// Turns a [`CanisterHttpResponseDivergence`] into a [`ConsensusResponse`] containing a rejection.
+///
+/// This function generates a detailed error message.
+/// This will enable a developer to get some insight into the nature of the divergence problems, which they are facing.
+/// It allows to get insight into whether the responses are split among a very small number of possible responses or each replica
+/// got a unique response.
+/// The first issue could point to some issue rate limiting (e.g. some replicas receive 429s) while the later would point to an
+/// issue with the transform function (e.g. some non-deterministic component such as timestamp has not been removed).
+///
+/// The function includes request id and timeout, which are also part of the hashed value.
+fn divergence_response_into_reject(
+    response: &CanisterHttpResponseDivergence,
+) -> Option<ConsensusResponse> {
+    // Get the id and timeout, which need to be reported in the error message as well
+    let Some((id, timeout)) = response
+        .shares
+        .first()
+        .map(|share| (share.content.id, share.content.timeout))
+    else {
+        // NOTE: We skip delivering the divergence response, if it has no shares
+        // Such a divergence response should never validate, therefore this should never happen
+        // However, if it where ever to happen, we can ignore it here.
+        // This is sound, since eventually a timeout will end the outstanding callback anyway.
+        return None;
+    };
+
+    // Count the different content hashes, that we have encountered in the divergence resonse
+    let mut hash_counts = BTreeMap::new();
+    response
+        .shares
+        .iter()
+        .map(|share| share.content.content_hash.clone().get().0)
+        .for_each(|share| {
+            hash_counts
+                .entry(share)
+                .and_modify(|count| *count += 1)
+                .or_insert(1);
+        });
+
+    // Now convert into a vector
+    let mut hash_counts = hash_counts.into_iter().collect::<Vec<_>>();
+
+    // Sort in ascending order by number of counts
+    hash_counts.sort_by_key(|(_, count)| *count);
+    // Convert them into hex strings
+    let hash_counts = hash_counts
+        .iter()
+        .rev()
+        .map(|(hash, count)| format!("[{}: {}]", hex::encode(hash), count))
+        .collect::<Vec<_>>();
+
+    Some(ConsensusResponse::new(
+        id,
+        Payload::Reject(RejectContext::new(
+            RejectCode::SysTransient,
+            format!(
+                "No consensus could be reached. Replicas had different responses. Details: request_id: {}, timeout: {}, hashes: {}",
+                id, timeout.as_nanos_since_unix_epoch(), hash_counts.join(", ")
+            ),
+        )),
+    ))
+}
+
 fn validation_failed(
     err: CanisterHttpPayloadValidationFailure,
 ) -> Result<(), PayloadValidationError> {

rs/https_outcalls/consensus/src/payload_builder.rs

@@ -3,11 +3,15 @@
 //!
 //! Some tests are run over a range of subnet configurations to check for corner cases.
 
-use crate::payload_builder::parse::{bytes_to_payload, payload_to_bytes};
+use crate::payload_builder::{
+    divergence_response_into_reject,
+    parse::{bytes_to_payload, payload_to_bytes},
+};
 
 use super::CanisterHttpPayloadBuilderImpl;
 use ic_artifact_pool::canister_http_pool::CanisterHttpPoolImpl;
 use ic_consensus_mocks::{dependencies_with_subnet_params, Dependencies};
+use ic_error_types::RejectCode;
 use ic_interfaces::{
     batch_payload::{BatchPayloadBuilder, PastPayload, ProposalContext},
     canister_http::{
@@ -38,12 +42,14 @@ use ic_types::{
     },
     consensus::get_faults_tolerated,
     crypto::{crypto_hash, BasicSig, BasicSigOf, CryptoHash, CryptoHashOf, Signed},
-    messages::CallbackId,
+    messages::{CallbackId, Payload, RejectContext},
     registry::RegistryClientError,
     signature::{BasicSignature, BasicSignatureBatch},
     time::UNIX_EPOCH,
     Height, NumBytes, RegistryVersion, Time,
 };
+use rand::Rng;
+use rand_chacha::{rand_core::SeedableRng, ChaCha20Rng};
 use std::{
     collections::BTreeMap,
     ops::DerefMut,
@@ -760,6 +766,58 @@ fn divergence_response_validation_test() {
     }
 }
 
+/// Check that the divergence error message is constructed correctly and readable
+#[test]
+fn divergence_error_message() {
+    let (_, metadata) = test_response_and_metadata(1);
+
+    let mut rng = ChaCha20Rng::seed_from_u64(1337);
+    let mut response_shares = (0..6)
+        .map(|node_id| {
+            let mut sample = metadata.clone();
+            let mut new_hash = [0; 32];
+            rng.fill(&mut new_hash);
+
+            sample.content_hash = CryptoHashOf::from(CryptoHash(new_hash.to_vec()));
+
+            Signed {
+                content: sample,
+                signature: BasicSignature {
+                    signature: BasicSigOf::new(BasicSig(vec![])),
+                    signer: node_test_id(node_id),
+                },
+            }
+        })
+        .collect::<Vec<_>>();
+
+    // Duplicate some responses
+    response_shares.push(response_shares[0].clone());
+    response_shares.push(response_shares[0].clone());
+    response_shares.push(response_shares[1].clone());
+
+    let divergence_response = CanisterHttpResponseDivergence {
+        shares: response_shares,
+    };
+
+    let divergence_reject = divergence_response_into_reject(&divergence_response).unwrap();
+
+    assert_eq!(
+        divergence_reject.payload,
+        Payload::Reject(RejectContext::new(
+            RejectCode::SysTransient,
+            "No consensus could be reached. Replicas had different responses. \
+            Details: request_id: 1, timeout: 10000000000, hashes: \
+            [30bb6555f891c35fbc820c74a7db7c1ae621f924340712e12febe78a9be0a908: 3], \
+            [e93edfdefc581e2bdb54a08b55dd67bc675afafbbb32697ef6b8bf9cc75fe69b: 2], \
+            [af4dcbc617e83bc998190e3031123dbd26bcf0c5a5013b5465017234a98f7d74: 1], \
+            [51a9af560377af0994fe4be465ea5adff3372623c6ac692c4d3e23b323ef8486: 1], \
+            [2b7e888246a3b450c67396062e53c8b6c4b776e082e7d2a81c5536e89fe6013e: 1], \
+            [000b3b9ca14f1136c076b7f681b0a496f5108f721833e6465d0671c014e60b43: 1]"
+                .to_string()
+        ))
+    );
+}
+
 /// Build some test metadata and response, which is valid and can be used in
 /// different tests
 pub(crate) fn test_response_and_metadata(