Native executor by gavofyork · Pull Request #47 · paritytech/substrate

gavofyork · 2018-01-23T19:40:29Z

Based on #42.

Executes the runtime natively, falling back to wasm if the code at compile time is different to that which should be executed.

Still todo:

Panic handler to avoid full on meltdown when runtime hits an error.

…g and direct usage.

Also tests.

pepyakin · 2018-01-28T17:40:50Z

wasm-runtime/std/src/lib.rs

+						$crate::vec::Vec::new()
+					} else {
+						unsafe {
+							$crate::vec::Vec::from_raw_parts(input_data, input_len, input_len)


as I said earlier in the chat, this will take ownership of this allocation and Vec will drop it at the end of the method. And if for some reason this pointer will be deallocated in other way it was deallocated then some bad things might happen.

I think that the best way to do this is to make client code responsible for deallocating input_data pointer (because the client code allocated this pointer in a first place) and use slice instead of a vector.

If this isn't desired for some reason, then I think it would be better to use slice and deallocate this pointer explicitly, preferably with a comment

rphmeier · 2018-01-28T20:31:55Z

wasm-runtime/polkadot/src/runtime/governance.rs

+	}
+}
+
+const APPROVALS_REQUIRED: &[u8] = b"gov:apr";


these constants should be at the top of the file

ok - i put them down below as they're private and i wanted all public API first.

for readability I usually prefer any constants to be declared before their usage rather than scattered throughout the file

otherwise it can be hard to tell if they are actually from that file or imported from elsewhere

rphmeier · 2018-01-28T22:36:29Z

executor/src/native_executor.rs

+		if code == &native_equivalent[..] {
+			runtime_std::with_externalities(ext, || match method {
+				"execute_block" => safe_call(|| runtime::execute_block(&data.0)),
+				"execute_transaction" => safe_call(|| runtime::execute_transaction(&data.0)),


I am little worried about this since &mut T isn't UnwindSafe. But this code compiles because we've hidden the mutable reference in the with_externalities global.

Doesn't have to be for this PR, but the entire runtime should eventually change to not use panics unless the program should actually halt.

pepyakin · 2018-01-28T22:41:59Z

executor/src/wasm_executor.rs

 		assert_eq!(
 			WasmExecutor.call(&mut ext, &test_code[..], "test_ed25519_verify", &CallData(calldata)).unwrap(),
-			vec![1]
+			vec![0]


Why this set to 0? Is this workaround?

0 is the legitimately expected result.

Hm, could you help me to understand this?

I thought this is a positive test, i.e. checks for successful verification.

Looking at test_ed25519_verify it returns casted bool taken from ed25519_verify to u8, so if ed25519_verify returns true then it should return vec![1u8] or vec![0u8] otherwise.

ed25519_verify have no documentation which one of boolean values returns in case of successful result, but I almost certain that true should be returned for successful verification.

you're right about point 1, but ed25519_verify returns a u32 not a bool, with success being 0 and error conditions (of which there are several types) being non-zero. test_ed25519_verify is just a passthrough with a u8 cast, so we still expect a 0 for success.

Hm, but if you follow my link, https://github.com/paritytech/polkadot/blob/1344dadd131629e39109ca7506d9f004da69eef4/wasm-runtime/std/src/lib.rs#L113
‘ed25519_verify’ in fact returbs a ‘bool’ not a u32

it is ‘ext_ed25519_verify’ returns u32

pepyakin · 2018-01-28T22:46:27Z

wasm-runtime/std/src/lib.rs

 pub fn ed25519_verify(sig: &[u8], msg: &[u8], pubkey: &[u8]) -> bool {
 	sig.len() != 64 || pubkey.len() != 32 || unsafe {
-		ext_ed25519_verify(&msg[0], msg.len() as u32, &sig[0], &pubkey[0])
+		ext_ed25519_verify(msg.as_ptr(), msg.len() as u32, sig.as_ptr(), pubkey.as_ptr())


It seems to me the code above contains an error, and can be rewritten as:

if sig.len() ≠ 64 { return true; } if pubkey.len() ≠ 32 { return true; } let err_code = unsafe { ext_ed25519_verify(msg.as_ptr(), msg.len() as u32, sig.as_ptr(), pubkey.as_ptr()) }; err_code == 0

I guess this isn't intended behavior...

Good catch!

pepyakin · 2018-01-29T10:37:59Z

executor/src/wasm_executor.rs

+		} else {
+			0
+		}
+	},


memcmp should return 0 if sl1 == sl2, i.e. all bytes are equal,
should return a negative value if first different byte in sl1 is less than byte at the same position in sl2 and should return a positive value if this different byte from sl1 is greater than corresponding byte from sl2.

The problem is memcmp::Memcmp for some reason returns bool value, and seems like it returns true if slices are equal. So this implementation returns 1 in case slices are equal and 0 in other cases.

Also, in case if s1 or s2 can't be read, you return zero. This is not strictly a problem, because passing s1 or s2 which are out of bounds is undefined behavior and for such case returning 0 is perfeclty legal.
However, I strongly recommend to trap here, because:

returning some value, especially 0 (meaning that s1 == s2), might lead to serious security problems,

such behavior will be inline with behavior of memcmp implemented within wasm itself.

Here you can find test cases which might help you to test implementation of memcmp extern.

pepyakin · 2018-01-29T10:44:00Z

wasm-runtime/pwasm-libc/src/lib.rs

 	ext_memcpy(dest, src, n)
 }

+/// memcpy extern


typo memcmp extern (or can we just get rid of these comments?)

rphmeier · 2018-01-29T16:36:44Z

executor/src/wasm_executor.rs

-			use memcmp::Memcmp;
-			(&sl1).memcmp(&sl2) as i32
+			unsafe {
+				memcmp(sl1.as_ptr() as *const u8 as *const c_void,


can be replaced with safe code:

match sl1.cmp(&sl2) { Ordering::Greater => 1, Ordering::Less => -1, Ordering::Equal => 0, }

https://doc.rust-lang.org/stable/src/core/slice/mod.rs.html#2571

it's specialized to compile down to a memcmp and be inlined.

Insert keys using curl

Feature/xyk documentation

* Add runtime/pos module * Add staking module and new substrate

Unify the styling of dependencies in Cargo.toml

Pass signature as &[u8] to avoid triggering From::from conversion

* Fix the module index used to create a encoded func Same as Events, modules with no calls should be ignored when calculating the module index. * Update substrate

gavofyork added 30 commits January 8, 2018 17:13

Introduce basic skeleton for Polkador runtime.

36c171c

Clean up the runtime skeleton.

f79b7bb

Make initial runtime skeleton compile.

b48e053

Compile polkadot-runtime both for Wasm ad native, allowing for testin…

4e25b24

…g and direct usage.

More fleshing out on runtime.

b28402c

Update native support.

bac50a4

Fix warning.

9a4360f

Update gitignore

0890e72

Update path.

4cf5fd1

Fix path.

12b15fd

Remove accidentally committed files.

47260c2

Add wasm binaries.

800eb20

Fix test.

10eaefe

Native storage support API.

24ba809

Add environmental module

36a49ff

Add native environment to make native source-code compatible with wasm.

50e7222

Also tests.

Finish up & polish environment stuff.

3e17caa

Avoid using reentrancy issues.

e807c9a

Add some docs and a test.

402ae29

Remove unneeded function.

762826b

Documentation

7e969c8

Tweak docs

64b0670

Remove TODOs.

d34f62a

Balance transfers + util methods.

89f35e3

Rejig tests and ensure authorities are addressed consistently.

5e42240

Add marshaller for xfer function

d97e2d6

Transaction dispatch test.

26ccc14

Minor fix.

7258e1f

Add test for ser/de transaction.

c02e335

Add ser/de for header.

28fa761

pepyakin reviewed Jan 28, 2018

View reviewed changes

gavofyork added 8 commits January 28, 2018 20:00

Use consts rather than direct names.

a461c60

Minor cleanup.

5ab4e21

Less constant-bloat.

36f358b

Bulid script fails fast.

11cc9c9

Remove old comment

80a3bdf

Avoid using the controversial Vec::from_raw_parts.

e2c6737

Name more consistently with std.

8b0c2a2

Keep naming consistent.

004fb1d

rphmeier reviewed Jan 28, 2018

View reviewed changes

Move constants to top of files (even though they're private)

15fad41

rphmeier reviewed Jan 28, 2018

View reviewed changes

pepyakin reviewed Jan 28, 2018

View reviewed changes

Fix verify logic.

1344dad

pepyakin reviewed Jan 29, 2018

View reviewed changes

arkpar self-assigned this Jan 29, 2018

arkpar approved these changes Jan 29, 2018

View reviewed changes

arkpar removed their assignment Jan 29, 2018

Correct implementation of memcmp

f41d078

rphmeier reviewed Jan 29, 2018

View reviewed changes

Make memcmp safe.

5806504

gavofyork merged commit 9d4beb3 into master Jan 30, 2018

gavofyork deleted the native-executor branch January 30, 2018 09:58

JoshOrndorff referenced this pull request in moonbeam-foundation/substrate Apr 21, 2021

Merge pull request #47 from PureStake/joshy-insert-keys-curl

a812449

Insert keys using curl

iStrike7 referenced this pull request in gasp-xyz/substrate Aug 3, 2021

Merge pull request #47 from mangata-finance/feature/xyk-documentation

f484f00

Feature/xyk documentation

liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021

Feature/bft pos (paritytech#47)

16b0ea9

* Add runtime/pos module * Add staking module and new substrate

liuchengxu referenced this pull request in autonomys/substrate Jun 3, 2022

Merge pull request #47 from liuchengxu/clean-up-deps

3d10233

Unify the styling of dependencies in Cargo.toml

vivekvpandya pushed a commit to zeitgeistpm/substrate that referenced this pull request Nov 29, 2022

Merge pull request paritytech#47 from PureStake/nish-fix-verify-panic

01bd16c

Pass signature as &[u8] to avoid triggering From::from conversion

Conversation

gavofyork commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Jan 29, 2018 • 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!

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 Jan 29, 2018 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gavofyork commented Jan 23, 2018 •

edited

Loading

pepyakin Jan 29, 2018 •

edited

Loading

pepyakin Jan 29, 2018 •

edited

Loading