Introduce srml/im-online #3079

cmichi · 2019-07-10T10:25:02Z

Addresses #2719.

There are not yet any tests, I'll add those in a follow-up PR. The PR is mostly here to quickly get this into the review pipeline and master.

I currently keep only the Vec<u8> version of Multiaddr and PeerId.to_string() in the runtime, since parity-multiaddr doesn't compile for wasm currently and libp2p::core::PeerId doesn't support encoding/serializing yet.

srml/im-online/src/lib.rs

tomaka

The way we fetch external addresses is really not great and goes in the opposite direction of what I'm doing, but considering that this is pressing this is an acceptable approach I'll do some refactoring later.

core/service/Cargo.toml

core/offchain/src/api.rs

core/offchain/Cargo.toml

core/offchain/src/api.rs

tomaka · 2019-07-10T10:46:58Z

Also, parity-multiaddr should normally compile fine for WASM. It's used in my WASM browser demo.

core/sr-primitives/src/testing.rs

Remove parity-multiaddr dependency Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

…te into cmichi-srml-im-online

It was updated in master in the meantime.

cmichi · 2019-07-10T14:23:35Z

@tomaka

Also, parity-multiaddr should normally compile fine for WASM. It's used in my WASM browser demo.

Ok, thanks. I'll keep the encoded version in WASM for now, since we will probably never need the info in WASM, but only "outside". So we can save the dependency until there is a need to have the encoded object in WASM.

I addressed your comments, can you please take a look again?

tomaka

The non-runtime part looks good to me. Someone else should give their approval for the runtime part.

tomusdrw

A bunch of grumbles.

core/network/src/service.rs

core/offchain/src/api.rs

core/primitives/src/offchain.rs

tomusdrw · 2019-07-10T14:42:52Z

core/sr-io/without_std.rs

+		///
+		/// # Returns
+		///
+		/// - The encoded `Result<offchain::LocalNetworkState, ()>`.


I was about to write a comment that other methods usually don't encode the data, but try to return them using some primitives, but actually I think it's quite alright (I think this small performance hit is acceptable, especially for offchain workers).
@pepyakin any reason why we don't do that for every call? We could even have a single method that needs to be implemented by Rust host that just takes (method: *const u8, method_len: u32, params: *const u8, method_len: u32) and returns a scale-encoded result. That would simplify the wasm_executor and externalities a lot.

That's a good question and I cannot answer it properly because I wasn't the most involved person in designing and adding the externalities.

About the single syscall-like entrypoint: I actually had some thoughts about this kind of design for srml-contracts wasm environment. This kind of build up would add some inflexibility and overhead into the substrate host interface. This doesn't just harms the compilation efficiency, but also introduces more high-level problems. For instance, if your runtime wants to call a function that takes two buffers and you have them at hand you cannot pass them directly and would have to encode them probably (re)allocating more than once.

Also, there is another special thing about this design: it has properties of dynamic dispatch. The default way of interaction between a wasm instance and the host is by resolving all imports at the instantiation time and if the instance imports something that doesn't exist the instance cannot be instantiated. On the other hand, with such a dynamic dispatch it would instantiate successfully (cause it imports only the syscall entrypoint) but when it would fail only at the runtime when it tries to call a non-supported function. In general this can be a feature or an issue, but I am inclined to think that in the case of substrate it is more an issue since I doubt that there is a lot to do with conditional logic (the runtime should behave the same regardless the set of available functions).

But yeah, it would definitely make the interface simpler.
As a reference, there is #2334 to achieve the same (but arguably more complex than what you propose)

srml/im-online/src/lib.rs

pepyakin

Gave a brief review, will do another round later

core/sr-io/without_std.rs

core/executor/src/wasm_executor.rs

Co-Authored-By: Sergei Pepyakin <sergei@parity.io>

gavofyork · 2019-07-18T17:22:48Z

core/primitives/src/offchain.rs

+	/// Returns information about the local node's network state.
+	fn network_state(&self) -> Result<OpaqueNetworkState, ()>;
+
+	/// Returns the locally configured authority public key, if available.


What is the "authority public key"? There are several keys in use for authorities now.

There exists a trait for that with this implementation, which I call to get the current authority public key. This whole part is currently getting reworked in #3034, so I had the assumption that it's still fine in master?

I started refactoring the offchain workers api (discussed with @tomusdrw) while @tomaka is refactoring the service factory/components. I'm on vacation till next week. The plan is to rebase #3034 on those PRs

node/runtime/src/lib.rs

core/service/Cargo.toml

core/offchain/Cargo.toml

gavofyork

Changes needed.

core/network/src/service.rs

bkchr · 2019-07-19T06:28:16Z

core/network/src/service.rs

@@ -520,6 +562,8 @@ enum ServerToWorkerMsg<B: BlockT, S: NetworkSpecialization<B>> {
 /// You are encouraged to poll this in a separate background thread or task.
 #[must_use = "The NetworkWorker must be polled in order for the network to work"]
 pub struct NetworkWorker<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> {
+	/// Updated by the `NetworkWorker` and loaded by the `NetworkService`.


Where is this loaded by the networkservice?

Here: https://github.com/paritytech/substrate/pull/3079/files#diff-a045635f3eec17c06b3c7ac7944acceaR500, do you have concerns regarding the name "loading" being associated with the method load()?

core/network/src/service.rs

bkchr · 2019-07-19T06:39:13Z

srml/im-online/src/lib.rs

@@ -0,0 +1,442 @@
+// Copyright 2017-2019 Parity Technologies (UK) Ltd.


The copyright date is incorrect. It should be just 2019.

srml/im-online/src/lib.rs

gavofyork · 2019-07-19T16:15:10Z

srml/aura/src/lib.rs

@@ -156,7 +156,7 @@ pub trait Trait: timestamp::Trait {
 	type HandleReport: HandleReport;

 	/// The identifier type for an authority.
-	type AuthorityId: Member + Parameter + TypedKey + Default;
+	type AuthorityId: Member + Parameter + TypedKey + Default + AsRef<[u8]>;


No - there is no need for AsRef([u8]) when it already implies Encode: just switch whatever(id.as_ref()) to id.using_encoded(|b| whatever(b)).

gavofyork

Still extraneous stuff

tomaka

Re-confirming

cmichi added 6 commits July 10, 2019 11:50

Fix grammar and typo

1e77e12

Extend network service

cfbec8c

Extend offchain API

5252851

Support creating unsigned UncheckedExtrinsic

1d5151b

Introduce srml/im-online

2c89345

Bump impl and spec version

e9a9028

cmichi added the A0-please_review Pull request needs code review. label Jul 10, 2019

cmichi requested a review from tomusdrw July 10, 2019 10:25

devops-parity added the B2-breaksapi label Jul 10, 2019

cmichi commented Jul 10, 2019

View reviewed changes

srml/im-online/src/lib.rs Outdated Show resolved Hide resolved

Fix web-wasm test

dc531dc

tomaka suggested changes Jul 10, 2019

View reviewed changes

core/service/Cargo.toml Show resolved Hide resolved

core/offchain/src/api.rs Outdated Show resolved Hide resolved

core/offchain/Cargo.toml Outdated Show resolved Hide resolved

core/offchain/src/api.rs Outdated Show resolved Hide resolved

tomaka suggested changes Jul 10, 2019

View reviewed changes

core/sr-primitives/src/testing.rs Outdated Show resolved Hide resolved

cmichi and others added 7 commits July 10, 2019 15:02

Merge branch 'master' into cmichi-srml-im-online

da1679b

Apply suggestions from code review

eeb4df5

Remove parity-multiaddr dependency Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

Replace transmute with from_raw_parts

4895cad

Replace PeerId.to_string() with .to_base58()

b8fe709

Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

Merge branch 'cmichi-srml-im-online' of github.com:paritytech/substra…

2256919

…te into cmichi-srml-im-online

Update Cargo.lock

0d1289b

Bump impl and spec version (again)

44cd516

It was updated in master in the meantime.

tomaka approved these changes Jul 10, 2019

View reviewed changes

tomusdrw reviewed Jul 10, 2019

View reviewed changes

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

mxinden reviewed Jul 12, 2019

View reviewed changes

srml/im-online/src/lib.rs Outdated Show resolved Hide resolved

pepyakin reviewed Jul 13, 2019

View reviewed changes

cmichi and others added 2 commits July 15, 2019 08:50

Merge branch 'master' into cmichi-srml-im-online

73f2bb5

Apply suggestions from code review

56d25bd

Co-Authored-By: Sergei Pepyakin <sergei@parity.io>

tomusdrw added A8-looksgood and removed A5-grumble labels Jul 18, 2019

gavofyork reviewed Jul 18, 2019

View reviewed changes

gavofyork reviewed Jul 19, 2019

View reviewed changes

node/runtime/src/lib.rs Outdated Show resolved Hide resolved

gavofyork reviewed Jul 19, 2019

View reviewed changes

core/service/Cargo.toml Outdated Show resolved Hide resolved

gavofyork reviewed Jul 19, 2019

View reviewed changes

core/offchain/Cargo.toml Outdated Show resolved Hide resolved

gavofyork suggested changes Jul 19, 2019

View reviewed changes

gavofyork added this to the 2.0-k milestone Jul 19, 2019

gavofyork added A5-grumble and removed A8-looksgood labels Jul 19, 2019

bkchr reviewed Jul 19, 2019

View reviewed changes

cmichi added 9 commits July 19, 2019 11:12

Introduce IsMember trait

0210000

Implement misc comments

7554808

Remove unused function

f20a19b

Fix test

e4ab8da

Fix external_addresses being written

e3cea15

Merge branch 'master' into cmichi-srml-im-online

c5828e8

Fix test

3f88973

Add necessary trait bound

537842d

Do not increment version

1e50257

gavofyork reviewed Jul 19, 2019

View reviewed changes

gavofyork suggested changes Jul 19, 2019

View reviewed changes

Update lib.rs

e73329f

tomaka approved these changes Jul 19, 2019

View reviewed changes

gavofyork approved these changes Jul 20, 2019

View reviewed changes

gavofyork merged commit aa8c06a into master Jul 20, 2019

gavofyork deleted the cmichi-srml-im-online branch July 20, 2019 01:19

dvc94ch mentioned this pull request Jul 20, 2019

Refactors the offchain worker api #3150

Merged

gnunicorn modified the milestones: 2.1, Polkadot Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce srml/im-online #3079

Introduce srml/im-online #3079

cmichi commented Jul 10, 2019

tomaka left a comment

tomaka commented Jul 10, 2019

cmichi commented Jul 10, 2019 •

edited

Loading

tomaka left a comment

tomusdrw left a comment

tomusdrw Jul 10, 2019

pepyakin Jul 13, 2019

pepyakin left a comment

gavofyork Jul 18, 2019

cmichi Jul 18, 2019 •

edited

Loading

gavofyork Jul 19, 2019

dvc94ch Jul 19, 2019

gavofyork left a comment

bkchr Jul 19, 2019

cmichi Jul 19, 2019

bkchr Jul 19, 2019

gavofyork Jul 19, 2019

gavofyork left a comment

tomaka left a comment

		@@ -0,0 +1,442 @@
		// Copyright 2017-2019 Parity Technologies (UK) Ltd.

Introduce srml/im-online #3079

Introduce srml/im-online #3079

Conversation

cmichi commented Jul 10, 2019

tomaka left a comment

Choose a reason for hiding this comment

tomaka commented Jul 10, 2019

cmichi commented Jul 10, 2019 • edited Loading

tomaka left a comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmichi Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork 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

gavofyork left a comment

Choose a reason for hiding this comment

tomaka left a comment

Choose a reason for hiding this comment

cmichi commented Jul 10, 2019 •

edited

Loading

cmichi Jul 18, 2019 •

edited

Loading