Skip to content

Commit

Permalink
fix: align methods for hash computation
Browse files Browse the repository at this point in the history
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
  • Loading branch information
huitseeker committed May 16, 2022
1 parent 5d0d4ee commit df693b1
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 6 deletions.
10 changes: 7 additions & 3 deletions narwhal/types/src/primary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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)))
}
}

Expand Down
7 changes: 4 additions & 3 deletions narwhal/worker/src/tests/processor_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down

0 comments on commit df693b1

Please sign in to comment.