fix: fix abi encoding range checks on negative inputs

Cargo.toml

@@ -133,6 +133,8 @@ jsonrpc = { version = "0.16.0", features = ["minreq_http"] }
 flate2 = "1.0.24"
 rand = "0.8.5"
 proptest = "1.2.0"
+proptest-derive = "0.4.0"
+
 
 im = { version = "15.1", features = ["serde"] }
 tracing = "0.1.40"

tooling/noirc_abi/Cargo.toml

@@ -23,6 +23,9 @@ num-traits = "0.2"
 [dev-dependencies]
 strum = "0.24"
 strum_macros = "0.24"
+proptest.workspace = true
+proptest-derive.workspace = true
+
 
 [features]
 bn254 = [
@@ -32,4 +35,4 @@ bn254 = [
 bls12_381 = [
     "acvm/bls12_381",
     "noirc_frontend/bls12_381",
-]
+]

tooling/noirc_abi/src/arbitrary.rs

@@ -0,0 +1,124 @@
+use iter_extended::{btree_map, vecmap};
+use proptest::prelude::*;
+
+use acvm::FieldElement;
+
+use crate::{
+    input_parser::InputValue, Abi, AbiParameter, AbiReturnType, AbiType, AbiVisibility, InputMap,
+    Sign,
+};
+use std::collections::{BTreeMap, HashSet};
+
+pub(super) use proptest_derive::Arbitrary;
+
+/// Mutates an iterator of mutable references to [`String`]s to ensure that all values are unique.
+fn ensure_unique_strings<'a>(iter: impl Iterator<Item = &'a mut String>) {
+    let mut seen_values: HashSet<String> = HashSet::default();
+    for value in iter {
+        while seen_values.contains(value.as_str()) {
+            value.push('1');
+        }
+        seen_values.insert(value.clone());
+    }
+}
+
+proptest::prop_compose! {
+    pub(super) fn arb_field_from_integer(bit_size: u32, sign: Sign)(value: u128)-> FieldElement {
+        let width = (bit_size % 128).clamp(1, 127);
+        if sign == Sign::Unsigned {
+            let max_value = 2u128.pow(width) - 1;
+            FieldElement::from(value.clamp(0, max_value))
+        } else {
+            let min_value = -(2i128.pow(width - 1));
+            let max_value = 2i128.pow(width - 1) - 1;
+
+            FieldElement::from((value as i128).clamp(min_value, max_value))
+        }
+    }
+}
+
+fn arb_primitive_abi_type_and_value(
+) -> impl proptest::strategy::Strategy<Value = (AbiType, InputValue)> {
+    proptest::prop_oneof![
+        any::<u128>().prop_map(|val| (AbiType::Field, InputValue::Field(FieldElement::from(val)))),
+        any::<(Sign, u32)>().prop_flat_map(|(sign, width)| {
+            let width = (width % 128).clamp(1, 127);
+            (
+                Just(AbiType::Integer { sign, width }),
+                arb_field_from_integer(width, sign).prop_map(InputValue::Field),
+            )
+        }),
+        any::<bool>()
+            .prop_map(|val| (AbiType::Boolean, InputValue::Field(FieldElement::from(val)))),
+        ".+".prop_map(|str| (
+            AbiType::String { length: str.len() as u32 },
+            InputValue::String(str)
+        ))
+    ]
+}
+
+fn arb_abi_type_and_value() -> impl proptest::strategy::Strategy<Value = (AbiType, InputValue)> {
+    let leaf = arb_primitive_abi_type_and_value();
+
+    leaf.prop_recursive(
+        8,   // 8 levels deep
+        256, // Shoot for maximum size of 256 nodes
+        10,  // We put up to 10 items per collection
+        |inner| {
+            prop_oneof![
+                // TODO: support `AbiType::Array`.
+                // This is non-trivial due to the need to get N `InputValue`s which are all compatible with
+                // the element's `AbiType`.`
+                prop::collection::vec(inner.clone(), 1..10).prop_map(|typ| {
+                    let (fields, values): (Vec<_>, Vec<_>) = typ.into_iter().unzip();
+                    let tuple_type = AbiType::Tuple { fields };
+                    (tuple_type, InputValue::Vec(values))
+                }),
+                (".*", prop::collection::vec((inner.clone(), ".*"), 1..10)).prop_map(
+                    |(path, mut typ)| {
+                        // Require that all field names are unique.
+                        ensure_unique_strings(typ.iter_mut().map(|(_, field_name)| field_name));
+
+                        let (types_and_values, names): (Vec<_>, Vec<_>) = typ.into_iter().unzip();
+                        let (types, values): (Vec<_>, Vec<_>) =
+                            types_and_values.into_iter().unzip();
+
+                        let fields = names.clone().into_iter().zip(types).collect();
+                        let struct_values = names.into_iter().zip(values).collect();
+                        let struct_type = AbiType::Struct { path, fields };
+
+                        (struct_type, InputValue::Struct(struct_values))
+                    }
+                ),
+            ]
+        },
+    )
+}
+
+proptest::prop_compose! {
+    pub(super) fn arb_abi_type()((typ, _) in arb_abi_type_and_value())-> AbiType {
+        typ
+    }
+}
+
+prop_compose! {
+    fn arb_abi_param_and_value()
+                ((typ, value) in arb_abi_type_and_value(), name: String, visibility: AbiVisibility)
+                -> (AbiParameter, InputValue) {
+        (AbiParameter{ name, typ, visibility }, value)
+    }
+}
+
+prop_compose! {
+    pub(super) fn arb_abi_and_input_map()
+        (mut parameters_with_values in proptest::collection::vec(arb_abi_param_and_value(), 0..100), return_type: Option<AbiReturnType>)
+        -> (Abi, InputMap) {
+            // Require that all parameter names are unique.
+            ensure_unique_strings(parameters_with_values.iter_mut().map(|(param_name,_)| &mut param_name.name));
+
+            let parameters  = vecmap(&parameters_with_values, |(param, _)| param.clone());
+            let input_map = btree_map(parameters_with_values, |(param, value)| (param.name, value));
+
+            (Abi { parameters, return_type, error_types: BTreeMap::default() }, input_map)
+    }
+}

tooling/noirc_abi/src/input_parser/mod.rs

@@ -7,7 +7,7 @@ use acvm::{AcirField, FieldElement};
 use serde::Serialize;
 
 use crate::errors::InputParserError;
-use crate::{Abi, AbiType};
+use crate::{Abi, AbiType, Sign};
 
 pub mod json;
 mod toml;
@@ -64,8 +64,16 @@ impl InputValue {
     ) -> Result<(), InputTypecheckingError> {
         match (self, abi_param) {
             (InputValue::Field(_), AbiType::Field) => Ok(()),
-            (InputValue::Field(field_element), AbiType::Integer { width, .. }) => {
-                if field_element.num_bits() <= *width {
+            (InputValue::Field(field_element), AbiType::Integer { width, sign }) => {
+                // Negative integers are represented by the range [p - 2^(width-1), p) so we must shift them up
+                // into the range [0, 2^(width-1)) so that we can treat them in the same way as unsigned integers.
+                let shifted_field_element = if *sign == Sign::Signed {
+                    *field_element + FieldElement::from(2u128.pow(*width - 1))
+                } else {
+                    *field_element
+                };
+
+                if shifted_field_element.num_bits() <= *width {
                     Ok(())
                 } else {
                     Err(InputTypecheckingError::OutsideOfValidRange {

tooling/noirc_abi/src/lib.rs

@@ -27,6 +27,9 @@ use std::{collections::BTreeMap, str};
 //
 // This ABI has nothing to do with ACVM or ACIR. Although they implicitly have a relationship
 
+#[cfg(test)]
+mod arbitrary;
+
 pub mod errors;
 pub mod input_parser;
 mod serialization;
@@ -77,6 +80,7 @@ pub enum AbiType {
 }
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
+#[cfg_attr(test, derive(arbitrary::Arbitrary))]
 #[serde(rename_all = "lowercase")]
 /// Represents whether the parameter is public or known only to the prover.
 pub enum AbiVisibility {
@@ -123,6 +127,7 @@ pub enum AbiDistinctness {
 }
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
+#[cfg_attr(test, derive(arbitrary::Arbitrary))]
 #[serde(rename_all = "lowercase")]
 pub enum Sign {
     Unsigned,
@@ -243,10 +248,12 @@ impl From<&AbiType> for PrintableType {
 }
 
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
+#[cfg_attr(test, derive(arbitrary::Arbitrary))]
 /// An argument or return value of the circuit's `main` function.
 pub struct AbiParameter {
     pub name: String,
     #[serde(rename = "type")]
+    #[cfg_attr(test, proptest(strategy = "arbitrary::arb_abi_type()"))]
     pub typ: AbiType,
     pub visibility: AbiVisibility,
 }
@@ -258,16 +265,21 @@ impl AbiParameter {
 }
 
 #[derive(Clone, Debug, Serialize, Deserialize)]
+#[cfg_attr(test, derive(arbitrary::Arbitrary))]
+
 pub struct AbiReturnType {
+    #[cfg_attr(test, proptest(strategy = "arbitrary::arb_abi_type()"))]
     pub abi_type: AbiType,
     pub visibility: AbiVisibility,
 }
 
 #[derive(Clone, Debug, Serialize, Deserialize)]
+#[cfg_attr(test, derive(arbitrary::Arbitrary))]
 pub struct Abi {
     /// An ordered list of the arguments to the program's `main` function, specifying their types and visibility.
     pub parameters: Vec<AbiParameter>,
     pub return_type: Option<AbiReturnType>,
+    #[cfg_attr(test, proptest(strategy = "proptest::prelude::Just(BTreeMap::from([]))"))]
     pub error_types: BTreeMap<ErrorSelector, AbiErrorType>,
 }
 
@@ -605,56 +617,18 @@ pub fn display_abi_error<F: AcirField>(
 
 #[cfg(test)]
 mod test {
-    use std::collections::BTreeMap;
+    use proptest::prelude::*;
 
-    use acvm::{AcirField, FieldElement};
+    use crate::arbitrary::arb_abi_and_input_map;
 
-    use crate::{
-        input_parser::InputValue, Abi, AbiParameter, AbiReturnType, AbiType, AbiVisibility,
-        InputMap,
-    };
+    proptest! {
+        #[test]
+        fn encoding_and_decoding_returns_original_witness_map((abi, input_map) in arb_abi_and_input_map()) {
+            let witness_map = abi.encode(&input_map, None).unwrap();
+            let (decoded_inputs, return_value) = abi.decode(&witness_map).unwrap();
 
-    #[test]
-    fn witness_encoding_roundtrip() {
-        let abi = Abi {
-            parameters: vec![
-                AbiParameter {
-                    name: "thing1".to_string(),
-                    typ: AbiType::Array { length: 2, typ: Box::new(AbiType::Field) },
-                    visibility: AbiVisibility::Public,
-                },
-                AbiParameter {
-                    name: "thing2".to_string(),
-                    typ: AbiType::Field,
-                    visibility: AbiVisibility::Public,
-                },
-            ],
-            return_type: Some(AbiReturnType {
-                abi_type: AbiType::Field,
-                visibility: AbiVisibility::Public,
-            }),
-            error_types: BTreeMap::default(),
-        };
-
-        // Note we omit return value from inputs
-        let inputs: InputMap = BTreeMap::from([
-            (
-                "thing1".to_string(),
-                InputValue::Vec(vec![
-                    InputValue::Field(FieldElement::one()),
-                    InputValue::Field(FieldElement::one()),
-                ]),
-            ),
-            ("thing2".to_string(), InputValue::Field(FieldElement::zero())),
-        ]);
-
-        let witness_map = abi.encode(&inputs, None).unwrap();
-        let (reconstructed_inputs, return_value) = abi.decode(&witness_map).unwrap();
-
-        for (key, expected_value) in inputs {
-            assert_eq!(reconstructed_inputs[&key], expected_value);
+            prop_assert_eq!(decoded_inputs, input_map);
+            prop_assert_eq!(return_value, None);
         }
-
-        assert!(return_value.is_none());
     }
 }