Split internal sealing from work preparation #2071
Conversation
| @@ -411,12 +419,13 @@ impl Miner { | |||
| // | NOTE Code below requires transaction_queue and sealing_work locks. | | |||
| // | Make sure to release the locks before calling that method. | | |||
| // -------------------------------------------------------------------------- | |||
| self.prepare_sealing(chain); | |||
| let (block, original_work_hash) = self.prepare_block(chain); | |||
| self.prepare_work(block, original_work_hash); | |||
There was a problem hiding this comment.
#![feature(fn_traits, unboxed_closures)]
use std::ops::Fn;
fn main() {
Fn::call(&foo, (1, 2.0));
}
fn foo(x: i32, y: f32) {
println!("({}, {})", x, y);
}(although having to account for the implicit self makes that impossible in this case)
There was a problem hiding this comment.
Constant interaction with Miner struct discourages pure functions though.
| @@ -71,6 +71,8 @@ pub trait Engine : Sync + Send { | |||
| /// Block transformation functions, after the transactions. | |||
| fn on_close_block(&self, _block: &mut ExecutedBlock) {} | |||
|
|
|||
| /// If true, generate_seal has to be implemented. | |||
| fn seals_internally(&self) -> bool { false } | |||
There was a problem hiding this comment.
nit: there should be a blank line following this
There was a problem hiding this comment.
There is plenty of methods next to each other in trait definitions in this and other files in the codebase. Id argue that for definitions its fine, as it often groups method families.
There was a problem hiding this comment.
I haven't noticed it personally, in general it is typical Rust style to provide a line of blank space between each function definition, at least if there are docs. (see an example from std: Iterator )
There was a problem hiding this comment.
I see, but Engine had this sort of logical grouping already. @arkpar suggested to leave it.
keorn
added
A3-inprogress ⏳
keorn
added
A0-pleasereview 🤓
tomusdrw
changed the title
|
|
||
| /// Uses Engine to seal the block internally and then imports it to chain. | ||
| fn seal_and_import_block_internally(&self, chain: &MiningBlockChainClient, block: ClosedBlock) -> bool { | ||
| if !block.transactions().is_empty() { |
There was a problem hiding this comment.
Tests triggering that if-path (or not triggering) would be nice
rphmeier
added
A8-looksgood 🦄
keorn
added
A3-inprogress ⏳
keorn
added
A0-pleasereview 🤓
NikVolf
added
A8-looksgood 🦄
| @@ -243,19 +245,15 @@ impl Miner { | |||
| } | |||
|
|
|||
| /// Prepares new block for sealing including top transactions from queue. | |||
| #[cfg_attr(feature="dev", allow(match_same_arms))] | |||
| #[cfg_attr(feature="dev", allow(cyclomatic_complexity))] | |||
There was a problem hiding this comment.
Any particular reason to remove these?
Previously the "miner" attempted to generate and import seal in
prepare_sealingmethod. Now methods are separated out more and flow is based inseals_internallyEngine method.