Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Make runtime api calls native when possible #1302

Merged
merged 15 commits into from
Jan 21, 2019
Merged

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Dec 20, 2018

This pull request implements support for making real native calls, without using the dispatch function in the native runtime.
The dispatch function is not removed, because for stuff like RPC it is still used.

The native call also does not drop encode/decode completely. For parameters/return values that are generic over the Block, will be encoded/decoded before calling the native function. This is a requirement, as the block differs between the node and the runtime.

There are still some optimizations, like don't encode all parameters, even if we only call the native function. Currently this needs to be done, as the state machine decides if it calls both wasm/native or just native or just wasm.

Fixes: #294

@bkchr bkchr added the A0-please_review Pull request needs code review. label Dec 20, 2018
@bkchr bkchr force-pushed the bkchr-api-make-native branch from 344d4f0 to 0ef307a Compare December 20, 2018 20:10
@gavofyork gavofyork requested a review from arkpar December 29, 2018 14:32
@gavofyork gavofyork added U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. A3-needsresolving labels Jan 7, 2019
@gavofyork
Copy link
Member

@arkpar could you take a look?

@@ -89,7 +89,7 @@ where
extrinsics: &mut Vec<Block::Extrinsic>
) -> error::Result<()> where T: BlockBuilderApi<Block, InherentData> {
api.map_api_result(|api| {
match api.apply_extrinsic(block_id, &xt)? {
match api.apply_extrinsic(block_id, xt.clone())? {
Copy link
Member

Choose a reason for hiding this comment

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

Why does it have to take it by value now? Some extrinsics, such as setting the code, may be expensive to clone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, the implementation of apply_extrinsics requires to get the extrinsics by value. If we could change the implementation of apply_extrinsics to take them by reference, we could omit the clone here.

@@ -77,7 +82,8 @@ where
initialised_block: &mut Option<BlockId<B>>,
prepare_environment_block: PB,
manager: ExecutionManager<EM>,
) -> error::Result<Vec<u8>> where ExecutionManager<EM>: Clone;
native_call: Option<NC>,
) -> error::Result<NativeOrEncoded<R>> where ExecutionManager<EM>: Clone;
Copy link
Member

Choose a reason for hiding this comment

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

Can both call_data and native_call be used at the same time? Would it make more sense to use an enum here and all the similar functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we call first into native and then into wasm, we need both. I already thought about it. I did not came to a quick solution. I think in another iteration of this code, we could provide some interface, to just create call_data, when it is really needed.

@gavofyork
Copy link
Member

@arkpar any other grumbles?

@bkchr bkchr force-pushed the bkchr-api-make-native branch from fdd2371 to aafec17 Compare January 21, 2019 10:04
@gavofyork gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jan 21, 2019
@gavofyork gavofyork removed A8-looksgood U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. labels Jan 21, 2019
@gavofyork gavofyork merged commit bf6a781 into master Jan 21, 2019
@gavofyork gavofyork deleted the bkchr-api-make-native branch January 21, 2019 13:32
@bkchr
Copy link
Member Author

bkchr commented Jan 22, 2019

For all that came here because they need to update their code:
A runtime api trait is not requiring to take all arguments by reference on the client side. You can now choose to take by value or by reference. This is important, as the native runtime is now able to be called, without encoding/decoding arguments.

MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
* Add simple benchmark for the runtime api

* Make the executor support native calls

* Some documentation

* Hide behind `feature = "std"`

* Rework the native calls

* Make all tests compile again

* Make every parameter using the Block serialized/deserialized in the native call

* Forward `UnwindSafe` requirement

* Remove debug stuff

* Add some documentation

* Fixes warnings

* Fixes errors after master rebase

* Fixes compilation after master rebase

* Fixes compilation after rebase
liuchengxu pushed a commit to liuchengxu/substrate that referenced this pull request May 31, 2023
Ensure new slot is fully processed in `MockPrimaryNode`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants