From df693b1df7db7af79439007ba89302aba0dfd4d4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= <francois@garillot.net>
Date: Mon, 16 May 2022 15:22:20 -0400
Subject: [PATCH] fix: align methods for hash computation

Our current solution for hashing batches has 3 components:
1. it relies on never deserializing a batch, so that we hash the serialized message,
2. it is not resilient to hash confusion, lenght extension, etc (see #[87]).
3. it is split in two functions that disagree with each other (see #[188]).

This stopgap PR does nothing about 1 & 2, but realigns the `impl Hash for Batch` so that it matches with the rest of the code base, and uses it in processor tests.
THe security fixes of the batch hashing will be done in #156.

Fixes #188
---
 narwhal/types/src/primary.rs                | 10 +++++++---
 narwhal/worker/src/tests/processor_tests.rs |  7 ++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/narwhal/types/src/primary.rs b/narwhal/types/src/primary.rs
index 3f07dff650db0..db3d54d197ea0 100644
--- a/narwhal/types/src/primary.rs
+++ b/narwhal/types/src/primary.rs
@@ -9,6 +9,7 @@ use blake2::{digest::Update, VarBlake2b};
 use bytes::Bytes;
 use config::{Committee, WorkerId};
 use crypto::{
+    ed25519::Ed25519PublicKey,
     traits::{EncodeDecodeBase64, VerifyingKey},
     Digest, Hash, SignatureService, DIGEST_LEN,
 };
@@ -67,9 +68,12 @@ impl Hash for Batch {
     type TypedDigest = BatchDigest;
 
     fn digest(&self) -> Self::TypedDigest {
-        BatchDigest(crypto::blake2b_256(|hasher| {
-            self.0.iter().for_each(|tx| hasher.update(tx))
-        }))
+        // While the `WorkerMessage` structure is generic, the parameter has no bearing on this enum variant
+        // TODO: fix in #[246]
+        let message = crate::worker::WorkerMessage::<Ed25519PublicKey>::Batch(self.clone());
+        let serialized = bincode::serialize(&message).expect("Batch serialization should not fail");
+
+        BatchDigest::new(crypto::blake2b_256(|hasher| hasher.update(&serialized)))
     }
 }
 
diff --git a/narwhal/worker/src/tests/processor_tests.rs b/narwhal/worker/src/tests/processor_tests.rs
index e058ef64a08f3..bdaa7e3848753 100644
--- a/narwhal/worker/src/tests/processor_tests.rs
+++ b/narwhal/worker/src/tests/processor_tests.rs
@@ -3,8 +3,8 @@
 // SPDX-License-Identifier: Apache-2.0
 use super::*;
 use crate::worker::WorkerMessage;
-use blake2::digest::Update;
 use crypto::ed25519::Ed25519PublicKey;
+use crypto::Hash;
 use store::rocks;
 use test_utils::{batch, temp_dir};
 use tokio::sync::mpsc::channel;
@@ -34,13 +34,14 @@ async fn hash_and_store() {
     );
 
     // Send a batch to the `Processor`.
-    let message = WorkerMessage::<Ed25519PublicKey>::Batch(batch());
+    let batch = batch();
+    let message = WorkerMessage::<Ed25519PublicKey>::Batch(batch.clone());
     let serialized = bincode::serialize(&message).unwrap();
     tx_batch.send(serialized.clone()).await.unwrap();
 
     // Ensure the `Processor` outputs the batch's digest.
     let output = rx_digest.recv().await.unwrap();
-    let digest = BatchDigest::new(crypto::blake2b_256(|hasher| hasher.update(&serialized)));
+    let digest = batch.digest();
     let expected = WorkerPrimaryMessage::OurBatch(digest, id);
     assert_eq!(output, expected);