From 23ec521dc3f6169a8b618059f9ff3cfc7db98b51 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Thu, 11 Jan 2024 10:37:37 +0100 Subject: [PATCH] Fix: deserialize PIE additional data as struct instead of hashmap Problem: Deserializing the PIE additional data as a hashmap of `BuiltinAdditionalData` enums because of an issue with deserializing untagged unions in `serde` (see https://github.com/serde-rs/json/issues/1103). Solution: add a new `AdditionalData` struct with explicit fields for each builtin, circumventing the untagged union issue. This solution has the advantage of always associating the correct data type for each builtin (it's not possible anymore to associate a builtin with a different data type), but requires modifications if a new builtin is added. --- .../pie_additional_data_test.json | 50 +++++++++ vm/src/tests/cairo_pie_test.rs | 39 +++---- vm/src/vm/runners/cairo_pie.rs | 103 ++++++++++++++++-- vm/src/vm/runners/cairo_runner.rs | 5 +- 4 files changed, 163 insertions(+), 34 deletions(-) create mode 100644 cairo_programs/manually_compiled/pie_additional_data_test.json diff --git a/cairo_programs/manually_compiled/pie_additional_data_test.json b/cairo_programs/manually_compiled/pie_additional_data_test.json new file mode 100644 index 0000000000..a0c245b9c1 --- /dev/null +++ b/cairo_programs/manually_compiled/pie_additional_data_test.json @@ -0,0 +1,50 @@ +{ + "output_builtin": { + "pages": { + "1": [ + 18, + 46 + ] + }, + "attributes": { + "gps_fact_topology": [ + 2, + 1, + 0, + 2 + ] + } + }, + "pedersen_builtin": [ + [ + 3, + 2 + ], + [ + 3, + 5 + ], + [ + 3, + 8 + ], + [ + 3, + 11 + ], + [ + 3, + 14 + ], + [ + 3, + 17 + ] + ], + "range_check_builtin": null, + "ecdsa_builtin": [], + "bitwise_builtin": null, + "ec_op_builtin": null, + "keccak_builtin": null, + "poseidon_builtin": null +} \ No newline at end of file diff --git a/vm/src/tests/cairo_pie_test.rs b/vm/src/tests/cairo_pie_test.rs index 933485fb73..1a9926b4a8 100644 --- a/vm/src/tests/cairo_pie_test.rs +++ b/vm/src/tests/cairo_pie_test.rs @@ -19,13 +19,12 @@ use crate::{ HASH_BUILTIN_NAME, OUTPUT_BUILTIN_NAME, RANGE_CHECK_BUILTIN_NAME, SIGNATURE_BUILTIN_NAME, }, - cairo_pie::{ - BuiltinAdditionalData, CairoPieMemory, OutputBuiltinAdditionalData, SegmentInfo, - }, + cairo_pie::{CairoPieMemory, OutputBuiltinAdditionalData, SegmentInfo}, cairo_runner::ExecutionResources, }, }; +use crate::vm::runners::cairo_pie::AdditionalData; #[cfg(all(not(feature = "std"), feature = "alloc"))] use alloc::{ string::{String, ToString}, @@ -92,23 +91,14 @@ fn pedersen_test() { }; assert_eq!(cairo_pie.execution_resources, expected_execution_resources); // additional_data - let expected_additional_data = HashMap::from([ - ( - OUTPUT_BUILTIN_NAME.to_string(), - BuiltinAdditionalData::Output(OutputBuiltinAdditionalData { - pages: HashMap::new(), - attributes: HashMap::new(), - }), - ), - ( - HASH_BUILTIN_NAME.to_string(), - BuiltinAdditionalData::Hash(vec![Relocatable::from((3, 2))]), - ), - ( - RANGE_CHECK_BUILTIN_NAME.to_string(), - BuiltinAdditionalData::None, - ), - ]); + let expected_additional_data = AdditionalData { + output_builtin: Some(OutputBuiltinAdditionalData { + pages: HashMap::new(), + attributes: HashMap::new(), + }), + pedersen_builtin: Some(vec![Relocatable::from((3, 2))]), + ..Default::default() + }; assert_eq!(cairo_pie.additional_data, expected_additional_data); // memory assert_eq!( @@ -170,9 +160,8 @@ fn common_signature() { }; assert_eq!(cairo_pie.execution_resources, expected_execution_resources); // additional_data - let expected_additional_data = HashMap::from([( - SIGNATURE_BUILTIN_NAME.to_string(), - BuiltinAdditionalData::Signature(HashMap::from([( + let expected_additional_data = AdditionalData { + ecdsa_builtin: Some(HashMap::from([( Relocatable::from((2, 0)), ( felt_str!( @@ -183,7 +172,9 @@ fn common_signature() { ), ), )])), - )]); + ..Default::default() + }; + assert_eq!(cairo_pie.additional_data, expected_additional_data); // memory assert_eq!( diff --git a/vm/src/vm/runners/cairo_pie.rs b/vm/src/vm/runners/cairo_pie.rs index e81499bb1c..3c61cd17fb 100644 --- a/vm/src/vm/runners/cairo_pie.rs +++ b/vm/src/vm/runners/cairo_pie.rs @@ -16,6 +16,9 @@ use super::cairo_runner::ExecutionResources; use crate::serde::deserialize_program::deserialize_biguint_from_number; use crate::stdlib::prelude::{String, Vec}; use crate::utils::CAIRO_PRIME; +use crate::vm::runners::builtin_runner::{ + HASH_BUILTIN_NAME, OUTPUT_BUILTIN_NAME, SIGNATURE_BUILTIN_NAME, +}; use crate::{ serde::deserialize_program::BuiltinName, stdlib::{collections::HashMap, prelude::*}, @@ -79,12 +82,58 @@ pub enum BuiltinAdditionalData { None, } -#[derive(Serialize, Clone, Debug, PartialEq, Eq)] +#[derive(Serialize, Deserialize, Clone, Debug, Default, PartialEq, Eq)] +pub struct AdditionalData { + #[serde(skip_serializing_if = "Option::is_none")] + pub output_builtin: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub pedersen_builtin: Option>, + #[serde(flatten)] + #[serde(skip_serializing_if = "Option::is_none")] + pub ecdsa_builtin: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub range_check_builtin: Option<()>, +} + +impl AdditionalData { + pub fn is_empty(&self) -> bool { + self.output_builtin.is_none() + && self.pedersen_builtin.is_none() + && self.ecdsa_builtin.is_none() + && self.range_check_builtin.is_none() + } +} + +impl From> for AdditionalData { + fn from(mut value: HashMap) -> Self { + let output_builtin_data = match value.remove(OUTPUT_BUILTIN_NAME) { + Some(BuiltinAdditionalData::Output(output_data)) => Some(output_data), + _ => None, + }; + let ecdsa_builtin_data = match value.remove(SIGNATURE_BUILTIN_NAME) { + Some(BuiltinAdditionalData::Signature(signature_data)) => Some(signature_data), + _ => None, + }; + let pedersen_builtin_data = match value.remove(HASH_BUILTIN_NAME) { + Some(BuiltinAdditionalData::Hash(pedersen_data)) => Some(pedersen_data), + _ => None, + }; + + Self { + output_builtin: output_builtin_data, + ecdsa_builtin: ecdsa_builtin_data, + pedersen_builtin: pedersen_builtin_data, + range_check_builtin: None, + } + } +} + +#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)] pub struct CairoPie { pub metadata: CairoPieMetadata, pub memory: CairoPieMemory, pub execution_resources: ExecutionResources, - pub additional_data: HashMap, + pub additional_data: AdditionalData, pub version: CairoPieVersion, } @@ -185,7 +234,7 @@ impl CairoPie { let metadata: CairoPieMetadata = Self::parse_zip_file(zip.by_name("metadata.json")?)?; let execution_resources: ExecutionResources = Self::parse_zip_file(zip.by_name("execution_resources.json")?)?; - let additional_data: HashMap = + let additional_data: AdditionalData = Self::parse_zip_file(zip.by_name("additional_data.json")?)?; let version: CairoPieVersion = Self::parse_zip_file(zip.by_name("version.json")?)?; @@ -732,13 +781,15 @@ mod test { assert_eq!( cairo_pie.additional_data, - HashMap::from([( - "output_builtin".to_string(), - BuiltinAdditionalData::Output(OutputBuiltinAdditionalData { + AdditionalData { + output_builtin: Some(OutputBuiltinAdditionalData { pages: Default::default(), attributes: Default::default(), - }) - )]) + }), + pedersen_builtin: None, + ecdsa_builtin: Some(HashMap::default()), + range_check_builtin: None, + } ); assert_eq!(cairo_pie.version.cairo_pie, CAIRO_PIE_VERSION); @@ -765,4 +816,40 @@ mod test { CairoPie::from_bytes(&cairo_pie_bytes).expect("Could not read CairoPie zip file"); validate_pie_content(cairo_pie); } + #[test] + fn test_deserialize_additional_data() { + let data = include_bytes!( + "../../../../cairo_programs/manually_compiled/pie_additional_data_test.json" + ); + let additional_data: AdditionalData = serde_json::from_slice(data).unwrap(); + let output_data = additional_data.output_builtin.unwrap(); + assert_eq!( + output_data.pages, + HashMap::from([( + 1, + PublicMemoryPage { + start: 18, + size: 46 + } + )]) + ); + assert_eq!( + output_data.attributes, + HashMap::from([("gps_fact_topology".to_string(), vec![2, 1, 0, 2])]) + ); + let pedersen_data = additional_data.pedersen_builtin.unwrap(); + assert_eq!( + pedersen_data, + vec![ + Relocatable::from((3, 2)), + Relocatable::from((3, 5)), + Relocatable::from((3, 8)), + Relocatable::from((3, 11)), + Relocatable::from((3, 14)), + Relocatable::from((3, 17)), + ] + ); + // TODO: add a test case with signature data + assert_matches!(additional_data.ecdsa_builtin, None); + } } diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index 0d5ba9c1d8..27f316ac9d 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/vm/src/vm/runners/cairo_runner.rs @@ -14,7 +14,7 @@ use crate::{ }, }; -use crate::vm::runners::cairo_pie::CAIRO_PIE_VERSION; +use crate::vm::runners::cairo_pie::{BuiltinAdditionalData, CAIRO_PIE_VERSION}; use crate::Felt252; use crate::{ hint_processor::hint_processor_definition::{HintProcessor, HintReference}, @@ -1391,7 +1391,8 @@ impl CairoRunner { .builtin_runners .iter() .map(|b| (b.name().to_string(), b.get_additional_data())) - .collect(), + .collect::>() + .into(), version: CairoPieVersion { cairo_pie: CAIRO_PIE_VERSION.to_string(), },