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 serde-rs/json#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.

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
+}

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!(

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<OutputBuiltinAdditionalData>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub pedersen_builtin: Option<Vec<Relocatable>>,
+    #[serde(flatten)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub ecdsa_builtin: Option<HashMap<Relocatable, (Felt252, Felt252)>>,
+    #[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<HashMap<String, BuiltinAdditionalData>> for AdditionalData {
+    fn from(mut value: HashMap<String, BuiltinAdditionalData>) -> 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<String, BuiltinAdditionalData>,
+    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<String, BuiltinAdditionalData> =
+        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")?)?;
 
@@ -762,13 +811,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);
@@ -795,4 +846,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);
+    }
 }

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},
@@ -1377,7 +1377,8 @@ impl CairoRunner {
                 .builtin_runners
                 .iter()
                 .map(|b| (b.name().to_string(), b.get_additional_data()))
-                .collect(),
+                .collect::<HashMap<String, BuiltinAdditionalData>>()
+                .into(),
             version: CairoPieVersion {
                 cairo_pie: CAIRO_PIE_VERSION.to_string(),
             },