feat: implement 0x20 - SHA3 Opcode #281
Conversation
|
Snapshot Comparison Report: No changes in gas consumption. |
|
Used Default::default() as array initiator. |
There was a problem hiding this comment.
Here's some modifications I would like to see:
- Don't explicitly call
memory.ensure_lengthin a method that computessha3. This is extremely dangerous and bug-prone. "internal" memory functions are meant to use internally, use public methods instead, likeload. - Instead of looping and padding in each iteration when the offset read is bigger than the (initial) memory size, we can use modulo operations to compute how many zeroes must be padded, and then create a
pad_leftfunction (similar topad_right) to add the required padding in one place - Reverting a word's endianness, and storing them in the array to hash should be in a utility function that doesn't take
memoryas parameter - Handling of the last input should be in a standalone function.
- Ideally we would first load all words, then apply transformations and hash them. In Cairo this might end up being more expensive, so we can do it progressively as we prepare the array of elements to be hashed
|
I have pushed a new version of the sha3 algorithm hoping it is clearer and better structured. ( There is a significant improvement in gas cost concerning big sized test 6993735300 -> 1703355056 for the bigger one. ) Indeed, it appears that the memory expansion gas cost calculation makes one of my test fail with what firstly appears to be an infinite loop that ends up with a With comments : Without comments : |
There was a problem hiding this comment.
There is still some refactoring to be done on how the function is constructed internally, otherwise it's a good work!
Here's an example of a keccak256 function taking an array of bytes as an input. As you can see it's around 12 lines long, and the intent by each line can be directly understood, even without having functions implementations available. This is ideally what you should aim for when writing code
Of course it's not applicable to every situation, but let's try to do our best
fn cairo_keccak_le_bytes(mut self: Array<u8>) -> u256 {
let mut words64: Array<u64> = array![];
let mut self = self.span();
let (last_word, last_word_bytes) = loop {
// Specifically handle last word
if self.len() < 8 {
let (value, n_bytes) = U64Trait::from_le_bytes(self);
break (value, n_bytes);
};
let mut current_word = self.slice(0, 8);
let (value, n_bytes) = U64Trait::from_le_bytes(current_word);
words64.append(value);
self = self.slice(8, self.len() - 8);
};
let mut hash = cairo_keccak(ref words64, last_word, last_word_bytes);
reverse_endianness(hash)
}
fn reverse_endianness(value: u256) -> u256 {
let new_low = integer::u128_byte_reverse(value.high);
let new_high = integer::u128_byte_reverse(value.low);
u256 { low: new_low, high: new_high }
}
I tried locally and I didn't have this problem! |
Ok that is even more weird. I tried it again and it gives me the same results. Here is my scarb --version output : Otherwise, could it be because I'm on WSL ? Would you have any other ideas ? |
There's a chance it's because of this. I'm running on macOS |
|
@ClementWalter @Eikix good for a final review |
I have taken tests found here : https://github.com/ethereum/go-ethereum/blob/master/tests/solidity/test/opCodes.js Beside tests with size or offset > u64.maxvalue, I've taken all I could, some are even e bit repetitve and I'm not sure they are really relevant in our case but I still added them. |
|
Ensure memory length to be set even if we skipped reading for zeroes. |
|
@Quentash can you add a test that checks the memory length when we read out of bound bytes? To make sure that the length is correctly updated. |
Of course, my bad !! |
|
Added memory length verification to tests. |
|
🥳 |
Implementation of SHA3 opcode (0x20).
The code has been run against official ethereum tests found at https://github.com/ethereum/tests/blob/develop/src/GeneralStateTestsFiller/VMTests/vmTests/sha3Filler.yml#L52
Only a few tests couldn't be run because the actual
memory.bytes_lenis a u32 and some tested value are u64, It has been discussed with @Eikix to leave it as it is for now and eventually refactor it later to handle u64 memory size.I have added comments in the code ( to eventually remove later ) to explain how the code bypasses continuous memory allocation when trying to access a memory slot beyond
memory.bytes_lenand will instead ensure that the memory is allocated at the end of the process, this seems way more efficient since some tests will fail otherwise.Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Resolves: #115
What is the new behavior?
Does this introduce a breaking change?