Implement runtime version checks in `set_code` #4548

bkchr · 2020-01-06T19:56:18Z

Check that the new runtime code given to set_code fullfills some
requirements:

spec_name matches
spec_version does not decreases
impl_version does not decreases
Either spec_version or impl_version increases

Check that the new runtime code given to `set_code` fullfills some requirements: - `spec_name` matches - `spec_version` does not decreases - `impl_version` does not decreases - Either `spec_version` and `impl_version` increase

gavofyork · 2020-01-10T08:45:55Z

looks like a promising direction.

…-checks

tomusdrw · 2020-01-13T12:54:13Z

client/src/client.rs

 	Block: BlockT,
 {
 	type Type = Block;
 }

 impl<B, E, Block, RA> Client<B, E, Block, RA> where
 	B: backend::Backend<Block>,
-	E: CallExecutor<Block>,
+	E: CallExecutor<Block> + 'static,


Shall we just require on CallExecutor trait that it's 'static? This repetition is going to be pretty cumbersome.

tomusdrw · 2020-01-13T12:58:49Z

primitives/io/src/lib.rs

+	///
+	/// Returns the SCALE encoded runtime version and `None` if the call failed.
+	fn runtime_version(&mut self, wasm: &[u8]) -> Option<Vec<u8>> {
+		// Create some dummy externalities, `Core_version` should not write data anyway.


Should we just error out if it does? Just to make sure that even if set_code fails due to checks there are no side-effects of that.

What kind of side effects? Even if the call would write something, it would not leak into the Externalities of the actual block. If that was your question.

Yes, that's what I meant, I wanted to be sure that the two contexts do not share anything.

tomusdrw · 2020-01-13T12:59:37Z

primitives/sandbox/with_std.rs

+		println!("{} {}", module_name, field_name);
+		let lol = ExternVal::HostFunc(HostFuncIndex(100));
+		let externval = self.map.get(&key).unwrap_or_else(|| {
+			// wasmi::Error::Instantiation(format!("Export {}:{} not found", module_name, field_name))


What's going on here 🗡️

Yeah, I had forgot some debugging stuff 😅 Good that you found it!

tomusdrw · 2020-01-13T12:59:56Z

primitives/sandbox/with_std.rs

@@ -262,10 +266,9 @@ impl<T> Instance<T> {
 		env_def_builder: &EnvironmentDefinitionBuilder<T>,
 		state: &mut T,
 	) -> Result<Instance<T>, Error> {
-		let module = Module::from_buffer(code).map_err(|_| Error::Module)?;
+		let module = Module::from_buffer(code).map_err(|e| { println!("{:?}", e); Error::Module})?;


A bunch of stray printlns in this file.

tomusdrw · 2020-01-13T13:01:25Z

primitives/state-machine/src/lib.rs

 	) -> Self {
+		extensions.register(CallInWasmExt::new(exec.clone()));


So it's now available to every runtime call I guess, what are the consequences here? Besides using it being super slow, as I guess a new wasm env has to be initiated.

Why super slow? The wasm blob needs to be initialized when we want to call some function in it, but there is nothing we can optimize (as we don't know the blob in front of).

We need the extension while building/executing a block. But there is also no direct method that you can call from the runtime, besides the one that only can call Core_version.

Sure, I know there is no other way to call Core_version from a different runtime blob, but afair when talking with Sergei he was saying that initializing wasm runtime (alloc memory, load module, etc) takes quite some time, hence I've mentioned that it's most likely going to be way slower than any other operation or host function we have, so we need to be careful to avoid using it too often.
I was wondering if there are any other (more severe) consequences of exposing this kind of function to the runtime - i.e. if it can be somehow abused.

. But there is also no direct method that you can call from the runtime, besides the one that only can call Core_version.

That's good to know, so it means that it's not easy to shoot yourself in a foot if you are a runtime developer, right?

Yeah it is probably no easy operation, but better to include less transactions than to stop the network again ;) If something does not work or for whatever reason we need to put a blob in without any checks, I added set_code_without_checks. This will just do it the old way, so we are sure that we can put a blob in :)

Yeah, the runtime developer can not access the CallInRuntimeExt in any other way. It is not exposed to the runtime. The developer can just call misc::runtime_version that uses this stuff internally.

Okay, that's fair, but I would really add extra docs to make sure it's not being called as it's a possible DoS vector. Imagine if we do that check whenever someone proposes a runtime, then people can craft arbitrary runtimes and do arbitrary operations within Core_version and it wouldn't be priced correctly. So we should only make sure to run in during set_code, not during any preliminary checks (that accepts unreviewed user's input).

gavofyork · 2020-01-13T14:19:59Z

client/executor/src/lib.rs

 	function: &str,
 	call_data: &[u8],
 	execution_method: WasmExecutionMethod,
-	ext: &mut E,
+	ext: &mut dyn Externalities,


all the storage accesses (and there are a lot) go through externalities. does this really have an insubstantial effect on performance?

We store the Externalities as trait pointer in the environmental anyway. The compiler can not optimize this anywax.

…-checks

bkchr · 2020-01-15T07:59:39Z

I added some temporary execution time measuring into runtime_version and the host call takes 60ms. Given the time it takes to execute the transaction itself, this sounds okay and reasonable.

gavofyork · 2020-01-15T17:55:05Z

will need resolving

…-checks

bkchr · 2020-01-16T07:54:37Z

will need resolving

Done

Implement runtime version checks in set_code

4dfb897

Check that the new runtime code given to `set_code` fullfills some requirements: - `spec_name` matches - `spec_version` does not decreases - `impl_version` does not decreases - Either `spec_version` and `impl_version` increase

bkchr added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 6, 2020

bkchr requested a review from gavofyork January 6, 2020 19:56

bkchr added 5 commits January 10, 2020 17:55

Make tests almost work

79edfd1

Merge remote-tracking branch 'origin/master' into bkchr-set-code-with…

46d85bd

…-checks

Some fixes after master merge

5fd35a1

Merge remote-tracking branch 'origin/master' into bkchr-set-code-with…

20e608d

…-checks

Fix tests

0ad8df5

bkchr marked this pull request as ready for review January 13, 2020 11:53

bkchr requested review from andresilva, Demi-Marie, pepyakin and tomusdrw as code owners January 13, 2020 11:53

bkchr 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 13, 2020

bkchr added 4 commits January 13, 2020 13:05

Add missed file

ca5df1b

Make depedency check happy?

045547c

Remove leftover sc-executor

88d8b93

AHHHHH

27be8c5

tomusdrw reviewed Jan 13, 2020

View reviewed changes

gavofyork reviewed Jan 13, 2020

View reviewed changes

bkchr added 6 commits January 13, 2020 15:32

Reset debug stuff

44c7d17

Remove some 'static

a30d94b

More 'static

2c6eaa0

Some docs

7484d66

Merge remote-tracking branch 'origin/master' into bkchr-set-code-with…

ec63f76

…-checks

Update Cargo.lock

8b59068

gavofyork added this to the 3.0 milestone Jan 15, 2020

Merge remote-tracking branch 'origin/master' into bkchr-set-code-with…

c471a62

…-checks

gavofyork approved these changes Jan 16, 2020

View reviewed changes

gavofyork merged commit 879e28a into master Jan 16, 2020

gavofyork deleted the bkchr-set-code-with-checks branch January 16, 2020 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement runtime version checks in `set_code` #4548

Implement runtime version checks in `set_code` #4548

bkchr commented Jan 6, 2020 •

edited

Loading

gavofyork commented Jan 10, 2020

tomusdrw Jan 13, 2020

tomusdrw Jan 13, 2020

bkchr Jan 13, 2020

tomusdrw Jan 14, 2020

tomusdrw Jan 13, 2020

bkchr Jan 13, 2020

tomusdrw Jan 13, 2020

tomusdrw Jan 13, 2020

bkchr Jan 13, 2020

tomusdrw Jan 14, 2020

bkchr Jan 14, 2020

tomusdrw Jan 14, 2020 •

edited

Loading

gavofyork Jan 13, 2020

bkchr Jan 13, 2020

bkchr commented Jan 15, 2020

gavofyork commented Jan 15, 2020

bkchr commented Jan 16, 2020

		) -> Self {
		extensions.register(CallInWasmExt::new(exec.clone()));

Implement runtime version checks in set_code #4548

Implement runtime version checks in set_code #4548

Conversation

bkchr commented Jan 6, 2020 • edited Loading

gavofyork commented Jan 10, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr commented Jan 15, 2020

gavofyork commented Jan 15, 2020

bkchr commented Jan 16, 2020

Implement runtime version checks in `set_code` #4548

Implement runtime version checks in `set_code` #4548

bkchr commented Jan 6, 2020 •

edited

Loading

tomusdrw Jan 14, 2020 •

edited

Loading