Initial: Offchain Workers by tomusdrw · Pull Request #1942 · paritytech/substrate

tomusdrw · 2019-03-07T09:26:56Z

Initial version of offchain workers (#1458)

The worker task is started after every block.
All modules by default do nothing in the offchain task, they can implement a behaviour by specifying fn offchain_worker() function in decl_module (alike on_initialise)
Runtime uses Executive module to dispatch the call to all modules.
Offchain tasks run with a different Context of execution, it means that additional externalities are available for those functions (currently only submit_extrinsic).
All inherents produced during execution end up in a very simple InherentsPool and are included as extrinsics (after regular inherents, but before transactions) when new block is created.

Additional work to folllow:

Implement a concrete example for delegated voting.
Support async fetch.
Handle long-running tasks gracefuly.
Require extrinsics to be included before a particular block
Use OpaqueTransaction for submit_extrinsic transaction type

…eworker

bkchr · 2019-03-07T11:17:05Z

core/cli/src/lib.rs

 		syncing: cli.syncing_execution.into(),
 		importing: cli.importing_execution.into(),
 		block_construction: cli.block_construction_execution.into(),
+		offchain_worker: ExecutionStrategy::NativeWhenPossible,


Should it always be NativeWhenPossible or configurable by the user at some point?

Could be configurable primarily to keep it consistent with all the other wasm execution. But I don't see any instance where you would not want to use native if it's there.

Maybe in debugging...

Yeah, I was not sure as well, but if it does not need to be configurable, we don't need to touch this struct at all.

I'll add the option for consistency.

given that native workers are not consensus critical, NativeWhenPossible seems like the most reasonable strategy

bkchr · 2019-03-07T11:35:00Z

core/primitives/src/lib.rs

+	/// or to the next produced block (inherent).
+	fn submit_extrinsic(&mut self, extrinsic: Vec<u8>);
+}
+impl<T: OffchainExt + ?Sized> OffchainExt for Box<T> {


Why do wen need this implementation? Box supports DerefMut, shouldn't that be enough?

Yeah, but DerefMut just means that we can use functions from that trait directly on Box<T>, we need to pass Box<T> to a generic function accepting T: OffchainExt though.

I wasn't sure about the API initially and chose to handle generic parameters instead of Box, but currently the only way we actually pass the offchain extensions is by ExecutionContext, and making it generic was introducing too much noise in the code so I chose to Box, but only there.

Let me know if you think it's better to change the other apis and structs to store Box instead of generic param.

bkchr · 2019-03-07T11:39:26Z

core/offchain/Cargo.toml

+[dependencies]
+client = { package = "substrate-client", path = "../../core/client" }
+consensus = { package = "substrate-consensus-common", path = "../../core/consensus/common" }
+futures = "0.1"


Suggested change

futures = "0.1"

futures = "0.1.25"

I don't see why it's better. Cargo.lock manages the concrete version anyway and it's just misleading to see 0.1.17 in some other packages (like core/client/Cargo.toml) while after all 0.1.25 is used.

@tomusdrw

Cargo.lock manages the concrete version anyway

not on downstream crates. we've been bitten by that before.

You should use futures = "0.1.x" where x is whatever you need for the features you use in this crate. Probably some of them were added after 0.1 -- your Cargo.toml says that any futures crate including 0.1 is supposed to work.

The best strategy is as Basti suggested; just use the latest patch version.

Yeah, I forgot that substrate is used as a library, I'm still not convinced though that it's a good strategy to just use latest patch version, the minimal futures version might be just managed in a single place (say service), cause one crate in the dependency tree is enough to define the minimal required version (specifying latest patch might be overly restrictive), that would also give us some consistency in other crates, right now it seems mostly random.

I should have said the easiest strategy, not best :)

Fair enough :)

bkchr · 2019-03-07T11:41:55Z

core/offchain/Cargo.toml

+authors = ["Parity Technologies <admin@parity.io>"]
+edition = "2018"
+
+[lib]


bkchr · 2019-03-07T11:48:35Z

core/client/src/client.rs

+				|| self.prepare_environment_block(at),
+				manager,
+				native_call,
+				Some(&mut ext),


Can we not just do:

match context { ExecutionContext::OffchainWorker(mut ext) => Some(&mut ext), _ => None, }

or something comparable, instead of cloning this function call?

Yeah, we can do that although it will result in a different type of Option<>. The NeverOffchainExt should be optimized out by the compiler, since it's not instantiable, but I guess it won't matter that much.

bkchr · 2019-03-07T12:00:48Z

core/offchain/primitives/src/lib.rs

+#![warn(missing_docs)]
+
+use client::decl_runtime_apis;
+use runtime_primitives::traits::{Header as HeaderT, Block as BlockT};


Suggested change

use runtime_primitives::traits::{Header as HeaderT, Block as BlockT};

use runtime_primitives::traits::NumberFor;

core/offchain/src/api.rs

bkchr · 2019-03-07T12:38:53Z

core/offchain/src/lib.rs

+		let has_api = runtime.has_api::<OffchainWorkerApi<Block>>(&at);
+		debug!("Checking offchain workers at {:?}: {:?}", at, has_api);
+
+		if let Ok(true) = has_api {


Suggested change

if let Ok(true) = has_api {

if has_api.unwrap_or(false) {

bkchr · 2019-03-07T12:46:00Z

core/sr-io/without_std.rs

+/// Depending on the kind of extrinsic it will either be:
+/// 1. scheduled to be included in the next produced block (inherent)
+/// 2. added to the pool and propagated (transaction)
+pub fn submit_extrinsic(data: Vec<u8>) {


Do we maybe want to switch to the following interface?

pub fn submit_extrinsic<T: Encode>(data: T) {

bkchr · 2019-03-07T12:48:09Z

core/executor/src/wasm_executor.rs

 		Ok(0)
 	},
+	ext_submit_extrinsic(msg_data: *const u8, len: u32) => {
+		let extrinsic = this.memory.get(msg_data, len as usize)


Do we maybe want to fail, when someone tries to call it on-chain?

this.ext.submit_extrinsic is going to fail or (as currently) trigger a warning. We can either panic (which I'm not a fan of in consensus code) or just return an Error?

Yeah return an Error. I don't know what happens with this error, but we will see it :D

Spoke with Sergei and errors are converted to traps in the wasm environment, so I've also added a panicking implementation in case of std environment.

Spoke with Sergei and the Errors are converted to traps in wasm environment so I've also added a panicking implementation in sr_io::with_std

svyatonik · 2019-03-22T09:19:00Z

core/state-machine/src/ext.rs

 	}
+
+	fn submit_extrinsic(&mut self, extrinsic: Vec<u8>) -> Result<(), ()> {
+		let _guard = panic_handler::AbortGuard::new(true);


I could possibly be wrong, but could you, please, verify @tomusdrw :

most of times runtime calls are performed with NeverOffchainExt;

NeverOffchainExt::submit_extrinsic is simply unreachable!();

this guard means that if we met panic!(), unreachable!(), etc in the code, the process will exit(1) (see panic_handler);

the runtime could try to call ext_submit_extrinsic anytime (i.e. from initialise_block), and, given (1)..(3) the process will exit with 1 instead of failing execution?

Oh, wait :) NeverOffchainExt::new returns None, so we will go to else branch here :)

svyatonik · 2019-03-22T09:28:12Z

core/offchain/src/lib.rs

+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+//! Substrate service. Starts a thread that spins up the network, client, and extrinsic pool.


Looks like docs are for substrate_service::Service

gui1117 · 2019-03-22T10:33:31Z

core/state-machine/src/ext.rs

 	/// data at this moment (block number).
 	changes_trie_transaction: Option<(u64, MemoryDB<H>, H::Out)>,
+	/// Additional externalities for offchain workers.
+	/// If None the some methods from the trait might not supported.


Suggested change

/// If None the some methods from the trait might not supported.

/// If None some methods from the trait might not supported.

My english is bad but this is a typo isn't it ?

pepyakin

Looks good to me! A few nits and one Q

pepyakin · 2019-03-22T09:44:30Z

core/state-machine/src/overlayed_changes.rs


 		let changes_trie_storage = InMemoryChangesTrieStorage::new();
-		let mut ext = Ext::new(&mut overlay, &backend, Some(&changes_trie_storage));
+		let mut ext = Ext::new(&mut overlay, &backend, Some(&changes_trie_storage), crate::NeverOffchainExt::new());


Can we wrap this?

Suggested change

let mut ext = Ext::new(&mut overlay, &backend, Some(&changes_trie_storage), crate::NeverOffchainExt::new());

let mut ext = Ext::new(

&mut overlay,

&backend,

Some(&changes_trie_storage),

crate::NeverOffchainExt::new(),

);

pepyakin · 2019-03-22T09:45:49Z

core/state-machine/src/lib.rs

 	{
-		let mut externalities = ext::Ext::new(self.overlay, self.backend, self.changes_trie_storage);
+		let offchain = self.offchain_ext.as_mut();
+		let mut externalities = ext::Ext::new(self.overlay, self.backend, self.changes_trie_storage, offchain.map(|x| &mut **x));


Can we wrap this?

Suggested change

let mut externalities = ext::Ext::new(self.overlay, self.backend, self.changes_trie_storage, offchain.map(|x| &mut **x));

let mut externalities = ext::Ext::new(

self.overlay,

self.backend,

self.changes_trie_storage,

offchain.map(|x| &mut **x),

);

pepyakin · 2019-03-22T09:50:51Z

core/primitives/src/lib.rs

+	///
+	/// The extrinsic will either go to the pool (signed)
+	/// or to the next produced block (inherent).
+	fn submit_extrinsic(&mut self, extrinsic: Vec<u8>);


I think it would be cool if we introduced a type alias for an OpaqueExtrinsic. This way it will be a) more apparent from the code that we deal with an extrinsic and b) will allow us to add some docs on why we use Vec for representing an extrinsic.

(Not to be addressed in this PR)

Yeah, I'm going to address that when valid_until parameter is introduced, then this will be a common type for Vec<u8> and BlockNumber

pepyakin · 2019-03-22T10:24:22Z

core/cli/src/lib.rs

+	config.offchain_worker = match (cli.offchain_worker, role) {
+		(params::OffchainWorkerEnabled::WhenValidating, service::Roles::AUTHORITY) => true,
+		(params::OffchainWorkerEnabled::Always, _) => true,
+		_ => false,


Let's use exhaustive matching so we don't trip up accidently if we add a new variant here.

Suggested change

_ => false,

(params::OffchainWorkerEnabled::Never, _) => false,

pepyakin · 2019-03-22T10:42:36Z

core/offchain/src/lib.rs

+
+		if has_api.unwrap_or(false) {
+			let (api, runner) = api::Api::new(pool.clone(), self.inherents_pool.clone(), at.clone());
+			self.executor.spawn(runner.process());


Sorry I don't know much about Rust futures, but can you ELI5 how this terminates?
Is the receiver returns Ready when tx is dropped?

Yeah, so when the work runtime is doing is finished the api transmitting end will be dropped, that in turn will cause the receiving.for_each stream to be terminated, so the entire future returned from process() will terminate.

But we currently allow the offchain workers to run arbitrarily long, it might make sense actually to kill them after some timeout, but that's something I don't really know how to do:

I think it's not possible to terminate wasm task currently

We could error when the worker attempts to use ext_submit_extrinsic (or even any other externality), but obviously it can also never happen.

svyatonik · 2019-03-22T14:32:52Z

core/offchain/src/lib.rs

+		A: ChainApi<Block=Block> + 'static,
+	{
+		let runtime = self.client.runtime_api();
+		let at = BlockId::number(*number);


One last question: why block is identified by number (i.e. not the hash)? What if not best block is imported? Then the offchain worker could be executed with state of some other block (and probably it could be executed several times if there are several competing forks of the height number). Is this intended?

Yes, I think that's expected. Since the offchain worker produces inherents for next block they might look different if an alternative fork is imported.
We might implement a way to invalidate previously generated inherents/extrinsics though, however
offchain workers are also supposed to have a local storage in the future, so two "competing" tasks could have some sort of communication between runs i.e. the second run for block at height X will be able to tell it's on a fork that now became canon and for instance submit extrinsics that will replace the previous ones.

Numeric block number will also later be used by the offchain worker code to calculate lifetime of produced extrinsics (i.e. you can say I submit this inherent after performing a super-long-running computation, but it's only valid before block bn + 3), or you can also use that to execute some computation every X blocks.

* Refactor state-machine stuff. * Fix tests. * WiP * WiP2 * Service support for offchain workers. * Service support for offchain workers. * Testing offchain worker. * Initial version working. * Pass side effects in call. * Pass OffchainExt in context. * Submit extrinsics to the pool. * Support inherents. * Insert to inherents pool. * Inserting to the pool asynchronously. * Add test to offchain worker. * Implement convenience syntax for modules. * Dispatching offchain worker through executive. * Fix offchain test. * Remove offchain worker from timestamp. * Update Cargo.lock. * Address review comments. * Use latest patch version for futures. * Add CLI parameter for offchain worker. * Fix compilation. * Fix test. * Fix extrinsics format for tests. * Fix RPC test. * Bump spec version. * Fix executive. * Fix support macro. * Address grumbles. * Bump runtime

Polkadot-Forum · 2023-04-07T13:36:33Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/offchain-workers-design-assumptions-vulnerabilities/2548/1

tomusdrw added 25 commits February 21, 2019 14:25

Refactor state-machine stuff.

28faa01

Merge branch 'master' into td-state-machine

d0597bf

Fix tests.

eb63210

WiP

98cbd8d

WiP2

4ba2fb3

Merge branch 'master' into td-offlineworker

af67cf0

Service support for offchain workers.

acd003e

Service support for offchain workers.

1d55220

Testing offchain worker.

c12dbb9

Merge remote-tracking branch 'origin/td-offlineworker' into td-offlin…

95c22f3

…eworker

Merge branch 'master' into td-offlineworker

c76c996

Initial version working.

66d484d

Pass side effects in call.

bf49911

Pass OffchainExt in context.

45b6d68

Submit extrinsics to the pool.

049a4e5

Merge branch 'master' into td-offlineworker

07082be

Support inherents.

37afd7d

Insert to inherents pool.

c820a89

Inserting to the pool asynchronously.

646c20e

Add test to offchain worker.

f953ffe

Implement convenience syntax for modules.

b10b7d2

Dispatching offchain worker through executive.

2d8b6f9

Fix offchain test.

3f2a8ec

Remove offchain worker from timestamp.

301fd98

Merge branch 'master' into td-offlineworker

e026333

tomusdrw added A0-please_review Pull request needs code review. M4-core labels Mar 7, 2019

devops-parity added the B2-breaksapi label Mar 7, 2019

Update Cargo.lock.

f577822

bkchr reviewed Mar 7, 2019

View reviewed changes

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

tomusdrw added 7 commits March 20, 2019 11:14

Merge branch 'master' into td-offlineworker

79ae654

Bump spec version.

5713923

Fix executive.

ed764bc

Fix support macro.

f9c1621

Merge branch 'master' into td-offlineworker

a8d2f33

Merge branch 'master' into td-offlineworker

cbdb76f

Merge branch 'master' into td-offlineworker

5036cd2

tomusdrw removed A3-needsresolving labels Mar 22, 2019

svyatonik reviewed Mar 22, 2019

View reviewed changes

gui1117 reviewed Mar 22, 2019

View reviewed changes

pepyakin approved these changes Mar 22, 2019

View reviewed changes

Address grumbles.

d20980d

svyatonik reviewed Mar 22, 2019

View reviewed changes

svyatonik approved these changes Mar 22, 2019 •

edited

Loading

View reviewed changes

tomusdrw and others added 2 commits March 25, 2019 14:19

Merge branch 'master' into td-offlineworker

dd8a756

Merge remote-tracking branch 'origin/master' into td-offlineworker

994918a

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

Bump runtime

aa37a4c

gavofyork removed the A4-gotissues label Mar 25, 2019

gavofyork merged commit a30d6a1 into master Mar 25, 2019

gavofyork deleted the td-offlineworker branch March 25, 2019 22:23

soulofamachine mentioned this pull request Mar 29, 2019

Tutorial: How to build an Oracle on Substrate (consensus based) polkadot-developers/substrate-developer-hub.github.io#63

Closed

lsaether mentioned this pull request May 3, 2019

Off chain worker information w3f/polkadot-wiki-old#145

Open

tomusdrw mentioned this pull request Jun 18, 2019

Off-chain oracle modules #1458

Closed

kigawas mentioned this pull request Jun 27, 2019

How to add a custom service/component? #2964

Closed

	use runtime_primitives::traits::{Header as HeaderT, Block as BlockT};
	use runtime_primitives::traits::NumberFor;

	/// If None the some methods from the trait might not supported.
	/// If None some methods from the trait might not supported.

	_ => false,
	(params::OffchainWorkerEnabled::Never, _) => false,

Conversation

tomusdrw commented Mar 7, 2019 • edited by sekisamu 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!

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!

Choose a reason for hiding this comment

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!

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 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

tomusdrw commented Mar 7, 2019 •

edited by sekisamu

Loading