Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add keccak512 hash #7428

Merged
2 commits merged into from
Oct 26, 2020
Merged

Add keccak512 hash #7428

2 commits merged into from
Oct 26, 2020

Conversation

drewstone
Copy link
Contributor

No description provided.

@shawntabrizi shawntabrizi added A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 26, 2020
@shawntabrizi
Copy link
Member

bot merge

@ghost
Copy link

ghost commented Oct 26, 2020

Trying merge.

@@ -146,6 +146,15 @@ pub fn keccak_256(data: &[u8]) -> [u8; 32] {
output
}

/// Do a keccak 512-bit hash and return result.
pub fn keccak_512(data: &[u8]) -> [u8; 64] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be exposed as a host function (sp_io)? If someone uses sp_core::hashing::keccak_512 it will compute the hash in the WASM instead.
CC @pepyakin

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, for getting native implementations one would need to put them under Hashing runtime interface in sp_io.

But this case is rather confusing, that implies that one needs to know to use sp_io instead of sp_core versions.

Copy link
Contributor

@tomusdrw tomusdrw Nov 12, 2020

Choose a reason for hiding this comment

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

Well if you look at other functions in sp_io::hashing host-side implementation it actually delegates to sp_core::hashing, so the entire file seems a bit confusing for me, cause it's in theory it should never be used in runtime directly, yet the crate compiles just fine and we jut rely on developers to use the correct stuff.

Copy link
Contributor

@tomusdrw tomusdrw Nov 12, 2020

Choose a reason for hiding this comment

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

Update: sp_core::hashing is gated under full-crypto, but I think it still deserves a bit more documentation.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants