Skip to content

Commit

Permalink
Fix SPLIT_FELT hint (#1387)
Browse files Browse the repository at this point in the history 
* Fix tests + add more

* Add integration test

* Fix test

* Add changelog entry + format cairo program

* fmt

* Fix test

* Clippy

* clippy
  • Loading branch information
@fmoletta
fmoletta authored Aug 22, 2023
1 parent 23aaa2a commit 132da2c
Show file tree
Hide file tree
Showing 5 changed files with 223 additions and 6 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#### [0.8.6] - 2023-8-11

* fix: Fix `SPLIT_FELT` hint [#1387](https://github.com/lambdaclass/cairo-vm/pull/1387)

* fix: Fix div_mod [#1383](https://github.com/lambdaclass/cairo-vm/pull/1383)

* Fixes `div_mod` function so that it behaves like the cairo-lang version
Expand Down
22 changes: 22 additions & 0 deletions cairo_programs/bad_programs/split_felt_bad_constants.cairo
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
const MAX_HIGH = 1;
const MAX_LOW = 1;

func main() {
let value =1;
hint_func(MAX_HIGH, MAX_LOW, value);
return();
}
func hint_func(MAX_HIGH: felt, MAX_LOW: felt, value: felt) -> (felt, felt) {
alloc_locals;
local low: felt;
local high: felt;
%{
from starkware.cairo.common.math_utils import assert_integer
assert ids.MAX_HIGH < 2**128 and ids.MAX_LOW < 2**128
assert PRIME - 1 == ids.MAX_HIGH * 2**128 + ids.MAX_LOW
assert_integer(ids.value)
ids.low = ids.value & ((1 << 128) - 1)
ids.high = ids.value >> 128
%}
return(low, high);
}
4 changes: 3 additions & 1 deletion vm/src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,9 @@ impl HintProcessorLogic for BuiltinHintProcessor {
&hint_data.ap_tracking,
"continue_loop",
),
hint_code::SPLIT_FELT => split_felt(vm, &hint_data.ids_data, &hint_data.ap_tracking),
hint_code::SPLIT_FELT => {
split_felt(vm, &hint_data.ids_data, &hint_data.ap_tracking, constants)
}
hint_code::UNSIGNED_DIV_REM => {
unsigned_div_rem(vm, &hint_data.ids_data, &hint_data.ap_tracking)
}
Expand Down
192 changes: 187 additions & 5 deletions vm/src/hint_processor/builtin_hint_processor/math_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,24 @@ pub fn split_felt(
vm: &mut VirtualMachine,
ids_data: &HashMap<String, HintReference>,
ap_tracking: &ApTracking,
constants: &HashMap<String, Felt252>,
) -> Result<(), HintError> {
let max_high = get_constant_from_var_name("MAX_HIGH", constants)?;
let max_low = get_constant_from_var_name("MAX_LOW", constants)?;
if max_high.bits() > 128 || max_low.bits() > 128 {
return Err(HintError::AssertionFailed(
"assert ids.MAX_HIGH < 2**128 and ids.MAX_LOW < 2**128"
.to_string()
.into_boxed_str(),
));
}
if Felt252::from(-1) != max_high * &Felt252::one().shl(128_u32) + max_low {
return Err(HintError::AssertionFailed(
"assert PRIME - 1 == ids.MAX_HIGH * 2**128 + ids.MAX_LOW"
.to_string()
.into_boxed_str(),
));
}
let value = get_integer_from_var_name("value", vm, ids_data, ap_tracking)?;
let value = value.as_ref();
//Main logic
Expand Down Expand Up @@ -2096,7 +2113,22 @@ mod tests {
("high".to_string(), HintReference::new(-3, 1, true, true)),
]);
//Execute the hint
assert_matches!(run_hint!(vm, ids_data, hint_code), Ok(()));
assert_matches!(
run_hint!(
vm,
ids_data,
hint_code,
exec_scopes_ref!(),
&HashMap::from([
("MAX_LOW".to_string(), Felt252::zero()),
(
"MAX_HIGH".to_string(),
felt_str!("10633823966279327296825105735305134080")
)
])
),
Ok(())
);
//Check hint memory inserts
check_memory![
vm.segments.memory,
Expand All @@ -2122,7 +2154,19 @@ mod tests {
let ids_data = ids_data!["low"];
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
run_hint!(
vm,
ids_data,
hint_code,
exec_scopes_ref!(),
&HashMap::from([
("MAX_LOW".to_string(), Felt252::zero()),
(
"MAX_HIGH".to_string(),
felt_str!("10633823966279327296825105735305134080")
)
])
),
Err(HintError::UnknownIdentifier(bx)) if bx.as_ref() == "value"
);
}
Expand All @@ -2149,7 +2193,19 @@ mod tests {

//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
run_hint!(
vm,
ids_data,
hint_code,
exec_scopes_ref!(),
&HashMap::from([
("MAX_LOW".to_string(), Felt252::zero()),
(
"MAX_HIGH".to_string(),
felt_str!("10633823966279327296825105735305134080")
)
])
),
Err(HintError::Memory(
MemoryError::InconsistentMemory(bx)
)) if *bx == (Relocatable::from((2, 0)),
Expand Down Expand Up @@ -2180,7 +2236,19 @@ mod tests {
]);
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
run_hint!(
vm,
ids_data,
hint_code,
exec_scopes_ref!(),
&HashMap::from([
("MAX_LOW".to_string(), Felt252::zero()),
(
"MAX_HIGH".to_string(),
felt_str!("10633823966279327296825105735305134080")
)
])
),
Err(HintError::Memory(
MemoryError::InconsistentMemory(bx)
)) if *bx == (Relocatable::from((2, 1)),
Expand All @@ -2206,11 +2274,125 @@ mod tests {
]);
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
run_hint!(
vm,
ids_data,
hint_code,
exec_scopes_ref!(),
&HashMap::from([
("MAX_LOW".to_string(), Felt252::zero()),
(
"MAX_HIGH".to_string(),
felt_str!("10633823966279327296825105735305134080")
)
])
),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("value".to_string(), (1,3).into())
);
}

#[test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
fn run_split_felt_no_constants() {
let hint_code =
"from starkware.cairo.common.math_utils import assert_integer\nassert ids.MAX_HIGH < 2**128 and ids.MAX_LOW < 2**128\nassert PRIME - 1 == ids.MAX_HIGH * 2**128 + ids.MAX_LOW\nassert_integer(ids.value)\nids.low = ids.value & ((1 << 128) - 1)\nids.high = ids.value >> 128";
let mut vm = vm_with_range_check!();
vm.segments = segments![
((1, 3), ("335438970432432812899076431678123043273", 10)),
((1, 4), (2, 0))
];
add_segments!(vm, 1);
//Initialize fp
vm.run_context.fp = 7;
//Create ids
let ids_data = HashMap::from([
("value".to_string(), HintReference::new_simple(-4)),
("low".to_string(), HintReference::new(-3, 0, true, true)),
("high".to_string(), HintReference::new(-3, 1, true, true)),
]);
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::MissingConstant(x)) if (*x) == "MAX_HIGH"
);
}

#[test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
fn run_split_felt_constants_over_128_bits() {
let hint_code =
"from starkware.cairo.common.math_utils import assert_integer\nassert ids.MAX_HIGH < 2**128 and ids.MAX_LOW < 2**128\nassert PRIME - 1 == ids.MAX_HIGH * 2**128 + ids.MAX_LOW\nassert_integer(ids.value)\nids.low = ids.value & ((1 << 128) - 1)\nids.high = ids.value >> 128";
let mut vm = vm_with_range_check!();
vm.segments = segments![
((1, 3), ("335438970432432812899076431678123043273", 10)),
((1, 4), (2, 0))
];
add_segments!(vm, 1);
//Initialize fp
vm.run_context.fp = 7;
//Create ids
let ids_data = HashMap::from([
("value".to_string(), HintReference::new_simple(-4)),
("low".to_string(), HintReference::new(-3, 0, true, true)),
("high".to_string(), HintReference::new(-3, 1, true, true)),
]);
//Execute the hint
assert_matches!(
run_hint!(
vm,
ids_data,
hint_code,
exec_scopes_ref!(),
&HashMap::from([
("MAX_LOW".to_string(), Felt252::from(-1)),
(
"MAX_HIGH".to_string(),
Felt252::from(-1),
)
])
),
Err(HintError::AssertionFailed(x)) if &(*x) == "assert ids.MAX_HIGH < 2**128 and ids.MAX_LOW < 2**128"
);
}

#[test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
fn run_split_felt_wrong_constants() {
let hint_code =
"from starkware.cairo.common.math_utils import assert_integer\nassert ids.MAX_HIGH < 2**128 and ids.MAX_LOW < 2**128\nassert PRIME - 1 == ids.MAX_HIGH * 2**128 + ids.MAX_LOW\nassert_integer(ids.value)\nids.low = ids.value & ((1 << 128) - 1)\nids.high = ids.value >> 128";
let mut vm = vm_with_range_check!();
vm.segments = segments![
((1, 3), ("335438970432432812899076431678123043273", 10)),
((1, 4), (2, 0))
];
add_segments!(vm, 1);
//Initialize fp
vm.run_context.fp = 7;
//Create ids
let ids_data = HashMap::from([
("value".to_string(), HintReference::new_simple(-4)),
("low".to_string(), HintReference::new(-3, 0, true, true)),
("high".to_string(), HintReference::new(-3, 1, true, true)),
]);
//Execute the hint
assert_matches!(
run_hint!(
vm,
ids_data,
hint_code,
exec_scopes_ref!(),
&HashMap::from([
("MAX_LOW".to_string(), Felt252::zero()),
(
"MAX_HIGH".to_string(),
Felt252::zero(),
)
])
),
Err(HintError::AssertionFailed(x)) if &(*x) == "assert PRIME - 1 == ids.MAX_HIGH * 2**128 + ids.MAX_LOW"
);
}

#[test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
fn run_assert_lt_felt_ok() {
Expand Down
9 changes: 9 additions & 0 deletions vm/src/tests/cairo_run_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,15 @@ fn error_msg_attr_div_by_zero() {
run_program_with_error(program_data.as_slice(), error_msg);
}

#[test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
fn split_felt_bad_constants() {
let program_data =
include_bytes!("../../../cairo_programs/bad_programs/split_felt_bad_constants.json");
let error_msg = "assert PRIME - 1 == ids.MAX_HIGH * 2**128 + ids.MAX_LOW";
run_program_with_error(program_data.as_slice(), error_msg);
}

#[test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
fn poseidon_builtin() {
Expand Down

1 comment on commit 132da2c

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.

Benchmark suite Current: 132da2c Previous: 23aaa2a Ratio
add_u64_with_felt/1 3 ns/iter (± 0) 2 ns/iter (± 0) 1.50
add_u64_with_felt/2 3 ns/iter (± 0) 2 ns/iter (± 0) 1.50
add_u64_with_felt/3 2 ns/iter (± 0) 1 ns/iter (± 0) 2
add_u64_with_felt/4 2 ns/iter (± 0) 1 ns/iter (± 0) 2
add_u64_with_felt/5 2 ns/iter (± 0) 1 ns/iter (± 0) 2
add_u64_with_felt/6 3 ns/iter (± 0) 2 ns/iter (± 0) 1.50
add_u64_with_felt/7 3 ns/iter (± 0) 2 ns/iter (± 0) 1.50
add_u64_with_felt/8 3 ns/iter (± 0) 2 ns/iter (± 0) 1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @unbalancedparentheses

Please sign in to comment.