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

Conversation

joepetrowski
Copy link
Contributor

This is step one in addressing Issue #1232

I have taken block execution from execute_and_import_block and put it into a new function block_execution, which returns a tuple of (storage_update,changes_update,storage_changes). The function always executes, so behavior should be exactly as before.

@joepetrowski joepetrowski added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 16, 2019
@parity-cla-bot
Copy link

It looks like @joepetrowski signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

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

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.

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.

@@ -70,6 +70,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;

transaction_state,
&mut overlay,
"Core_execute_block",
&<Block as BlockT>::new(import_headers.pre().clone(), body.clone().unwrap_or_default()).encode(),
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.

@bkchr bkchr merged commit 0630a6a into master Jan 23, 2019
@bkchr bkchr deleted the joe1232b branch January 23, 2019 14:27
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
…ch#1455)

* type error

* borrow problem

* compiles but run error

* tests pass ready for review

* tests pass ready for review

* fix comments by rob

* removed clone

* style fix

* updating

* resolved conflicts

* fixed comments
liuchengxu pushed a commit to liuchengxu/substrate that referenced this pull request May 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants