-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat(levm): implement precompile blake2f #1563
Conversation
Have you tried using a rust crate like Blake2. Couldn't that get some of the complexity out of the way here? |
I feel like some of the errors propagated should actually be internal errors if they are not intended to happen independently of the input received. |
I tried to use related crates, but I couldn't find one that does what we specifically need. The functionality is similar to the functionalities that are related to the Blake2 algorithm, but this one is more configurable than the others, since it receives parameters that the others do not. |
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 think that comments can be added for more clarity.
Can we abstract the logic for parsing the input into one function? (If you think it is worth it, otherwise we can leave comments but it will be easier to read if we do)
return Err(VMError::PrecompileError(PrecompileError::ParsingInputError)); | ||
} | ||
|
||
let rounds: U256 = calldata |
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.
why U256
for a 4 byte value?
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.
It's an intermediate size to then move on to a usize, I couldn't find a prettier way to do it.
#[test] | ||
fn blake2f_test() { |
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.
Maxi, what do you think about adding a source for these tests? Just in the case in the future it's easier to find them
Something like:
#[test] | |
fn blake2f_test() { | |
#[test] | |
// Source: https://eips.ethereum.org/EIPS/eip-152#test-vector-1 | |
fn blake2f_test() { |
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.
Done in Add source of first test
#[test] | ||
fn blake2f_test_2() { |
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.
Ditto
#[test] | |
fn blake2f_test_2() { | |
#[test] | |
// Source: https://eips.ethereum.org/EIPS/eip-152#test-vector-2 | |
fn blake2f_test_2() { |
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.
"Done" in Add source of second test
ret[a] = v[a].wrapping_add(v[b]).wrapping_add(x); | ||
ret[d] = (ret[d] ^ ret[a]).rotate_right(R1); | ||
ret[c] = ret[c].wrapping_add(ret[d]); | ||
ret[b] = (ret[b] ^ ret[c]).rotate_right(R2); | ||
ret[a] = ret[a].wrapping_add(ret[b]).wrapping_add(y); | ||
ret[d] = (ret[d] ^ ret[a]).rotate_right(R3); | ||
ret[c] = ret[c].wrapping_add(ret[d]); | ||
ret[b] = (ret[b] ^ ret[c]).rotate_right(R4); |
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.
@maximopalopoli Is it possible that you are not applying the modulus operator here?
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.
crates/vm/levm/src/precompiles.rs
Outdated
let mut m = [0; 16]; | ||
for i in 0..8_usize { |
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.
Don't we have to iterate up to 16 here?
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.
@maximopalopoli I think this is it!
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.
Done in Fix times iterating when reading m!
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.
good job! it must have been hard to implement this
Motivation
The goal is to implement the blake2f precompile.
Description
The blake compressing functionality is based on RFC 7693.
There are some related EF tests not passing, I left a commented test that simulate one of those cases. The test cases that do not pass are from
stPreCompiledContracts/blake2B.json
.