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!: Composable foreign call handlers #6857

Merged
merged 7 commits into from
Dec 19, 2024

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Dec 18, 2024

Description

Problem*

Followup for #6849

Summary*

Adds a foreign_calls::layer::Layer construct which is used to compose ForeignCallExecutor instances. DefaultForeignCallExecutor, DebugForeignCallExecutor and TestForeignCallExecutor are reimplemented with this layering.

The DefaultForeignCallExecutor allows us to inject a base layer, which the TestForeignCallExecutor uses to change the behaviour when nothing handles the call; this way we can reuse the default composition without having to repeat the logic in the test, similar to how the DefaultDebugForeignCallExecutor did it earlier.

Follow up for this would be:

  • Make the mock layer in the DefaultForeignCallExecutor optional, so that during normal execution no contract can install a mock and then handle foreign calls that should hit URLs. A DisabledMockForeignCallExecutor has been added in this PR, but it's unused. (I'm not sure this is a legit concern, but I didn't see any reason it wouldn't work).
  • Make run_test accept a generic function to create a ForeignCallExecutor on top of the Unhandled base handler, instead of explicitly creating a DefaultForeignCallExecutor, so that we don't depend on RPCForeignCallExecutor in the tests, and then we can compile them to Wasm. feat!: nargo::ops::test::run_test generic in ForeignCallExecutor #6858

Additional Context

It's a breaking change because it changes the return type of DefaultForeignCallHandler::new, although the way it's called is the same for now.

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 18, 2024

Peak Memory Sample

Program Peak Memory
keccak256 78.15M
workspace 122.51M
regression_4709 423.26M
ram_blowup_regression 1.58G
private-kernel-tail 206.71M
private-kernel-reset 720.29M
private-kernel-inner 291.91M
parity-root 171.71M

Copy link
Contributor

github-actions bot commented Dec 18, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 0m1.497s 8%
regression_4709 0m0.809s -1%
ram_blowup_regression 0m14.784s 1%
rollup-base-public 3m24.081s 0%
rollup-base-private 3m20.824s 7%
private-kernel-tail 0m1.257s 8%
private-kernel-reset 0m7.964s 2%
private-kernel-inner 0m2.406s 6%
parity-root 0m0.904s -7%
noir-contracts 2m56.222s 5%

@aakoshh aakoshh force-pushed the 6742-refactor-foreign-calls branch from 16ffefd to 17fda21 Compare December 18, 2024 15:17
@aakoshh aakoshh marked this pull request as ready for review December 18, 2024 15:17
@aakoshh aakoshh requested a review from asterite December 18, 2024 15:17
@aakoshh aakoshh changed the title feat: Composable foreign call handlers feat!: Composable foreign call handlers Dec 18, 2024
Copy link
Contributor

github-actions bot commented Dec 18, 2024

Changes to Brillig bytecode sizes

Generated at commit: 0a9fd880ef5463d6d6a9e226947f98343be1e014, compared to commit: e2549dfdd8ea93a8d63a4404c2aaada62455018e

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
poseidonsponge_x5_254 +3 ❌ +0.07%
sha256_regression -3 ✅ -0.04%

Full diff report 👇
Program Brillig opcodes (+/-) %
poseidonsponge_x5_254 4,254 (+3) +0.07%
sha256_regression 6,917 (-3) -0.04%

Copy link
Contributor

Changes to number of Brillig opcodes executed

Generated at commit: 0a9fd880ef5463d6d6a9e226947f98343be1e014, compared to commit: e2549dfdd8ea93a8d63a4404c2aaada62455018e

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
poseidonsponge_x5_254 +3 ❌ +0.00%
sha256_regression -3 ✅ -0.00%

Full diff report 👇
Program Brillig opcodes (+/-) %
poseidonsponge_x5_254 183,753 (+3) +0.00%
sha256_regression 118,704 (-3) -0.00%

@aakoshh aakoshh requested a review from a team December 18, 2024 16:33
Copy link
Collaborator

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

If I'm understanding things correctly, DefaultForeignCallExecutor used to have three internal executors and it would try them one after the other. Now in this PR they are layered on top of each other, so the effect is the same... except that we end up without duplicated code.

@aakoshh
Copy link
Contributor Author

aakoshh commented Dec 18, 2024

That's right, the DefaultForeignCallExecutor knew exactly which requests should go where, and then the handlers it dispatched to also inspected the call to decide whether they should be the ones handling it. Sometimes the DefaultForeignCallExecutor looked for this NoHandler error as a legitimate signal to carry on to the next handler. Namely the MockForeignCallExecutor may or may not be able to handle arbitrary calls, depending on the calls history came before.

Then the DefaultForeignCallExecutor returned an empty result. To inject further logic before this happens, the TestForeignCallExecutor had to repeat the whole setup, which now it doesn't have to any more, although it could have been achieved in different ways I suppose.

With layering there is a chance to combine layers differently, and also to remove duplication by passing around builder functions. There is more reliance on the NoHandlers error mechanism, at the cost of cloning more strings, but I think we could even remove the wrapped String as the content might never be accessed.

In the next extension we could add something like layers::Either that could be for example either the MockExecutor or the DisabledMockExecutor, or add a flag to the MockExecutor would be fine as well.

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, I really like the removal of duplication between test and default executors.

@aakoshh aakoshh merged commit 6ce3263 into 6742-jsonrpsee Dec 19, 2024
82 checks passed
@aakoshh aakoshh deleted the 6742-refactor-foreign-calls branch December 19, 2024 09:17
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.

3 participants