From ecaf63da19a3a76e7c2940721d9044b1980a588f Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:01:10 +0000 Subject: [PATCH 1/4] fix: correct signed integer handling in `noirc_abi` (#6638) Co-authored-by: Ary Borenszweig Co-authored-by: guipublic Co-authored-by: guipublic <47281315+guipublic@users.noreply.github.com> --- .../input_parser/json.txt | 7 +++ .../input_parser/toml.txt | 9 +++ tooling/noirc_abi/src/input_parser/json.rs | 47 ++++++++++++++- tooling/noirc_abi/src/input_parser/mod.rs | 57 ++++++++++++++++++- tooling/noirc_abi/src/input_parser/toml.rs | 47 ++++++++++++++- .../test/browser/abi_encode.test.ts | 5 +- .../test/node/abi_encode.test.ts | 5 +- .../noirc_abi_wasm/test/shared/abi_encode.ts | 4 +- 8 files changed, 168 insertions(+), 13 deletions(-) create mode 100644 tooling/noirc_abi/proptest-regressions/input_parser/json.txt create mode 100644 tooling/noirc_abi/proptest-regressions/input_parser/toml.txt diff --git a/tooling/noirc_abi/proptest-regressions/input_parser/json.txt b/tooling/noirc_abi/proptest-regressions/input_parser/json.txt new file mode 100644 index 00000000000..19de8eeaf48 --- /dev/null +++ b/tooling/noirc_abi/proptest-regressions/input_parser/json.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc b3f9ae88d54944ca274764f4d99a2023d4b0ac09beb89bc599cbba1e45dd3620 # shrinks to (typ, value) = (Integer { sign: Signed, width: 1 }, -1) diff --git a/tooling/noirc_abi/proptest-regressions/input_parser/toml.txt b/tooling/noirc_abi/proptest-regressions/input_parser/toml.txt new file mode 100644 index 00000000000..1448cb67ef1 --- /dev/null +++ b/tooling/noirc_abi/proptest-regressions/input_parser/toml.txt @@ -0,0 +1,9 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 9d200afb8f5c01e3414d24eebe1436a7eef5377a46a9a9235aaa7f81e0b33656 # shrinks to (typ, value) = (Integer { sign: Signed, width: 8 }, -1) +cc 7fd29637e5566d819992185c1a95438e9949a555928a911b3918eed2e3f7a1fd # shrinks to (typ, value) = (Integer { sign: Signed, width: 64 }, -1) +cc 8ecbda39d887674b53ca23a861ac30fbb10c123bb70c57e69b336c86a3d9dea8 # shrinks to (abi, input_map) = (Abi { parameters: [AbiParameter { name: "¡", typ: Struct { path: "�)\u{1b}=�?Ⱥ\u{59424}?{\u{e4d5e}%Ѩ/Q\u{36a17}/*\";\u{b}&iC_\u{d313f}S\u{1b}\u{9dfec}\r/\u{10530d}", fields: [("?p*\"/\u{202e}\u{6f038}\u{537ca}.y@~𘛶?4\u{1b}*", Field), (".Ⱥ/$\u{7f}\u{103c06}%\\\u{202e}][0\u{88479}]\"*~\u{36fd5}\u{5}\u{feff}]{/", Tuple { fields: [String { length: 937 }] }), ("r\u{ac3a5}&:", Boolean), ("$d6🕴/:|�\u{37f8b}\r\u{a13b7}C$𲁹\\&\u{f8712}?\u{db61c}t%\u{57be1}\0", Field), ("/\u{6378b}\u{a426c}¥\u{7}/\u{fcb29}$\u{53c6b}\u{12d6f}\u{12bd3}.\u{f2f82}\u{8613e}*$\u{fd32f}\u{e29f7}\0𨺉'¬\"1", Struct { path: "\\\u{4a5ac}<\u{9e505}\u{4f3af}🕴&?<:^\u{7}\u{88}\u{3e1ff}(¥\u{531f3}K{:¥𦺀", fields: [("n\0Ѩ/\u{1b}𥐰\u{a4906}�¥`{\u{389d4}`1\u{7708a})\u{3dac4}8\u{93e5f}㒭\\\"\u{e6824}\u{b}Ѩ\u{88946}Ⱥ{", Integer { sign: Signed, width: 127 })] }), ("¥🕴\u{1b}¥🕴=sR\0\u{35f36}\u{867dc}>ä\u{202e}f:BȺ?:``*·¥\u{74ca5}\"", Tuple { fields: [Boolean, Field, String { length: 205 }, String { length: 575 }, Integer { sign: Signed, width: 124 }, String { length: 923 }, String { length: 294 }] })] }, visibility: Public }], return_type: None, error_types: {} }, {"¡": Struct({"$d6🕴/:|�\u{37f8b}\r\u{a13b7}C$𲁹\\&\u{f8712}?\u{db61c}t%\u{57be1}\0": Field(-8275115097504119425402713293372777967031130481426075481525511323101167533940), ".Ⱥ/$\u{7f}\u{103c06}%\\\u{202e}][0\u{88479}]\"*~\u{36fd5}\u{5}\u{feff}]{/": Vec([String("A \0A 0 aA0 a0aa00 A\000 0 \0\0aA\0\0a \0 \0a 0A\0A\0 Aa0aAA0A\0aa\00 0\0\0\0\0\00a Aa0 \0 a A0 \0AA0A Aa Aa\00aAaAaaA0A0 aA0 \0 Aa\00 \0000AAA a \0AAaaA\0\0a A0a0AA\0aA00 aA a0A\0AAa0a\0A0a\0\0A0A \00Aaaaa a A AO.*D\r.`bD4a\n*\u{15}\\B\"ace.8&A\t[AV8w<\u{18}\"\u{f}4`^Q\u{1b}U*$Z/\0\u{b}]qw${`\"=X&A\\\u{e}%`\\:\"$\u{1}.(6_C:\u{7}a`V=N**\u{1b})#Y\u{7f}#\u{b}$l\t}.Mns5!\t*$g\u{18}\rC\u{11}\"$=\u{7}.?&\u{1}yW\t.Y|<6\u{12}\u{e}/4JJ*&/V$`\"&`x#R\np\\%'*\n:P\0K\u{b}*`\r7Ym\t_\u{b}=$\u{16}`0v\u{7f}'NV^N4J<9=G*A:!b\u{1c}:'c{ST&z![\u{7f}/.={E*pmaWC\u{7f}7p{<\"']\u{8}?`\u{1b}\"\\\u{1}$\u{18}/!\u{16}-\t:E7CUs%_qw*xf.S\t\u{4}'=\"&%t'\u{1f}\u{7f}\u{b}$.=f\u{6}\"$A}xV_$\u{1a}nH\n\u{1b}?<&\n\u{15}U\\-b\u{1d}|\u{b}\u{2}t \rwA{L\u{11}\u{6}\u{10}\0\u{1b}G[x?&Yi?&7\u{b}?\r\u{1f}b\\$=\u{b}x& Q/\t\u{4}|X\"7\"{\0\0j'.\0\\e1zR.\u{c}\n<\u{b}Q*R+y8\u{19}(o\u{1f}@m\nt+\u{7f}Q\\+.Rn?\u{17}UZ\"$\u{b}/\0B=9=\t{\u{8}qZ&`!:D{\u{6}IO.H\u{7f}:?/3@\r\u{1b}oä\u{202e}f:BȺ?:``*·¥\u{74ca5}\"": Vec([Field(1), Field(8822392870083219098626030699076694602179106416928939583840848325203494062169), String("*TXn;{}\"_)_9\nk\\#ts\u{10}%\\c\n/2._::Oj*\u{7f}\0\r&PUMl\u{10}$/u?L}\u{7f}*P&<%=\u{7}S#%A\n \u{e}\\#v!\"\nepRp.{vH{&@\t\u{1f}\u{b}?=T\u{f}\"B\u{11}\n/{HY.\u{16}\n\nj<&\u{3}{f\n/9J*&x.$/,\r\0\u{1c}'\u{5}\u{13}\u{1b}`T\0`\n&/&\u{15}\u{b}w:{SK\u{7f}\\apR%/'0`0\n'd$$\u{7f}Vs\t<{\nDTT\\F\n\u{15}y.\\\t*-)&D$*u\u{b}\u{1b}?{\u{b}/\n\u{7f}0*.7\0\n:\u{b}.rSk<6~>{#"), String(".\"JA%q6i\ra/:F\u{16}?q<\t\rN\\13?H<;?{`\u{1d}p{.\"5?*@'N\"\u{1a}P,\u{1b}\u{7f}c+dt5':Y\u{1b}k/G>k/eM$XIX')\u{1b}'&\u{7f}\\\r\u{1b}`'P_.\n.?\0p`Y\u{c}`._\u{b}B\0\ng/*v$jfJ:\u{c}\u{1b}Pv}xn7ph@#{_<{.JD?r%'E\n7s9n/],u![;%*\u{2}{y`MgRdok8\"%<*>*{GyFJ}?\0W%#\0\u{1b}\u{7f}\u{16}G:\t=w\u{7f}:q\u{7f}:{k?\u{b}(:ca{$*1X/cw\u{1b}Z6I\rX\0\u{1b}(.^14\r\\=s\u{1b}w\u{3}F~\n\u{1e})/$0:=[\u{1},\\\\\tg\u{16}:],J`\0N\n\u{1b}\u{1b}\u{1b}{.xb\u{1a}\r'12#?e\\#/\tA\u{7f}\".\\Ke=\\?!v+P\u{17}\r\u{12}x.=A.`0<&?\niR/*WW\rnV)5vY.~\n _h\0&5f#\r\u{2}-S%\t s..\u{7f}!X}\"=\"?\u{5}y\u{4}`fr&R&d: 1Ht\"4`y_/S.71#{|%$%&ehy\u{16}J_\u{e}=:.%'\"N=J:\r:{&.\u{12}\u{b})&N\u{10}R_3;11\u{b}Qd<`<{?xF:~\"%<=<<\03:t??&\r;{\u{13}?__Y\u{6})\\k,vs?\n`G(*\n!\u{1b}[@z\0$?*yKLJh_\u{13}FkY'\\?T^\u{1f}$1n`'[\n\u{7f}\0+l\u{b}\u{1a}E\u{b}&(/\u{b}\rr\t:&\0+N'N:oC:*``IN\u{b}*.:\t$7+'*U:\t Result { let json_value = match (value, abi_type) { + (InputValue::Field(f), AbiType::Integer { sign: crate::Sign::Signed, width }) => { + JsonTypes::String(field_to_signed_hex(*f, *width)) + } (InputValue::Field(f), AbiType::Field | AbiType::Integer { .. }) => { JsonTypes::String(Self::format_field_string(*f)) } @@ -143,6 +146,9 @@ impl InputValue { ) -> Result { let input_value = match (value, param_type) { (JsonTypes::String(string), AbiType::String { .. }) => InputValue::String(string), + (JsonTypes::String(string), AbiType::Integer { sign: crate::Sign::Signed, width }) => { + InputValue::Field(parse_str_to_signed(&string, *width)?) + } ( JsonTypes::String(string), AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean, @@ -192,3 +198,40 @@ impl InputValue { Ok(input_value) } } + +#[cfg(test)] +mod test { + use proptest::prelude::*; + + use crate::{ + arbitrary::arb_abi_and_input_map, + input_parser::{arbitrary::arb_signed_integer_type_and_value, json::JsonTypes, InputValue}, + }; + + use super::{parse_json, serialize_to_json}; + + proptest! { + #[test] + fn serializing_and_parsing_returns_original_input((abi, input_map) in arb_abi_and_input_map()) { + let json = serialize_to_json(&input_map, &abi).expect("should be serializable"); + let parsed_input_map = parse_json(&json, &abi).expect("should be parsable"); + + prop_assert_eq!(parsed_input_map, input_map); + } + + #[test] + fn signed_integer_serialization_roundtrip((typ, value) in arb_signed_integer_type_and_value()) { + let string_input = JsonTypes::String(value.to_string()); + let input_value = InputValue::try_from_json(string_input, &typ, "foo").expect("should be parsable"); + let JsonTypes::String(output_string) = JsonTypes::try_from_input_value(&input_value, &typ).expect("should be serializable") else { + panic!("wrong type output"); + }; + let output_number = if let Some(output_string) = output_string.strip_prefix("-0x") { + -i64::from_str_radix(output_string, 16).unwrap() + } else { + i64::from_str_radix(output_string.strip_prefix("0x").unwrap(), 16).unwrap() + }; + prop_assert_eq!(output_number, value); + } + } +} diff --git a/tooling/noirc_abi/src/input_parser/mod.rs b/tooling/noirc_abi/src/input_parser/mod.rs index d7bbb0adfe3..b7732235eb2 100644 --- a/tooling/noirc_abi/src/input_parser/mod.rs +++ b/tooling/noirc_abi/src/input_parser/mod.rs @@ -248,6 +248,11 @@ mod serialization_tests { typ: AbiType::Field, visibility: AbiVisibility::Private, }, + AbiParameter { + name: "signed_example".into(), + typ: AbiType::Integer { sign: Sign::Signed, width: 8 }, + visibility: AbiVisibility::Private, + }, AbiParameter { name: "bar".into(), typ: AbiType::Struct { @@ -272,6 +277,7 @@ mod serialization_tests { let input_map: BTreeMap = BTreeMap::from([ ("foo".into(), InputValue::Field(FieldElement::one())), + ("signed_example".into(), InputValue::Field(FieldElement::from(240u128))), ( "bar".into(), InputValue::Struct(BTreeMap::from([ @@ -317,7 +323,9 @@ fn parse_str_to_field(value: &str) -> Result { } fn parse_str_to_signed(value: &str, width: u32) -> Result { - let big_num = if let Some(hex) = value.strip_prefix("0x") { + let big_num = if let Some(hex) = value.strip_prefix("-0x") { + BigInt::from_str_radix(hex, 16).map(|value| -value) + } else if let Some(hex) = value.strip_prefix("0x") { BigInt::from_str_radix(hex, 16) } else { BigInt::from_str_radix(value, 10) @@ -357,12 +365,23 @@ fn field_from_big_int(bigint: BigInt) -> FieldElement { } } +fn field_to_signed_hex(f: FieldElement, bit_size: u32) -> String { + let f_u128 = f.to_u128(); + let max = 2_u128.pow(bit_size - 1) - 1; + if f_u128 > max { + let f = FieldElement::from(2_u128.pow(bit_size) - f_u128); + format!("-0x{}", f.to_hex()) + } else { + format!("0x{}", f.to_hex()) + } +} + #[cfg(test)] mod test { use acvm::{AcirField, FieldElement}; use num_bigint::BigUint; - use super::parse_str_to_field; + use super::{parse_str_to_field, parse_str_to_signed}; fn big_uint_from_field(field: FieldElement) -> BigUint { BigUint::from_bytes_be(&field.to_be_bytes()) @@ -400,4 +419,38 @@ mod test { let noncanonical_field = FieldElement::modulus().to_string(); assert!(parse_str_to_field(&noncanonical_field).is_err()); } + + #[test] + fn test_parse_str_to_signed() { + let value = parse_str_to_signed("1", 8).unwrap(); + assert_eq!(value, FieldElement::from(1_u128)); + + let value = parse_str_to_signed("-1", 8).unwrap(); + assert_eq!(value, FieldElement::from(255_u128)); + + let value = parse_str_to_signed("-1", 16).unwrap(); + assert_eq!(value, FieldElement::from(65535_u128)); + } +} + +#[cfg(test)] +mod arbitrary { + use proptest::prelude::*; + + use crate::{AbiType, Sign}; + + pub(super) fn arb_signed_integer_type_and_value() -> BoxedStrategy<(AbiType, i64)> { + (2u32..=64) + .prop_flat_map(|width| { + let typ = Just(AbiType::Integer { width, sign: Sign::Signed }); + let value = if width == 64 { + // Avoid overflow + i64::MIN..i64::MAX + } else { + -(2i64.pow(width - 1))..(2i64.pow(width - 1) - 1) + }; + (typ, value) + }) + .boxed() + } } diff --git a/tooling/noirc_abi/src/input_parser/toml.rs b/tooling/noirc_abi/src/input_parser/toml.rs index 321d3511b5d..6f2be68a0c4 100644 --- a/tooling/noirc_abi/src/input_parser/toml.rs +++ b/tooling/noirc_abi/src/input_parser/toml.rs @@ -1,4 +1,4 @@ -use super::{parse_str_to_field, parse_str_to_signed, InputValue}; +use super::{field_to_signed_hex, parse_str_to_field, parse_str_to_signed, InputValue}; use crate::{errors::InputParserError, Abi, AbiType, MAIN_RETURN_NAME}; use acvm::{AcirField, FieldElement}; use iter_extended::{try_btree_map, try_vecmap}; @@ -60,7 +60,7 @@ pub(crate) fn serialize_to_toml( Ok(toml_string) } -#[derive(Debug, Deserialize, Serialize, Clone)] +#[derive(Debug, Deserialize, Serialize, Clone, PartialEq)] #[serde(untagged)] enum TomlTypes { // This is most likely going to be a hex string @@ -83,6 +83,9 @@ impl TomlTypes { abi_type: &AbiType, ) -> Result { let toml_value = match (value, abi_type) { + (InputValue::Field(f), AbiType::Integer { sign: crate::Sign::Signed, width }) => { + TomlTypes::String(field_to_signed_hex(*f, *width)) + } (InputValue::Field(f), AbiType::Field | AbiType::Integer { .. }) => { let f_str = format!("0x{}", f.to_hex()); TomlTypes::String(f_str) @@ -126,6 +129,7 @@ impl InputValue { ) -> Result { let input_value = match (value, param_type) { (TomlTypes::String(string), AbiType::String { .. }) => InputValue::String(string), + ( TomlTypes::String(string), AbiType::Field @@ -139,7 +143,7 @@ impl InputValue { TomlTypes::Integer(integer), AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean, ) => { - let new_value = FieldElement::from(i128::from(integer)); + let new_value = FieldElement::from(u128::from(integer)); InputValue::Field(new_value) } @@ -179,3 +183,40 @@ impl InputValue { Ok(input_value) } } + +#[cfg(test)] +mod test { + use proptest::prelude::*; + + use crate::{ + arbitrary::arb_abi_and_input_map, + input_parser::{arbitrary::arb_signed_integer_type_and_value, toml::TomlTypes, InputValue}, + }; + + use super::{parse_toml, serialize_to_toml}; + + proptest! { + #[test] + fn serializing_and_parsing_returns_original_input((abi, input_map) in arb_abi_and_input_map()) { + let toml = serialize_to_toml(&input_map, &abi).expect("should be serializable"); + let parsed_input_map = parse_toml(&toml, &abi).expect("should be parsable"); + + prop_assert_eq!(parsed_input_map, input_map); + } + + #[test] + fn signed_integer_serialization_roundtrip((typ, value) in arb_signed_integer_type_and_value()) { + let string_input = TomlTypes::String(value.to_string()); + let input_value = InputValue::try_from_toml(string_input.clone(), &typ, "foo").expect("should be parsable"); + let TomlTypes::String(output_string) = TomlTypes::try_from_input_value(&input_value, &typ).expect("should be serializable") else { + panic!("wrong type output"); + }; + let output_number = if let Some(output_string) = output_string.strip_prefix("-0x") { + -i64::from_str_radix(output_string, 16).unwrap() + } else { + i64::from_str_radix(output_string.strip_prefix("0x").unwrap(), 16).unwrap() + }; + prop_assert_eq!(output_number, value); + } + } +} diff --git a/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts b/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts index e1aaf0dc2c0..77903ea8ddd 100644 --- a/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts +++ b/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts @@ -15,7 +15,8 @@ it('recovers original inputs when abi encoding and decoding', async () => { const foo: Field = inputs.foo as Field; const bar: Field[] = inputs.bar as Field[]; expect(BigInt(decoded_inputs.inputs.foo)).to.be.equal(BigInt(foo)); - expect(BigInt(decoded_inputs.inputs.bar[0])).to.be.equal(BigInt(bar[0])); - expect(BigInt(decoded_inputs.inputs.bar[1])).to.be.equal(BigInt(bar[1])); + expect(parseInt(decoded_inputs.inputs.bar[0])).to.be.equal(parseInt(bar[0])); + expect(parseInt(decoded_inputs.inputs.bar[1])).to.be.equal(parseInt(bar[1])); + expect(parseInt(decoded_inputs.inputs.bar[2])).to.be.equal(parseInt(bar[2])); expect(decoded_inputs.return_value).to.be.null; }); diff --git a/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts b/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts index a49c10b6ea6..4bb46f1ddd1 100644 --- a/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts +++ b/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts @@ -11,7 +11,8 @@ it('recovers original inputs when abi encoding and decoding', async () => { const foo: Field = inputs.foo as Field; const bar: Field[] = inputs.bar as Field[]; expect(BigInt(decoded_inputs.inputs.foo)).to.be.equal(BigInt(foo)); - expect(BigInt(decoded_inputs.inputs.bar[0])).to.be.equal(BigInt(bar[0])); - expect(BigInt(decoded_inputs.inputs.bar[1])).to.be.equal(BigInt(bar[1])); + expect(parseInt(decoded_inputs.inputs.bar[0])).to.be.equal(parseInt(bar[0])); + expect(parseInt(decoded_inputs.inputs.bar[1])).to.be.equal(parseInt(bar[1])); + expect(parseInt(decoded_inputs.inputs.bar[2])).to.be.equal(parseInt(bar[2])); expect(decoded_inputs.return_value).to.be.null; }); diff --git a/tooling/noirc_abi_wasm/test/shared/abi_encode.ts b/tooling/noirc_abi_wasm/test/shared/abi_encode.ts index 62eb7658f43..b789bb05371 100644 --- a/tooling/noirc_abi_wasm/test/shared/abi_encode.ts +++ b/tooling/noirc_abi_wasm/test/shared/abi_encode.ts @@ -5,7 +5,7 @@ export const abi: Abi = { { name: 'foo', type: { kind: 'field' }, visibility: 'private' }, { name: 'bar', - type: { kind: 'array', length: 2, type: { kind: 'field' } }, + type: { kind: 'array', length: 3, type: { kind: 'integer', sign: 'signed', width: 32 } }, visibility: 'private', }, ], @@ -15,5 +15,5 @@ export const abi: Abi = { export const inputs: InputMap = { foo: '1', - bar: ['1', '2'], + bar: ['1', '2', '-1'], }; From f0c5fe95072d592934193c9a138ab9db34ca52be Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:21:20 +0000 Subject: [PATCH 2/4] chore: refactor foreign call executors (#6659) --- compiler/noirc_printable_type/src/lib.rs | 3 + tooling/acvm_cli/src/cli/execute_cmd.rs | 2 +- tooling/debugger/src/foreign_calls.rs | 2 +- tooling/nargo/src/foreign_calls/mocker.rs | 176 +++++++ tooling/nargo/src/foreign_calls/mod.rs | 146 ++++++ tooling/nargo/src/foreign_calls/print.rs | 36 ++ tooling/nargo/src/foreign_calls/rpc.rs | 227 ++++++++ tooling/nargo/src/lib.rs | 1 + tooling/nargo/src/ops/execute.rs | 3 +- tooling/nargo/src/ops/foreign_calls.rs | 494 ------------------ tooling/nargo/src/ops/mod.rs | 2 - tooling/nargo/src/ops/test.rs | 6 +- tooling/nargo_cli/benches/criterion.rs | 2 +- tooling/nargo_cli/src/cli/execute_cmd.rs | 2 +- tooling/nargo_cli/src/cli/info_cmd.rs | 2 +- tooling/nargo_cli/tests/stdlib-props.rs | 5 +- .../src/cli/execution_flamegraph_cmd.rs | 2 +- 17 files changed, 601 insertions(+), 510 deletions(-) create mode 100644 tooling/nargo/src/foreign_calls/mocker.rs create mode 100644 tooling/nargo/src/foreign_calls/mod.rs create mode 100644 tooling/nargo/src/foreign_calls/print.rs create mode 100644 tooling/nargo/src/foreign_calls/rpc.rs delete mode 100644 tooling/nargo/src/ops/foreign_calls.rs diff --git a/compiler/noirc_printable_type/src/lib.rs b/compiler/noirc_printable_type/src/lib.rs index 5ab04c6f576..838a2472125 100644 --- a/compiler/noirc_printable_type/src/lib.rs +++ b/compiler/noirc_printable_type/src/lib.rs @@ -69,6 +69,9 @@ pub enum PrintableValueDisplay { #[derive(Debug, Error)] pub enum ForeignCallError { + #[error("No handler could be found for foreign call `{0}`")] + NoHandler(String), + #[error("Foreign call inputs needed for execution are missing")] MissingForeignCallInputs, diff --git a/tooling/acvm_cli/src/cli/execute_cmd.rs b/tooling/acvm_cli/src/cli/execute_cmd.rs index c453936568c..bf5969718e5 100644 --- a/tooling/acvm_cli/src/cli/execute_cmd.rs +++ b/tooling/acvm_cli/src/cli/execute_cmd.rs @@ -8,7 +8,7 @@ use clap::Args; use crate::cli::fs::inputs::{read_bytecode_from_file, read_inputs_from_file}; use crate::errors::CliError; -use nargo::ops::{execute_program, DefaultForeignCallExecutor}; +use nargo::{foreign_calls::DefaultForeignCallExecutor, ops::execute_program}; use super::fs::witness::{create_output_witness_string, save_witness_to_dir}; diff --git a/tooling/debugger/src/foreign_calls.rs b/tooling/debugger/src/foreign_calls.rs index 6a773a4b348..ecf27a22f29 100644 --- a/tooling/debugger/src/foreign_calls.rs +++ b/tooling/debugger/src/foreign_calls.rs @@ -3,7 +3,7 @@ use acvm::{ pwg::ForeignCallWaitInfo, AcirField, FieldElement, }; -use nargo::ops::{DefaultForeignCallExecutor, ForeignCallExecutor}; +use nargo::foreign_calls::{DefaultForeignCallExecutor, ForeignCallExecutor}; use noirc_artifacts::debug::{DebugArtifact, DebugVars, StackFrame}; use noirc_errors::debug_info::{DebugFnId, DebugVarId}; use noirc_printable_type::ForeignCallError; diff --git a/tooling/nargo/src/foreign_calls/mocker.rs b/tooling/nargo/src/foreign_calls/mocker.rs new file mode 100644 index 00000000000..c93d16bbaf6 --- /dev/null +++ b/tooling/nargo/src/foreign_calls/mocker.rs @@ -0,0 +1,176 @@ +use acvm::{ + acir::brillig::{ForeignCallParam, ForeignCallResult}, + pwg::ForeignCallWaitInfo, + AcirField, +}; +use noirc_printable_type::{decode_string_value, ForeignCallError}; +use serde::{Deserialize, Serialize}; + +use super::{ForeignCall, ForeignCallExecutor}; + +/// This struct represents an oracle mock. It can be used for testing programs that use oracles. +#[derive(Debug, PartialEq, Eq, Clone)] +struct MockedCall { + /// The id of the mock, used to update or remove it + id: usize, + /// The oracle it's mocking + name: String, + /// Optionally match the parameters + params: Option>>, + /// The parameters with which the mock was last called + last_called_params: Option>>, + /// The result to return when this mock is called + result: ForeignCallResult, + /// How many times should this mock be called before it is removed + times_left: Option, +} + +impl MockedCall { + fn new(id: usize, name: String) -> Self { + Self { + id, + name, + params: None, + last_called_params: None, + result: ForeignCallResult { values: vec![] }, + times_left: None, + } + } +} + +impl MockedCall { + fn matches(&self, name: &str, params: &[ForeignCallParam]) -> bool { + self.name == name && (self.params.is_none() || self.params.as_deref() == Some(params)) + } +} + +#[derive(Debug, Default)] +pub(crate) struct MockForeignCallExecutor { + /// Mocks have unique ids used to identify them in Noir, allowing to update or remove them. + last_mock_id: usize, + /// The registered mocks + mocked_responses: Vec>, +} + +impl MockForeignCallExecutor { + fn extract_mock_id( + foreign_call_inputs: &[ForeignCallParam], + ) -> Result<(usize, &[ForeignCallParam]), ForeignCallError> { + let (id, params) = + foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; + let id = + usize::try_from(id.unwrap_field().try_to_u64().expect("value does not fit into u64")) + .expect("value does not fit into usize"); + Ok((id, params)) + } + + fn find_mock_by_id(&self, id: usize) -> Option<&MockedCall> { + self.mocked_responses.iter().find(|response| response.id == id) + } + + fn find_mock_by_id_mut(&mut self, id: usize) -> Option<&mut MockedCall> { + self.mocked_responses.iter_mut().find(|response| response.id == id) + } + + fn parse_string(param: &ForeignCallParam) -> String { + let fields: Vec<_> = param.fields().to_vec(); + decode_string_value(&fields) + } +} + +impl Deserialize<'a>> ForeignCallExecutor + for MockForeignCallExecutor +{ + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + let foreign_call_name = foreign_call.function.as_str(); + match ForeignCall::lookup(foreign_call_name) { + Some(ForeignCall::CreateMock) => { + let mock_oracle_name = Self::parse_string(&foreign_call.inputs[0]); + assert!(ForeignCall::lookup(&mock_oracle_name).is_none()); + let id = self.last_mock_id; + self.mocked_responses.push(MockedCall::new(id, mock_oracle_name)); + self.last_mock_id += 1; + + Ok(F::from(id).into()) + } + Some(ForeignCall::SetMockParams) => { + let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; + self.find_mock_by_id_mut(id) + .unwrap_or_else(|| panic!("Unknown mock id {}", id)) + .params = Some(params.to_vec()); + + Ok(ForeignCallResult::default()) + } + Some(ForeignCall::GetMockLastParams) => { + let (id, _) = Self::extract_mock_id(&foreign_call.inputs)?; + let mock = + self.find_mock_by_id(id).unwrap_or_else(|| panic!("Unknown mock id {}", id)); + + let last_called_params = mock + .last_called_params + .clone() + .unwrap_or_else(|| panic!("Mock {} was never called", mock.name)); + + Ok(last_called_params.into()) + } + Some(ForeignCall::SetMockReturns) => { + let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; + self.find_mock_by_id_mut(id) + .unwrap_or_else(|| panic!("Unknown mock id {}", id)) + .result = ForeignCallResult { values: params.to_vec() }; + + Ok(ForeignCallResult::default()) + } + Some(ForeignCall::SetMockTimes) => { + let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; + let times = + params[0].unwrap_field().try_to_u64().expect("Invalid bit size of times"); + + self.find_mock_by_id_mut(id) + .unwrap_or_else(|| panic!("Unknown mock id {}", id)) + .times_left = Some(times); + + Ok(ForeignCallResult::default()) + } + Some(ForeignCall::ClearMock) => { + let (id, _) = Self::extract_mock_id(&foreign_call.inputs)?; + self.mocked_responses.retain(|response| response.id != id); + Ok(ForeignCallResult::default()) + } + _ => { + let mock_response_position = self + .mocked_responses + .iter() + .position(|response| response.matches(foreign_call_name, &foreign_call.inputs)); + + if let Some(response_position) = mock_response_position { + // If the program has registered a mocked response to this oracle call then we prefer responding + // with that. + + let mock = self + .mocked_responses + .get_mut(response_position) + .expect("Invalid position of mocked response"); + + mock.last_called_params = Some(foreign_call.inputs.clone()); + + let result = mock.result.values.clone(); + + if let Some(times_left) = &mut mock.times_left { + *times_left -= 1; + if *times_left == 0 { + self.mocked_responses.remove(response_position); + } + } + + Ok(result.into()) + } else { + Err(ForeignCallError::NoHandler(foreign_call_name.to_string())) + } + } + } + } +} diff --git a/tooling/nargo/src/foreign_calls/mod.rs b/tooling/nargo/src/foreign_calls/mod.rs new file mode 100644 index 00000000000..0ef3247ee59 --- /dev/null +++ b/tooling/nargo/src/foreign_calls/mod.rs @@ -0,0 +1,146 @@ +use std::path::PathBuf; + +use acvm::{acir::brillig::ForeignCallResult, pwg::ForeignCallWaitInfo, AcirField}; +use mocker::MockForeignCallExecutor; +use noirc_printable_type::ForeignCallError; +use print::PrintForeignCallExecutor; +use rand::Rng; +use rpc::RPCForeignCallExecutor; +use serde::{Deserialize, Serialize}; + +mod mocker; +mod print; +mod rpc; + +pub trait ForeignCallExecutor { + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError>; +} + +/// This enumeration represents the Brillig foreign calls that are natively supported by nargo. +/// After resolution of a foreign call, nargo will restart execution of the ACVM +pub enum ForeignCall { + Print, + CreateMock, + SetMockParams, + GetMockLastParams, + SetMockReturns, + SetMockTimes, + ClearMock, +} + +impl std::fmt::Display for ForeignCall { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.name()) + } +} + +impl ForeignCall { + pub(crate) fn name(&self) -> &'static str { + match self { + ForeignCall::Print => "print", + ForeignCall::CreateMock => "create_mock", + ForeignCall::SetMockParams => "set_mock_params", + ForeignCall::GetMockLastParams => "get_mock_last_params", + ForeignCall::SetMockReturns => "set_mock_returns", + ForeignCall::SetMockTimes => "set_mock_times", + ForeignCall::ClearMock => "clear_mock", + } + } + + pub(crate) fn lookup(op_name: &str) -> Option { + match op_name { + "print" => Some(ForeignCall::Print), + "create_mock" => Some(ForeignCall::CreateMock), + "set_mock_params" => Some(ForeignCall::SetMockParams), + "get_mock_last_params" => Some(ForeignCall::GetMockLastParams), + "set_mock_returns" => Some(ForeignCall::SetMockReturns), + "set_mock_times" => Some(ForeignCall::SetMockTimes), + "clear_mock" => Some(ForeignCall::ClearMock), + _ => None, + } + } +} + +#[derive(Debug, Default)] +pub struct DefaultForeignCallExecutor { + /// The executor for any [`ForeignCall::Print`] calls. + printer: Option, + mocker: MockForeignCallExecutor, + external: Option, +} + +impl DefaultForeignCallExecutor { + pub fn new( + show_output: bool, + resolver_url: Option<&str>, + root_path: Option, + package_name: Option, + ) -> Self { + let id = rand::thread_rng().gen(); + let printer = if show_output { Some(PrintForeignCallExecutor) } else { None }; + let external_resolver = resolver_url.map(|resolver_url| { + RPCForeignCallExecutor::new(resolver_url, id, root_path, package_name) + }); + DefaultForeignCallExecutor { + printer, + mocker: MockForeignCallExecutor::default(), + external: external_resolver, + } + } +} + +impl Deserialize<'a>> ForeignCallExecutor + for DefaultForeignCallExecutor +{ + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + let foreign_call_name = foreign_call.function.as_str(); + match ForeignCall::lookup(foreign_call_name) { + Some(ForeignCall::Print) => { + if let Some(printer) = &mut self.printer { + printer.execute(foreign_call) + } else { + Ok(ForeignCallResult::default()) + } + } + Some( + ForeignCall::CreateMock + | ForeignCall::SetMockParams + | ForeignCall::GetMockLastParams + | ForeignCall::SetMockReturns + | ForeignCall::SetMockTimes + | ForeignCall::ClearMock, + ) => self.mocker.execute(foreign_call), + + None => { + // First check if there's any defined mock responses for this foreign call. + match self.mocker.execute(foreign_call) { + Err(ForeignCallError::NoHandler(_)) => (), + response_or_error => return response_or_error, + }; + + if let Some(external_resolver) = &mut self.external { + // If the user has registered an external resolver then we forward any remaining oracle calls there. + match external_resolver.execute(foreign_call) { + Err(ForeignCallError::NoHandler(_)) => (), + response_or_error => return response_or_error, + }; + } + + // If all executors have no handler for the given foreign call then we cannot + // return a correct response to the ACVM. The best we can do is to return an empty response, + // this allows us to ignore any foreign calls which exist solely to pass information from inside + // the circuit to the environment (e.g. custom logging) as the execution will still be able to progress. + // + // We optimistically return an empty response for all oracle calls as the ACVM will error + // should a response have been required. + Ok(ForeignCallResult::default()) + } + } + } +} diff --git a/tooling/nargo/src/foreign_calls/print.rs b/tooling/nargo/src/foreign_calls/print.rs new file mode 100644 index 00000000000..d0befa0bd0b --- /dev/null +++ b/tooling/nargo/src/foreign_calls/print.rs @@ -0,0 +1,36 @@ +use acvm::{acir::brillig::ForeignCallResult, pwg::ForeignCallWaitInfo, AcirField}; +use noirc_printable_type::{ForeignCallError, PrintableValueDisplay}; + +use super::{ForeignCall, ForeignCallExecutor}; + +#[derive(Debug, Default)] +pub(super) struct PrintForeignCallExecutor; + +impl ForeignCallExecutor for PrintForeignCallExecutor { + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + let foreign_call_name = foreign_call.function.as_str(); + match ForeignCall::lookup(foreign_call_name) { + Some(ForeignCall::Print) => { + let skip_newline = foreign_call.inputs[0].unwrap_field().is_zero(); + + let foreign_call_inputs = foreign_call + .inputs + .split_first() + .ok_or(ForeignCallError::MissingForeignCallInputs)? + .1; + + let display_values: PrintableValueDisplay = foreign_call_inputs.try_into()?; + let display_string = + format!("{display_values}{}", if skip_newline { "" } else { "\n" }); + + print!("{display_string}"); + + Ok(ForeignCallResult::default()) + } + _ => Err(ForeignCallError::NoHandler(foreign_call_name.to_string())), + } + } +} diff --git a/tooling/nargo/src/foreign_calls/rpc.rs b/tooling/nargo/src/foreign_calls/rpc.rs new file mode 100644 index 00000000000..dab64819b56 --- /dev/null +++ b/tooling/nargo/src/foreign_calls/rpc.rs @@ -0,0 +1,227 @@ +use std::path::PathBuf; + +use acvm::{acir::brillig::ForeignCallResult, pwg::ForeignCallWaitInfo, AcirField}; +use jsonrpc::{arg as build_json_rpc_arg, minreq_http::Builder, Client}; +use noirc_printable_type::ForeignCallError; +use serde::{Deserialize, Serialize}; + +use super::ForeignCallExecutor; + +#[derive(Debug)] +pub(super) struct RPCForeignCallExecutor { + /// A randomly generated id for this `DefaultForeignCallExecutor`. + /// + /// This is used so that a single `external_resolver` can distinguish between requests from multiple + /// instantiations of `DefaultForeignCallExecutor`. + id: u64, + /// JSON RPC client to resolve foreign calls + external_resolver: Client, + /// Root path to the program or workspace in execution. + root_path: Option, + /// Name of the package in execution + package_name: Option, +} + +#[derive(Debug, Serialize, Deserialize)] +struct ResolveForeignCallRequest { + /// A session ID which allows the external RPC server to link this foreign call request to other foreign calls + /// for the same program execution. + /// + /// This is intended to allow a single RPC server to maintain state related to multiple program executions being + /// performed in parallel. + session_id: u64, + + #[serde(flatten)] + /// The foreign call which the external RPC server is to provide a response for. + function_call: ForeignCallWaitInfo, + + #[serde(skip_serializing_if = "Option::is_none")] + /// Root path to the program or workspace in execution. + root_path: Option, + #[serde(skip_serializing_if = "Option::is_none")] + /// Name of the package in execution + package_name: Option, +} + +impl RPCForeignCallExecutor { + pub(super) fn new( + resolver_url: &str, + id: u64, + root_path: Option, + package_name: Option, + ) -> Self { + let mut transport_builder = + Builder::new().url(resolver_url).expect("Invalid oracle resolver URL"); + + if let Some(Ok(timeout)) = + std::env::var("NARGO_FOREIGN_CALL_TIMEOUT").ok().map(|timeout| timeout.parse()) + { + let timeout_duration = std::time::Duration::from_millis(timeout); + transport_builder = transport_builder.timeout(timeout_duration); + }; + let oracle_resolver = Client::with_transport(transport_builder.build()); + + RPCForeignCallExecutor { external_resolver: oracle_resolver, id, root_path, package_name } + } +} + +impl Deserialize<'a>> ForeignCallExecutor + for RPCForeignCallExecutor +{ + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + let encoded_params = vec![build_json_rpc_arg(ResolveForeignCallRequest { + session_id: self.id, + function_call: foreign_call.clone(), + root_path: self.root_path.clone().map(|path| path.to_str().unwrap().to_string()), + package_name: self.package_name.clone(), + })]; + + let req = self.external_resolver.build_request("resolve_foreign_call", &encoded_params); + + let response = self.external_resolver.send_request(req)?; + + let parsed_response: ForeignCallResult = response.result()?; + + Ok(parsed_response) + } +} + +#[cfg(test)] +mod tests { + use acvm::{ + acir::brillig::ForeignCallParam, brillig_vm::brillig::ForeignCallResult, + pwg::ForeignCallWaitInfo, FieldElement, + }; + use jsonrpc_core::Result as RpcResult; + use jsonrpc_derive::rpc; + use jsonrpc_http_server::{Server, ServerBuilder}; + + use super::{ForeignCallExecutor, RPCForeignCallExecutor, ResolveForeignCallRequest}; + + #[allow(unreachable_pub)] + #[rpc] + pub trait OracleResolver { + #[rpc(name = "resolve_foreign_call")] + fn resolve_foreign_call( + &self, + req: ResolveForeignCallRequest, + ) -> RpcResult>; + } + + struct OracleResolverImpl; + + impl OracleResolverImpl { + fn echo(&self, param: ForeignCallParam) -> ForeignCallResult { + vec![param].into() + } + + fn sum(&self, array: ForeignCallParam) -> ForeignCallResult { + let mut res: FieldElement = 0_usize.into(); + + for value in array.fields() { + res += value; + } + + res.into() + } + } + + impl OracleResolver for OracleResolverImpl { + fn resolve_foreign_call( + &self, + req: ResolveForeignCallRequest, + ) -> RpcResult> { + let response = match req.function_call.function.as_str() { + "sum" => self.sum(req.function_call.inputs[0].clone()), + "echo" => self.echo(req.function_call.inputs[0].clone()), + "id" => FieldElement::from(req.session_id as u128).into(), + + _ => panic!("unexpected foreign call"), + }; + Ok(response) + } + } + + fn build_oracle_server() -> (Server, String) { + let mut io = jsonrpc_core::IoHandler::new(); + io.extend_with(OracleResolverImpl.to_delegate()); + + // Choosing port 0 results in a random port being assigned. + let server = ServerBuilder::new(io) + .start_http(&"127.0.0.1:0".parse().expect("Invalid address")) + .expect("Could not start server"); + + let url = format!("http://{}", server.address()); + (server, url) + } + + #[test] + fn test_oracle_resolver_echo() { + let (server, url) = build_oracle_server(); + + let mut executor = RPCForeignCallExecutor::new(&url, 1, None, None); + + let foreign_call: ForeignCallWaitInfo = ForeignCallWaitInfo { + function: "echo".to_string(), + inputs: vec![ForeignCallParam::Single(1_u128.into())], + }; + + let result = executor.execute(&foreign_call); + assert_eq!(result.unwrap(), ForeignCallResult { values: foreign_call.inputs }); + + server.close(); + } + + #[test] + fn test_oracle_resolver_sum() { + let (server, url) = build_oracle_server(); + + let mut executor = RPCForeignCallExecutor::new(&url, 2, None, None); + + let foreign_call: ForeignCallWaitInfo = ForeignCallWaitInfo { + function: "sum".to_string(), + inputs: vec![ForeignCallParam::Array(vec![1_usize.into(), 2_usize.into()])], + }; + + let result = executor.execute(&foreign_call); + assert_eq!(result.unwrap(), FieldElement::from(3_usize).into()); + + server.close(); + } + + #[test] + fn foreign_call_executor_id_is_persistent() { + let (server, url) = build_oracle_server(); + + let mut executor = RPCForeignCallExecutor::new(&url, 3, None, None); + + let foreign_call: ForeignCallWaitInfo = + ForeignCallWaitInfo { function: "id".to_string(), inputs: Vec::new() }; + + let result_1 = executor.execute(&foreign_call).unwrap(); + let result_2 = executor.execute(&foreign_call).unwrap(); + assert_eq!(result_1, result_2); + + server.close(); + } + + #[test] + fn oracle_resolver_rpc_can_distinguish_executors() { + let (server, url) = build_oracle_server(); + + let mut executor_1 = RPCForeignCallExecutor::new(&url, 4, None, None); + let mut executor_2 = RPCForeignCallExecutor::new(&url, 5, None, None); + + let foreign_call: ForeignCallWaitInfo = + ForeignCallWaitInfo { function: "id".to_string(), inputs: Vec::new() }; + + let result_1 = executor_1.execute(&foreign_call).unwrap(); + let result_2 = executor_2.execute(&foreign_call).unwrap(); + assert_ne!(result_1, result_2); + + server.close(); + } +} diff --git a/tooling/nargo/src/lib.rs b/tooling/nargo/src/lib.rs index 88f07e0c292..74b7f54d860 100644 --- a/tooling/nargo/src/lib.rs +++ b/tooling/nargo/src/lib.rs @@ -9,6 +9,7 @@ pub mod constants; pub mod errors; +pub mod foreign_calls; pub mod ops; pub mod package; pub mod workspace; diff --git a/tooling/nargo/src/ops/execute.rs b/tooling/nargo/src/ops/execute.rs index 09ef554d2aa..57116ec2efd 100644 --- a/tooling/nargo/src/ops/execute.rs +++ b/tooling/nargo/src/ops/execute.rs @@ -10,10 +10,9 @@ use acvm::{acir::circuit::Circuit, acir::native_types::WitnessMap}; use acvm::{AcirField, BlackBoxFunctionSolver}; use crate::errors::ExecutionError; +use crate::foreign_calls::ForeignCallExecutor; use crate::NargoError; -use super::foreign_calls::ForeignCallExecutor; - struct ProgramExecutor<'a, F, B: BlackBoxFunctionSolver, E: ForeignCallExecutor> { functions: &'a [Circuit], diff --git a/tooling/nargo/src/ops/foreign_calls.rs b/tooling/nargo/src/ops/foreign_calls.rs deleted file mode 100644 index 30785949a46..00000000000 --- a/tooling/nargo/src/ops/foreign_calls.rs +++ /dev/null @@ -1,494 +0,0 @@ -use std::path::PathBuf; - -use acvm::{ - acir::brillig::{ForeignCallParam, ForeignCallResult}, - pwg::ForeignCallWaitInfo, - AcirField, -}; -use jsonrpc::{arg as build_json_rpc_arg, minreq_http::Builder, Client}; -use noirc_printable_type::{decode_string_value, ForeignCallError, PrintableValueDisplay}; -use rand::Rng; -use serde::{Deserialize, Serialize}; - -pub trait ForeignCallExecutor { - fn execute( - &mut self, - foreign_call: &ForeignCallWaitInfo, - ) -> Result, ForeignCallError>; -} - -/// This enumeration represents the Brillig foreign calls that are natively supported by nargo. -/// After resolution of a foreign call, nargo will restart execution of the ACVM -pub enum ForeignCall { - Print, - CreateMock, - SetMockParams, - GetMockLastParams, - SetMockReturns, - SetMockTimes, - ClearMock, -} - -impl std::fmt::Display for ForeignCall { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.name()) - } -} - -impl ForeignCall { - pub(crate) fn name(&self) -> &'static str { - match self { - ForeignCall::Print => "print", - ForeignCall::CreateMock => "create_mock", - ForeignCall::SetMockParams => "set_mock_params", - ForeignCall::GetMockLastParams => "get_mock_last_params", - ForeignCall::SetMockReturns => "set_mock_returns", - ForeignCall::SetMockTimes => "set_mock_times", - ForeignCall::ClearMock => "clear_mock", - } - } - - pub(crate) fn lookup(op_name: &str) -> Option { - match op_name { - "print" => Some(ForeignCall::Print), - "create_mock" => Some(ForeignCall::CreateMock), - "set_mock_params" => Some(ForeignCall::SetMockParams), - "get_mock_last_params" => Some(ForeignCall::GetMockLastParams), - "set_mock_returns" => Some(ForeignCall::SetMockReturns), - "set_mock_times" => Some(ForeignCall::SetMockTimes), - "clear_mock" => Some(ForeignCall::ClearMock), - _ => None, - } - } -} - -/// This struct represents an oracle mock. It can be used for testing programs that use oracles. -#[derive(Debug, PartialEq, Eq, Clone)] -struct MockedCall { - /// The id of the mock, used to update or remove it - id: usize, - /// The oracle it's mocking - name: String, - /// Optionally match the parameters - params: Option>>, - /// The parameters with which the mock was last called - last_called_params: Option>>, - /// The result to return when this mock is called - result: ForeignCallResult, - /// How many times should this mock be called before it is removed - times_left: Option, -} - -impl MockedCall { - fn new(id: usize, name: String) -> Self { - Self { - id, - name, - params: None, - last_called_params: None, - result: ForeignCallResult { values: vec![] }, - times_left: None, - } - } -} - -impl MockedCall { - fn matches(&self, name: &str, params: &[ForeignCallParam]) -> bool { - self.name == name && (self.params.is_none() || self.params.as_deref() == Some(params)) - } -} - -#[derive(Debug, Default)] -pub struct DefaultForeignCallExecutor { - /// A randomly generated id for this `DefaultForeignCallExecutor`. - /// - /// This is used so that a single `external_resolver` can distinguish between requests from multiple - /// instantiations of `DefaultForeignCallExecutor`. - id: u64, - - /// Mocks have unique ids used to identify them in Noir, allowing to update or remove them. - last_mock_id: usize, - /// The registered mocks - mocked_responses: Vec>, - /// Whether to print [`ForeignCall::Print`] output. - show_output: bool, - /// JSON RPC client to resolve foreign calls - external_resolver: Option, - /// Root path to the program or workspace in execution. - root_path: Option, - /// Name of the package in execution - package_name: Option, -} - -#[derive(Debug, Serialize, Deserialize)] -struct ResolveForeignCallRequest { - /// A session ID which allows the external RPC server to link this foreign call request to other foreign calls - /// for the same program execution. - /// - /// This is intended to allow a single RPC server to maintain state related to multiple program executions being - /// performed in parallel. - session_id: u64, - - #[serde(flatten)] - /// The foreign call which the external RPC server is to provide a response for. - function_call: ForeignCallWaitInfo, - - #[serde(skip_serializing_if = "Option::is_none")] - /// Root path to the program or workspace in execution. - root_path: Option, - #[serde(skip_serializing_if = "Option::is_none")] - /// Name of the package in execution - package_name: Option, -} - -impl DefaultForeignCallExecutor { - pub fn new( - show_output: bool, - resolver_url: Option<&str>, - root_path: Option, - package_name: Option, - ) -> Self { - let oracle_resolver = resolver_url.map(|resolver_url| { - let mut transport_builder = - Builder::new().url(resolver_url).expect("Invalid oracle resolver URL"); - - if let Some(Ok(timeout)) = - std::env::var("NARGO_FOREIGN_CALL_TIMEOUT").ok().map(|timeout| timeout.parse()) - { - let timeout_duration = std::time::Duration::from_millis(timeout); - transport_builder = transport_builder.timeout(timeout_duration); - }; - Client::with_transport(transport_builder.build()) - }); - DefaultForeignCallExecutor { - show_output, - external_resolver: oracle_resolver, - id: rand::thread_rng().gen(), - mocked_responses: Vec::new(), - last_mock_id: 0, - root_path, - package_name, - } - } -} - -impl DefaultForeignCallExecutor { - fn extract_mock_id( - foreign_call_inputs: &[ForeignCallParam], - ) -> Result<(usize, &[ForeignCallParam]), ForeignCallError> { - let (id, params) = - foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; - let id = - usize::try_from(id.unwrap_field().try_to_u64().expect("value does not fit into u64")) - .expect("value does not fit into usize"); - Ok((id, params)) - } - - fn find_mock_by_id(&self, id: usize) -> Option<&MockedCall> { - self.mocked_responses.iter().find(|response| response.id == id) - } - - fn find_mock_by_id_mut(&mut self, id: usize) -> Option<&mut MockedCall> { - self.mocked_responses.iter_mut().find(|response| response.id == id) - } - - fn parse_string(param: &ForeignCallParam) -> String { - let fields: Vec<_> = param.fields().to_vec(); - decode_string_value(&fields) - } - - fn execute_print(foreign_call_inputs: &[ForeignCallParam]) -> Result<(), ForeignCallError> { - let skip_newline = foreign_call_inputs[0].unwrap_field().is_zero(); - - let foreign_call_inputs = - foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?.1; - let display_string = Self::format_printable_value(foreign_call_inputs, skip_newline)?; - - print!("{display_string}"); - - Ok(()) - } - - fn format_printable_value( - foreign_call_inputs: &[ForeignCallParam], - skip_newline: bool, - ) -> Result { - let display_values: PrintableValueDisplay = foreign_call_inputs.try_into()?; - - let result = format!("{display_values}{}", if skip_newline { "" } else { "\n" }); - - Ok(result) - } -} - -impl Deserialize<'a>> ForeignCallExecutor - for DefaultForeignCallExecutor -{ - fn execute( - &mut self, - foreign_call: &ForeignCallWaitInfo, - ) -> Result, ForeignCallError> { - let foreign_call_name = foreign_call.function.as_str(); - match ForeignCall::lookup(foreign_call_name) { - Some(ForeignCall::Print) => { - if self.show_output { - Self::execute_print(&foreign_call.inputs)?; - } - Ok(ForeignCallResult::default()) - } - Some(ForeignCall::CreateMock) => { - let mock_oracle_name = Self::parse_string(&foreign_call.inputs[0]); - assert!(ForeignCall::lookup(&mock_oracle_name).is_none()); - let id = self.last_mock_id; - self.mocked_responses.push(MockedCall::new(id, mock_oracle_name)); - self.last_mock_id += 1; - - Ok(F::from(id).into()) - } - Some(ForeignCall::SetMockParams) => { - let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; - self.find_mock_by_id_mut(id) - .unwrap_or_else(|| panic!("Unknown mock id {}", id)) - .params = Some(params.to_vec()); - - Ok(ForeignCallResult::default()) - } - Some(ForeignCall::GetMockLastParams) => { - let (id, _) = Self::extract_mock_id(&foreign_call.inputs)?; - let mock = - self.find_mock_by_id(id).unwrap_or_else(|| panic!("Unknown mock id {}", id)); - - let last_called_params = mock - .last_called_params - .clone() - .unwrap_or_else(|| panic!("Mock {} was never called", mock.name)); - - Ok(last_called_params.into()) - } - Some(ForeignCall::SetMockReturns) => { - let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; - self.find_mock_by_id_mut(id) - .unwrap_or_else(|| panic!("Unknown mock id {}", id)) - .result = ForeignCallResult { values: params.to_vec() }; - - Ok(ForeignCallResult::default()) - } - Some(ForeignCall::SetMockTimes) => { - let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; - let times = - params[0].unwrap_field().try_to_u64().expect("Invalid bit size of times"); - - self.find_mock_by_id_mut(id) - .unwrap_or_else(|| panic!("Unknown mock id {}", id)) - .times_left = Some(times); - - Ok(ForeignCallResult::default()) - } - Some(ForeignCall::ClearMock) => { - let (id, _) = Self::extract_mock_id(&foreign_call.inputs)?; - self.mocked_responses.retain(|response| response.id != id); - Ok(ForeignCallResult::default()) - } - None => { - let mock_response_position = self - .mocked_responses - .iter() - .position(|response| response.matches(foreign_call_name, &foreign_call.inputs)); - - if let Some(response_position) = mock_response_position { - // If the program has registered a mocked response to this oracle call then we prefer responding - // with that. - - let mock = self - .mocked_responses - .get_mut(response_position) - .expect("Invalid position of mocked response"); - - mock.last_called_params = Some(foreign_call.inputs.clone()); - - let result = mock.result.values.clone(); - - if let Some(times_left) = &mut mock.times_left { - *times_left -= 1; - if *times_left == 0 { - self.mocked_responses.remove(response_position); - } - } - - Ok(result.into()) - } else if let Some(external_resolver) = &self.external_resolver { - // If the user has registered an external resolver then we forward any remaining oracle calls there. - - let encoded_params = vec![build_json_rpc_arg(ResolveForeignCallRequest { - session_id: self.id, - function_call: foreign_call.clone(), - root_path: self - .root_path - .clone() - .map(|path| path.to_str().unwrap().to_string()), - package_name: self.package_name.clone(), - })]; - - let req = - external_resolver.build_request("resolve_foreign_call", &encoded_params); - - let response = external_resolver.send_request(req)?; - - let parsed_response: ForeignCallResult = response.result()?; - - Ok(parsed_response) - } else { - // If there's no registered mock oracle response and no registered resolver then we cannot - // return a correct response to the ACVM. The best we can do is to return an empty response, - // this allows us to ignore any foreign calls which exist solely to pass information from inside - // the circuit to the environment (e.g. custom logging) as the execution will still be able to progress. - // - // We optimistically return an empty response for all oracle calls as the ACVM will error - // should a response have been required. - Ok(ForeignCallResult::default()) - } - } - } - } -} - -#[cfg(test)] -mod tests { - use acvm::{ - acir::brillig::ForeignCallParam, brillig_vm::brillig::ForeignCallResult, - pwg::ForeignCallWaitInfo, FieldElement, - }; - use jsonrpc_core::Result as RpcResult; - use jsonrpc_derive::rpc; - use jsonrpc_http_server::{Server, ServerBuilder}; - - use crate::ops::{DefaultForeignCallExecutor, ForeignCallExecutor}; - - use super::ResolveForeignCallRequest; - - #[allow(unreachable_pub)] - #[rpc] - pub trait OracleResolver { - #[rpc(name = "resolve_foreign_call")] - fn resolve_foreign_call( - &self, - req: ResolveForeignCallRequest, - ) -> RpcResult>; - } - - struct OracleResolverImpl; - - impl OracleResolverImpl { - fn echo(&self, param: ForeignCallParam) -> ForeignCallResult { - vec![param].into() - } - - fn sum(&self, array: ForeignCallParam) -> ForeignCallResult { - let mut res: FieldElement = 0_usize.into(); - - for value in array.fields() { - res += value; - } - - res.into() - } - } - - impl OracleResolver for OracleResolverImpl { - fn resolve_foreign_call( - &self, - req: ResolveForeignCallRequest, - ) -> RpcResult> { - let response = match req.function_call.function.as_str() { - "sum" => self.sum(req.function_call.inputs[0].clone()), - "echo" => self.echo(req.function_call.inputs[0].clone()), - "id" => FieldElement::from(req.session_id as u128).into(), - - _ => panic!("unexpected foreign call"), - }; - Ok(response) - } - } - - fn build_oracle_server() -> (Server, String) { - let mut io = jsonrpc_core::IoHandler::new(); - io.extend_with(OracleResolverImpl.to_delegate()); - - // Choosing port 0 results in a random port being assigned. - let server = ServerBuilder::new(io) - .start_http(&"127.0.0.1:0".parse().expect("Invalid address")) - .expect("Could not start server"); - - let url = format!("http://{}", server.address()); - (server, url) - } - - #[test] - fn test_oracle_resolver_echo() { - let (server, url) = build_oracle_server(); - - let mut executor = - DefaultForeignCallExecutor::::new(false, Some(&url), None, None); - - let foreign_call = ForeignCallWaitInfo { - function: "echo".to_string(), - inputs: vec![ForeignCallParam::Single(1_u128.into())], - }; - - let result = executor.execute(&foreign_call); - assert_eq!(result.unwrap(), ForeignCallResult { values: foreign_call.inputs }); - - server.close(); - } - - #[test] - fn test_oracle_resolver_sum() { - let (server, url) = build_oracle_server(); - - let mut executor = DefaultForeignCallExecutor::new(false, Some(&url), None, None); - - let foreign_call = ForeignCallWaitInfo { - function: "sum".to_string(), - inputs: vec![ForeignCallParam::Array(vec![1_usize.into(), 2_usize.into()])], - }; - - let result = executor.execute(&foreign_call); - assert_eq!(result.unwrap(), FieldElement::from(3_usize).into()); - - server.close(); - } - - #[test] - fn foreign_call_executor_id_is_persistent() { - let (server, url) = build_oracle_server(); - - let mut executor = - DefaultForeignCallExecutor::::new(false, Some(&url), None, None); - - let foreign_call = ForeignCallWaitInfo { function: "id".to_string(), inputs: Vec::new() }; - - let result_1 = executor.execute(&foreign_call).unwrap(); - let result_2 = executor.execute(&foreign_call).unwrap(); - assert_eq!(result_1, result_2); - - server.close(); - } - - #[test] - fn oracle_resolver_rpc_can_distinguish_executors() { - let (server, url) = build_oracle_server(); - - let mut executor_1 = - DefaultForeignCallExecutor::::new(false, Some(&url), None, None); - let mut executor_2 = - DefaultForeignCallExecutor::::new(false, Some(&url), None, None); - - let foreign_call = ForeignCallWaitInfo { function: "id".to_string(), inputs: Vec::new() }; - - let result_1 = executor_1.execute(&foreign_call).unwrap(); - let result_2 = executor_2.execute(&foreign_call).unwrap(); - assert_ne!(result_1, result_2); - - server.close(); - } -} diff --git a/tooling/nargo/src/ops/mod.rs b/tooling/nargo/src/ops/mod.rs index f70577a14f1..04efeb5a9ec 100644 --- a/tooling/nargo/src/ops/mod.rs +++ b/tooling/nargo/src/ops/mod.rs @@ -4,7 +4,6 @@ pub use self::compile::{ compile_workspace, report_errors, }; pub use self::execute::{execute_program, execute_program_with_profiling}; -pub use self::foreign_calls::{DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor}; pub use self::optimize::{optimize_contract, optimize_program}; pub use self::transform::{transform_contract, transform_program}; @@ -13,7 +12,6 @@ pub use self::test::{run_test, TestStatus}; mod check; mod compile; mod execute; -mod foreign_calls; mod optimize; mod test; mod transform; diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index 370a4235f61..51d7b8c55aa 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -9,9 +9,11 @@ use noirc_driver::{compile_no_check, CompileError, CompileOptions}; use noirc_errors::{debug_info::DebugInfo, FileDiagnostic}; use noirc_frontend::hir::{def_map::TestFunction, Context}; -use crate::{errors::try_to_diagnose_runtime_error, NargoError}; +use crate::{ + errors::try_to_diagnose_runtime_error, foreign_calls::DefaultForeignCallExecutor, NargoError, +}; -use super::{execute_program, DefaultForeignCallExecutor}; +use super::execute_program; pub enum TestStatus { Pass, diff --git a/tooling/nargo_cli/benches/criterion.rs b/tooling/nargo_cli/benches/criterion.rs index 488cbfcd243..51de97df139 100644 --- a/tooling/nargo_cli/benches/criterion.rs +++ b/tooling/nargo_cli/benches/criterion.rs @@ -115,7 +115,7 @@ fn criterion_test_execution(c: &mut Criterion, test_program_dir: &Path, force_br let artifacts = RefCell::new(None); let mut foreign_call_executor = - nargo::ops::DefaultForeignCallExecutor::new(false, None, None, None); + nargo::foreign_calls::DefaultForeignCallExecutor::new(false, None, None, None); c.bench_function(&benchmark_name, |b| { b.iter_batched( diff --git a/tooling/nargo_cli/src/cli/execute_cmd.rs b/tooling/nargo_cli/src/cli/execute_cmd.rs index 8dc71b1c7e5..fa95d3123c6 100644 --- a/tooling/nargo_cli/src/cli/execute_cmd.rs +++ b/tooling/nargo_cli/src/cli/execute_cmd.rs @@ -7,7 +7,7 @@ use clap::Args; use nargo::constants::PROVER_INPUT_FILE; use nargo::errors::try_to_diagnose_runtime_error; -use nargo::ops::DefaultForeignCallExecutor; +use nargo::foreign_calls::DefaultForeignCallExecutor; use nargo::package::{CrateName, Package}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_abi::input_parser::{Format, InputValue}; diff --git a/tooling/nargo_cli/src/cli/info_cmd.rs b/tooling/nargo_cli/src/cli/info_cmd.rs index cf416b1fa5f..769a1f79d81 100644 --- a/tooling/nargo_cli/src/cli/info_cmd.rs +++ b/tooling/nargo_cli/src/cli/info_cmd.rs @@ -4,7 +4,7 @@ use clap::Args; use iter_extended::vecmap; use nargo::{ constants::PROVER_INPUT_FILE, - ops::DefaultForeignCallExecutor, + foreign_calls::DefaultForeignCallExecutor, package::{CrateName, Package}, }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; diff --git a/tooling/nargo_cli/tests/stdlib-props.rs b/tooling/nargo_cli/tests/stdlib-props.rs index 0013a90b4ff..370d2c04052 100644 --- a/tooling/nargo_cli/tests/stdlib-props.rs +++ b/tooling/nargo_cli/tests/stdlib-props.rs @@ -2,10 +2,7 @@ use std::{cell::RefCell, collections::BTreeMap, path::Path}; use acvm::{acir::native_types::WitnessStack, AcirField, FieldElement}; use iter_extended::vecmap; -use nargo::{ - ops::{execute_program, DefaultForeignCallExecutor}, - parse_all, -}; +use nargo::{foreign_calls::DefaultForeignCallExecutor, ops::execute_program, parse_all}; use noirc_abi::input_parser::InputValue; use noirc_driver::{ compile_main, file_manager_with_stdlib, prepare_crate, CompilationResult, CompileOptions, diff --git a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs index 981d08a3eb1..6d6da89f660 100644 --- a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs @@ -8,7 +8,7 @@ use crate::flamegraph::{BrilligExecutionSample, FlamegraphGenerator, InfernoFlam use crate::fs::{read_inputs_from_file, read_program_from_file}; use crate::opcode_formatter::format_brillig_opcode; use bn254_blackbox_solver::Bn254BlackBoxSolver; -use nargo::ops::DefaultForeignCallExecutor; +use nargo::foreign_calls::DefaultForeignCallExecutor; use noirc_abi::input_parser::Format; use noirc_artifacts::debug::DebugArtifact; From e3a0914c6dede6f54f426ed7d790a0c98a7e0908 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:50:49 +0000 Subject: [PATCH 3/4] feat: allow ignoring test failures from foreign calls (#6660) --- .github/workflows/test-js-packages.yml | 30 +++-- tooling/lsp/src/requests/test_run.rs | 5 + tooling/nargo/src/foreign_calls/mod.rs | 6 +- tooling/nargo/src/foreign_calls/print.rs | 2 +- tooling/nargo/src/foreign_calls/rpc.rs | 4 +- tooling/nargo/src/ops/test.rs | 146 +++++++++++++++++++++-- tooling/nargo_cli/src/cli/test_cmd.rs | 6 + tooling/nargo_cli/tests/stdlib-tests.rs | 6 + 8 files changed, 180 insertions(+), 25 deletions(-) diff --git a/.github/workflows/test-js-packages.yml b/.github/workflows/test-js-packages.yml index 4a5d0b8179b..422a30ed08f 100644 --- a/.github/workflows/test-js-packages.yml +++ b/.github/workflows/test-js-packages.yml @@ -519,12 +519,25 @@ jobs: fail-fast: false matrix: project: - # Disabled as these are currently failing with many visibility errors - - { repo: AztecProtocol/aztec-nr, path: ./ } + - { repo: noir-lang/ec, path: ./ } + - { repo: noir-lang/eddsa, path: ./ } + - { repo: noir-lang/mimc, path: ./ } + - { repo: noir-lang/noir_sort, path: ./ } + - { repo: noir-lang/noir-edwards, path: ./ } + - { repo: noir-lang/noir-bignum, path: ./ } + - { repo: noir-lang/noir_bigcurve, path: ./ } + - { repo: noir-lang/noir_base64, path: ./ } + - { repo: noir-lang/noir_string_search, path: ./ } + - { repo: noir-lang/sparse_array, path: ./ } + - { repo: noir-lang/noir_rsa, path: ./lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/aztec-nr } - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-contracts } - # Disabled as aztec-packages requires a setup-step in order to generate a `Nargo.toml` - #- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits } - - { repo: noir-lang/noir-edwards, path: ./, ref: 3188ea74fe3b059219a2ea87899589c266256d74 } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/parity-lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/private-kernel-lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/reset-kernel-lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/rollup-lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/types } + name: Check external repo - ${{ matrix.project.repo }} steps: - name: Checkout @@ -554,9 +567,12 @@ jobs: # Github actions seems to not expand "**" in globs by default. shopt -s globstar sed -i '/^compiler_version/d' ./**/Nargo.toml - - name: Run nargo check + + - name: Run nargo test working-directory: ./test-repo/${{ matrix.project.path }} - run: nargo check + run: nargo test --silence-warnings + env: + NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: true # This is a job which depends on all test jobs and reports the overall status. # This allows us to add/remove test jobs without having to update the required workflows. diff --git a/tooling/lsp/src/requests/test_run.rs b/tooling/lsp/src/requests/test_run.rs index 50c699bb6a6..937fdcc0a5e 100644 --- a/tooling/lsp/src/requests/test_run.rs +++ b/tooling/lsp/src/requests/test_run.rs @@ -101,6 +101,11 @@ fn on_test_run_request_inner( result: "fail".to_string(), message: Some(message), }, + TestStatus::Skipped => NargoTestRunResult { + id: params.id.clone(), + result: "skipped".to_string(), + message: None, + }, TestStatus::CompileError(diag) => NargoTestRunResult { id: params.id.clone(), result: "error".to_string(), diff --git a/tooling/nargo/src/foreign_calls/mod.rs b/tooling/nargo/src/foreign_calls/mod.rs index 0ef3247ee59..16ed71e11e3 100644 --- a/tooling/nargo/src/foreign_calls/mod.rs +++ b/tooling/nargo/src/foreign_calls/mod.rs @@ -8,9 +8,9 @@ use rand::Rng; use rpc::RPCForeignCallExecutor; use serde::{Deserialize, Serialize}; -mod mocker; -mod print; -mod rpc; +pub(crate) mod mocker; +pub(crate) mod print; +pub(crate) mod rpc; pub trait ForeignCallExecutor { fn execute( diff --git a/tooling/nargo/src/foreign_calls/print.rs b/tooling/nargo/src/foreign_calls/print.rs index d0befa0bd0b..92fcd65ae28 100644 --- a/tooling/nargo/src/foreign_calls/print.rs +++ b/tooling/nargo/src/foreign_calls/print.rs @@ -4,7 +4,7 @@ use noirc_printable_type::{ForeignCallError, PrintableValueDisplay}; use super::{ForeignCall, ForeignCallExecutor}; #[derive(Debug, Default)] -pub(super) struct PrintForeignCallExecutor; +pub(crate) struct PrintForeignCallExecutor; impl ForeignCallExecutor for PrintForeignCallExecutor { fn execute( diff --git a/tooling/nargo/src/foreign_calls/rpc.rs b/tooling/nargo/src/foreign_calls/rpc.rs index dab64819b56..0653eb1c7e3 100644 --- a/tooling/nargo/src/foreign_calls/rpc.rs +++ b/tooling/nargo/src/foreign_calls/rpc.rs @@ -8,7 +8,7 @@ use serde::{Deserialize, Serialize}; use super::ForeignCallExecutor; #[derive(Debug)] -pub(super) struct RPCForeignCallExecutor { +pub(crate) struct RPCForeignCallExecutor { /// A randomly generated id for this `DefaultForeignCallExecutor`. /// /// This is used so that a single `external_resolver` can distinguish between requests from multiple @@ -44,7 +44,7 @@ struct ResolveForeignCallRequest { } impl RPCForeignCallExecutor { - pub(super) fn new( + pub(crate) fn new( resolver_url: &str, id: u64, root_path: Option, diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index 51d7b8c55aa..6e7a7d1b7db 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -1,16 +1,28 @@ use std::path::PathBuf; use acvm::{ - acir::native_types::{WitnessMap, WitnessStack}, - BlackBoxFunctionSolver, FieldElement, + acir::{ + brillig::ForeignCallResult, + native_types::{WitnessMap, WitnessStack}, + }, + pwg::ForeignCallWaitInfo, + AcirField, BlackBoxFunctionSolver, FieldElement, }; use noirc_abi::Abi; use noirc_driver::{compile_no_check, CompileError, CompileOptions}; use noirc_errors::{debug_info::DebugInfo, FileDiagnostic}; use noirc_frontend::hir::{def_map::TestFunction, Context}; +use noirc_printable_type::ForeignCallError; +use rand::Rng; +use serde::{Deserialize, Serialize}; use crate::{ - errors::try_to_diagnose_runtime_error, foreign_calls::DefaultForeignCallExecutor, NargoError, + errors::try_to_diagnose_runtime_error, + foreign_calls::{ + mocker::MockForeignCallExecutor, print::PrintForeignCallExecutor, + rpc::RPCForeignCallExecutor, DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor, + }, + NargoError, }; use super::execute_program; @@ -18,12 +30,13 @@ use super::execute_program; pub enum TestStatus { Pass, Fail { message: String, error_diagnostic: Option }, + Skipped, CompileError(FileDiagnostic), } impl TestStatus { pub fn failed(&self) -> bool { - !matches!(self, TestStatus::Pass) + !matches!(self, TestStatus::Pass | TestStatus::Skipped) } } @@ -50,23 +63,42 @@ pub fn run_test>( if test_function_has_no_arguments { // Run the backend to ensure the PWG evaluates functions like std::hash::pedersen, // otherwise constraints involving these expressions will not error. + let mut foreign_call_executor = TestForeignCallExecutor::new( + show_output, + foreign_call_resolver_url, + root_path, + package_name, + ); + let circuit_execution = execute_program( &compiled_program.program, WitnessMap::new(), blackbox_solver, - &mut DefaultForeignCallExecutor::new( - show_output, - foreign_call_resolver_url, - root_path, - package_name, - ), + &mut foreign_call_executor, ); - test_status_program_compile_pass( + + let status = test_status_program_compile_pass( test_function, compiled_program.abi, compiled_program.debug, circuit_execution, - ) + ); + + let ignore_foreign_call_failures = + std::env::var("NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS") + .is_ok_and(|var| &var == "true"); + + if let TestStatus::Fail { .. } = status { + if ignore_foreign_call_failures + && foreign_call_executor.encountered_unknown_foreign_call + { + TestStatus::Skipped + } else { + status + } + } else { + status + } } else { #[cfg(target_arch = "wasm32")] { @@ -217,3 +249,93 @@ fn check_expected_failure_message( error_diagnostic, } } + +/// A specialized foreign call executor which tracks whether it has encountered any unknown foreign calls +struct TestForeignCallExecutor { + /// The executor for any [`ForeignCall::Print`] calls. + printer: Option, + mocker: MockForeignCallExecutor, + external: Option, + + encountered_unknown_foreign_call: bool, +} + +impl TestForeignCallExecutor { + fn new( + show_output: bool, + resolver_url: Option<&str>, + root_path: Option, + package_name: Option, + ) -> Self { + let id = rand::thread_rng().gen(); + let printer = if show_output { Some(PrintForeignCallExecutor) } else { None }; + let external_resolver = resolver_url.map(|resolver_url| { + RPCForeignCallExecutor::new(resolver_url, id, root_path, package_name) + }); + TestForeignCallExecutor { + printer, + mocker: MockForeignCallExecutor::default(), + external: external_resolver, + encountered_unknown_foreign_call: false, + } + } +} + +impl Deserialize<'a>> ForeignCallExecutor + for TestForeignCallExecutor +{ + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + // If the circuit has reached a new foreign call opcode then it can't have failed from any previous unknown foreign calls. + self.encountered_unknown_foreign_call = false; + + let foreign_call_name = foreign_call.function.as_str(); + match ForeignCall::lookup(foreign_call_name) { + Some(ForeignCall::Print) => { + if let Some(printer) = &mut self.printer { + printer.execute(foreign_call) + } else { + Ok(ForeignCallResult::default()) + } + } + + Some( + ForeignCall::CreateMock + | ForeignCall::SetMockParams + | ForeignCall::GetMockLastParams + | ForeignCall::SetMockReturns + | ForeignCall::SetMockTimes + | ForeignCall::ClearMock, + ) => self.mocker.execute(foreign_call), + + None => { + // First check if there's any defined mock responses for this foreign call. + match self.mocker.execute(foreign_call) { + Err(ForeignCallError::NoHandler(_)) => (), + response_or_error => return response_or_error, + }; + + if let Some(external_resolver) = &mut self.external { + // If the user has registered an external resolver then we forward any remaining oracle calls there. + match external_resolver.execute(foreign_call) { + Err(ForeignCallError::NoHandler(_)) => (), + response_or_error => return response_or_error, + }; + } + + self.encountered_unknown_foreign_call = true; + + // If all executors have no handler for the given foreign call then we cannot + // return a correct response to the ACVM. The best we can do is to return an empty response, + // this allows us to ignore any foreign calls which exist solely to pass information from inside + // the circuit to the environment (e.g. custom logging) as the execution will still be able to progress. + // + // We optimistically return an empty response for all oracle calls as the ACVM will error + // should a response have been required. + Ok(ForeignCallResult::default()) + } + } + } +} diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 7b0201226ef..aa0ee1bb94b 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -255,6 +255,12 @@ fn display_test_report( ); } } + TestStatus::Skipped { .. } => { + writer + .set_color(ColorSpec::new().set_fg(Some(Color::Yellow))) + .expect("Failed to set color"); + writeln!(writer, "skipped").expect("Failed to write to stderr"); + } TestStatus::CompileError(err) => { noirc_errors::reporter::report_all( file_manager.as_file_map(), diff --git a/tooling/nargo_cli/tests/stdlib-tests.rs b/tooling/nargo_cli/tests/stdlib-tests.rs index bdc92e625ab..99f0c9a2e7f 100644 --- a/tooling/nargo_cli/tests/stdlib-tests.rs +++ b/tooling/nargo_cli/tests/stdlib-tests.rs @@ -138,6 +138,12 @@ fn display_test_report( ); } } + TestStatus::Skipped { .. } => { + writer + .set_color(ColorSpec::new().set_fg(Some(Color::Yellow))) + .expect("Failed to set color"); + writeln!(writer, "skipped").expect("Failed to write to stderr"); + } TestStatus::CompileError(err) => { noirc_errors::reporter::report_all( file_manager.as_file_map(), From 6acef6d795cd74dee4a21e82c5a912d58b40b06c Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Mon, 2 Dec 2024 17:11:52 +0100 Subject: [PATCH 4/4] feat: add memory report into the CI (#6630) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .github/workflows/memory_report.yml | 88 +++++++++++++++++++++++++++++ cspell.json | 1 + test_programs/memory_report.sh | 48 ++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 .github/workflows/memory_report.yml create mode 100755 test_programs/memory_report.sh diff --git a/.github/workflows/memory_report.yml b/.github/workflows/memory_report.yml new file mode 100644 index 00000000000..a1b28aee31d --- /dev/null +++ b/.github/workflows/memory_report.yml @@ -0,0 +1,88 @@ +name: Report Peak Memory + +on: + push: + branches: + - master + pull_request: + +jobs: + build-nargo: + runs-on: ubuntu-latest + strategy: + matrix: + target: [x86_64-unknown-linux-gnu] + + steps: + - name: Checkout Noir repo + uses: actions/checkout@v4 + + - name: Setup toolchain + uses: dtolnay/rust-toolchain@1.74.1 + + - uses: Swatinem/rust-cache@v2 + with: + key: ${{ matrix.target }} + cache-on-failure: true + save-if: ${{ github.event_name != 'merge_group' }} + + - name: Build Nargo + run: cargo build --package nargo_cli --release + + - name: Package artifacts + run: | + mkdir dist + cp ./target/release/nargo ./dist/nargo + + - name: Upload artifact + uses: actions/upload-artifact@v4 + with: + name: nargo + path: ./dist/* + retention-days: 3 + + generate_memory_report: + needs: [build-nargo] + runs-on: ubuntu-latest + permissions: + pull-requests: write + + steps: + - uses: actions/checkout@v4 + + - name: Download nargo binary + uses: actions/download-artifact@v4 + with: + name: nargo + path: ./nargo + + - name: Set nargo on PATH + run: | + nargo_binary="${{ github.workspace }}/nargo/nargo" + chmod +x $nargo_binary + echo "$(dirname $nargo_binary)" >> $GITHUB_PATH + export PATH="$PATH:$(dirname $nargo_binary)" + nargo -V + + - name: Generate Memory report + working-directory: ./test_programs + run: | + chmod +x memory_report.sh + ./memory_report.sh + mv memory_report.json ../memory_report.json + + - name: Parse memory report + id: memory_report + uses: noir-lang/noir-bench-report@d61bc78ece3c8df1e72914b3d5136bad403d5bdf + with: + report: memory_report.json + header: | + # Memory Report + memory_report: true + + - name: Add memory report to sticky comment + if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target' + uses: marocchino/sticky-pull-request-comment@v2 + with: + header: memory + message: ${{ steps.memory_report.outputs.markdown }} \ No newline at end of file diff --git a/cspell.json b/cspell.json index 36bba737cd7..15bba2cb5f8 100644 --- a/cspell.json +++ b/cspell.json @@ -106,6 +106,7 @@ "Guillaume", "gzipped", "hasher", + "heaptrack", "hexdigit", "higher-kinded", "Hindley-Milner", diff --git a/test_programs/memory_report.sh b/test_programs/memory_report.sh new file mode 100755 index 00000000000..edb254c4ae4 --- /dev/null +++ b/test_programs/memory_report.sh @@ -0,0 +1,48 @@ +#!/usr/bin/env bash +set -e + +sudo apt-get install heaptrack + +NARGO="nargo" + + +# Tests to be profiled for memory report +tests_to_profile=("keccak256" "workspace" "regression_4709") + +current_dir=$(pwd) +execution_success_path="$current_dir/execution_success" +test_dirs=$(ls $execution_success_path) + +FIRST="1" + +echo "{\"memory_reports\": [ " > memory_report.json + + +for test_name in ${tests_to_profile[@]}; do + full_path=$execution_success_path"/"$test_name + cd $full_path + + if [ $FIRST = "1" ] + then + FIRST="0" + else + echo " ," >> $current_dir"/memory_report.json" + fi + heaptrack --output $current_dir/$test_name"_heap" $NARGO compile --force + if test -f $current_dir/$test_name"_heap.gz"; + then + heaptrack --analyze $current_dir/$test_name"_heap.gz" > $current_dir/$test_name"_heap_analysis.txt" + rm $current_dir/$test_name"_heap.gz" + else + heaptrack --analyze $current_dir/$test_name"_heap.zst" > $current_dir/$test_name"_heap_analysis.txt" + rm $current_dir/$test_name"_heap.zst" + fi + consumption="$(grep 'peak heap memory consumption' $current_dir/$test_name'_heap_analysis.txt')" + len=${#consumption}-30 + peak=${consumption:30:len} + rm $current_dir/$test_name"_heap_analysis.txt" + echo -e " {\n \"artifact_name\":\"$test_name\",\n \"peak_memory\":\"$peak\"\n }" >> $current_dir"/memory_report.json" +done + +echo "]}" >> $current_dir"/memory_report.json" +