Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Switch to using jsonrpsee for foreign calls; refactor run_test; foreign call layering #6849

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Dec 17, 2024

Description

Problem*

Resolves #6742

Summary*

Instead of jsonrpc we use jsonrpsee to make calls to the Oracle function resolver.

Also includes #6857 and #6858 which effectively make nargo::ops::test receive a generic ForeignCallExecutor (a breaking change in the API) so that we can use it with Wasm, which otherwise could not compile the RpcForeignCallExecutor.

Additional Context

Our codebase is mostly synchronous, but jsonrpsee is async. I created a tokio::runtime::Runtime in the executor where I can block on the calls, however this doesn't work well with the #[tokio::test], runtimes don't nest. Because of this the tests require a bit more supporting machinery than before.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Copy link
Contributor

github-actions bot commented Dec 17, 2024

Peak Memory Sample

Program Peak Memory
keccak256 78.48M
workspace 123.65M
regression_4709 422.91M
ram_blowup_regression 1.58G
private-kernel-tail 201.61M
private-kernel-reset 716.87M
private-kernel-inner 291.68M
parity-root 171.93M

Copy link
Contributor

github-actions bot commented Dec 17, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 1.460s 7%
regression_4709 0.834s 6%
ram_blowup_regression 15.760s 1%
rollup-base-public 116.604s -4%
rollup-base-private 98.162s -3%
private-kernel-tail 1.073s 1%
private-kernel-reset 7.565s -2%
private-kernel-inner 2.072s 0%
parity-root 0.706s -10%
noir-contracts 88.220s -1%

@aakoshh
Copy link
Contributor Author

aakoshh commented Dec 17, 2024

@TomAFrench it looks like it doesn't compile to Wasm because the http-client feature brings in tokio:net which brings mio as a dependency. It doesn't look like noir_wasm uses the foreign_calls module, so I guess I can try putting that behind a feature flag, so we don't have these network related dependencies when we're compiling to Wasm.

@TomAFrench
Copy link
Member

Yeah we don't need an RPC server in wasm as we expect that to use callbacks directly. We can put it behind a feature flag then.

@aakoshh
Copy link
Contributor Author

aakoshh commented Dec 18, 2024

@TomAFrench the problem is not with the RPC server, which was only used as a dev-dependency, it's the RPC client itself. I wanted rpc to be behind a feature flag, but then realised that ops::test directly depends on being able to make HTTP calls: TestForeignCallExecutor uses RPCForeignCallExecutor. By contrast, ProgramExecutor in ops::execute only uses the ForeignCallExecutor trait, which would be easier to isolate from the network related stuff in foreign_calls module.

I created a test and execute feature flag, so that noir_wasm can depend only on compilation machinery, which is the only one it has #[wasm_bindgen] for right now, but by disabling test in Wasm, #6835 is moot at the moment.

We can refactor the whole module so that a ForeignCallExecutor is passed to the ops::test::run_test as a dependency, similar to how it's done in ProgramExecutor::new, rather than as a URL, which I think is useless within Wasm; at least I don't see any injected methods that would handle network calls. It would be a breaking change in the test API, though. I opened a PR for this: #6858

in wasm as we expect that to use callbacks directly

Perhaps this is related, can you elaborate on what you wanted to do?

@aakoshh aakoshh requested a review from sirasistant December 18, 2024 10:31
tooling/nargo/Cargo.toml Outdated Show resolved Hide resolved
@aakoshh aakoshh changed the title chore: Switch to using jsonrpsee for foreign calls feat!: Switch to using jsonrpsee for foreign calls; refactor run_test; foreign call layering Dec 19, 2024
@aakoshh aakoshh requested a review from a team December 19, 2024 09:24
Copy link
Contributor

github-actions bot commented Dec 19, 2024

Execution Sample

Program Execution Time %
sha256_regression 0.636s 2%
regression_4709 0.407s 4%
ram_blowup_regression 4.418s 1%
rollup-base-public 21.033s 1%
rollup-base-private 19.250s 1%
private-kernel-tail 0.697s -3%
private-kernel-reset 1.536s 3%
private-kernel-inner 0.963s 2%
parity-root 0.522s -3%

compiler/wasm/Cargo.toml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jsonrpc dependency is unmaintained and is failing cargo deny
2 participants