Skip to content
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

Refactor verify_secure_runner #768

Merged
merged 50 commits into from
Jan 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
a7b6827
Start re-implementing BuiltinRunner::run_security_checks
fmoletta Jan 23, 2023
5373b21
Add method verify_auto_deductions_for_addr + finish re-implementation
fmoletta Jan 23, 2023
6f11822
Fix loop variable
fmoletta Jan 23, 2023
64ac435
Clippy
fmoletta Jan 23, 2023
40b3cec
Style
fmoletta Jan 23, 2023
35385f7
Fix math + add missing check
fmoletta Jan 23, 2023
59b421e
Fix logic
fmoletta Jan 23, 2023
b4e703e
Add test + repurpose test
fmoletta Jan 23, 2023
7d5e2c8
Fix test
fmoletta Jan 23, 2023
1d93388
Fix potential substraction with overflow
fmoletta Jan 23, 2023
261b428
Remove test inconsistencies
fmoletta Jan 23, 2023
0c2a12d
Use iterators for offset_max & offset_len
fmoletta Jan 23, 2023
290d83a
Use assumption for max_offset
fmoletta Jan 23, 2023
c33a2fc
Add misc tests
fmoletta Jan 23, 2023
d159160
Add more tests
fmoletta Jan 23, 2023
6d97641
Merge branch 'main' of github.com:lambdaclass/cairo-rs into clean-bui…
fmoletta Jan 23, 2023
96dc92c
More exhaustive clippy
fmoletta Jan 23, 2023
e58e3fd
Merge branch 'main' of github.com:lambdaclass/cairo-rs into clean-bui…
fmoletta Jan 23, 2023
9ea5006
Save initial progress
fmoletta Jan 23, 2023
bf79dbb
Save progress
fmoletta Jan 23, 2023
c2a5147
Return early ok if empty builtin segment
fmoletta Jan 24, 2023
f7fbf42
Fix comparison bounds
fmoletta Jan 24, 2023
6a33860
Merge branch 'clean-builtin-code' into refactor-verify-secure-runner
fmoletta Jan 24, 2023
d343038
Remove uneeded mut
fmoletta Jan 24, 2023
bdeb8bf
Merge branch 'clean-builtin-code' into refactor-verify-secure-runner
fmoletta Jan 24, 2023
61bc4c6
Add warning, fix comparison, add tests
fmoletta Jan 24, 2023
b0b1d61
Fix test
fmoletta Jan 24, 2023
6ccd5d4
Fix tests
fmoletta Jan 24, 2023
aeeb763
Quick solution for stop_ptr not being updated
fmoletta Jan 24, 2023
0472302
Fix error variant
fmoletta Jan 24, 2023
d5cc3db
Save progress
fmoletta Jan 24, 2023
b0220ad
Fix error varaint in test
fmoletta Jan 24, 2023
5c31f2c
Fix test
fmoletta Jan 24, 2023
88316fb
Add temporary addresses check
fmoletta Jan 24, 2023
66f0cc6
Add tests for temporary addresses check
fmoletta Jan 24, 2023
77c6604
Remove previous implementation
fmoletta Jan 24, 2023
5fbd565
Simplify segment info + clippy
fmoletta Jan 24, 2023
d931b1e
Style
fmoletta Jan 24, 2023
51607a1
Merge branch 'main' of github.com:lambdaclass/cairo-rs into refactor-…
fmoletta Jan 24, 2023
1dcae18
Fix
fmoletta Jan 24, 2023
390ba6a
Fix error strings
fmoletta Jan 24, 2023
ad7ea94
Add comment
fmoletta Jan 24, 2023
735aa54
Add changelog entry
fmoletta Jan 24, 2023
7c22341
Add tests for read_return_values
fmoletta Jan 24, 2023
dd92a4c
Clippy tests
fmoletta Jan 24, 2023
b0bc964
Update CHANGELOG.md
fmoletta Jan 24, 2023
6ac05b3
Change return of get_segment_info to vec
fmoletta Jan 25, 2023
5f7eb38
Merge branch 'refactor-verify-secure-runner' of github.com:lambdaclas…
fmoletta Jan 25, 2023
14b6d3a
Fix typos
fmoletta Jan 25, 2023
d4396b8
Add FIXME comment
fmoletta Jan 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

#### Upcoming Changes

* Refactor `Refactor verify_secure_runner` [#768](https://github.com/lambdaclass/cairo-rs/pull/768)
Public Api changes:
* Remove builtin name from the return value of `BuiltinRunner::get_memory_segment_addresses`
* Simplify the return value of `CairoRunner::get_builtin_segments_info` to `Vec<(usize, usize)>`
* CairoRunner::read_return_values now receives a mutable reference to VirtualMachine
Bugfixes:
* CairoRunner::read_return_values now updates the `stop_ptr` of each builtin after calling `BuiltinRunner::final_stack`
Oppen marked this conversation as resolved.
Show resolved Hide resolved

* Use CairoArg enum instead of Any in CairoRunner::run_from_entrypoint [#686](https://github.com/lambdaclass/cairo-rs/pull/686)
* Public Api changes:
* Remove `Result` from `MaybeRelocatable::mod_floor`, it now returns a `MaybeRelocatable`
Expand Down
2 changes: 1 addition & 1 deletion src/cairo_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub fn cairo_run(

vm.verify_auto_deductions()?;
if proof_mode {
cairo_runner.read_return_values(&vm)?;
cairo_runner.read_return_values(&mut vm)?;
cairo_runner.finalize_segments(&mut vm)?;
}
cairo_runner.relocate(&mut vm)?;
Expand Down
6 changes: 5 additions & 1 deletion src/vm/errors/runner_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,10 @@ pub enum RunnerError {
InvalidLayoutName(String),
#[error("Run has already ended.")]
RunAlreadyFinished,
#[error("Run must be ended before calling finalize_segments.")]
#[error("end_run must be called before finalize_segments.")]
FinalizeNoEndRun,
#[error("end_run must be called before read_return_values.")]
ReadReturnValuesNoEndRun,
#[error("Builtin {0} not included.")]
BuiltinNotIncluded(String),
#[error("Builtin segment name collision on '{0}'")]
Expand Down Expand Up @@ -95,4 +97,6 @@ pub enum RunnerError {
SafeDivFailUsize(usize, usize),
#[error(transparent)]
MemoryError(#[from] MemoryError),
#[error("Negative builtin base")]
NegBuiltinBase,
}
6 changes: 6 additions & 0 deletions src/vm/errors/vm_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,12 @@ pub enum VirtualMachineError {
Hint(usize, Box<HintError>),
#[error("Unexpected Failure")]
Unexpected,
#[error("Out of bounds access to builtin segment")]
OutOfBoundsBuiltinSegmentAccess,
#[error("Out of bounds access to program segment")]
OutOfBoundsProgramSegmentAccess,
#[error("Negative builtin base")]
NegBuiltinBase,
#[error("Security Error: Invalid Memory Value: temporary address not relocated: {0}")]
InvalidMemoryValueTemporaryAddress(Relocatable),
}
9 changes: 3 additions & 6 deletions src/vm/runners/builtin_runner/bitwise.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ impl BitwiseBuiltinRunner {
Ok(self.cells_per_instance as usize * value)
}

pub fn get_memory_segment_addresses(&self) -> (&'static str, (isize, Option<usize>)) {
("bitwise", (self.base, self.stop_ptr))
pub fn get_memory_segment_addresses(&self) -> (isize, Option<usize>) {
(self.base, self.stop_ptr)
}

pub fn get_used_cells(&self, vm: &VirtualMachine) -> Result<usize, MemoryError> {
Expand Down Expand Up @@ -466,10 +466,7 @@ mod tests {
fn get_memory_segment_addresses() {
let builtin = BitwiseBuiltinRunner::new(&BitwiseInstanceDef::default(), true);

assert_eq!(
builtin.get_memory_segment_addresses(),
("bitwise", (0, None)),
);
assert_eq!(builtin.get_memory_segment_addresses(), (0, None),);
}

#[test]
Expand Down
6 changes: 3 additions & 3 deletions src/vm/runners/builtin_runner/ec_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,8 @@ impl EcOpBuiltinRunner {
Ok(self.cells_per_instance as usize * value)
}

pub fn get_memory_segment_addresses(&self) -> (&'static str, (isize, Option<usize>)) {
("ec_op", (self.base, self.stop_ptr))
pub fn get_memory_segment_addresses(&self) -> (isize, Option<usize>) {
(self.base, self.stop_ptr)
}

pub fn get_used_cells(&self, vm: &VirtualMachine) -> Result<usize, MemoryError> {
Expand Down Expand Up @@ -944,7 +944,7 @@ mod tests {
fn get_memory_segment_addresses() {
let builtin = EcOpBuiltinRunner::new(&EcOpInstanceDef::default(), true);

assert_eq!(builtin.get_memory_segment_addresses(), ("ec_op", (0, None)));
assert_eq!(builtin.get_memory_segment_addresses(), (0, None));
}

#[test]
Expand Down
9 changes: 3 additions & 6 deletions src/vm/runners/builtin_runner/hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ impl HashBuiltinRunner {
Ok(self.cells_per_instance as usize * value)
}

pub fn get_memory_segment_addresses(&self) -> (&'static str, (isize, Option<usize>)) {
("pedersen", (self.base, self.stop_ptr))
pub fn get_memory_segment_addresses(&self) -> (isize, Option<usize>) {
(self.base, self.stop_ptr)
}

pub fn get_used_cells(&self, vm: &VirtualMachine) -> Result<usize, MemoryError> {
Expand Down Expand Up @@ -457,10 +457,7 @@ mod tests {
fn get_memory_segment_addresses() {
let builtin = HashBuiltinRunner::new(256, true);

assert_eq!(
builtin.get_memory_segment_addresses(),
("pedersen", (0, None)),
);
assert_eq!(builtin.get_memory_segment_addresses(), (0, None),);
}

#[test]
Expand Down
9 changes: 3 additions & 6 deletions src/vm/runners/builtin_runner/keccak.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ impl KeccakBuiltinRunner {
Ok(self.cells_per_instance as usize * value)
}

pub fn get_memory_segment_addresses(&self) -> (&'static str, (isize, Option<usize>)) {
("keccak", (self.base, self.stop_ptr))
pub fn get_memory_segment_addresses(&self) -> (isize, Option<usize>) {
(self.base, self.stop_ptr)
}

pub fn get_used_cells(&self, vm: &VirtualMachine) -> Result<usize, MemoryError> {
Expand Down Expand Up @@ -452,10 +452,7 @@ mod tests {
fn get_memory_segment_addresses() {
let builtin = KeccakBuiltinRunner::new(&KeccakInstanceDef::default(), true);

assert_eq!(
builtin.get_memory_segment_addresses(),
("keccak", (0, None))
);
assert_eq!(builtin.get_memory_segment_addresses(), (0, None));
}

#[test]
Expand Down
30 changes: 10 additions & 20 deletions src/vm/runners/builtin_runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ impl BuiltinRunner {
}
}

// Important note: the second returned value corresponds to the builtin's stop_ptr, which must be updated after calling this method
// It is not updated inside this method due to mutability problems
pub fn final_stack(
&self,
vm: &VirtualMachine,
Expand Down Expand Up @@ -188,7 +190,7 @@ impl BuiltinRunner {
Ok((0..segment_size).map(|i| (base, i).into()).collect())
}

pub fn get_memory_segment_addresses(&self) -> (&'static str, (isize, Option<usize>)) {
pub fn get_memory_segment_addresses(&self) -> (isize, Option<usize>) {
match self {
BuiltinRunner::Bitwise(ref bitwise) => bitwise.get_memory_segment_addresses(),
BuiltinRunner::EcOp(ref ec) => ec.get_memory_segment_addresses(),
Expand Down Expand Up @@ -977,31 +979,19 @@ mod tests {
fn get_memory_segment_addresses_test() {
let bitwise_builtin: BuiltinRunner =
BitwiseBuiltinRunner::new(&BitwiseInstanceDef::default(), true).into();
assert_eq!(
bitwise_builtin.get_memory_segment_addresses(),
("bitwise", (0, None)),
);
assert_eq!(bitwise_builtin.get_memory_segment_addresses(), (0, None),);
let ec_op_builtin: BuiltinRunner =
EcOpBuiltinRunner::new(&EcOpInstanceDef::default(), true).into();
assert_eq!(
ec_op_builtin.get_memory_segment_addresses(),
("ec_op", (0, None)),
);
assert_eq!(ec_op_builtin.get_memory_segment_addresses(), (0, None),);
let hash_builtin: BuiltinRunner = HashBuiltinRunner::new(8, true).into();
assert_eq!(
hash_builtin.get_memory_segment_addresses(),
("pedersen", (0, None)),
);
assert_eq!(hash_builtin.get_memory_segment_addresses(), (0, None),);
let output_builtin: BuiltinRunner = OutputBuiltinRunner::new(true).into();
assert_eq!(
output_builtin.get_memory_segment_addresses(),
("output", (0, None)),
);
assert_eq!(output_builtin.get_memory_segment_addresses(), (0, None),);
let range_check_builtin: BuiltinRunner =
BuiltinRunner::RangeCheck(RangeCheckBuiltinRunner::new(8, 8, true));
assert_eq!(
range_check_builtin.get_memory_segment_addresses(),
("range_check", (0, None)),
(0, None),
);
}

Expand All @@ -1020,7 +1010,7 @@ mod tests {
true,
));
let vm = vm!();
// Unsed builtin shouldnt fail security checks
// Unused builtin shouldn't fail security checks
assert_eq!(builtin.run_security_checks(&vm), Ok(()),);
}

Expand Down Expand Up @@ -1488,7 +1478,7 @@ mod tests {

for mut br in builtins {
br.set_stop_ptr(ptr);
let (_, (_, stop_ptr)) = br.get_memory_segment_addresses();
let (_, stop_ptr) = br.get_memory_segment_addresses();
assert_eq!(stop_ptr, Some(ptr));
}
}
Expand Down
11 changes: 4 additions & 7 deletions src/vm/runners/builtin_runner/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ impl OutputBuiltinRunner {
Ok(0)
}

pub fn get_memory_segment_addresses(&self) -> (&'static str, (isize, Option<usize>)) {
("output", (self.base, self.stop_ptr))
pub fn get_memory_segment_addresses(&self) -> (isize, Option<usize>) {
(self.base, self.stop_ptr)
}

pub fn get_used_cells(&self, vm: &VirtualMachine) -> Result<usize, MemoryError> {
Expand Down Expand Up @@ -93,7 +93,7 @@ impl OutputBuiltinRunner {
vm.get_relocatable(&(pointer.sub_usize(1)).map_err(|_| RunnerError::FinalStack)?)
{
if self.base() != stop_pointer.segment_index {
return Err(RunnerError::InvalidStopPointer("range_check".to_string()));
return Err(RunnerError::InvalidStopPointer("output".to_string()));
}
let stop_ptr = stop_pointer.offset;
let used = self
Expand Down Expand Up @@ -292,10 +292,7 @@ mod tests {
fn get_memory_segment_addresses() {
let builtin = OutputBuiltinRunner::new(true);

assert_eq!(
builtin.get_memory_segment_addresses(),
("output", (0, None)),
);
assert_eq!(builtin.get_memory_segment_addresses(), (0, None),);
}

#[test]
Expand Down
9 changes: 3 additions & 6 deletions src/vm/runners/builtin_runner/range_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ impl RangeCheckBuiltinRunner {
Ok(self.cells_per_instance as usize * value)
}

pub fn get_memory_segment_addresses(&self) -> (&'static str, (isize, Option<usize>)) {
("range_check", (self.base, self.stop_ptr))
pub fn get_memory_segment_addresses(&self) -> (isize, Option<usize>) {
(self.base, self.stop_ptr)
}

pub fn get_used_cells(&self, vm: &VirtualMachine) -> Result<usize, MemoryError> {
Expand Down Expand Up @@ -473,10 +473,7 @@ mod tests {
fn get_memory_segment_addresses() {
let builtin = RangeCheckBuiltinRunner::new(8, 8, true);

assert_eq!(
builtin.get_memory_segment_addresses(),
("range_check", (0, None)),
);
assert_eq!(builtin.get_memory_segment_addresses(), (0, None),);
}

#[test]
Expand Down
6 changes: 3 additions & 3 deletions src/vm/runners/builtin_runner/signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ impl SignatureBuiltinRunner {
Ok(self.cells_per_instance as usize * value)
}

pub fn get_memory_segment_addresses(&self) -> (&'static str, (isize, Option<usize>)) {
("ecdsa", (self.base, self.stop_ptr))
pub fn get_memory_segment_addresses(&self) -> (isize, Option<usize>) {
(self.base, self.stop_ptr)
}

pub fn get_used_cells(&self, vm: &VirtualMachine) -> Result<usize, MemoryError> {
Expand Down Expand Up @@ -378,7 +378,7 @@ mod tests {
fn get_memory_segment_addresses() {
let builtin = SignatureBuiltinRunner::new(&EcdsaInstanceDef::default(), true);

assert_eq!(builtin.get_memory_segment_addresses(), ("ecdsa", (0, None)));
assert_eq!(builtin.get_memory_segment_addresses(), (0, None));
}

#[test]
Expand Down
Loading