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

Switch from sha256 to keccak256 #23

merged 8 commits into from
Nov 9, 2023

Conversation

moshababo
Copy link
Contributor

Switching from sha256 to keccak256 as the general-purpose hash function, to enhance compatibility with EVM/zkEVM.

Closing BFT-361.

@moshababo moshababo requested a review from slowli November 3, 2023 13:59
@@ -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
Collaborator

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.

pompon0
pompon0 previously approved these changes Nov 6, 2023
brunoffranca
brunoffranca previously approved these changes Nov 6, 2023
node/libs/crypto/src/keccak256/test.rs Outdated Show resolved Hide resolved
slowli
slowli previously approved these changes Nov 7, 2023

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.

node/deny.toml Outdated Show resolved Hide resolved
@moshababo moshababo dismissed stale reviews from slowli, brunoffranca, and pompon0 via 6e562ac November 9, 2023 01:23
# Conflicts:
#	node/Cargo.lock
#	node/actors/network/src/noise/stream.rs
#	node/deny.toml
#	node/libs/crypto/src/keccak256/mod.rs
#	node/libs/crypto/src/lib.rs
#	node/libs/roles/src/node/messages.rs
#	node/libs/roles/src/validator/messages/block.rs
#	node/libs/roles/src/validator/messages/msg.rs
@pompon0 pompon0 merged commit 9a3bb21 into main Nov 9, 2023
4 checks passed
@pompon0 pompon0 deleted the keccak256 branch November 9, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants