add get_u32_range to impl VirtualMachine add get_u32 and get_u32_range to impl Memory #1936
Conversation
ohad-nir-starkware
requested review from
igaray,
Oppen,
fmoletta,
juanbono,
pefontana and
gabrielbosio
as code owners
ohad-nir-starkware
changed the base branch from
main
to
ohadn/opcode_extension
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
de7e16c to
4e1c5ed
Compare
|
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
4e1c5ed to
d220115
Compare
|
Benchmark Results for unmodified programs 🚀
|
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
d220115 to
dd1e261
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1936 +/- ##
=======================================
Coverage 96.36% 96.37%
=======================================
Files 102 102
Lines 41173 41238 +65
=======================================
+ Hits 39677 39742 +65
Misses 1496 1496 ☔ View full report in Codecov by Sentry. |
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
dd1e261 to
f355cc7
Compare
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @ohad-nir-starkware, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/vm_core.rs line 936 at r2 (raw file):
pub fn get_u32_range( &self, addr: &MaybeRelocatable,
the other functions in this file get Relocatable, why this is different?
ohad-nir-starkware
force-pushed
the
ohadn/opcode_extension
branch
from
6dbac44 to
e15c6e6
Compare
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
2 times, most recently
from
961435d to
a4b95e7
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/vm_core.rs line 936 at r2 (raw file):
Previously, DavidLevitGurevich wrote…
the other functions in this file get
Relocatable, why this is different?
refactored the code, now it gets Relocatable.
ohad-nir-starkware
changed the title
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
a4b95e7 to
7cc076a
Compare
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
7cc076a to
4bd3a02
Compare
vm/src/vm/vm_memory/memory.rs
Outdated
| let value = Cow::into_owned(self.get_integer(key)?); | ||
| let le_digits = value.to_le_digits(); | ||
| if le_digits[0] >= (1_u64 << 32) | ||
| || le_digits[1] != 0 | ||
| || le_digits[2] != 0 | ||
| || le_digits[3] != 0 | ||
| { | ||
| return Err(MemoryError::ExpectedU32(Box::new(key))); | ||
| } | ||
| Ok(le_digits[0] as u32) |
There was a problem hiding this comment.
Similarly to get_usize method, I think we could call felt.to_u32() here
vm/src/vm/errors/memory_errors.rs
Outdated
| #[error("Expected u32 valued at address {0} to be u32")] | ||
| ExpectedU32(Box<Relocatable>), |
There was a problem hiding this comment.
I think the following is a bit more clearer and consistent:
| #[error("Expected u32 valued at address {0} to be u32")] | |
| ExpectedU32(Box<Relocatable>), | |
| #[error("Expected u32 at address {0}")] | |
| ExpectedU32(Box<Relocatable>), |
There was a problem hiding this comment.
Considering your comment on felt.to_u32(), we might not need this variant of MemoryError
ohad-nir-starkware
force-pushed
the
ohadn/opcode_extension
branch
from
e15c6e6 to
1d1be8b
Compare
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
4bd3a02 to
41ff50b
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/errors/memory_errors.rs
Outdated
| #[error("Expected u32 valued at address {0} to be u32")] | ||
| ExpectedU32(Box<Relocatable>), |
There was a problem hiding this comment.
Considering your comment on felt.to_u32(), we might not need this variant of MemoryError
vm/src/vm/vm_memory/memory.rs
Outdated
| let value = Cow::into_owned(self.get_integer(key)?); | ||
| let le_digits = value.to_le_digits(); | ||
| if le_digits[0] >= (1_u64 << 32) | ||
| || le_digits[1] != 0 | ||
| || le_digits[2] != 0 | ||
| || le_digits[3] != 0 | ||
| { | ||
| return Err(MemoryError::ExpectedU32(Box::new(key))); | ||
| } | ||
| Ok(le_digits[0] as u32) |
There was a problem hiding this comment.
Reviewed 1 of 4 files at r3.
Reviewable status: 2 of 5 files reviewed, 6 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, and @pefontana)
vm/src/vm/vm_core.rs line 933 at r5 (raw file):
/// Gets n u32 values from memory starting from addr (n being size), pub fn get_u32_range(&self, addr: Relocatable, size: usize) -> Result<Vec<u32>, MemoryError> {
Add failure cases to docstring with tests for them.
Code quote:
pub fn get_u32_range(&self, addr: Relocatable, size: usize) -> Result<Vec<u32>, MemoryError>vm/src/vm/vm_core.rs line 4383 at r5 (raw file):
#[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn get_u32_range_ok() {
Add a test covering the error flow too.
Code quote:
get_u32_range_okvm/src/vm/vm_memory/memory.rs line 635 at r5 (raw file):
/// Gets a range of u32 memory values from addr to addr + size /// Fails if there if any of the values inside the range is missing (memory gap) or is not a u32
Add tests covering both of those failure scenarios.
Also, maybe add a test for the happy flow (though the one in the vm module pretty much tests this logic, so up to you).
Code quote:
Fails if there if any of the values inside the range is missing (memory gap) or is not a u32vm/src/vm/vm_memory/memory.rs line 641 at r5 (raw file):
for i in 0..size { values.push(self.get_u32((addr + i)?)?); }
Would something along the lines of:
(0..size) .map(|i| (addr + i).and_then(|a| self.get_u32(a))) .collect()
work instead? (prolly would also need to pay attention to the error from the addition).
Non-blocking.
Code quote:
let mut values = Vec::new();
for i in 0..size {
values.push(self.get_u32((addr + i)?)?);
}
There was a problem hiding this comment.
Reviewed 1 of 4 files at r3, all commit messages.
Reviewable status: 2 of 5 files reviewed, 7 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @ohad-nir-starkware, @Oppen, and @pefontana)
vm/src/vm/decoding/decoder.rs line 11 at r5 (raw file):
// ... 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 /// Decodes an instruction. The encoding is little endian, so flags go from bit 64 to 48.
63
Code quote:
64
vm/src/vm/decoding/decoder.rs
Outdated
| @@ -8,7 +8,7 @@ use crate::{ | |||
| // opcode_extension| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg | |||
| // ... 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 | |||
|
|
|||
| /// Decodes an instruction. The encoding is little endian, so flags go from bit 63 to 48. | |||
| /// Decodes an instruction. The encoding is little endian, so flags go from bit 64 to 48. | |||
There was a problem hiding this comment.
This change attempts to sync a change done in another PR (#1933). It would be better to have this change in that PR instead of this one.
| #[test] | ||
| #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] | ||
| fn get_u32_too_big() { | ||
| let mut segments = MemorySegmentManager::new(); | ||
| segments.add(); | ||
| segments | ||
| .memory | ||
| .insert(Relocatable::from((0, 0)), &Felt252::from(1_u64 << 32)) | ||
| .unwrap(); | ||
| assert_matches!( | ||
| segments.memory.get_u32(Relocatable::from((0, 0))), | ||
| Err(MemoryError::Math(MathError::Felt252ToU32Conversion( | ||
| bx | ||
| ))) if *bx == Felt252::from(1_u64 << 32) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please add more test cases:
- Happy paths
- Memory gap inside the range passed to
get_u32_range
There was a problem hiding this comment.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @fmoletta, @igaray, @juanbono, @JulianGCalderon, @ohad-nir-starkware, @Oppen, and @pefontana)
ohad-nir-starkware
force-pushed
the
ohadn/opcode_extension
branch
from
1d1be8b to
dfd3f66
Compare
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
41ff50b to
e5c96eb
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/vm_core.rs line 933 at r5 (raw file):
Previously, YairVaknin-starkware wrote…
Add failure cases to docstring with tests for them.
Done.
vm/src/vm/vm_core.rs line 4383 at r5 (raw file):
Previously, YairVaknin-starkware wrote…
Add a test covering the error flow too.
Done.
vm/src/vm/decoding/decoder.rs line 11 at r5 (raw file):
Previously, YairVaknin-starkware wrote…
63
Done.
vm/src/vm/vm_memory/memory.rs line 635 at r5 (raw file):
Previously, YairVaknin-starkware wrote…
Add tests covering both of those failure scenarios.
Also, maybe add a test for the happy flow (though the one in the vm module pretty much tests this logic, so up to you).
Done.
vm/src/vm/vm_memory/memory.rs line 641 at r5 (raw file):
Previously, YairVaknin-starkware wrote…
Would something along the lines of:
(0..size) .map(|i| (addr + i).and_then(|a| self.get_u32(a))) .collect()
work instead? (prolly would also need to pay attention to the error from the addition).Non-blocking.
not sure, but this is how it was done in get_integer_range, get_continuous_rangeand get_range
vm/src/vm/decoding/decoder.rs
Outdated
| @@ -8,7 +8,7 @@ use crate::{ | |||
| // opcode_extension| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg | |||
| // ... 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 | |||
|
|
|||
| /// Decodes an instruction. The encoding is little endian, so flags go from bit 63 to 48. | |||
| /// Decodes an instruction. The encoding is little endian, so flags go from bit 64 to 48. | |||
| #[test] | ||
| #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] | ||
| fn get_u32_too_big() { | ||
| let mut segments = MemorySegmentManager::new(); | ||
| segments.add(); | ||
| segments | ||
| .memory | ||
| .insert(Relocatable::from((0, 0)), &Felt252::from(1_u64 << 32)) | ||
| .unwrap(); | ||
| assert_matches!( | ||
| segments.memory.get_u32(Relocatable::from((0, 0))), | ||
| Err(MemoryError::Math(MathError::Felt252ToU32Conversion( | ||
| bx | ||
| ))) if *bx == Felt252::from(1_u64 << 32) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Reviewed 2 of 3 files at r6, all commit messages.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, and @pefontana)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r5.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, and @pefontana)
There was a problem hiding this comment.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, and @pefontana)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, and @pefontana)
ohad-nir-starkware
changed the base branch from
ohadn/opcode_extension
to
main
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
e5c96eb to
9c699c9
Compare
vm/src/vm/decoding/decoder.rs
Outdated
| @@ -42,7 +43,7 @@ pub fn decode_instruction(encoded_instr: u64) -> Result<Instruction, VirtualMach | |||
| let off2 = decode_offset(encoded_instr >> OFF2_OFF & OFFX_MASK); | |||
|
|
|||
| // Grab flags | |||
| let flags = encoded_instr >> FLAGS_OFFSET; | |||
| let flags = (encoded_instr >> FLAGS_OFFSET) & FLAGS_MASK; | |||
There was a problem hiding this comment.
Please revert this change
vm/src/vm/decoding/decoder.rs
Outdated
| @@ -31,6 +31,7 @@ pub fn decode_instruction(encoded_instr: u64) -> Result<Instruction, VirtualMach | |||
|
|
|||
| // Flags start on the 48th bit. | |||
| const FLAGS_OFFSET: u64 = 48; | |||
| const FLAGS_MASK: u64 = 0x7FFF; | |||
There was a problem hiding this comment.
Please revert this change
ohad-nir-starkware
force-pushed
the
ohadn/get_u32_range
branch
from
9c699c9 to
4598d76
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, and @pefontana)
vm/src/vm/decoding/decoder.rs
Outdated
| @@ -31,6 +31,7 @@ pub fn decode_instruction(encoded_instr: u64) -> Result<Instruction, VirtualMach | |||
|
|
|||
| // Flags start on the 48th bit. | |||
| const FLAGS_OFFSET: u64 = 48; | |||
| const FLAGS_MASK: u64 = 0x7FFF; | |||
vm/src/vm/decoding/decoder.rs
Outdated
| @@ -42,7 +43,7 @@ pub fn decode_instruction(encoded_instr: u64) -> Result<Instruction, VirtualMach | |||
| let off2 = decode_offset(encoded_instr >> OFF2_OFF & OFFX_MASK); | |||
|
|
|||
| // Grab flags | |||
| let flags = encoded_instr >> FLAGS_OFFSET; | |||
| let flags = (encoded_instr >> FLAGS_OFFSET) & FLAGS_MASK; | |||
There was a problem hiding this comment.
Reviewed 2 of 3 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions
add get_u32_range to impl VirtualMachine add get_u32 and get_u32_range to impl Memory
Description
The Blake opcode will use u32 values stored as Felt252s inside memory cells.
In preparation for the implementation of the opcode, I added the function add get_u32_range to impl VirtualMachine and added the functions get_u32 and get_u32_range to impl Memory.
get_u32 receives a Relocatable and returns Result<u32, MemoryError>.
If the input address points to a u32 sized integer
numit returns OK(num),otherwise it returns a MemoryError::ExpectedU32 error.
get_u32_range receives a starting address and a size and returns Result<Vec, MemoryError>.
If all the locations addr, addr+1, ... addr+size-1 contain u32 integers, it returns them as a vector inside OK.
If any of those addresses don't contain a u32 sized integer, it returns an error.
Checklist
This change is