Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions bin/node/testing/src/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ impl BenchDb {
ExecutionExtensions::new(profile.into_execution_strategies(), None),
Box::new(TaskExecutor::new()),
None,
Default::default(),
).expect("Should not fail");

(client, backend)
Expand Down
10 changes: 9 additions & 1 deletion client/api/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
use std::sync::Arc;
use std::collections::HashMap;
use sp_core::ChangesTrieConfigurationRange;
use sp_core::offchain::OffchainStorage;
use sp_core::offchain::{OffchainStorage,storage::OffchainOverlayedChanges};
use sp_runtime::{generic::BlockId, Justification, Storage};
use sp_runtime::traits::{Block as BlockT, NumberFor, HashFor};
use sp_state_machine::{
Expand Down Expand Up @@ -148,6 +148,14 @@ pub trait BlockImportOperation<Block: BlockT> {
child_update: ChildStorageCollection,
) -> sp_blockchain::Result<()>;

/// Set offchain storage changes.
fn update_offchain_storage(
&mut self,
_offchain_update: OffchainOverlayedChanges,
) -> sp_blockchain::Result<()> {
Ok(())
}

/// Inject changes trie data into the database.
fn update_changes_trie(
&mut self,
Expand Down
3 changes: 2 additions & 1 deletion client/api/src/call_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use sp_state_machine::{
};
use sc_executor::{RuntimeVersion, NativeVersion};
use sp_externalities::Extensions;
use sp_core::NativeOrEncoded;
use sp_core::{NativeOrEncoded,offchain::storage::OffchainOverlayedChanges};

use sp_api::{ProofRecorder, InitializeBlock, StorageTransactionCache};
use crate::execution_extensions::ExecutionExtensions;
Expand Down Expand Up @@ -84,6 +84,7 @@ pub trait CallExecutor<B: BlockT> {
method: &str,
call_data: &[u8],
changes: &RefCell<OverlayedChanges>,
offchain_changes: &RefCell<OffchainOverlayedChanges>,
storage_transaction_cache: Option<&RefCell<
StorageTransactionCache<B, <Self::Backend as crate::backend::Backend<B>>::State>,
>>,
Expand Down
4 changes: 2 additions & 2 deletions client/api/src/execution_extensions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ impl<Block: traits::Block> ExecutionExtensions<Block> {
if let ExecutionContext::OffchainCall(Some(ext)) = context {
extensions.register(
OffchainExt::new(offchain::LimitedExternalities::new(capabilities, ext.0))
)
);
}

(manager, extensions)
Expand All @@ -202,4 +202,4 @@ impl<Block: traits::Block> offchain::TransactionPool for TransactionPoolAdapter<

self.pool.submit_at(&self.at, xt)
}
}
}
12 changes: 12 additions & 0 deletions client/cli/src/arg_enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,18 @@ arg_enum! {
}
}


arg_enum! {
/// Whether off-chain workers are enabled.
#[allow(missing_docs)]
#[derive(Debug, Clone)]
pub enum OffchainWorkerEnabled {
Always,
Never,
WhenValidating,
}
}

/// Default value for the `--execution-syncing` parameter.
pub const DEFAULT_EXECUTION_SYNCING: ExecutionStrategy = ExecutionStrategy::NativeElseWasm;
/// Default value for the `--execution-import-block` parameter.
Expand Down
8 changes: 7 additions & 1 deletion client/cli/src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@ macro_rules! substrate_cli_subcommands {
}
}

fn offchain_worker_params(&self) -> Option<&$crate::OffchainWorkerParams> {
match self {
$($enum::$variant(cmd) => cmd.offchain_worker_params()),*
}
}

fn base_path(&self) -> $crate::Result<::std::option::Option<::std::path::PathBuf>> {
match self {
$($enum::$variant(cmd) => cmd.base_path()),*
Expand Down Expand Up @@ -327,7 +333,7 @@ macro_rules! substrate_cli_subcommands {
}
}

fn offchain_worker(&self, role: &::sc_service::Role) -> $crate::Result<bool> {
fn offchain_worker(&self, role: &::sc_service::Role) -> $crate::Result<::sc_service::config::OffchainWorkerConfig> {
match self {
$($enum::$variant(cmd) => cmd.offchain_worker(role)),*
}
Expand Down
41 changes: 9 additions & 32 deletions client/cli/src/commands/runcmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use crate::params::KeystoreParams;
use crate::params::NetworkParams;
use crate::params::SharedParams;
use crate::params::TransactionPoolParams;
use crate::params::OffchainWorkerParams;
use crate::CliConfiguration;
use regex::Regex;
use sc_service::{
Expand All @@ -28,18 +29,7 @@ use sc_service::{
};
use sc_telemetry::TelemetryEndpoints;
use std::net::{IpAddr, Ipv4Addr, SocketAddr};
use structopt::{clap::arg_enum, StructOpt};

arg_enum! {
/// Whether off-chain workers are enabled.
#[allow(missing_docs)]
#[derive(Debug, Clone)]
pub enum OffchainWorkerEnabled {
Always,
Never,
WhenValidating,
}
}
use structopt::StructOpt;

/// The `run` command used to run a node.
#[derive(Debug, StructOpt, Clone)]
Expand Down Expand Up @@ -173,17 +163,9 @@ pub struct RunCmd {
#[structopt(long = "telemetry-url", value_name = "URL VERBOSITY", parse(try_from_str = parse_telemetry_endpoints))]
pub telemetry_endpoints: Vec<(String, u8)>,

/// Should execute offchain workers on every block.
///
/// By default it's only enabled for nodes that are authoring new blocks.
#[structopt(
long = "offchain-worker",
value_name = "ENABLED",
possible_values = &OffchainWorkerEnabled::variants(),
case_insensitive = true,
default_value = "WhenValidating"
)]
pub offchain_worker: OffchainWorkerEnabled,
#[allow(missing_docs)]
#[structopt(flatten)]
pub offchain_worker_params: OffchainWorkerParams,

#[allow(missing_docs)]
#[structopt(flatten)]
Expand Down Expand Up @@ -300,6 +282,10 @@ impl CliConfiguration for RunCmd {
Some(&self.keystore_params)
}

fn offchain_worker_params(&self) -> Option<&OffchainWorkerParams> {
Some(&self.offchain_worker_params)
}

fn node_name(&self) -> Result<String> {
let name: String = match (self.name.as_ref(), self.get_keyring()) {
(Some(name), _) => name.to_string(),
Expand Down Expand Up @@ -439,15 +425,6 @@ impl CliConfiguration for RunCmd {
Ok(self.unsafe_rpc_expose)
}

fn offchain_worker(&self, role: &Role) -> Result<bool> {
Ok(match (&self.offchain_worker, role) {
(OffchainWorkerEnabled::WhenValidating, Role::Authority { .. }) => true,
(OffchainWorkerEnabled::Always, _) => true,
(OffchainWorkerEnabled::Never, _) => false,
(OffchainWorkerEnabled::WhenValidating, _) => false,
})
}

fn transaction_pool(&self) -> Result<TransactionPoolOptions> {
Ok(self.pool_config.transaction_pool())
}
Expand Down
21 changes: 14 additions & 7 deletions client/cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@
use crate::error::Result;
use crate::{
init_logger, ImportParams, KeystoreParams, NetworkParams, NodeKeyParams,
PruningParams, SharedParams, SubstrateCli,
OffchainWorkerParams, PruningParams, SharedParams, SubstrateCli,
};
use crate::arg_enums::Database;
use app_dirs::{AppDataType, AppInfo};
use names::{Generator, Name};
use sc_service::config::{
Configuration, DatabaseConfig, ExecutionStrategies, ExtTransport, KeystoreConfig,
NetworkConfiguration, NodeKeyConfig, PrometheusConfig, PruningMode, Role, TelemetryEndpoints,
TransactionPoolOptions, WasmExecutionMethod,
NetworkConfiguration, NodeKeyConfig, OffchainWorkerConfig, PrometheusConfig, PruningMode,
Role, TelemetryEndpoints, TransactionPoolOptions, WasmExecutionMethod,
};
use sc_service::{ChainSpec, TracingReceiver};
use std::future::Future;
Expand Down Expand Up @@ -67,6 +67,11 @@ pub trait CliConfiguration: Sized {
None
}

/// Get the OffchainWorkerParams for this object
fn offchain_worker_params(&self) -> Option<&OffchainWorkerParams> {
None
}

/// Get the NodeKeyParams for this object
fn node_key_params(&self) -> Option<&NodeKeyParams> {
self.network_params()
Expand Down Expand Up @@ -301,11 +306,13 @@ pub trait CliConfiguration: Sized {
Ok(Default::default())
}

/// Returns `Ok(true)` if offchain worker should be used
/// Returns a offchain worker config wrapped in `Ok(_)`
///
/// By default this is `false`.
fn offchain_worker(&self, _role: &Role) -> Result<bool> {
Ok(Default::default())
/// By default offchain workers are disabled.
fn offchain_worker(&self, role: &Role) -> Result<OffchainWorkerConfig> {
self.offchain_worker_params()
.map(|x| x.offchain_worker(role))
.unwrap_or_else(|| { Ok(OffchainWorkerConfig::default()) })
}

/// Returns `Ok(true)` if authoring should be forced
Expand Down
2 changes: 2 additions & 0 deletions client/cli/src/params/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ mod node_key_params;
mod pruning_params;
mod shared_params;
mod transaction_pool_params;
mod offchain_worker_params;

use std::fmt::Debug;
use std::str::FromStr;
Expand All @@ -29,6 +30,7 @@ pub use crate::params::import_params::*;
pub use crate::params::keystore_params::*;
pub use crate::params::network_params::*;
pub use crate::params::node_key_params::*;
pub use crate::params::offchain_worker_params::*;
pub use crate::params::pruning_params::*;
pub use crate::params::shared_params::*;
pub use crate::params::transaction_pool_params::*;
Expand Down
77 changes: 77 additions & 0 deletions client/cli/src/params/offchain_worker_params.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// Copyright 2020 Parity Technologies (UK) Ltd.
// This file is part of Substrate.

// Substrate is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Substrate is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.


//! Offchain worker related configuration parameters.
//!
//! A subset of configuration parameters which are relevant to
//! the inner working of offchain workers. The usage is solely
//! targeted at handling input parameter parsing providing
//! a reasonable abstraction.
use structopt::StructOpt;
use sc_service::config::OffchainWorkerConfig;
use sc_network::config::Role;

use crate::error;
use crate::OffchainWorkerEnabled;


/// Offchain worker related parameters.
#[derive(Debug, StructOpt, Clone)]
pub struct OffchainWorkerParams {
/// Should execute offchain workers on every block.
///
/// By default it's only enabled for nodes that are authoring new blocks.
#[structopt(
long = "offchain-worker",
value_name = "ENABLED",
possible_values = &OffchainWorkerEnabled::variants(),
case_insensitive = true,
default_value = "WhenValidating"
)]
pub enabled: OffchainWorkerEnabled,

/// Allow access to offchain workers indexing API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does the term indexing here comes from? It doesn't really connect for me to the description in the next line. Indexing implies some sort of caching similar to databases. Yet the description just says: Enables a runtime to write directly to ...

Maybe something like: ENABLE_OFFCHAIN_IMPORT_DB

Copy link
Contributor Author

@drahnr drahnr Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data that is going to be written is in some form a cache for i.e. slashing based on this historical data. I am not fixed on the terminology here, but just calling it import seems to be to unspecific. I would be tempted to call it OFFCHAIN_INDEX_API rather than INDEXING

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomusdrw I don't have opinion about this, I would prefer to keep it mostly as is to drive this to completion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this API allows the block import to insert something to offchain DB, so that it's available later.

  1. We want to emphasize, this is a write-only API, so I don't think that it should involve names like DB access or anything like that.
  2. It allows you to produce some data that forms something like an index of imported blocks. IMHO indexing emphasizes that the data is going to be complete (i.e. we can produce an index of all transactions in blocks accesible externaly via RPC, etc).
  3. I'm a bit reluctant to use "offchain" too much, to avoid the confusion with offchain workers. As @drahnr mentioned, these 2 concepts are pretty much disjoint (indexed data is already available via offchain-db RPCs, so you can access it externally). There are use cases however that will use both APIs - in such case whatever is put into the offchain index is consumed by offchain workers.

I'm not super happy with the naming, open to any suggestions. If we keep the current one, I'd like to request more attention to details and keep everything consistent, i.e.:
we should always use a full name Offchain Indexing API (or just Indexing API), but never mix it up with "workers", cause it has nothing to do.
The concepts we have:

  1. Off-chain DB - database that is not part of the consensus (i.e not trie)
  2. Off-chain Workers - an opt-in piece of code being run after block import, which has access to extended set of APIs (Off-chain (Worker) APIs), that includes access to Off-chain DB.
  3. Off-chain indexing API - an opt-in API that allows Off-chain DB to be populated (write) by block import.

///
/// Enables a runtime to write directly to a offchain workers
/// DB during block import.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// DB during block import.

#[structopt(
long = "enable-offchain-indexing",
value_name = "ENABLE_OFFCHAIN_INDEXING"
)]
pub indexing_enabled: bool,
Copy link
Contributor

@rphmeier rphmeier Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wpank @shawntabrizi we're going to want validators to run with this flag. Are there documentation updates corresponding?

@drahnr @tomusdrw Otherwise, would it make sense to automatically set indexing_enabled if we are a validator the same way we are automatically enabling offchain-workers for validators?

Copy link
Contributor Author

@drahnr drahnr Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a particular reason why it should should be disabled for validators by default.

}

impl OffchainWorkerParams {
/// Load spec to `Configuration` from `OffchainWorkerParams` and spec factory.
pub fn offchain_worker(
&self,
role: &Role,
) -> error::Result<OffchainWorkerConfig>
{
let enabled = match (&self.enabled, role) {
(OffchainWorkerEnabled::WhenValidating, Role::Authority { .. }) => true,
(OffchainWorkerEnabled::Always, _) => true,
(OffchainWorkerEnabled::Never, _) => false,
(OffchainWorkerEnabled::WhenValidating, _) => false,
};

let indexing_enabled = enabled && self.indexing_enabled;

Ok(OffchainWorkerConfig { enabled, indexing_enabled})
}
}
Loading