Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from sha256 to keccak256 #23

Merged
merged 8 commits into from
Nov 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
489 changes: 264 additions & 225 deletions node/Cargo.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ rand = "0.8.0"
rocksdb = "0.21.0"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0.95"
sha2 = "0.10.6"
sha3 = "0.10.8"
snow = "0.9.3"
syn = "2.0.17"
tempfile = "3"
Expand Down
6 changes: 3 additions & 3 deletions node/actors/network/src/noise/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use zksync_concurrency::{
ctx, io,
io::{AsyncRead as _, AsyncWrite as _},
};
use zksync_consensus_crypto::{sha256::Sha256, ByteFmt};
use zksync_consensus_crypto::{keccak256::Keccak256, ByteFmt};

/// Fixed noise configuration. Nodes need to use the same noise
/// configuration to be able to connect to each other.
Expand Down Expand Up @@ -75,7 +75,7 @@ impl Default for Buffer {
pub(crate) struct Stream<S = MeteredStream> {
/// Hash of the handshake messages.
/// Uniquely identifies the noise session.
id: Sha256,
id: Keccak256,
/// Underlying TCP stream.
#[pin]
inner: S,
Expand Down Expand Up @@ -138,7 +138,7 @@ where

/// Returns the noise session id.
/// See `Stream::id`.
pub(crate) fn id(&self) -> Sha256 {
pub(crate) fn id(&self) -> Keccak256 {
self.id
}

Expand Down
7 changes: 4 additions & 3 deletions node/deny.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,12 @@ skip = [
{ name = "regex-syntax", version = "=0.6.29" },

# Old versions required by hyper.
{ name = "socket2", version = "=0.4.9" },
{ name = "hashbrown", version = "=0.12.3" }, # (hyper -> h2 -> indexmap -> hashbrown)
{ name = "socket2", version = "0.4.10" },

# Old versions required by ark-bn254.
{ name = "syn", version = "=1.0.109" }
{ name = "syn", version = "=1.0.109" },
{ name = "hashbrown", version = "=0.13.2" },
{ name = "itertools", version = "=0.10.5" }
]

[sources]
Expand Down
2 changes: 1 addition & 1 deletion node/libs/concurrency/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ anyhow.workspace = true
once_cell.workspace = true
pin-project.workspace = true
rand.workspace = true
sha2.workspace = true
sha3.workspace = true
thiserror.workspace = true
time.workspace = true
tokio.workspace = true
Expand Down
8 changes: 4 additions & 4 deletions node/libs/concurrency/src/ctx/rng.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use rand::{
rngs::{OsRng, StdRng},
Rng, SeedableRng,
};
use sha2::{digest::Update as _, Digest as _};
use sha3::{digest::Update as _, Digest as _};
use std::sync::atomic::{AtomicU64, Ordering};

/// Splittable Pseudorandom Number Generators using Cryptographic Hashing
Expand All @@ -16,13 +16,13 @@ use std::sync::atomic::{AtomicU64, Ordering};
/// which is not crypto-secure, but for tests should be good enough.
pub(super) struct SplitProvider {
/// Hash builder of the prefix of the ID.
builder: sha2::Sha256,
builder: sha3::Keccak256,
/// Child branches constructed so far from this branch.
branch: AtomicU64,
}

impl SplitProvider {
fn next_builder(&self) -> sha2::Sha256 {
fn next_builder(&self) -> sha3::Keccak256 {
let branch = self.branch.fetch_add(1, Ordering::SeqCst);
self.builder.clone().chain(branch.to_le_bytes())
}
Expand All @@ -43,7 +43,7 @@ impl Provider {
pub(super) fn test() -> Provider {
Self::Split(
SplitProvider {
builder: sha2::Sha256::new().chain("Lzr81nDW8eSOMH".as_bytes()),
builder: sha3::Keccak256::new().chain("Lzr81nDW8eSOMH".as_bytes()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has nothing to do with evm. Is keccak faster than sha?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends. On hardware implementations Keccak/SHA3 is faster, but if there's no hardware support then it's slower than SHA2.
That being said, I just think it's easier for now to just use one hash algorithm for everything. If/when we do code profiling and realize that hashing is a bottleneck, we can change to a faster algorithm like BLAKE3 wherever we can.

branch: 0.into(),
}
.into(),
Expand Down
2 changes: 1 addition & 1 deletion node/libs/crypto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ num-traits.workspace = true
ed25519-dalek.workspace = true
hex.workspace = true
rand.workspace = true
sha2.workspace = true
sha3.workspace = true
thiserror.workspace = true
tracing.workspace = true

Expand Down
4 changes: 2 additions & 2 deletions node/libs/crypto/src/bn254/hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

use ark_bn254::{G1Affine, G1Projective};
use ark_ec::AffineRepr as _;
use sha2::Digest as _;
use sha3::Digest as _;

/// Hashes an arbitrary message and maps it to an elliptic curve point in G1.
pub(crate) fn hash_to_g1(msg: &[u8]) -> G1Projective {
for i in 0..256 {
// Hash the message with the index as suffix.
let bytes: [u8; 32] = sha2::Sha256::new()
let bytes: [u8; 32] = sha3::Keccak256::new()
.chain_update(msg)
.chain_update((i as u32).to_be_bytes())
.finalize()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,23 @@
//! Wrappers for the SHA256 cryptographic hash algorithm.
//! Wrappers for the Keccak256 cryptographic hash algorithm.
use crate::ByteFmt;
use sha2::{digest::Update as _, Digest as _};
use sha3::{digest::Update as _, Digest as _};

#[cfg(test)]
mod test;
pub mod testonly;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-topic (?): Why does this module need to be public? AFAICT, it doesn't contain public items.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you refer to line 5 (added) or 6 (already exists).

  • testonly: it needs to be public, although it works without the pub modifier, because visibility is bound to the trait implementation target type.
  • test: I forgot to add the #[cfg(test)] attribute. Added now.

/// SHA256 hash.
/// Keccak256 hash.
#[derive(Copy, Clone, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct Sha256(pub(crate) [u8; 32]);
pub struct Keccak256(pub(crate) [u8; 32]);

impl Sha256 {
/// Computes a SHA-256 hash of a message.
impl Keccak256 {
/// Computes a Keccak256 hash of a message.
pub fn new(msg: &[u8]) -> Self {
Self(sha2::Sha256::new().chain(msg).finalize().into())
Self(sha3::Keccak256::new().chain(msg).finalize().into())
}

/// Interprets the specified `bytes` as a hash digest (i.e., a reverse operation to [`Self::as_bytes()`]).
/// It is caller's responsibility to ensure that `bytes` are actually a SHA-256 hash digest.
/// It is caller's responsibility to ensure that `bytes` are actually a Keccak256 hash digest.
pub fn from_bytes(bytes: [u8; 32]) -> Self {
Self(bytes)
}
Expand All @@ -26,7 +28,7 @@ impl Sha256 {
}
}

impl ByteFmt for Sha256 {
impl ByteFmt for Keccak256 {
fn decode(bytes: &[u8]) -> anyhow::Result<Self> {
Ok(Self(bytes.try_into()?))
}
Expand Down
33 changes: 33 additions & 0 deletions node/libs/crypto/src/keccak256/test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#[test]
fn test_keccak256() -> Result<(), Box<dyn std::error::Error>> {
use crate::keccak256::Keccak256;

// Test vectors obtained from https://emn178.github.io/online-tools/keccak_256.html
let test_vectors: Vec<(&[u8], [u8; 32])> = vec![
(
b"testing",
hex::decode("5f16f4c7f149ac4f9510d9cf8cf384038ad348b3bcdc01915f95de12df9d1b02")?
.try_into()
.unwrap(),
),
(
b"",
hex::decode("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470")?
.try_into()
.unwrap(),
),
(
&[0x12, 0x34, 0x56],
hex::decode("6adf031833174bbe4c85eafe59ddb54e6584648c2c962c6f94791ab49caa0ad4")?
.try_into()
.unwrap(),
),
];

for (input, expected_hash) in &test_vectors {
let hash = Keccak256::new(input);
assert_eq!(hash.as_bytes(), expected_hash);
}

Ok(())
}
13 changes: 13 additions & 0 deletions node/libs/crypto/src/keccak256/testonly.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//! Random hash generation, intended for use in testing

use crate::keccak256::Keccak256;
use rand::{
distributions::{Distribution, Standard},
Rng,
};

impl Distribution<Keccak256> for Standard {
fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Keccak256 {
Keccak256(rng.gen())
}
}
2 changes: 1 addition & 1 deletion node/libs/crypto/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ pub mod bls12_381;
pub mod bn254;
pub mod ed25519;
mod fmt;
pub mod sha256;
pub mod keccak256;
13 changes: 0 additions & 13 deletions node/libs/crypto/src/sha256/testonly.rs

This file was deleted.

12 changes: 7 additions & 5 deletions node/libs/roles/src/node/messages.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::node;
use zksync_consensus_crypto::{sha256, ByteFmt, Text, TextFmt};
use zksync_consensus_crypto::{keccak256, ByteFmt, Text, TextFmt};
use zksync_consensus_utils::enum_util::{BadVariantError, Variant};

/// The ID for an authentication session.
Expand All @@ -17,7 +17,7 @@ pub enum Msg {
impl Msg {
/// Get the hash of this message.
pub fn hash(&self) -> MsgHash {
MsgHash(sha256::Sha256::new(&zksync_protobuf::canonical(self)))
MsgHash(keccak256::Keccak256::new(&zksync_protobuf::canonical(self)))
}
}

Expand Down Expand Up @@ -52,7 +52,7 @@ impl<V: Variant<Msg> + Clone> Signed<V> {
}

/// The hash of a message.
pub struct MsgHash(pub(super) sha256::Sha256);
pub struct MsgHash(pub(super) keccak256::Keccak256);

impl ByteFmt for MsgHash {
fn encode(&self) -> Vec<u8> {
Expand All @@ -66,11 +66,13 @@ impl ByteFmt for MsgHash {
impl TextFmt for MsgHash {
fn encode(&self) -> String {
format!(
"validator_msg:sha256:{}",
"validator_msg:keccak256:{}",
hex::encode(ByteFmt::encode(&self.0))
)
}
fn decode(text: Text) -> anyhow::Result<Self> {
text.strip("validator_msg:sha256:")?.decode_hex().map(Self)
text.strip("validator_msg:keccak256:")?
.decode_hex()
.map(Self)
}
}
12 changes: 6 additions & 6 deletions node/libs/roles/src/validator/conv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,23 @@ use zksync_protobuf::{read_required, required, ProtoFmt};
impl ProtoFmt for BlockHeaderHash {
type Proto = proto::BlockHeaderHash;
fn read(r: &Self::Proto) -> anyhow::Result<Self> {
Ok(Self(ByteFmt::decode(required(&r.sha256)?)?))
Ok(Self(ByteFmt::decode(required(&r.keccak256)?)?))
}
fn build(&self) -> Self::Proto {
Self::Proto {
sha256: Some(self.0.encode()),
keccak256: Some(self.0.encode()),
}
}
}

impl ProtoFmt for PayloadHash {
type Proto = proto::PayloadHash;
fn read(r: &Self::Proto) -> anyhow::Result<Self> {
Ok(Self(ByteFmt::decode(required(&r.sha256)?)?))
Ok(Self(ByteFmt::decode(required(&r.keccak256)?)?))
}
fn build(&self) -> Self::Proto {
Self::Proto {
sha256: Some(self.0.encode()),
keccak256: Some(self.0.encode()),
}
}
}
Expand Down Expand Up @@ -322,12 +322,12 @@ impl ProtoFmt for MsgHash {
type Proto = proto::MsgHash;

fn read(r: &Self::Proto) -> anyhow::Result<Self> {
Ok(Self(ByteFmt::decode(required(&r.sha256)?)?))
Ok(Self(ByteFmt::decode(required(&r.keccak256)?)?))
}

fn build(&self) -> Self::Proto {
Self::Proto {
sha256: Some(self.0.encode()),
keccak256: Some(self.0.encode()),
}
}
}
Expand Down
Loading
Loading