Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Reduce provisioner work #6328

Merged
20 commits merged into from
Dec 2, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
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
3 changes: 3 additions & 0 deletions Cargo.lock

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

2 changes: 2 additions & 0 deletions node/client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ authors = ["Parity Technologies <admin@parity.io>"]
edition = "2021"

[dependencies]
async-trait = "0.1.57"
futures = "0.3.21"
frame-benchmarking = { git = "https://github.com/paritytech/substrate", branch = "master" }
frame-benchmarking-cli = { git = "https://github.com/paritytech/substrate", branch = "master" }
pallet-transaction-payment = { git = "https://github.com/paritytech/substrate", branch = "master" }
Expand Down
7 changes: 4 additions & 3 deletions node/client/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ pub fn benchmark_inherent_data(
// Assume that all runtimes have the `timestamp` pallet.
let d = std::time::Duration::from_millis(0);
let timestamp = sp_timestamp::InherentDataProvider::new(d.into());
timestamp.provide_inherent_data(&mut inherent_data)?;
futures::executor::block_on(timestamp.provide_inherent_data(&mut inherent_data))?;

let para_data = polkadot_primitives::v2::InherentData {
bitfields: Vec::new(),
Expand All @@ -368,8 +368,9 @@ pub fn benchmark_inherent_data(
parent_header: header,
};

polkadot_node_core_parachains_inherent::ParachainsInherentDataProvider::from_data(para_data)
.provide_inherent_data(&mut inherent_data)?;
inherent_data
.put_data(polkadot_primitives::v2::PARACHAINS_INHERENT_IDENTIFIER, &para_data)
.expect("put data failed");
This conversation was marked as resolved.
Show resolved Hide resolved

Ok(inherent_data)
}
Expand Down
1 change: 1 addition & 0 deletions node/core/parachains-inherent/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ gum = { package = "tracing-gum", path = "../../gum" }
thiserror = "1.0.31"
async-trait = "0.1.57"
polkadot-node-subsystem = { path = "../../subsystem" }
polkadot-overseer = { path = "../../overseer" }
polkadot-primitives = { path = "../../../primitives" }
sp-blockchain = { git = "https://github.com/paritytech/substrate", branch = "master" }
sp-inherents = { git = "https://github.com/paritytech/substrate", branch = "master" }
Expand Down
41 changes: 26 additions & 15 deletions node/core/parachains-inherent/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,32 +29,33 @@ use polkadot_node_subsystem::{
errors::SubsystemError, messages::ProvisionerMessage, overseer::Handle,
};
use polkadot_primitives::v2::{Block, Hash, InherentData as ParachainsInherentData};
use sp_blockchain::HeaderBackend;
use sp_runtime::generic::BlockId;
use std::time;
use std::{sync::Arc, time};

pub(crate) const LOG_TARGET: &str = "parachain::parachains-inherent";

/// How long to wait for the provisioner, before giving up.
const PROVISIONER_TIMEOUT: time::Duration = core::time::Duration::from_millis(2500);

/// Provides the parachains inherent data.
pub struct ParachainsInherentDataProvider {
inherent_data: ParachainsInherentData,
pub struct ParachainsInherentDataProvider<C: sp_blockchain::HeaderBackend<Block>> {
pub client: Arc<C>,
pub overseer: polkadot_overseer::Handle,
pub parent: Hash,
}

impl ParachainsInherentDataProvider {
/// Create a [`Self`] directly from some [`ParachainsInherentData`].
pub fn from_data(inherent_data: ParachainsInherentData) -> Self {
Self { inherent_data }
impl<C: sp_blockchain::HeaderBackend<Block>> ParachainsInherentDataProvider<C> {
/// Create a new [`Self`].
pub fn new(client: Arc<C>, overseer: polkadot_overseer::Handle, parent: Hash) -> Self {
ParachainsInherentDataProvider { client, overseer, parent }
}

/// Create a new instance of the [`ParachainsInherentDataProvider`].
pub async fn create<C: HeaderBackend<Block>>(
client: &C,
pub async fn create(
client: Arc<C>,
Copy link
Member

Choose a reason for hiding this comment

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

Most of the logic in this function should be moved into the provide_inherent_data function.

Copy link
Author

Choose a reason for hiding this comment

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

Why so? I like how this is actually a separate function handing all the data gathering.

Copy link
Member

Choose a reason for hiding this comment

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

You can still put it into a separate function. I just don't like the duplication of the InherentDataProvider. This doesn't make any sense to me.

mut overseer: Handle,
parent: Hash,
Comment on lines +54 to 57
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub async fn create(
client: Arc<C>,
mut overseer: Handle,
parent: Hash,
async fn create(&self

Copy link
Author

Choose a reason for hiding this comment

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

Revert as this would need selfto be mut which in the end would need self in InherentDataProvider to be mut which feels wrong to change for this one case.

) -> Result<Self, Error> {
) -> Result<ParachainsInherentData, Error> {
let pid = async {
let (sender, receiver) = futures::channel::oneshot::channel();
gum::trace!(
Expand Down Expand Up @@ -119,18 +120,28 @@ impl ParachainsInherentDataProvider {
},
};

Ok(Self { inherent_data })
Ok(inherent_data)
}
}

#[async_trait::async_trait]
impl sp_inherents::InherentDataProvider for ParachainsInherentDataProvider {
fn provide_inherent_data(
impl<C: sp_blockchain::HeaderBackend<Block>> sp_inherents::InherentDataProvider
for ParachainsInherentDataProvider<C>
{
async fn provide_inherent_data(
&self,
dst_inherent_data: &mut sp_inherents::InherentData,
) -> Result<(), sp_inherents::Error> {
let inherent_data = ParachainsInherentDataProvider::create(
self.client.clone(),
self.overseer.clone(),
self.parent,
)
.await
.map_err(|e| sp_inherents::Error::Application(Box::new(e)))?;
Comment on lines +135 to +141
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let inherent_data = ParachainsInherentDataProvider::create(
self.client.clone(),
self.overseer.clone(),
self.parent,
)
.await
.map_err(|e| sp_inherents::Error::Application(Box::new(e)))?;
let inherent_data = self.create()
.await
.map_err(|e| sp_inherents::Error::Application(Box::new(e)))?;

Copy link
Author

Choose a reason for hiding this comment

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

Revert as this would need selfto be mut which in the end would need self in InherentDataProvider to be mut which feels wrong to change for this one case.


dst_inherent_data
.put_data(polkadot_primitives::v2::PARACHAINS_INHERENT_IDENTIFIER, &self.inherent_data)
.put_data(polkadot_primitives::v2::PARACHAINS_INHERENT_IDENTIFIER, &inherent_data)
}

async fn try_handle_error(
Expand Down
11 changes: 6 additions & 5 deletions node/service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1154,11 +1154,12 @@ where
let overseer_handle = overseer_handle.clone();

async move {
let parachain = polkadot_node_core_parachains_inherent::ParachainsInherentDataProvider::create(
&*client_clone,
overseer_handle,
parent,
).await.map_err(|e| Box::new(e))?;
let parachain =
polkadot_node_core_parachains_inherent::ParachainsInherentDataProvider::new(
client_clone,
overseer_handle,
parent,
);

let timestamp = sp_timestamp::InherentDataProvider::from_system_time();

Expand Down