Adds new execution strategy nativeElseWasm #1546

marcio-diaz · 2019-01-24T11:15:44Z

PR for issue #1395.

bkchr · 2019-01-24T13:00:52Z

core/cli/src/lib.rs

@@ -237,6 +237,7 @@ where
 			"both" => service::ExecutionStrategy::Both,
 			"native" => service::ExecutionStrategy::NativeWhenPossible,
 			"wasm" => service::ExecutionStrategy::AlwaysWasm,
+			"nativeElseWasm" => service::ExecutionStrategy::NativeElseWasm,


I think we should stick to all lowercase?

bkchr

You do not use the block_construction execution strategy while constructing blocks or the syncing execution strategy while syncing. You need to make use of them.
The pr also needs to be resolved.

core/sr-api-macros/src/impl_runtime_apis.rs

marcio-diaz · 2019-01-31T19:18:04Z

You do not use the block_construction execution strategy while constructing blocks or the syncing execution strategy while syncing. You need to make use of them.
The pr also needs to be resolved.

I believe block constructions is fixed by now. Need to re-check syncing.

fixed

gavofyork · 2019-02-01T09:09:03Z

we keep Cargo.lock :)

gavofyork · 2019-02-01T09:12:34Z

important bits of logic look good.

gavofyork · 2019-02-01T13:00:09Z

Will need another merge

bkchr · 2019-02-01T15:50:59Z

core/consensus/aura/primitives/src/lib.rs

@@ -19,6 +19,7 @@
 #![cfg_attr(not(feature = "std"), no_std)]

 use substrate_client::decl_runtime_apis;
+use substrate_client::runtime_api::ExecutionContext;


The macro should not enforce the user to include any stuff that is used internally. Please use ExecutionContext by absolute path as we do it for other types as well.

bkchr · 2019-02-01T15:53:28Z

core/sr-api-macros/src/decl_runtime_apis.rs

+				TraitItem::Method(method) => {
+					let mut new_method = method.clone();
+					new_method.sig.ident = Ident::new(&format!("{}_with_context", &method.sig.ident), Span::call_site());
+					new_method.sig.decl.inputs.push(context_arg.clone());


Could we put the context argument after BlockId?

bkchr · 2019-02-01T15:53:51Z

core/sr-api-macros/src/impl_runtime_apis.rs

@@ -250,7 +250,9 @@ fn generate_block_and_block_id_ty(
 	let block = quote!( <#runtime as #crate_::runtime_api::#trait_>::#assoc_type );
 	let block_id = quote!( #crate_::runtime_api::BlockId<#block> );

-	(block, block_id)
+	let ts = (block, block_id);


Some left over debug stuff.

bkchr · 2019-02-01T16:05:23Z

core/sr-api-macros/src/impl_runtime_apis.rs

+			let mut execution_context = parse_quote!( ExecutionContext::Other );
+			let mut call_generator_name = input.sig.ident.to_string();
+
+			let with_context_str = "_with_context";


Instead of doing all this string parsing here, I would propose to drop generate_impl_with_context() and these modifications.

Switch to use fold_impl_item. Iterate manual over all methods and call the function that generates the method once with the original impl_item_method and a second time where you just replace the name and add the corresponding function argument.

Yeah, makes much more sense 👍 .

bkchr

Looks good so far. Just some last nits + fix failing tests.

bkchr · 2019-02-11T08:09:49Z

core/sr-api-macros/src/decl_runtime_apis.rs

@@ -339,8 +339,9 @@ impl<'a> ToClientSideDecl<'a> {

 		items.into_iter().for_each(|i| match i {
 			TraitItem::Method(method) => {
-				let (fn_decl, fn_impl) = self.fold_trait_item_method(method);
+				let (fn_decl, fn_impl, fn_decl_ctx) = self.fold_trait_item_method(method.clone());


Suggested change

let (fn_decl, fn_impl, fn_decl_ctx) = self.fold_trait_item_method(method.clone());

let (fn_decl, fn_impl, fn_decl_ctx) = self.fold_trait_item_method(method);

bkchr · 2019-02-11T08:11:34Z

core/sr-api-macros/src/decl_runtime_apis.rs

@@ -726,6 +742,8 @@ pub fn decl_runtime_apis_impl(input: proc_macro::TokenStream) -> proc_macro::Tok
 	let runtime_decls = generate_runtime_decls(&api_decls);
 	let client_side_decls = generate_client_side_decls(&api_decls);

+	// println!("generating this declaration:\n\n{}\n\n", quote!( #client_side_decls ));


Left over debug stuff.

bkchr · 2019-02-11T08:12:18Z

core/sr-api-macros/src/utils.rs

@@ -15,7 +15,8 @@
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.

 use proc_macro2::{TokenStream, Span};
-use syn::{Result, Ident, FnDecl, parse_quote, Type, Pat, spanned::Spanned, FnArg, Error};
+use syn::{Result, Ident, FnDecl, parse_quote, Type, Pat, spanned::Spanned,
+	FnArg, Error};


Please remove this line break.

core/sr-api-macros/tests/runtime_calls.rs

bkchr · 2019-02-11T08:15:11Z

core/sr-api-macros/tests/runtime_calls.rs

 	let runtime_api = client.runtime_api();
 	let block_id = BlockId::Number(client.info().unwrap().chain.best_number);

 	#[allow(deprecated)]
 	let res = runtime_api.function_signature_changed_before_version_2(&block_id).unwrap();
 	assert_eq!(&res, &[1, 2]);
 }
+
+#[test]
+#[should_panic]


Please provide an expected message, see above for an example. (So we make sure that the panic is the one we expect).

bkchr · 2019-02-11T08:18:27Z

node/runtime/src/lib.rs

@@ -54,6 +54,7 @@ use substrate_primitives::OpaqueMetadata;

 #[cfg(any(feature = "std", test))]
 pub use runtime_primitives::BuildStorage;
+pub use runtime_primitives::ExecutionContext;


This should not be required here.

bkchr · 2019-02-11T08:19:50Z

node/runtime/src/lib.rs

@@ -65,8 +66,8 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("node"),
 	impl_name: create_runtime_str!("substrate-node"),
 	authoring_version: 10,
-	spec_version: 26,
-	impl_version: 26,
+	spec_version: 27,


You don't need to increase the spec_version. impl_version should be enough. CI will still report an issue, but that is okay for the runtime check.

bkchr · 2019-02-11T08:26:07Z

core/client/src/light/call_executor.rs

@@ -80,7 +80,8 @@ where
 {
 	type Error = ClientError;

-	fn call(&self, id: &BlockId<Block>, method: &str, call_data: &[u8]) -> ClientResult<Vec<u8>> {
+	fn call(&self, id: &BlockId<Block>, method: &str, call_data: &[u8], strategy: ExecutionStrategy)


Suggested change

fn call(&self, id: &BlockId<Block>, method: &str, call_data: &[u8], strategy: ExecutionStrategy)

fn call(&self, id: &BlockId<Block>, method: &str, call_data: &[u8], _: ExecutionStrategy)

* fix: adds new execution strategy nativeElseWasm and replace nativeWhenPossible with it * feat: adds cmd line params for execution strategies * fix: uses of cmd line execution strategies * chore: remove white spaces * chore: remove println * chore: remove whitespace * fix: generating functions with context * feat: add function to generate with_context declarations * fix: add implementation for with_context function calls * fix: add execution context to call_api_at function * fix: making use of context to select strategy for block_builder * chore: cleaning up * fix: merging issues * fix tests * add wasm files * chore: small doc for context fields * chore: delete redundant docs * fix: use full path for ExecutionContext * fix: add context functions from inside fold_item_impl * chore: remove clone * fix: moving generative function to utils, remove unused imports * fix: add missing full path for ExecutionContext * fix: merge issues * update wasm files * fix: update to keep up with changes in master * chore: remove unused functions, clean up * fix test * fix grumbles * fix: add more tests * fix: some refactorings * feat: add execution strategy to call * chore: small improvements * fix: add message to panic * fix tests

marcio-diaz added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 24, 2019

bkchr reviewed Jan 24, 2019

View reviewed changes

marcio-diaz added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 26, 2019

bkchr previously requested changes Jan 28, 2019

View reviewed changes

bkchr added A3-needsresolving A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jan 28, 2019

bkchr reviewed Jan 31, 2019

View reviewed changes

core/sr-api-macros/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved

marcio-diaz force-pushed the mar-improve-execution-strategies branch 2 times, most recently from bf49528 to 6ba8ef8 Compare January 31, 2019 18:52

marcio-diaz added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. A3-needsresolving labels Feb 1, 2019

gavofyork added A3-needsresolving and removed A0-please_review Pull request needs code review. labels Feb 1, 2019

marcio-diaz force-pushed the mar-improve-execution-strategies branch from 0d4e57c to 32d5681 Compare February 1, 2019 14:36

marcio-diaz added A0-please_review Pull request needs code review. and removed A3-needsresolving labels Feb 1, 2019

bkchr previously requested changes Feb 1, 2019

View reviewed changes

gavofyork added A5-grumble and removed A0-please_review Pull request needs code review. labels Feb 3, 2019

marcio-diaz added A0-please_review Pull request needs code review. and removed A5-grumble labels Feb 5, 2019

gavofyork added the A3-needsresolving label Feb 5, 2019

seerscode added 18 commits February 10, 2019 14:50

fix: merging issues

4e38359

fix tests

760ea1e

add wasm files

ceb8e82

chore: small doc for context fields

24fc9f5

chore: delete redundant docs

9e82e27

fix: use full path for ExecutionContext

0055e78

fix: add context functions from inside fold_item_impl

0190ba6

chore: remove clone

eb838ea

fix: moving generative function to utils, remove unused imports

ac293f8

fix: add missing full path for ExecutionContext

7861ad5

fix: merge issues

7b47bd3

update wasm files

316ed97

fix: update to keep up with changes in master

b98b0e9

chore: remove unused functions, clean up

065b40c

fix test

74a43c1

fix grumbles

8240541

fix: add more tests

2d8baf8

fix: some refactorings

5a6b489

marcio-diaz force-pushed the mar-improve-execution-strategies branch from ee931a6 to 5a6b489 Compare February 10, 2019 15:20

feat: add execution strategy to call

2bd981f

bkchr reviewed Feb 11, 2019

View reviewed changes

seerscode added 3 commits February 11, 2019 10:29

chore: small improvements

68589b6

fix: add message to panic

809391b

fix tests

6407f81

marcio-diaz force-pushed the mar-improve-execution-strategies branch from 3544003 to 6407f81 Compare February 11, 2019 10:31

bkchr approved these changes Feb 11, 2019

View reviewed changes

fix merge conflicts

90d9d19

marcio-diaz merged commit 5f4ebb4 into master Feb 11, 2019

bkchr deleted the mar-improve-execution-strategies branch February 11, 2019 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds new execution strategy nativeElseWasm #1546

Adds new execution strategy nativeElseWasm #1546

marcio-diaz commented Jan 24, 2019

bkchr Jan 24, 2019

gavofyork Jan 29, 2019

bkchr left a comment

marcio-diaz commented Jan 31, 2019

gavofyork commented Feb 1, 2019

gavofyork commented Feb 1, 2019

gavofyork commented Feb 1, 2019

bkchr Feb 1, 2019

bkchr Feb 1, 2019

bkchr Feb 1, 2019

bkchr Feb 1, 2019

marcio-diaz Feb 1, 2019

bkchr left a comment

bkchr Feb 11, 2019

bkchr Feb 11, 2019

bkchr Feb 11, 2019

bkchr Feb 11, 2019

bkchr Feb 11, 2019

bkchr Feb 11, 2019

bkchr Feb 11, 2019

	let (fn_decl, fn_impl, fn_decl_ctx) = self.fold_trait_item_method(method.clone());
	let (fn_decl, fn_impl, fn_decl_ctx) = self.fold_trait_item_method(method);

	fn call(&self, id: &BlockId<Block>, method: &str, call_data: &[u8], strategy: ExecutionStrategy)
	fn call(&self, id: &BlockId<Block>, method: &str, call_data: &[u8], _: ExecutionStrategy)

Adds new execution strategy nativeElseWasm #1546

Adds new execution strategy nativeElseWasm #1546

Conversation

marcio-diaz commented Jan 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

marcio-diaz commented Jan 31, 2019

gavofyork commented Feb 1, 2019

gavofyork commented Feb 1, 2019

gavofyork commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment