Skip to content

Commit

Permalink
fix: return error when hint's PC is invalid (#1340)
Browse files Browse the repository at this point in the history
* Add test for failing program

* Return error when some hint's PC isn't valid

* Fix tests (+add test)

* Appease clippy

* Add some docs

* Update changelog

* Ignore manually compiled jsons in workflows cache

* Fix warning in wasm-demo

* Fix hash not hashing wasm-demo program

* Fix mismatching paths
  • Loading branch information
MegaRedHand committed Jul 20, 2023
1 parent 2a1f732 commit 7e5378a
Show file tree
Hide file tree
Showing 10 changed files with 157 additions and 27 deletions.
48 changes: 36 additions & 12 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ jobs:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: ${{ matrix.program-target }}-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
restore-keys: ${{ matrix.program-target }}-cache-
Expand Down Expand Up @@ -82,6 +83,7 @@ jobs:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_test_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
- name: Fetch proof programs
Expand All @@ -90,21 +92,27 @@ jobs:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: cairo_proof_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_proof_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
- name: Fetch bench programs
uses: actions/cache/restore@v3
with:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: cairo_bench_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_bench_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
- name: Fetch test contracts
uses: actions/cache/restore@v3
with:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: cairo_1_test_contracts-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_1_test_contracts-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
- name: Run clippy
run: make clippy

Expand Down Expand Up @@ -139,6 +147,7 @@ jobs:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_test_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
- name: Fetch proof programs
Expand All @@ -147,21 +156,27 @@ jobs:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: cairo_proof_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_proof_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
- name: Fetch bench programs
uses: actions/cache/restore@v3
with:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: cairo_bench_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_bench_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
- name: Fetch test contracts
uses: actions/cache/restore@v3
with:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: cairo_1_test_contracts-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_1_test_contracts-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}

# NOTE: we do this separately because --workspace operates in weird ways
- name: Check all features (felt)
Expand Down Expand Up @@ -215,6 +230,7 @@ jobs:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_test_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
- name: Fetch proof programs
Expand All @@ -223,14 +239,18 @@ jobs:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: cairo_proof_programs-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_proof_programs-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
- name: Fetch test contracts
uses: actions/cache/restore@v3
with:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: cairo_1_test_contracts-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: cairo_1_test_contracts-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}

- name: Install testing tools
uses: taiki-e/install-action@v2
Expand Down Expand Up @@ -314,7 +334,7 @@ jobs:
path: |
cairo_programs/**/*.memory
cairo_programs/**/*.trace
key: ${{ matrix.program-target }}-reference-trace-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
key: ${{ matrix.program-target }}-reference-trace-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
restore-keys: ${{ matrix.program-target }}-reference-trace-cache-

- name: Python3 Build
Expand All @@ -334,7 +354,9 @@ jobs:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: ${{ matrix.program-target }}-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: ${{ matrix.program-target }}-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
fail-on-cache-miss: true

# This is not pretty, but we need `make` to see the compiled programs are
Expand Down Expand Up @@ -377,7 +399,9 @@ jobs:
path: |
cairo_programs/**/*.casm
cairo_programs/**/*.json
key: ${{ matrix.program-target }}-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
!cairo_programs/manually_compiled/*
examples/wasm-demo/src/array_sum.json
key: ${{ matrix.program-target }}-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
fail-on-cache-miss: true

- name: Generate traces
Expand Down Expand Up @@ -467,7 +491,7 @@ jobs:
path: |
cairo_programs/**/*.memory
cairo_programs/**/*.trace
key: ${{ matrix.program-target }}-reference-trace-cache-${{ hashFiles( 'cairo_programs/**/*.cairo' ) }}
key: ${{ matrix.program-target }}-reference-trace-cache-${{ hashFiles('cairo_programs/**/*.cairo', 'examples/wasm-demo/src/array_sum.cairo') }}
fail-on-cache-miss: true

- name: Fetch traces for cairo-vm
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#### Upcoming Changes

* fix: return error when a parsed hint's PC is invalid [#1340](https://github.com/lambdaclass/cairo-vm/pull/1340)

* chore(examples): remove _wee_alloc_ dependency from _wasm-demo_ example and _ensure-no_std_ dummy crate [#1337](https://github.com/lambdaclass/cairo-vm/pull/1337)

* docs: improved crate documentation [#1334](https://github.com/lambdaclass/cairo-vm/pull/1334)
Expand Down
32 changes: 32 additions & 0 deletions cairo_programs/manually_compiled/invalid_hint_pc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"attributes": [],
"builtins": [],
"compiler_version": "0.11.0",
"data": [],
"debug_info": {
"instruction_locations": {}
},
"hints": {
"18446744073709551615": [
{
"accessible_scopes": [],
"code": "",
"flow_tracking_data": {
"ap_tracking": {
"group": 0,
"offset": 0
},
"reference_ids": {}
}
}
]
},
"identifiers": {
"__main__.main": {}
},
"main_scope": "",
"prime": "0x800000000000011000000000000000000000000000000000000000000000001",
"reference_manager": {
"references": []
}
}
2 changes: 1 addition & 1 deletion cairo_programs/manually_compiled/valid_program_a.json
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@
}
}
],
"46": [
"4": [
{
"accessible_scopes": [
"__main__",
Expand Down
1 change: 0 additions & 1 deletion examples/wasm-demo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ extern "C" {
fn log(msg: &str);
}

#[cfg(feature = "console_error_panic_hook")]
#[wasm_bindgen(start)]
pub fn start() {
crate::utils::set_panic_hook();
Expand Down
56 changes: 52 additions & 4 deletions vm/src/serde/deserialize_program.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,8 @@ pub fn parse_program_json(
}
}

let (hints, hints_ranges) = Program::flatten_hints(&program_json.hints);
let (hints, hints_ranges) =
Program::flatten_hints(&program_json.hints, program_json.data.len())?;

let shared_program_data = SharedProgramData {
data: program_json.data,
Expand Down Expand Up @@ -877,7 +878,7 @@ mod tests {
}],
),
(
46,
4,
vec![HintParams {
code: "import math".to_string(),
accessible_scopes: vec![
Expand Down Expand Up @@ -910,7 +911,7 @@ mod tests {
/// Deserialize a program without an entrypoint.
#[test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
fn deserialize_program_without_entrypoint_test() {
fn deserialize_program_without_entrypoint() {
let reader =
include_bytes!("../../../cairo_programs/manually_compiled/valid_program_a.json");

Expand Down Expand Up @@ -946,7 +947,7 @@ mod tests {
}],
),
(
46,
4,
vec![HintParams {
code: "import math".to_string(),
accessible_scopes: vec![
Expand Down Expand Up @@ -1527,4 +1528,51 @@ mod tests {
.unwrap()
);
}

#[test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
fn deserialize_program_with_invalid_hint_pc() {
let reader = br#"{
"attributes": [],
"builtins": [],
"compiler_version": "0.11.0",
"data": [
"0x41241"
],
"debug_info": {
"instruction_locations": {}
},
"hints": {
"1": [
{
"accessible_scopes": [],
"code": "",
"flow_tracking_data": {
"ap_tracking": {
"group": 0,
"offset": 0
},
"reference_ids": {}
}
}
]
},
"identifiers": {
"__main__.main": {}
},
"main_scope": "",
"prime": "0x800000000000011000000000000000000000000000000000000000000000001",
"reference_manager": {
"references": []
}
}"#;

let deserialization_result = deserialize_and_parse_program(reader, Some("main"));

assert!(deserialization_result.is_err());
assert_matches!(
deserialization_result.unwrap_err(),
ProgramError::InvalidHintPc(1, 1)
);
}
}
10 changes: 10 additions & 0 deletions vm/src/tests/cairo_run_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -982,3 +982,13 @@ fn cairo_run_overflowing_dict() {
include_bytes!("../../../cairo_programs/manually_compiled/overflowing_dict.json");
run_program_with_error(program_data, "Unknown memory cell at address");
}

#[test]
fn cairo_run_big_hint_pcs() {
let program_data =
include_bytes!("../../../cairo_programs/manually_compiled/invalid_hint_pc.json");
run_program_with_error(
program_data,
"Hint PC (18446744073709551615) is greater or equal to program length (0)",
);
}
2 changes: 2 additions & 0 deletions vm/src/types/errors/program_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ pub enum ProgramError {
ConstWithoutValue(String),
#[error("Expected prime {PRIME_STR}, got {0}")]
PrimeDiffers(String),
#[error("Hint PC ({0}) is greater or equal to program length ({1})")]
InvalidHintPc(usize, usize),
}

#[cfg(test)]
Expand Down
27 changes: 19 additions & 8 deletions vm/src/types/program.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ use arbitrary::Arbitrary;
pub(crate) struct SharedProgramData {
pub(crate) data: Vec<MaybeRelocatable>,
pub(crate) hints: Vec<HintParams>,
pub(crate) hints_ranges: Vec<Option<(usize, NonZeroUsize)>>,
/// This maps a PC to the range of hints in `hints` that correspond to it.
pub(crate) hints_ranges: Vec<HintRange>,
pub(crate) main: Option<usize>,
//start and end labels will only be used in proof-mode
pub(crate) start: Option<usize>,
Expand All @@ -60,6 +61,11 @@ pub(crate) struct SharedProgramData {
pub(crate) reference_manager: Vec<HintReference>,
}

/// Represents a range of hints corresponding to a PC.
///
/// Is [`None`] if the range is empty, and it is [`Some`] tuple `(start, length)` otherwise.
type HintRange = Option<(usize, NonZeroUsize)>;

#[cfg_attr(all(feature = "arbitrary", feature = "std"), derive(Arbitrary))]
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Program {
Expand Down Expand Up @@ -91,7 +97,7 @@ impl Program {
}
}

let (hints, hints_ranges) = Self::flatten_hints(&hints);
let (hints, hints_ranges) = Self::flatten_hints(&hints, data.len())?;

let shared_program_data = SharedProgramData {
data,
Expand All @@ -114,18 +120,23 @@ impl Program {

pub(crate) fn flatten_hints(
hints: &HashMap<usize, Vec<HintParams>>,
) -> (Vec<HintParams>, Vec<Option<(usize, NonZeroUsize)>>) {
program_length: usize,
) -> Result<(Vec<HintParams>, Vec<HintRange>), ProgramError> {
let bounds = hints
.iter()
.map(|(pc, hs)| (*pc, hs.len()))
.reduce(|(max_pc, full_len), (pc, len)| (max_pc.max(pc), full_len + len));
.reduce(|(max_hint_pc, full_len), (pc, len)| (max_hint_pc.max(pc), full_len + len));

let Some((max_pc, full_len)) = bounds else {
return (Vec::new(), Vec::new());
let Some((max_hint_pc, full_len)) = bounds else {
return Ok((Vec::new(), Vec::new()));
};

if max_hint_pc >= program_length {
return Err(ProgramError::InvalidHintPc(max_hint_pc, program_length));
}

let mut hints_values = Vec::with_capacity(full_len);
let mut hints_ranges = vec![None; max_pc + 1];
let mut hints_ranges = vec![None; max_hint_pc + 1];

for (pc, hs) in hints.iter().filter(|(_, hs)| !hs.is_empty()) {
let range = (
Expand All @@ -136,7 +147,7 @@ impl Program {
hints_values.extend_from_slice(&hs[..]);
}

(hints_values, hints_ranges)
Ok((hints_values, hints_ranges))
}

#[cfg(feature = "std")]
Expand Down
Loading

1 comment on commit 7e5378a

@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: 7e5378a Previous: 2a1f732 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 4 ns/iter (± 0) 2 ns/iter (± 0) 2
add_u64_with_felt/7 4 ns/iter (± 0) 2 ns/iter (± 0) 2
add_u64_with_felt/8 4 ns/iter (± 0) 2 ns/iter (± 0) 2

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

CC: @unbalancedparentheses

Please sign in to comment.