Prerequisites for `validate_block` in Cumulus #1926

bkchr · 2019-03-05T14:29:47Z

Adds prerequisites for implementing validate_block in Cumulus.

core/sr-io/without_std.rs

…b825e26

tomusdrw

lgtm

core/executor/src/wasm_executor.rs

core/sr-io/without_std.rs

core/test-runtime/src/lib.rs

srml/executive/src/lib.rs

rphmeier · 2019-03-11T12:28:17Z

core/executor/src/wasm_executor.rs

-				memory.get(offset, length)
-					.map_err(|_| ErrorKind::Runtime.into())
+			Ok(val) => {
+				match filter_result(val, &memory)? {


outer block is not necessary anymore

gavofyork · 2019-03-13T19:23:14Z

will need impl_version bumping (assuming logic of runtime modules remains unchanged)

gavofyork · 2019-03-13T19:29:09Z

core/test-runtime/src/lib.rs

+	/// Convert into a signed extrinsic.
+	#[cfg(feature = "std")]
+	pub fn into_signed_tx(self) -> Extrinsic {
+		let signature = keyring::Keyring::from_raw_public(


should just be keyring::Keyring::from_public(self.from). If you're type-running something's wrong.

gavofyork · 2019-03-13T19:32:15Z

@rphmeier you ok to sign off on this?

bkchr · 2019-03-13T19:40:38Z

Why do I need to increase the impl_version here?

pepyakin · 2019-03-14T10:55:26Z

core/executor/src/wasm_executor.rs

+			|res, memory| {
+				if let Some(I64(r)) = res {
+					let offset = r as u32;
+					let length = (r >> 32) as usize;


Hm, I wonder if this is correct. Can you check my logic?

r is i64.

According to the reference, right shift is logical for unsinged values but arithmetic for signed values.

So 0x8000_0000_0000_0000 >> 32 is an arithmetic shift and will give you 0xFFFF_FFFF_8000_0000.

@tomusdrw FYI. We both need to go back and learn shifting :D

rphmeier · 2019-03-14T12:16:19Z

core/sr-io/without_std.rs

+	type Target = T;
+
+	fn deref(&self) -> &Self::Target {
+		unsafe { &(*self.0.as_ptr()).0 }


This is not a safe implementation, though.

// let STATIC_FN = ExchangeableFunction { ... }; let y = STATIC_FN.deref(); STATIC_FN.replace_implementation(...); // violates borrowing rules and introduces UB y();

Basically: it's only safe to use this struct if we can ensure that borrows through deref do not last longer than a single invocation of the internal value. The public API should ensure this. I'm not sure Deref is the right choice with that in mind. Maybe a fn with_inner<F: FnOnce(&T)>(&self, f: F).

rphmeier · 2019-03-14T12:19:43Z

core/sr-io/without_std.rs

+}
+
+// WASM does not support threads, so this is safe; qed.
+unsafe impl<T> Sync for ExchangeableFunction<T> {}


what happens when we have a native-linked runtime? and if we would do runtime calls in multiple threads?

This is never compiled in native. Only for WASM.

pepyakin · 2019-03-14T13:04:09Z

core/sr-io/without_std.rs

+	/// # Returns
+	///
+	/// Returns the original implementation wrapped in [`RestoreImplementation`].
+	pub unsafe fn replace_implementation(&'static self, new_impl: T)  -> RestoreImplementation<T> {


Can you mention in the documentation why this is unsafe?

pepyakin · 2019-03-14T13:04:44Z

core/sr-io/without_std.rs

+/// A function which implementation can be exchanged.
+///
+/// Internally this works by swapping function pointers.
+pub struct ExchangeableFunction<T>(Cell<(T, bool)>);


Can you mention what is this bool stands for?

pepyakin · 2019-03-14T13:06:18Z

core/sr-io/without_std.rs

+}
+
+/// Restores a function implementation on drop.
+pub struct RestoreImplementation<T: 'static>(&'static ExchangeableFunction<T>, Option<T>);


Can you briefly describe what each member is? (In comments)

…48d9890

* Adds benchmark for direct/indirect wasm function calls * Store the benchmark function pointer in a `Cell` * Add some documentation * Make function implementations exchangeable * Add parachain stub * Add macro for registering the `validate_block` function * Make all functions replace-able by unimplemented * Some more refactoring * Adds tests for executing empty parachain block * Work on a new test with empty witness data * Don't exchange `ext_print_*` stuff * Some cleanup and one more function for `validate_block` * More tests and more functions * Fixes after merging master * Use `parity-codec` `derive` feature * CHange implementation of `wasm-nice-panic-message` * Move `parachain` stuff to cumulus * Updated wasm files * Integrate feedback * Switch to `ExchangeableFunction` struct * More fixes * Switch to Cell and panic on multiple replaces * Increase `impl_version` * Fix shifting * Make the API more verbose of `ExchangeableFunction` * Increase `impl_version`

bkchr added 12 commits March 1, 2019 17:54

Adds benchmark for direct/indirect wasm function calls

ded8fdb

Store the benchmark function pointer in a Cell

a8052d1

Add some documentation

456ddac

Make function implementations exchangeable

c9a5b11

Add parachain stub

0638c55

Add macro for registering the validate_block function

7bb9d20

Make all functions replace-able by unimplemented

3d650b9

Some more refactoring

b41e6d0

Adds tests for executing empty parachain block

b1c99b8

Work on a new test with empty witness data

2524f95

Don't exchange ext_print_* stuff

4e6e32d

Some cleanup and one more function for validate_block

603897b

rphmeier reviewed Mar 5, 2019

View reviewed changes

core/sr-io/without_std.rs Outdated Show resolved Hide resolved

bkchr added 4 commits March 5, 2019 16:45

More tests and more functions

c1630ea

Merge branch 'master' into bkchr-validate_block

e4962c9

Fixes after merging master

ab63852

Use parity-codec derive feature

eeab2ce

devops-parity added the A4-gotissues label Mar 5, 2019

bkchr and others added 3 commits March 6, 2019 10:08

CHange implementation of wasm-nice-panic-message

481749d

Move parachain stuff to cumulus

73f282f

devops-parity updated wasm runtime blobs 98cfecb1 and merged in master …

8090c35

…b825e26

bkchr changed the title ~~Implement validate_block for Cumulus~~ Prerequisites for validate_block in Cumulus Mar 7, 2019

bkchr requested a review from rphmeier March 7, 2019 13:02

bkchr marked this pull request as ready for review March 7, 2019 13:02

bkchr added 2 commits March 7, 2019 14:04

Merge remote-tracking branch 'origin/master' into bkchr-validate_block

e1124fb

Updated wasm files

dd09ee6

bkchr mentioned this pull request Mar 7, 2019

Adds first version of validate_block paritytech/cumulus#3

Merged

tomusdrw approved these changes Mar 7, 2019

View reviewed changes

Merge branch 'master' into bkchr-validate_block

d0b1f5a

rphmeier reviewed Mar 11, 2019

View reviewed changes

bkchr added 2 commits March 11, 2019 15:23

Switch to ExchangeableFunction struct

ea99855

More fixes

682eafa

gavofyork added A0-please_review Pull request needs code review. and removed A4-gotissues labels Mar 13, 2019

Merge remote-tracking branch 'origin/master' into bkchr-validate_block

c0131c2

gavofyork reviewed Mar 13, 2019

View reviewed changes

gavofyork approved these changes Mar 13, 2019

View reviewed changes

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

bkchr added 2 commits March 14, 2019 11:15

Switch to Cell and panic on multiple replaces

f958f0f

Increase impl_version

99823e8

pepyakin reviewed Mar 14, 2019

View reviewed changes

rphmeier reviewed Mar 14, 2019

View reviewed changes

pepyakin reviewed Mar 14, 2019

View reviewed changes

bkchr and others added 3 commits March 14, 2019 14:47

Fix shifting

b382c7f

Make the API more verbose of ExchangeableFunction

7f390ce

devops-parity updated wasm runtime blobs 99191521 and merged in master …

a8e8b0b

…48d9890

devops-parity added the A4-gotissues label Mar 14, 2019

pepyakin approved these changes Mar 14, 2019

View reviewed changes

rphmeier approved these changes Mar 14, 2019

View reviewed changes

Increase impl_version

3800235

bkchr merged commit 6945bdf into master Mar 14, 2019

bkchr deleted the bkchr-validate_block branch March 14, 2019 20:29

Prerequisites for validate_block in Cumulus #1926

Prerequisites for validate_block in Cumulus #1926

Uh oh!

Conversation

bkchr commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Mar 13, 2019

Uh oh!

bkchr commented Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepyakin Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Prerequisites for `validate_block` in Cumulus #1926

Prerequisites for `validate_block` in Cumulus #1926

bkchr commented Mar 5, 2019 •

edited

Loading

pepyakin Mar 14, 2019 •

edited

Loading