-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[crypto+move] add support for Blake2b-256 in Move #5436
[crypto+move] add support for Blake2b-256 in Move #5436
Conversation
Thank you for the PR @blasrodri! There are two flavors of Blake: Blake2b, Blake2s (see https://www.blake2.net/). Can you please update the PR title and all of the code (e.g., function names, gas parameter names, comments, etc.) to clearly indicate which one you are implementing? |
Thank you for the update @blasrodri! Can you include in the PR's description the performance impact of going from |
Oh it's just because i checked the Substrate implemention and it uses blake2_rfc. But ill do a quick benchmark, and post the results. So that we can pick the better one |
use blake2::{
digest::{Update, VariableOutput},
Blake2bVar,
};
use criterion::{black_box, criterion_group, criterion_main, Criterion};
fn blake2_blake2b_256(data: &[u8]) -> Vec<u8> {
// Blake2bVarCore::new_with_params(salt, persona, key_size, output_size)
let mut hasher = Blake2bVar::new(32).unwrap();
hasher.update(data);
let mut buf = vec![0u8; 32];
hasher.finalize_variable(&mut buf).unwrap();
buf
}
fn blake2_rfc_blake2b_256(data: &[u8]) -> Vec<u8> {
blake2_blake2b_256(data).to_vec()
}
fn criterion_benchmark(c: &mut Criterion) {
assert_eq!(
blake2_blake2b_256(b"testing"),
blake2_rfc_blake2b_256(b"testing")
);
c.bench_function("blake2_blake2b_256", |b| {
b.iter(|| blake2_blake2b_256(black_box(b"testing")))
});
c.bench_function("blake2_rfc_blake2b_256", |b| {
b.iter(|| blake2_rfc_blake2b_256(black_box(b"testing")))
});
}
criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);
|
Thank you @blasrodri! Let's stick to Could you please add your benchmarks to this hash.rs file in This will help me estimate gas costs more accurately. |
I made a mistake on the previous script. Now, I've added the two implementations on the
|
Sweet! I forgot to ask the most obvious question: are these numbers for |
Yes ^_^
|
/// Whether the new BLAKE2B-256 hash function natives is enabled. | ||
/// This is needed because of the introduction of new native functions. | ||
/// Lifetime: transient | ||
const BLAKE2B_256_NATIVE: u64 = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads up that this might need to be further bumped up given recent changes. We'll have to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no issues here.
Another request: could you re-run benchmarks both for SHA2-256 and your newly-added Blake2b function and post the results here? This will allow us to calibrate the gas costs a little better. The SHA2-256 ones are also in |
Sure! Will do it today |
SHA2
BLAKE2B
|
Signed-off-by: Blas Rodriguez Irizar <[email protected]>
Signed-off-by: Blas Rodriguez Irizar <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Please rebase and fix on your end. I can potentially help, but I think there might be some gnarly changes. |
I'm coordinating with Blas on re-basing and merging. No worries @davidiw. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
This reverts commit de212ad.
Taken a lot of inspiration from #4181 (comment)
Description
Add Blake2b-256 hashing in Move, to support folks writing bridge contracts to other platforms, as per conversations with @JoshLind and @alinush.
Test Plan
This PR merely exports an existing hash function crate (i.e.:
blake2_rfc
), which are is believed to be trustworthy implementation as it's used in Substrate (https://crates.io/crates/sp-core-hashing/4.0.0/dependencies). Therefore, we only made sure the native implementation give the expected hash values forb""
,b"testing"
andb"testing again"
.This change is