From 21b06a64c5c3121eafee045ff4a11172e260f082 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 7 Jun 2024 16:51:19 +0000 Subject: [PATCH 1/7] feat!: add session id to foreign call RPC requests --- Cargo.lock | 2 + Cargo.toml | 1 + acvm-repo/acvm/Cargo.toml | 3 +- acvm-repo/acvm/src/pwg/brillig.rs | 3 +- acvm-repo/brillig/src/foreign_call.rs | 1 + tooling/acvm_cli/Cargo.toml | 2 +- tooling/nargo/Cargo.toml | 1 + tooling/nargo/src/ops/foreign_calls.rs | 57 +++++++++++++++++++------- 8 files changed, 53 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 61bfdd08c0e..8e40315cdd7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,6 +50,7 @@ dependencies = [ "paste", "proptest", "rand 0.8.5", + "serde", "thiserror", "tracing", ] @@ -2485,6 +2486,7 @@ dependencies = [ "noirc_errors", "noirc_frontend", "noirc_printable_type", + "rand 0.8.5", "rayon", "serde", "tempfile", diff --git a/Cargo.toml b/Cargo.toml index 182580f8d67..d23089adb56 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -131,6 +131,7 @@ similar-asserts = "1.5.0" tempfile = "3.6.0" jsonrpc = { version = "0.16.0", features = ["minreq_http"] } flate2 = "1.0.24" +rand = "0.8.5" im = { version = "15.1", features = ["serde"] } tracing = "0.1.40" diff --git a/acvm-repo/acvm/Cargo.toml b/acvm-repo/acvm/Cargo.toml index 577978939b0..0bd6ab6b7c7 100644 --- a/acvm-repo/acvm/Cargo.toml +++ b/acvm-repo/acvm/Cargo.toml @@ -16,6 +16,7 @@ repository.workspace = true num-bigint.workspace = true thiserror.workspace = true tracing.workspace = true +serde.workspace = true acir.workspace = true brillig_vm.workspace = true @@ -36,7 +37,7 @@ bls12_381 = [ ] [dev-dependencies] -rand = "0.8.5" +rand.workspace = true proptest = "1.2.0" paste = "1.0.14" ark-bls12-381 = { version = "^0.4.0", default-features = false, features = ["curve"] } \ No newline at end of file diff --git a/acvm-repo/acvm/src/pwg/brillig.rs b/acvm-repo/acvm/src/pwg/brillig.rs index 7e6c207b69a..3a639df044a 100644 --- a/acvm-repo/acvm/src/pwg/brillig.rs +++ b/acvm-repo/acvm/src/pwg/brillig.rs @@ -13,6 +13,7 @@ use acir::{ }; use acvm_blackbox_solver::BlackBoxFunctionSolver; use brillig_vm::{FailureReason, MemoryValue, VMStatus, VM}; +use serde::{Deserialize, Serialize}; use crate::{pwg::OpcodeNotSolvable, OpcodeResolutionError}; @@ -286,7 +287,7 @@ impl<'b, B: BlackBoxFunctionSolver, F: AcirField> BrilligSolver<'b, F, B> { /// where the result of the foreign call has not yet been provided. /// /// The caller must resolve this opcode externally based upon the information in the request. -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)] pub struct ForeignCallWaitInfo { /// An identifier interpreted by the caller process pub function: String, diff --git a/acvm-repo/brillig/src/foreign_call.rs b/acvm-repo/brillig/src/foreign_call.rs index a439d5c3202..9a45a4d2f20 100644 --- a/acvm-repo/brillig/src/foreign_call.rs +++ b/acvm-repo/brillig/src/foreign_call.rs @@ -3,6 +3,7 @@ use serde::{Deserialize, Serialize}; /// Single output of a [foreign call][crate::Opcode::ForeignCall]. #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)] +#[serde(untagged)] pub enum ForeignCallParam { Single(F), Array(Vec), diff --git a/tooling/acvm_cli/Cargo.toml b/tooling/acvm_cli/Cargo.toml index 72424405d36..1cfd1f3b270 100644 --- a/tooling/acvm_cli/Cargo.toml +++ b/tooling/acvm_cli/Cargo.toml @@ -33,6 +33,6 @@ tracing-subscriber.workspace = true tracing-appender = "0.2.3" [dev-dependencies] -rand = "0.8.5" +rand.workspace = true proptest = "1.2.0" paste = "1.0.14" diff --git a/tooling/nargo/Cargo.toml b/tooling/nargo/Cargo.toml index 48047d10ea6..8abec267d20 100644 --- a/tooling/nargo/Cargo.toml +++ b/tooling/nargo/Cargo.toml @@ -24,6 +24,7 @@ codespan-reporting.workspace = true tracing.workspace = true rayon = "1.8.0" jsonrpc.workspace = true +rand.workspace = true [dev-dependencies] # TODO: This dependency is used to generate unit tests for `get_all_paths_in_dir` diff --git a/tooling/nargo/src/ops/foreign_calls.rs b/tooling/nargo/src/ops/foreign_calls.rs index c6b284beb13..921743aa0c1 100644 --- a/tooling/nargo/src/ops/foreign_calls.rs +++ b/tooling/nargo/src/ops/foreign_calls.rs @@ -5,6 +5,8 @@ use acvm::{ }; 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( @@ -96,6 +98,12 @@ impl MockedCall { #[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 @@ -106,6 +114,14 @@ pub struct DefaultForeignCallExecutor { external_resolver: Option, } +#[derive(Debug, Serialize, Deserialize)] +struct ResolveForeignCallRequest { + id: u64, + + #[serde(flatten)] + function_call: ForeignCallWaitInfo +} + impl DefaultForeignCallExecutor { pub fn new(show_output: bool, resolver_url: Option<&str>) -> Self { let oracle_resolver = resolver_url.map(|resolver_url| { @@ -123,6 +139,7 @@ impl DefaultForeignCallExecutor { DefaultForeignCallExecutor { show_output, external_resolver: oracle_resolver, + id: rand::thread_rng().gen(), ..DefaultForeignCallExecutor::default() } } @@ -275,10 +292,12 @@ impl ForeignCallExecutor for DefaultForeignCallExecutor { } 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<_> = - foreign_call.inputs.iter().map(build_json_rpc_arg).collect(); + let encoded_params = vec![build_json_rpc_arg(ResolveForeignCallRequest{id:self.id, function_call: foreign_call.clone() })]; + + let req = + external_resolver.build_request("resolve_foreign_call", &encoded_params); - let req = external_resolver.build_request(foreign_call_name, &encoded_params); + println!("{req:?}"); let response = external_resolver.send_request(req)?; @@ -310,27 +329,24 @@ mod tests { use jsonrpc_derive::rpc; use jsonrpc_http_server::{Server, ServerBuilder}; - use crate::ops::{DefaultForeignCallExecutor, ForeignCallExecutor}; + use crate::ops::{ DefaultForeignCallExecutor, ForeignCallExecutor, + }; + + use super::ResolveForeignCallRequest; #[allow(unreachable_pub)] #[rpc] pub trait OracleResolver { - #[rpc(name = "echo")] - fn echo( - &self, - param: ForeignCallParam, - ) -> RpcResult>; - - #[rpc(name = "sum")] - fn sum( + #[rpc(name = "resolve_foreign_call")] + fn resolve_foreign_call( &self, - array: ForeignCallParam, + req: ResolveForeignCallRequest, ) -> RpcResult>; } struct OracleResolverImpl; - impl OracleResolver for OracleResolverImpl { + impl OracleResolverImpl { fn echo( &self, param: ForeignCallParam, @@ -352,6 +368,19 @@ mod tests { } } + impl OracleResolver for OracleResolverImpl { + fn resolve_foreign_call( + &self, + req: ResolveForeignCallRequest, + ) -> RpcResult> { + 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()), + _ => panic!("unexpected foreign call"), + } + } + } + fn build_oracle_server() -> (Server, String) { let mut io = jsonrpc_core::IoHandler::new(); io.extend_with(OracleResolverImpl.to_delegate()); From 3b46e106c8edafb7f5902e5a24590071e0b7806a Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 7 Jun 2024 17:04:01 +0000 Subject: [PATCH 2/7] chore: update docs --- docs/docs/how_to/how-to-oracles.md | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/docs/docs/how_to/how-to-oracles.md b/docs/docs/how_to/how-to-oracles.md index 2811968c634..62ead1f534f 100644 --- a/docs/docs/how_to/how-to-oracles.md +++ b/docs/docs/how_to/how-to-oracles.md @@ -137,24 +137,17 @@ app.listen(5555); Now, we will add our `getSqrt` method, as expected by the `#[oracle(getSqrt)]` decorator in our Noir code. It maps through the params array and returns their square roots: ```js -server.addMethod("getSqrt", async (params) => { - const values = params[0].Array.map((field) => { +server.addMethod("resolve_function_call", async (params) => { + if params.function !== "getSqrt" { + throw Error("Unexpected foreign call") + }; + const values = params.inputs[0].Array.map((field) => { return `${Math.sqrt(parseInt(field, 16))}`; }); return { values: [{ Array: values }] }; }); ``` -:::tip - -Brillig expects an object with an array of values. Each value is an object declaring to be `Single` or `Array` and returning a field element *as a string*. For example: - -```json -{ "values": [{ "Array": ["1", "2"] }]} -{ "values": [{ "Single": "1" }]} -{ "values": [{ "Single": "1" }, { "Array": ["1", "2"] }]} -``` - If you're using Typescript, the following types may be helpful in understanding the expected return value and making sure they're easy to follow: ```js From 161fe916e0c4c6a3e2df40ddcbecd0df2052cab1 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 7 Jun 2024 17:06:21 +0000 Subject: [PATCH 3/7] chore: fmt --- tooling/nargo/src/ops/foreign_calls.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tooling/nargo/src/ops/foreign_calls.rs b/tooling/nargo/src/ops/foreign_calls.rs index 921743aa0c1..5ff3e58cf24 100644 --- a/tooling/nargo/src/ops/foreign_calls.rs +++ b/tooling/nargo/src/ops/foreign_calls.rs @@ -119,7 +119,7 @@ struct ResolveForeignCallRequest { id: u64, #[serde(flatten)] - function_call: ForeignCallWaitInfo + function_call: ForeignCallWaitInfo, } impl DefaultForeignCallExecutor { @@ -292,7 +292,10 @@ impl ForeignCallExecutor for DefaultForeignCallExecutor { } 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{id:self.id, function_call: foreign_call.clone() })]; + let encoded_params = vec![build_json_rpc_arg(ResolveForeignCallRequest { + id: self.id, + function_call: foreign_call.clone(), + })]; let req = external_resolver.build_request("resolve_foreign_call", &encoded_params); @@ -329,8 +332,7 @@ mod tests { use jsonrpc_derive::rpc; use jsonrpc_http_server::{Server, ServerBuilder}; - use crate::ops::{ DefaultForeignCallExecutor, ForeignCallExecutor, - }; + use crate::ops::{DefaultForeignCallExecutor, ForeignCallExecutor}; use super::ResolveForeignCallRequest; From 2d4797b6fe5e40f393250daf1c8d393eaeb75839 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Fri, 7 Jun 2024 17:14:26 +0000 Subject: [PATCH 4/7] chore: add test for executors being distinguishable --- tooling/nargo/src/ops/foreign_calls.rs | 37 ++++++++++++++++++++------ 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/tooling/nargo/src/ops/foreign_calls.rs b/tooling/nargo/src/ops/foreign_calls.rs index 5ff3e58cf24..e8a2a73b773 100644 --- a/tooling/nargo/src/ops/foreign_calls.rs +++ b/tooling/nargo/src/ops/foreign_calls.rs @@ -300,8 +300,6 @@ impl ForeignCallExecutor for DefaultForeignCallExecutor { let req = external_resolver.build_request("resolve_foreign_call", &encoded_params); - println!("{req:?}"); - let response = external_resolver.send_request(req)?; let parsed_response: ForeignCallResult = response.result()?; @@ -352,21 +350,21 @@ mod tests { fn echo( &self, param: ForeignCallParam, - ) -> RpcResult> { - Ok(vec![param].into()) + ) -> ForeignCallResult { + vec![param].into() } fn sum( &self, array: ForeignCallParam, - ) -> RpcResult> { + ) -> ForeignCallResult { let mut res: FieldElement = 0_usize.into(); for value in array.fields() { res += value; } - Ok(res.into()) + res.into() } } @@ -375,11 +373,14 @@ mod tests { &self, req: ResolveForeignCallRequest, ) -> RpcResult> { - match req.function_call.function.as_str() { + 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.id as u128).into(), + _ => panic!("unexpected foreign call"), - } + }; + Ok(response.into()) } } @@ -429,4 +430,24 @@ mod tests { 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)); + let mut executor_2 = DefaultForeignCallExecutor::new(false, Some(&url)); + + 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(); + } } From e5368f52bf5ec1aac3ea0670143a27de6377c955 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 10 Jun 2024 10:26:31 +0100 Subject: [PATCH 5/7] chore: fmt --- tooling/nargo/src/ops/foreign_calls.rs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/tooling/nargo/src/ops/foreign_calls.rs b/tooling/nargo/src/ops/foreign_calls.rs index e8a2a73b773..3063d6e21fc 100644 --- a/tooling/nargo/src/ops/foreign_calls.rs +++ b/tooling/nargo/src/ops/foreign_calls.rs @@ -347,17 +347,11 @@ mod tests { struct OracleResolverImpl; impl OracleResolverImpl { - fn echo( - &self, - param: ForeignCallParam, - ) -> ForeignCallResult { + fn echo(&self, param: ForeignCallParam) -> ForeignCallResult { vec![param].into() } - fn sum( - &self, - array: ForeignCallParam, - ) -> ForeignCallResult { + fn sum(&self, array: ForeignCallParam) -> ForeignCallResult { let mut res: FieldElement = 0_usize.into(); for value in array.fields() { @@ -380,7 +374,7 @@ mod tests { _ => panic!("unexpected foreign call"), }; - Ok(response.into()) + Ok(response) } } @@ -431,7 +425,6 @@ mod tests { server.close(); } - #[test] fn oracle_resolver_rpc_can_distinguish_executors() { let (server, url) = build_oracle_server(); @@ -439,10 +432,7 @@ mod tests { let mut executor_1 = DefaultForeignCallExecutor::new(false, Some(&url)); let mut executor_2 = DefaultForeignCallExecutor::new(false, Some(&url)); - let foreign_call = ForeignCallWaitInfo { - function: "id".to_string(), - inputs: Vec::new() - }; + 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(); From 1e0332e76b32e72f68aa8fb765496d97b1dc068a Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 10 Jun 2024 10:33:12 +0100 Subject: [PATCH 6/7] chore: add sanity check that id is persistent --- tooling/nargo/src/ops/foreign_calls.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tooling/nargo/src/ops/foreign_calls.rs b/tooling/nargo/src/ops/foreign_calls.rs index 3063d6e21fc..292450733f4 100644 --- a/tooling/nargo/src/ops/foreign_calls.rs +++ b/tooling/nargo/src/ops/foreign_calls.rs @@ -425,6 +425,21 @@ mod tests { server.close(); } + #[test] + fn foreign_call_executor_id_is_persistent() { + let (server, url) = build_oracle_server(); + + let mut executor = DefaultForeignCallExecutor::new(false, Some(&url)); + + 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(); From f325e28b8c5bf245c222a295c0850ba375f63562 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 10 Jun 2024 10:34:00 +0100 Subject: [PATCH 7/7] chore: rename `id` to `session_id` in `ResolveForeignCallRequest` --- tooling/nargo/src/ops/foreign_calls.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tooling/nargo/src/ops/foreign_calls.rs b/tooling/nargo/src/ops/foreign_calls.rs index 292450733f4..115337c1636 100644 --- a/tooling/nargo/src/ops/foreign_calls.rs +++ b/tooling/nargo/src/ops/foreign_calls.rs @@ -116,9 +116,15 @@ pub struct DefaultForeignCallExecutor { #[derive(Debug, Serialize, Deserialize)] struct ResolveForeignCallRequest { - id: u64, + /// 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, } @@ -293,7 +299,7 @@ impl ForeignCallExecutor for DefaultForeignCallExecutor { // 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 { - id: self.id, + session_id: self.id, function_call: foreign_call.clone(), })]; @@ -370,7 +376,7 @@ mod tests { 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.id as u128).into(), + "id" => FieldElement::from(req.session_id as u128).into(), _ => panic!("unexpected foreign call"), };