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

Use correct CreateInherentDataProviders impl for manual seal #8852

Merged
merged 3 commits into from
May 31, 2021
Merged

Use correct CreateInherentDataProviders impl for manual seal #8852

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f00a63c
use correct CreateInherentDataProviders impl for manual seal
May 18, 2021
0411312
add babe inherent provider
May 19, 2021
9d2930e
move client into factory fn
May 27, 2021
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
26 changes: 9 additions & 17 deletions bin/node/test-runner-example/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ use sc_consensus_babe::BabeBlockImport;
use sp_keystore::SyncCryptoStorePtr;
use sp_keyring::sr25519::Keyring::Alice;
use sp_consensus_babe::AuthorityId;
use sc_consensus_manual_seal::{ConsensusDataProvider, consensus::babe::BabeConsensusDataProvider};
use sc_consensus_manual_seal::{
ConsensusDataProvider, consensus::babe::{BabeConsensusDataProvider, SlotTimestampProvider},
};
use sp_runtime::{traits::IdentifyAccount, MultiSigner, generic::Era};
use node_cli::chain_spec::development_config;

Expand Down Expand Up @@ -59,10 +61,7 @@ impl ChainInfo for NodeTemplateChainInfo {
Self::SelectChain,
>;
type SignedExtras = node_runtime::SignedExtra;
type InherentDataProviders = (
sp_timestamp::InherentDataProvider,
sp_consensus_babe::inherents::InherentDataProvider,
);
type InherentDataProviders = (SlotTimestampProvider, sp_consensus_babe::inherents::InherentDataProvider);

fn signed_extras(from: <Self::Runtime as frame_system::Config>::AccountId) -> Self::SignedExtras {
(
Expand Down Expand Up @@ -90,7 +89,7 @@ impl ChainInfo for NodeTemplateChainInfo {
TaskManager,
Box<dyn CreateInherentDataProviders<
Self::Block,
(),
Arc<TFullClient<Self::Block, Self::RuntimeApi, Self::Executor>>,
Copy link
Member

Choose a reason for hiding this comment

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

This change doesn't make any sense at all.

InherentDataProviders = Self::InherentDataProviders
>>,
Option<
Expand Down Expand Up @@ -143,17 +142,10 @@ impl ChainInfo for NodeTemplateChainInfo {
backend,
keystore.sync_keystore(),
task_manager,
Box::new(move |_, _| {
let slot_duration = slot_duration.clone();
async move {
let timestamp = sp_timestamp::InherentDataProvider::from_system_time();
let slot = sp_consensus_babe::inherents::InherentDataProvider::from_timestamp_and_duration(
*timestamp,
slot_duration.slot_duration(),
);

Ok((timestamp, slot))
}
Box::new(|_, client| async move {
let provider = SlotTimestampProvider::new(client).map_err(|err| format!("{:?}", err))?;
Copy link
Member

Choose a reason for hiding this comment

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

You have the client above and can just pass it here. You don't need to pass it to the closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this closure is a factory function, it needs to be called more than once. This is also evident in the implementation for CreateInherentDataProviders trait

substrate/primitives/inherents/src/client_side.rs

Lines 43 to 60 in 90cfb95

impl<F, Block, IDP, ExtraArgs, Fut> CreateInherentDataProviders<Block, ExtraArgs> for F
where
Block: BlockT,
F: Fn(Block::Hash, ExtraArgs) -> Fut + Sync + Send,
Fut: std::future::Future<Output = Result<IDP, Box<dyn std::error::Error + Send + Sync>>> + Send + 'static,
IDP: InherentDataProvider + 'static,
ExtraArgs: Send + 'static,
{
type InherentDataProviders = IDP;
async fn create_inherent_data_providers(
&self,
parent: Block::Hash,
extra_args: ExtraArgs,
) -> Result<Self::InherentDataProviders, Box<dyn std::error::Error + Send + Sync>> {
(*self)(parent, extra_args).await
}
}

It's implemented for Fn, if a closure captures outer variables, it becomes an FnOnce so it can only be called once, because the variables are moved into it once. Luckily whoever implemented the CreateInherentDataProviders trait allowed you to pass variables to the factory function, hence the change above.

Copy link
Member

Choose a reason for hiding this comment

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

Because this closure is a factory function, it needs to be called more than once. This is also evident in the implementation for CreateInherentDataProviders trait

I know.... But this doesn't mean that you can not do what I said...

https://github.com/paritytech/polkadot/blob/master/node/service/src/lib.rs#L929-L954

It's implemented for Fn, if a closure captures outer variables, it becomes an FnOnce so it can only be called once, because the variables are moved into it once. Luckily whoever implemented the CreateInherentDataProviders trait allowed you to pass variables to the factory function, hence the change above.

This was not done to pass the client there.

let babe = sp_consensus_babe::inherents::InherentDataProvider::new(provider.slot().into());
Ok((provider, babe))
}),
Some(Box::new(consensus_data_provider)),
select_chain,
Expand Down
9 changes: 7 additions & 2 deletions client/consensus/manual-seal/src/consensus/babe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ impl SlotTimestampProvider {

// looks like this isn't the first block, rehydrate the fake time.
// otherwise we'd be producing blocks for older slots.
let duration = if info.best_number != Zero::zero() {
let time = if info.best_number != Zero::zero() {
let header = client.header(BlockId::Hash(info.best_hash))?.unwrap();
let slot = find_pre_digest::<B>(&header).unwrap().slot();
// add the slot duration so there's no collision of slots
Expand All @@ -274,10 +274,15 @@ impl SlotTimestampProvider {
};

Ok(Self {
time: atomic::AtomicU64::new(duration),
time: atomic::AtomicU64::new(time),
slot_duration,
})
}

/// Get the current slot number
pub fn slot(&self) -> u64 {
self.time.load(atomic::Ordering::SeqCst) / self.slot_duration
}
}

#[async_trait::async_trait]
Expand Down
4 changes: 2 additions & 2 deletions client/consensus/manual-seal/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ pub async fn run_manual_seal<B, BI, CB, E, C, A, SC, CS, CIDP>(
CS: Stream<Item=EngineCommand<<B as BlockT>::Hash>> + Unpin + 'static,
SC: SelectChain<B> + 'static,
TransactionFor<C, B>: 'static,
CIDP: CreateInherentDataProviders<B, ()>,
CIDP: CreateInherentDataProviders<B, Arc<C>>,
{
while let Some(command) = commands_stream.next().await {
match command {
Expand Down Expand Up @@ -237,7 +237,7 @@ pub async fn run_instant_seal<B, BI, CB, E, C, A, SC, CIDP>(
E::Proposer: Proposer<B, Transaction = TransactionFor<C, B>>,
SC: SelectChain<B> + 'static,
TransactionFor<C, B>: 'static,
CIDP: CreateInherentDataProviders<B, ()>,
CIDP: CreateInherentDataProviders<B, Arc<C>>,
{
// instant-seal creates blocks as soon as transactions are imported
// into the transaction pool.
Expand Down
7 changes: 2 additions & 5 deletions client/consensus/manual-seal/src/seal_block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ pub async fn seal_block<B, BI, SC, C, E, P, CIDP>(
P: txpool::ChainApi<Block=B>,
SC: SelectChain<B>,
TransactionFor<C, B>: 'static,
CIDP: CreateInherentDataProviders<B, ()>,
CIDP: CreateInherentDataProviders<B, Arc<C>>,
{
let future = async {
if pool.validated_pool().status().ready == 0 && !create_empty {
Expand All @@ -111,10 +111,7 @@ pub async fn seal_block<B, BI, SC, C, E, P, CIDP>(

let inherent_data_providers =
create_inherent_data_providers
.create_inherent_data_providers(
parent.hash(),
(),
)
.create_inherent_data_providers(parent.hash(), client.clone())
.await
.map_err(|e| Error::Other(e))?;

Expand Down
2 changes: 1 addition & 1 deletion test-utils/test-runner/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ pub trait ChainInfo: Sized {
Box<
dyn CreateInherentDataProviders<
Self::Block,
(),
Arc<TFullClient<Self::Block, Self::RuntimeApi, Self::Executor>>,
InherentDataProviders = Self::InherentDataProviders
>
>,
Expand Down