Skip to content

Bundled execution runtime #300

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

Merged
merged 5 commits into from
Mar 30, 2022
Merged

Bundled execution runtime #300

merged 5 commits into from
Mar 30, 2022

Conversation

nazar-pc
Copy link
Member

Another attempt to combine runtimes. This time execution runtime is simply embedded into consensus runtime as a binary slice and is available via runtime API.

Not as clean as the previous attempt unfortunately, but avoids all possible pitfalls there.

First commit is just to preserve file history (file change history continues to work even after file renaming).

@nazar-pc nazar-pc requested a review from vedhavyas March 27, 2022 23:38
Copy link
Contributor

@liuchengxu liuchengxu left a comment

Choose a reason for hiding this comment

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

This approach is close to my previous attempt in another branch, but I'm not sure about the complexity(subspace-wasm-tools, wasm-builder feature) here is necessary. I have restored that branch and updated it accordingly, it's way simpler, perhaps you could take a look https://github.com/subspace/subspace/compare/integrate-cirrus-runtime-into-subspace

@nazar-pc
Copy link
Member Author

First of all, your branch doesn't seen include one runtime within another. I'm not even sure it compiles because execution_wasm_binary.rs must be included consensus_wasm_binary.rs, but is actually built after it. wasm-builder feature was added specifically to make this possible.

subspace-wasm-tools was introduced to avoid most of the code duplication between regular and test runtimes.

Also building one crate from within another isn't great, but my approach isn't amazing either. Eventually we can bring everything into one crate and build with different features enabled to obtain different wasm bundles without guessing paths of other crates, but we are not at that stage yet.

@liuchengxu
Copy link
Contributor

liuchengxu commented Mar 28, 2022

From what I see, what subspace-wasm-tools does is essentially https://github.com/paritytech/substrate/blob/65b44e91be/utils/wasm-builder/src/builder.rs#L258-L268, I don't see why we can't achieve it by making use of the existing substrate-wasm-builder, which is flexible enough to do it.

Placing the library in the dependencies but don't use it the source file actually is essentially to run the build script of that library, which is no different to running the build script manually, so https://github.com/subspace/subspace/blob/629c0e67b8/crates/subspace-runtime/build.rs#L28-L42 should have the same effect with adding parachain-template-runtime into the deps of subspace-runtime.

Honstly I'm unsure about either way as I haven't tested them (working on the test based on #301), but I'm about to that step, perhaps we could wait until I confirm the execution runtime is indeed callable for the primary node.

@nazar-pc nazar-pc force-pushed the bundled-execution-runtime branch from 49bf2e4 to 03bd066 Compare March 28, 2022 04:13
@nazar-pc
Copy link
Member Author

I don't see why we can't achieve it by making use of the existing substrate-wasm-builder, which is flexible enough to do it.

Wouldn't that mean building runtime twice, once under the crate itself and second time under build script in the parent runtime?

Placing the library in the dependencies but don't use it the source file actually is essentially to run the build script of that library, which is no different to running the build script manually

I think there is a significant difference. Peeking into internal build system is not nice. Grabbing build artifacts isn't ideal either, but it is still nicer IMHO.

@liuchengxu
Copy link
Contributor

Wouldn't that mean building runtime twice, once under the crate itself and second time under build script in the parent runtime?

If we manipulate the build script, then we don't have to add the library into the dependencies so it won't be built twice.

I think there is a significant difference.

Yeah, probably true, but also probably does not matter regarding our specific use case here. I agree manipulating the build system is not nice, but given that Substrate already uses the build script to build the WASM runtime, we don't make it worse by building the execution runtime in the build script as well :D. Anyway, I think we can revisit after testing the fraud proof verification on the primary node.

@liuchengxu
Copy link
Contributor

liuchengxu commented Mar 29, 2022

I have tested this PR in https://github.com/subspace/subspace/tree/test-300 and my solution in https://github.com/subspace/subspace/tree/test-xlc, both of them will work, I'm not surprised about that as they are essentially doing the same thing. Aside from fewer code changes, now I even think my solution is even cleaner than only adding the library to the dependencies and having to modify the build script at the same time(adding it to the dependency without actually using it the source code does not bring in more explicitness IMHO) :D.

@nazar-pc
Copy link
Member Author

Honestly, I do not understand why https://github.com/subspace/subspace/tree/test-xlc even compiles. It includes execution_wasm_binary.rs in the build that I would expect to build before that. Have you tried wasm execution instead of native during testing? The fact that it compiles doesn't mean nested runtime is included.

@liuchengxu
Copy link
Contributor

Your concern is right, the secondary runtime is not available for https://github.com/subspace/subspace/tree/test-xlc when I use the wasm execution, I'm a little lost, it's confusing to me, I'll have a closer look...

Copy link
Contributor

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

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

LGTM

@nazar-pc nazar-pc enabled auto-merge March 30, 2022 13:07
@nazar-pc nazar-pc merged commit 87eed02 into main Mar 30, 2022
@nazar-pc nazar-pc deleted the bundled-execution-runtime branch March 30, 2022 14:15
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.

4 participants