Skip to content

Commit

Permalink
Use BuiltinName enum instead of string representation (#1722)
Browse files Browse the repository at this point in the history
* Save progres

* Progress

* Progress

* Clippy

* Impl generic serialization for suffixed ver

* Implement custom generic deserialization

* Rename module

* Complete API

* Add docs & examples

* Fix docs

* Remove reference from method

* Add clippy allow

* Add changelog entry

* Add no-std import

* Fix

* fmt

* Use a wrapper for CairoPie additional data

* Update changelog

* Fix tests + fmt

* Update cairo1-run/src/cairo_run.rs

Co-authored-by: Juan Bono <[email protected]>

---------

Co-authored-by: Juan Bono <[email protected]>
  • Loading branch information
fmoletta and juanbono authored Apr 19, 2024
1 parent 15bf794 commit 7796f24
Show file tree
Hide file tree
Showing 30 changed files with 555 additions and 469 deletions.
19 changes: 19 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,25 @@

#### Upcoming Changes

* refactor(BREAKING): Use `BuiltinName` enum instead of string representation [#1722](https://github.com/lambdaclass/cairo-vm/pull/1722)
* `BuiltinName` moved from `crate::serde::deserialize_program` module to `crate::types::builtin_name`.
* Implement `BuiltinName` methods `to_str`, `to_str_with_suffix`, `from_str` & `from_str_with_suffix`.
* Remove `BuiltinName` method `name`.
* All builtin-related error variants now store `BuiltinName` instead of `&'static str` or `String`.
* Remove constants: `OUTPUT_BUILTIN_NAME`, `HASH_BUILTIN_NAME`, `RANGE_CHECK_BUILTIN_NAME`,`RANGE_CHECK_96_BUILTIN_NAME`, `SIGNATURE_BUILTIN_NAME`, `BITWISE_BUILTIN_NAME`, `EC_OP_BUILTIN_NAME`, `KECCAK_BUILTIN_NAME`, `POSEIDON_BUILTIN_NAME`, `SEGMENT_ARENA_BUILTIN_NAME`, `ADD_MOD_BUILTIN_NAME` &
`MUL_MOD_BUILTIN_NAME`.
* Remove `BuiltinRunner` & `ModBuiltinRunner` method `identifier`
* Structs containing string representation of builtin names now use `BuiltinName` instead:
* `AirPrivateInput(pub HashMap<&'static str, Vec<PrivateInput>>)` -> `AirPrivateInput(pub HashMap<BuiltinName, Vec<PrivateInput>>)`.
* `CairoPieMetadata` field `additional_data`: `HashMap<String, BuiltinAdditionalData>,` -> `CairoPieAdditionalData` with `CairoPieAdditionalData(pub HashMap<BuiltinName, BuiltinAdditionalData>)`
* `CairoPieMetadata` field `builtin_segments`: `HashMap<String, SegmentInfo>` -> `HashMap<BuiltinName, SegmentInfo>`.
* `ExecutiobResources` field `builtin_instance_counter`: `HashMap<String, usize>` -> `HashMap<BuiltinName, usize>`
* Methods returning string representation of builtin names now use `BuiltinName` instead:
* `BuiltinRunner`, `ModBuiltinRunner` & `RangeCheckBuiltinRunner` method `name`: `&'static str` -> `BuiltinName`.
* `CairoRunner` method `get_builtin_segment_info_for_pie`: `Result<HashMap<String, cairo_pie::SegmentInfo>, RunnerError>` -> `Result<HashMap<BuiltinName, cairo_pie::SegmentInfo>, RunnerError>`

Notes: Serialization of vm outputs that now contain `BuiltinName` & `Display` implementation of `BuiltinName` have not been affected by this PR

* feat: Add `recursive_with_poseidon` layout[#1724](https://github.com/lambdaclass/cairo-vm/pull/1724)

* refactor(BREAKING): Use an enum to represent layout name[#1715](https://github.com/lambdaclass/cairo-vm/pull/1715)
Expand Down
36 changes: 14 additions & 22 deletions cairo1-run/src/cairo_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,14 @@ use cairo_lang_utils::{casts::IntoOrPanic, unordered_hash_map::UnorderedHashMap}
use cairo_vm::{
hint_processor::cairo_1_hint_processor::hint_processor::Cairo1HintProcessor,
math_utils::signed_felt,
serde::deserialize_program::{
ApTracking, BuiltinName, FlowTrackingData, HintParams, ReferenceManager,
serde::deserialize_program::{ApTracking, FlowTrackingData, HintParams, ReferenceManager},
types::{
builtin_name::BuiltinName, layout_name::LayoutName, program::Program,
relocatable::MaybeRelocatable,
},
types::{layout_name::LayoutName, program::Program, relocatable::MaybeRelocatable},
vm::{
errors::{runner_errors::RunnerError, vm_errors::VirtualMachineError},
runners::{
builtin_runner::{
BITWISE_BUILTIN_NAME, EC_OP_BUILTIN_NAME, HASH_BUILTIN_NAME, OUTPUT_BUILTIN_NAME,
POSEIDON_BUILTIN_NAME, RANGE_CHECK_BUILTIN_NAME, SIGNATURE_BUILTIN_NAME,
},
cairo_runner::{CairoRunner, RunResources, RunnerMode},
},
runners::cairo_runner::{CairoRunner, RunResources, RunnerMode},
vm_core::VirtualMachine,
},
Felt252,
Expand Down Expand Up @@ -236,10 +231,7 @@ pub fn cairo_run_program(
.iter()
.enumerate()
.map(|(i, builtin)| {
(
builtin.name(),
(vm.get_ap() - (builtins.len() - 1 - i)).unwrap(),
)
(*builtin, (vm.get_ap() - (builtins.len() - 1 - i)).unwrap())
})
.collect(),
false,
Expand Down Expand Up @@ -723,14 +715,14 @@ fn finalize_builtins(
let mut builtin_name_to_stack_pointer = HashMap::new();
for (id, size) in ret_types_and_sizes {
if let Some(ref name) = id.debug_name {
let builtin_name = match &*name.to_string() {
"RangeCheck" => RANGE_CHECK_BUILTIN_NAME,
"Poseidon" => POSEIDON_BUILTIN_NAME,
"EcOp" => EC_OP_BUILTIN_NAME,
"Bitwise" => BITWISE_BUILTIN_NAME,
"Pedersen" => HASH_BUILTIN_NAME,
"Output" => OUTPUT_BUILTIN_NAME,
"Ecdsa" => SIGNATURE_BUILTIN_NAME,
let builtin_name = match name.as_str() {
"RangeCheck" => BuiltinName::range_check,
"Poseidon" => BuiltinName::poseidon,
"EcOp" => BuiltinName::ec_op,
"Bitwise" => BuiltinName::bitwise,
"Pedersen" => BuiltinName::pedersen,
"Output" => BuiltinName::output,
"Ecdsa" => BuiltinName::ecdsa,
_ => {
stack_pointer.offset += size as usize;
continue;
Expand Down
58 changes: 25 additions & 33 deletions vm/src/air_private_input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@ use crate::{
collections::{BTreeMap, HashMap},
prelude::{String, Vec},
},
vm::runners::builtin_runner::{
ADD_MOD_BUILTIN_NAME, BITWISE_BUILTIN_NAME, EC_OP_BUILTIN_NAME, HASH_BUILTIN_NAME,
KECCAK_BUILTIN_NAME, MUL_MOD_BUILTIN_NAME, POSEIDON_BUILTIN_NAME, RANGE_CHECK_BUILTIN_NAME,
SIGNATURE_BUILTIN_NAME,
},
types::builtin_name::BuiltinName,
};
use serde::{Deserialize, Serialize};

Expand Down Expand Up @@ -40,7 +36,7 @@ pub struct AirPrivateInputSerializable {

// Contains only builtin public inputs, useful for library users
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct AirPrivateInput(pub HashMap<&'static str, Vec<PrivateInput>>);
pub struct AirPrivateInput(pub HashMap<BuiltinName, Vec<PrivateInput>>);

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
#[serde(untagged)]
Expand Down Expand Up @@ -159,21 +155,21 @@ impl AirPrivateInput {
AirPrivateInputSerializable {
trace_path,
memory_path,
pedersen: self.0.get(HASH_BUILTIN_NAME).cloned(),
range_check: self.0.get(RANGE_CHECK_BUILTIN_NAME).cloned(),
ecdsa: self.0.get(SIGNATURE_BUILTIN_NAME).cloned(),
bitwise: self.0.get(BITWISE_BUILTIN_NAME).cloned(),
ec_op: self.0.get(EC_OP_BUILTIN_NAME).cloned(),
keccak: self.0.get(KECCAK_BUILTIN_NAME).cloned(),
poseidon: self.0.get(POSEIDON_BUILTIN_NAME).cloned(),
pedersen: self.0.get(&BuiltinName::pedersen).cloned(),
range_check: self.0.get(&BuiltinName::range_check).cloned(),
ecdsa: self.0.get(&BuiltinName::ecdsa).cloned(),
bitwise: self.0.get(&BuiltinName::bitwise).cloned(),
ec_op: self.0.get(&BuiltinName::ec_op).cloned(),
keccak: self.0.get(&BuiltinName::keccak).cloned(),
poseidon: self.0.get(&BuiltinName::poseidon).cloned(),
add_mod: self
.0
.get(ADD_MOD_BUILTIN_NAME)
.get(&BuiltinName::add_mod)
.and_then(|pi| pi.first())
.cloned(),
mul_mod: self
.0
.get(MUL_MOD_BUILTIN_NAME)
.get(&BuiltinName::mul_mod)
.and_then(|pi| pi.first())
.cloned(),
}
Expand All @@ -188,13 +184,13 @@ impl From<AirPrivateInputSerializable> for AirPrivateInput {
inputs.insert(input_name, input);
}
};
insert_input(HASH_BUILTIN_NAME, private_input.pedersen);
insert_input(RANGE_CHECK_BUILTIN_NAME, private_input.range_check);
insert_input(SIGNATURE_BUILTIN_NAME, private_input.ecdsa);
insert_input(BITWISE_BUILTIN_NAME, private_input.bitwise);
insert_input(EC_OP_BUILTIN_NAME, private_input.ec_op);
insert_input(KECCAK_BUILTIN_NAME, private_input.keccak);
insert_input(POSEIDON_BUILTIN_NAME, private_input.poseidon);
insert_input(BuiltinName::pedersen, private_input.pedersen);
insert_input(BuiltinName::range_check, private_input.range_check);
insert_input(BuiltinName::ecdsa, private_input.ecdsa);
insert_input(BuiltinName::bitwise, private_input.bitwise);
insert_input(BuiltinName::ec_op, private_input.ec_op);
insert_input(BuiltinName::keccak, private_input.keccak);
insert_input(BuiltinName::poseidon, private_input.poseidon);

Self(inputs)
}
Expand All @@ -213,10 +209,6 @@ mod tests {
use {
super::*,
crate::air_private_input::{AirPrivateInput, AirPrivateInputSerializable},
crate::vm::runners::builtin_runner::{
BITWISE_BUILTIN_NAME, EC_OP_BUILTIN_NAME, HASH_BUILTIN_NAME, KECCAK_BUILTIN_NAME,
POSEIDON_BUILTIN_NAME, RANGE_CHECK_BUILTIN_NAME, SIGNATURE_BUILTIN_NAME,
},
assert_matches::assert_matches,
};

Expand Down Expand Up @@ -285,13 +277,13 @@ mod tests {

let private_input = AirPrivateInput::from(serializable_private_input.clone());

assert_matches!(private_input.0.get(HASH_BUILTIN_NAME), data if data == serializable_private_input.pedersen.as_ref());
assert_matches!(private_input.0.get(RANGE_CHECK_BUILTIN_NAME), data if data == serializable_private_input.range_check.as_ref());
assert_matches!(private_input.0.get(SIGNATURE_BUILTIN_NAME), data if data == serializable_private_input.ecdsa.as_ref());
assert_matches!(private_input.0.get(BITWISE_BUILTIN_NAME), data if data == serializable_private_input.bitwise.as_ref());
assert_matches!(private_input.0.get(EC_OP_BUILTIN_NAME), data if data == serializable_private_input.ec_op.as_ref());
assert_matches!(private_input.0.get(KECCAK_BUILTIN_NAME), data if data == serializable_private_input.keccak.as_ref());
assert_matches!(private_input.0.get(POSEIDON_BUILTIN_NAME), data if data == serializable_private_input.poseidon.as_ref());
assert_matches!(private_input.0.get(&BuiltinName::pedersen), data if data == serializable_private_input.pedersen.as_ref());
assert_matches!(private_input.0.get(&BuiltinName::range_check), data if data == serializable_private_input.range_check.as_ref());
assert_matches!(private_input.0.get(&BuiltinName::ecdsa), data if data == serializable_private_input.ecdsa.as_ref());
assert_matches!(private_input.0.get(&BuiltinName::bitwise), data if data == serializable_private_input.bitwise.as_ref());
assert_matches!(private_input.0.get(&BuiltinName::ec_op), data if data == serializable_private_input.ec_op.as_ref());
assert_matches!(private_input.0.get(&BuiltinName::keccak), data if data == serializable_private_input.keccak.as_ref());
assert_matches!(private_input.0.get(&BuiltinName::poseidon), data if data == serializable_private_input.poseidon.as_ref());
}

#[test]
Expand Down
17 changes: 7 additions & 10 deletions vm/src/program_hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ use starknet_crypto::{pedersen_hash, FieldElement};

use crate::Felt252;

use crate::serde::deserialize_program::BuiltinName;
use crate::stdlib::vec::Vec;
use crate::types::builtin_name::BuiltinName;
use crate::types::relocatable::MaybeRelocatable;
use crate::vm::runners::cairo_pie::StrippedProgram;

Expand Down Expand Up @@ -58,15 +58,12 @@ where
///
/// Converts the builtin name to bytes then attempts to create a field element from
/// these bytes. This function will fail if the builtin name is over 31 characters.
fn builtin_to_field_element(builtin: &BuiltinName) -> Result<FieldElement, ProgramHashError> {
fn builtin_name_to_field_element(
builtin_name: &BuiltinName,
) -> Result<FieldElement, ProgramHashError> {
// The Python implementation uses the builtin name without suffix
let builtin_name = builtin
.name()
.strip_suffix("_builtin")
.unwrap_or(builtin.name());

FieldElement::from_byte_slice_be(builtin_name.as_bytes())
.map_err(|_| ProgramHashError::InvalidProgramBuiltin(builtin.name()))
FieldElement::from_byte_slice_be(builtin_name.to_str().as_bytes())
.map_err(|_| ProgramHashError::InvalidProgramBuiltin(builtin_name.to_str()))
}

/// The `value: FieldElement` is `pub(crate)` and there is no accessor.
Expand Down Expand Up @@ -111,7 +108,7 @@ pub fn compute_program_hash_chain(
let builtin_list: Result<Vec<FieldElement>, _> = program
.builtins
.iter()
.map(builtin_to_field_element)
.map(builtin_name_to_field_element)
.collect();
let builtin_list = builtin_list?;

Expand Down
49 changes: 2 additions & 47 deletions vm/src/serde/deserialize_program.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! # Program deserialization
//!
//! This module contains the logic for [`Program`] deserialization.
//! Users shouldn't need to use it directly (except for [`BuiltinName`]).
//! Users shouldn't need to use it directly
//!
//! To generate a [`Program`] from a JSON string, see [`Program::from_bytes()`].
//! To do the same from a JSON file, see [`Program::from_file()`].
Expand All @@ -13,13 +13,11 @@ use crate::{
prelude::*,
sync::Arc,
},
types::builtin_name::BuiltinName,
utils::CAIRO_PRIME,
vm::runners::builtin_runner::RANGE_CHECK_96_BUILTIN_NAME,
};

use crate::utils::PRIME_STR;
use crate::vm::runners::builtin_runner::SEGMENT_ARENA_BUILTIN_NAME;
use crate::vm::runners::builtin_runner::{ADD_MOD_BUILTIN_NAME, MUL_MOD_BUILTIN_NAME};
use crate::Felt252;
use crate::{
serde::deserialize_utils,
Expand All @@ -29,11 +27,6 @@ use crate::{
program::{HintsCollection, Program, SharedProgramData},
relocatable::MaybeRelocatable,
},
vm::runners::builtin_runner::{
BITWISE_BUILTIN_NAME, EC_OP_BUILTIN_NAME, HASH_BUILTIN_NAME, KECCAK_BUILTIN_NAME,
OUTPUT_BUILTIN_NAME, POSEIDON_BUILTIN_NAME, RANGE_CHECK_BUILTIN_NAME,
SIGNATURE_BUILTIN_NAME,
},
};
use num_bigint::BigUint;
use num_traits::{float::FloatCore, Num};
Expand All @@ -43,44 +36,6 @@ use serde_json::Number;
#[cfg(all(feature = "arbitrary", feature = "std"))]
use arbitrary::{self, Arbitrary, Unstructured};

// This enum is used to deserialize program builtins into &str and catch non-valid names
#[cfg_attr(all(feature = "arbitrary", feature = "std"), derive(Arbitrary))]
#[derive(Serialize, Deserialize, Debug, PartialEq, Copy, Clone, Eq, Hash)]
#[allow(non_camel_case_types)]
pub enum BuiltinName {
output,
range_check,
pedersen,
ecdsa,
keccak,
bitwise,
ec_op,
poseidon,
segment_arena,
range_check96,
add_mod,
mul_mod,
}

impl BuiltinName {
pub fn name(&self) -> &'static str {
match self {
BuiltinName::output => OUTPUT_BUILTIN_NAME,
BuiltinName::range_check => RANGE_CHECK_BUILTIN_NAME,
BuiltinName::pedersen => HASH_BUILTIN_NAME,
BuiltinName::ecdsa => SIGNATURE_BUILTIN_NAME,
BuiltinName::keccak => KECCAK_BUILTIN_NAME,
BuiltinName::bitwise => BITWISE_BUILTIN_NAME,
BuiltinName::ec_op => EC_OP_BUILTIN_NAME,
BuiltinName::poseidon => POSEIDON_BUILTIN_NAME,
BuiltinName::segment_arena => SEGMENT_ARENA_BUILTIN_NAME,
BuiltinName::range_check96 => RANGE_CHECK_96_BUILTIN_NAME,
BuiltinName::add_mod => ADD_MOD_BUILTIN_NAME,
BuiltinName::mul_mod => MUL_MOD_BUILTIN_NAME,
}
}
}

#[cfg_attr(all(feature = "arbitrary", feature = "std"), derive(Arbitrary, Clone))]
#[derive(Deserialize, Debug)]
pub struct ProgramJson {
Expand Down
13 changes: 8 additions & 5 deletions vm/src/serde/serialize_program.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
use crate::stdlib::{
collections::{BTreeMap, HashMap},
prelude::*,
use crate::{
stdlib::{
collections::{BTreeMap, HashMap},
prelude::*,
},
types::builtin_name::BuiltinName,
};

use serde::{Deserialize, Serialize};

use super::deserialize_program::{
ApTracking, Attribute, BuiltinName, DebugInfo, FlowTrackingData, HintParams, Identifier,
Member, ProgramJson, Reference, ReferenceManager, ValueAddress,
ApTracking, Attribute, DebugInfo, FlowTrackingData, HintParams, Identifier, Member,
ProgramJson, Reference, ReferenceManager, ValueAddress,
};
use crate::types::program::Program;
use crate::types::relocatable::MaybeRelocatable;
Expand Down
Loading

0 comments on commit 7796f24

Please sign in to comment.