feat: implement MPT part of generic extraction (Part 1) #385
Conversation
…ntifiers` to `&[u64]`.
…mputation functions.
mp2-common/src/storage_key.rs
Outdated
| ) -> KeccakMPTWires { | ||
| // location = keccak(inputs) + offset | ||
| let keccak_base = KeccakCircuit::<{ INPUT_PADDED_LEN }>::hash_to_bytes(b, &inputs); | ||
| let base = keccak_base.output.arr.pack(b, Endianness::Big); |
There was a problem hiding this comment.
Just noticed that hash_to_bytes decompose the output of Keccak in little-endian, while here you are packing it again in big-endian. Wouldn't be better to use hash_vector, which returns the output as u32 limbs which can be directly be employed to initialize a UInt256Target?
There was a problem hiding this comment.
I tried the hash_vector, seems its result is little-endian packed u32s, but we need to initialize Uint256Target with big-endian u32s. So I keep the hash_to_bytes (then pack to big-endian) in the updating commit e5c21d1. Please correct me if I was wrong, thanks.
There was a problem hiding this comment.
It really depends how EVM is interpreting the output of Keccak as a U256 to add offset. I think your current way of initializing the UInt256Target makes sense, but maybe a range-check on each byte of the hash_to_bytes output is needed, as hash_to_bytes should be safe to use without range-checking the output bytes only if those bytes are then fed as input to another Keccak, which is not the case here. Wdyt?
There was a problem hiding this comment.
Yes, I see, I added this byte range check in commit 3548cc5.
mp2-common/src/storage_key.rs
Outdated
| // Unwrap safe because input always fixed 32 bytes. | ||
| &InputData::Assigned(&Vector::from_vec(&inputs).unwrap()), | ||
| ); | ||
| let location_bytes = inputs |
There was a problem hiding this comment.
Up to here most of the code looks like the same as assign_single. Isn't is possible to refactor it in a common method to avoid code duplication?
There was a problem hiding this comment.
I refactor this code in commit aea77c6.
mp2-common/src/storage_key.rs
Outdated
| @@ -515,7 +619,7 @@ impl MappingSlot { | |||
| let inputs = mapping_key.into_iter().chain(padded_slot).collect_vec(); | |||
| // Then compute the expected resulting hash for MPT key derivation. | |||
| let base = keccak256(&inputs).try_into().unwrap(); | |||
| KeccakMPT::assign(pw, &wires.keccak_mpt, inputs, base, offset); | |||
| KeccakStructMPT::assign(pw, &wires.keccak_mpt, inputs, base, offset); | |||
There was a problem hiding this comment.
Same as above: most of the code between assign_simple and assign_struct looks the same, wouldn't it be worthy to refactor the methods to avoid code duplication?
There was a problem hiding this comment.
I refactor this code in commit aea77c6.
mp2-common/src/storage_key.rs
Outdated
| ) -> KeccakMPTWires { | ||
| // location = keccak(inputs) + offset | ||
| let keccak_base = KeccakCircuit::<{ INPUT_PADDED_LEN }>::hash_to_bytes(b, &inputs); | ||
| let base = keccak_base.output.arr.pack(b, Endianness::Big); |
There was a problem hiding this comment.
It really depends how EVM is interpreting the output of Keccak as a U256 to add offset. I think your current way of initializing the UInt256Target makes sense, but maybe a range-check on each byte of the hash_to_bytes output is needed, as hash_to_bytes should be safe to use without range-checking the output bytes only if those bytes are then fed as input to another Keccak, which is not the case here. Wdyt?
silathdiir
changed the title
…ic-mpt-extraction
Related issue https://github.com/Lagrange-Labs/zkmr-tracking/issues/265
Design document MPT Extraction
Summary
ColumnGadgetandMetadataGadget.