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

Split execute_and_import_block in client into two functions #1455

Merged
merged 16 commits into from
Jan 23, 2019
Merged
89 changes: 55 additions & 34 deletions core/client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ use state_machine::{
DBValue, Backend as StateBackend, CodeExecutor, ChangesTrieAnchorBlockId,
ExecutionStrategy, ExecutionManager, prove_read,
ChangesTrieRootsStorage, ChangesTrieStorage,
key_changes, key_changes_proof, OverlayedChanges
key_changes, key_changes_proof, OverlayedChanges,
};

use crate::backend::{self, BlockImportOperation};
Expand All @@ -66,6 +66,9 @@ pub type ImportNotifications<Block> = mpsc::UnboundedReceiver<BlockImportNotific
/// A stream of block finality notifications.
pub type FinalityNotifications<Block> = mpsc::UnboundedReceiver<FinalityNotification<Block>>;

type StorageUpdate<B, Block> = <<<B as backend::Backend<Block, Blake2Hasher>>::BlockImportOperation as backend::BlockImportOperation<Block, Blake2Hasher>>::State as state_machine::Backend<Blake2Hasher>>::Transaction;
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
type StorageUpdate<B, Block> = <<<B as backend::Backend<Block, Blake2Hasher>>::BlockImportOperation as backend::BlockImportOperation<Block, Blake2Hasher>>::State as state_machine::Backend<Blake2Hasher>>::Transaction;
type StorageUpdate<B, Block> = <<<B as backend::Backend<Block, Blake2Hasher>>::BlockImportOperation as BlockImportOperation<Block, Blake2Hasher>>::State as state_machine::Backend<Blake2Hasher>>::Transaction;

type ChangesUpdate = trie::MemoryDB<Blake2Hasher>;

/// Substrate Client
pub struct Client<B, E, Block, RA> where Block: BlockT {
backend: Arc<B>,
Expand Down Expand Up @@ -588,39 +591,9 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
}

let mut transaction = self.backend.begin_operation(BlockId::Hash(parent_hash))?;
let (storage_update, changes_update, storage_changes) = match transaction.state()? {
Some(transaction_state) => {
let mut overlay = Default::default();
let r = self.executor.call_at_state(
transaction_state,
&mut overlay,
"Core_execute_block",
&<Block as BlockT>::new(import_headers.pre().clone(), body.clone().unwrap_or_default()).encode(),
match (origin, self.block_execution_strategy) {
(BlockOrigin::NetworkInitialSync, _) | (_, ExecutionStrategy::NativeWhenPossible) =>
ExecutionManager::NativeWhenPossible,
(_, ExecutionStrategy::AlwaysWasm) => ExecutionManager::AlwaysWasm,
_ => ExecutionManager::Both(|wasm_result, native_result| {
let header = import_headers.post();
warn!("Consensus error between wasm and native block execution at block {}", hash);
warn!(" Header {:?}", header);
warn!(" Native result {:?}", native_result);
warn!(" Wasm result {:?}", wasm_result);
telemetry!("block.execute.consensus_failure";
"hash" => ?hash,
"origin" => ?origin,
"header" => ?header
);
wasm_result
}),
},
);
let (_, storage_update, changes_update) = r?;
overlay.commit_prospective();
(Some(storage_update), Some(changes_update), Some(overlay.into_committed().collect()))
},
None => (None, None, None)
};

// TODO: correct path logic
Copy link
Member

Choose a reason for hiding this comment

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

Every TODO should be a github issue and the link should be added to the comment.

let (storage_update,changes_update,storage_changes) = self.block_execution(&import_headers, origin, hash, body.clone(), &transaction)?;

// TODO: non longest-chain rule.
let is_new_best = finalized || match fork_choice {
Expand Down Expand Up @@ -691,6 +664,54 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
Ok(ImportResult::Queued)
}

fn block_execution(
&self,
import_headers: &PrePostHeader<Block::Header>,
origin: BlockOrigin,
hash: Block::Hash,
body: Option<Vec<Block::Extrinsic>>,
transaction: &B::BlockImportOperation,
) -> error::Result<(Option<StorageUpdate<B, Block>>, Option<Option<ChangesUpdate>>, Option<Vec<(Vec<u8>, Option<Vec<u8>>)>>)> where
Copy link
Contributor

Choose a reason for hiding this comment

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

style: could split the tuple args each onto their own line with another level of indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

E: CallExecutor<Block, Blake2Hasher> + Send + Sync + Clone,
{
match transaction.state()? {
Some(transaction_state) => {
let mut overlay = Default::default();
let mut r = self.executor.call_at_state(
transaction_state,
&mut overlay,
"Core_execute_block",
&<Block as BlockT>::new(import_headers.pre().clone(), body.clone().unwrap_or_default()).encode(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think body needs to be cloned here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

Copy link
Member

Choose a reason for hiding this comment

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

I think we should open an issue for that. I would like to see some function Block::encode_from_parts(header: &Header, ext: &Vec<Extrensics>) or something. This could be used here and would not require to clone the stuff.

match (origin, self.block_execution_strategy) {
(BlockOrigin::NetworkInitialSync, _) | (_, ExecutionStrategy::NativeWhenPossible) =>
ExecutionManager::NativeWhenPossible,
(_, ExecutionStrategy::AlwaysWasm) => ExecutionManager::AlwaysWasm,
_ => ExecutionManager::Both(|wasm_result, native_result| {
let header = import_headers.post();
warn!("Consensus error between wasm and native block execution at block {}", hash);
warn!(" Header {:?}", header);
warn!(" Native result {:?}", native_result);
warn!(" Wasm result {:?}", wasm_result);
telemetry!("block.execute.consensus_failure";
"hash" => ?hash,
"origin" => ?origin,
"header" => ?header
);
wasm_result
}),
},
);
let (_, storage_update, changes_update) = r?;
overlay.commit_prospective();

Ok((Some(storage_update), Some(changes_update), Some(overlay.into_committed().collect())))
},

None => Ok((None, None, None))
}

}

/// Finalizes all blocks up to given. If a justification is provided it is
/// stored with the given finalized block (any other finalized blocks are
/// left unjustified).
Expand Down