Merge pull request #1232 from ethcore/simplified_block_opening

simplified block opening

ethcore/src/client/client.rs

@@ -756,18 +756,16 @@ impl<V> BlockChainClient for Client<V> where V: Verifier {
 }
 
 impl<V> MiningBlockChainClient for Client<V> where V: Verifier {
-	fn prepare_sealing(&self, author: Address, gas_floor_target: U256, extra_data: Bytes, transactions: Vec<SignedTransaction>)
-		-> (Option<ClosedBlock>, HashSet<H256>) {
+	fn prepare_open_block(&self, author: Address, gas_floor_target: U256, extra_data: Bytes) -> OpenBlock {
 		let engine = self.engine.deref().deref();
 		let h = self.chain.best_block_hash();
-		let mut invalid_transactions = HashSet::new();
 
-		let mut b = OpenBlock::new(
+		let mut open_block = OpenBlock::new(
 			engine,
 			&self.vm_factory,
 			false,	// TODO: this will need to be parameterised once we want to do immediate mining insertion.
 			self.state_db.lock().unwrap().boxed_clone(),
-			match self.chain.block_header(&h) { Some(ref x) => x, None => { return (None, invalid_transactions) } },
+			&self.chain.block_header(&h).expect("h is best block hash: so it's header must exist: qed"),
 			self.build_last_hashes(h.clone()),
 			author,
 			gas_floor_target,
@@ -782,44 +780,10 @@ impl<V> MiningBlockChainClient for Client<V> where V: Verifier {
 			.into_iter()
 			.take(engine.maximum_uncle_count())
 			.foreach(|h| {
-				b.push_uncle(h).unwrap();
+				open_block.push_uncle(h).unwrap();
 			});
 
-		// Add transactions
-		let block_number = b.block().header().number();
-		let min_tx_gas = U256::from(self.engine.schedule(&b.env_info()).tx_gas);
-
-		for tx in transactions {
-			// Push transaction to block
-			let hash = tx.hash();
-			let import = b.push_transaction(tx, None);
-
-			match import {
-				Err(Error::Execution(ExecutionError::BlockGasLimitReached { gas_limit, gas_used, .. })) => {
-					trace!(target: "miner", "Skipping adding transaction to block because of gas limit: {:?}", hash);
-					// Exit early if gas left is smaller then min_tx_gas
-					if gas_limit - gas_used < min_tx_gas {
-						break;
-					}
-				},
-				Err(e) => {
-					invalid_transactions.insert(hash);
-					trace!(target: "miner",
-						   "Error adding transaction to block: number={}. transaction_hash={:?}, Error: {:?}",
-						   block_number, hash, e);
-				},
-				_ => {}
-			}
-		}
-
-		// And close
-		let b = b.close();
-		trace!(target: "miner", "Sealing: number={}, hash={}, diff={}",
-			   b.block().header().number(),
-			   b.hash(),
-			   b.block().header().difficulty()
-		);
-		(Some(b), invalid_transactions)
+		open_block
 	}
 
 	fn try_seal(&self, block: LockedBlock, seal: Vec<Bytes>) -> Result<SealedBlock, LockedBlock> {

ethcore/src/client/mod.rs

@@ -31,13 +31,12 @@ pub use self::trace::Filter as TraceFilter;
 pub use executive::{Executed, Executive, TransactOptions};
 pub use env_info::{LastHashes, EnvInfo};
 
-use std::collections::HashSet;
 use util::bytes::Bytes;
 use util::hash::{Address, H256, H2048};
 use util::numbers::U256;
 use blockchain::TreeRoute;
 use block_queue::BlockQueueInfo;
-use block::{ClosedBlock, LockedBlock, SealedBlock};
+use block::{LockedBlock, SealedBlock, OpenBlock};
 use header::{BlockNumber, Header};
 use transaction::{LocalizedTransaction, SignedTransaction};
 use log_entry::LocalizedLogEntry;
@@ -201,7 +200,7 @@ pub trait MiningBlockChainClient : BlockChainClient {
 	/// Attempts to seal given block. Returns `SealedBlock` on success and the same block in case of error.
 	fn try_seal(&self, block: LockedBlock, seal: Vec<Bytes>) -> Result<SealedBlock, LockedBlock>;
 
-	/// Returns ClosedBlock prepared for sealing.
-	fn prepare_sealing(&self, author: Address, gas_floor_target: U256, extra_data: Bytes, transactions: Vec<SignedTransaction>)
-		-> (Option<ClosedBlock>, HashSet<H256>);
+	/// Returns OpenBlock prepared for closing.
+	fn prepare_open_block(&self, author: Address, gas_floor_target: U256, extra_data: Bytes)
+		-> OpenBlock;
 }

ethcore/src/client/test_client.rs

@@ -31,7 +31,7 @@ use evm::Factory as EvmFactory;
 use miner::{Miner, MinerService};
 
 use block_queue::BlockQueueInfo;
-use block::{SealedBlock, ClosedBlock, LockedBlock};
+use block::{SealedBlock, LockedBlock, OpenBlock};
 use executive::Executed;
 use error::{ExecutionError};
 use trace::LocalizedTrace;
@@ -245,8 +245,8 @@ impl MiningBlockChainClient for TestBlockChainClient {
 	}
 
 
-	fn prepare_sealing(&self, _author: Address, _gas_floor_target: U256, _extra_data: Bytes, _transactions: Vec<SignedTransaction>) -> (Option<ClosedBlock>, HashSet<H256>) {
-		(None, HashSet::new())
+	fn prepare_open_block(&self, _author: Address, _gas_floor_target: U256, _extra_data: Bytes) -> OpenBlock {
+		unimplemented!();
 	}
 }
 

ethcore/src/miner/miner.rs

@@ -117,90 +117,89 @@ impl Miner {
 		//   otherwise, leave everything alone.
 		// otherwise, author a fresh block.
 */
-
-		let (b, invalid_transactions) = match sealing_work.pop_if(|b| b.block().fields().header.parent_hash() == &best_hash) {
+		let mut open_block = match sealing_work.pop_if(|b| b.block().fields().header.parent_hash() == &best_hash) {
 			Some(old_block) => {
 				trace!(target: "miner", "Already have previous work; updating and returning");
 				// add transactions to old_block
 				let e = self.engine();
-				let mut invalid_transactions = HashSet::new();
-				let mut block = old_block.reopen(e, chain.vm_factory());
-				let block_number = block.block().fields().header.number();
-
-				// TODO: push new uncles, too.
-				// TODO: refactor with chain.prepare_sealing
-				for tx in transactions {
-					let hash = tx.hash();
-					let res = block.push_transaction(tx, None);
-					match res {
-						Err(Error::Execution(ExecutionError::BlockGasLimitReached { gas_limit, gas_used, .. })) => {
-							trace!(target: "miner", "Skipping adding transaction to block because of gas limit: {:?}", hash);
-							// Exit early if gas left is smaller then min_tx_gas
-							let min_tx_gas: U256 = 21000.into();	// TODO: figure this out properly.
-							if gas_limit - gas_used < min_tx_gas {
-								break;
-							}
-						},
-						Err(Error::Transaction(TransactionError::AlreadyImported)) => {}	// already have transaction - ignore
-						Err(e) => {
-							invalid_transactions.insert(hash);
-							trace!(target: "miner",
-								   "Error adding transaction to block: number={}. transaction_hash={:?}, Error: {:?}",
-								   block_number, hash, e);
-						},
-						_ => {}	// imported ok
-					}
-				}
-				(Some(block.close()), invalid_transactions)
+				old_block.reopen(e, chain.vm_factory())
 			}
 			None => {
 				// block not found - create it.
 				trace!(target: "miner", "No existing work - making new block");
-				chain.prepare_sealing(
+				chain.prepare_open_block(
 					self.author(),
 					self.gas_floor_target(),
-					self.extra_data(),
-					transactions,
+					self.extra_data()
 				)
 			}
 		};
+
+		let mut invalid_transactions = HashSet::new();
+		let block_number = open_block.block().fields().header.number();
+		// TODO: push new uncles, too.
+		for tx in transactions {
+			let hash = tx.hash();
+			match open_block.push_transaction(tx, None) {
+				Err(Error::Execution(ExecutionError::BlockGasLimitReached { gas_limit, gas_used, .. })) => {
+					trace!(target: "miner", "Skipping adding transaction to block because of gas limit: {:?}", hash);
+					// Exit early if gas left is smaller then min_tx_gas
+					let min_tx_gas: U256 = 21000.into();	// TODO: figure this out properly.
+					if gas_limit - gas_used < min_tx_gas {
+						break;
+					}
+				},
+				Err(Error::Transaction(TransactionError::AlreadyImported)) => {}	// already have transaction - ignore
+				Err(e) => {
+					invalid_transactions.insert(hash);
+					trace!(target: "miner",
+						   "Error adding transaction to block: number={}. transaction_hash={:?}, Error: {:?}",
+						   block_number, hash, e);
+				},
+				_ => {}	// imported ok
+			}
+		}
+
+		let block = open_block.close();
+
 		let mut queue = self.transaction_queue.lock().unwrap();
 		let fetch_account = |a: &Address| AccountDetails {
 			nonce: chain.latest_nonce(a),
 			balance: chain.latest_balance(a),
 		};
+
 		for hash in invalid_transactions.into_iter() {
 			queue.remove_invalid(&hash, &fetch_account);
 		}
-		if let Some(block) = b {
-			if !block.transactions().is_empty() {
-				trace!(target: "miner", "prepare_sealing: block has transaction - attempting internal seal.");
-				// block with transactions - see if we can seal immediately.
-				let s = self.engine().generate_seal(block.block(), match self.accounts {
-					Some(ref x) => Some(&**x),
-					None => None,
-				});
-				if let Some(seal) = s {
-					trace!(target: "miner", "prepare_sealing: managed internal seal. importing...");
-					if let Ok(sealed) = chain.try_seal(block.lock(), seal) {
-						if let Ok(_) = chain.import_block(sealed.rlp_bytes()) {
-							trace!(target: "miner", "prepare_sealing: sealed internally and imported. leaving.");
-						} else {
-							warn!("prepare_sealing: ERROR: could not import internally sealed block. WTF?");
-						}
+
+		if !block.transactions().is_empty() {
+			trace!(target: "miner", "prepare_sealing: block has transaction - attempting internal seal.");
+			// block with transactions - see if we can seal immediately.
+			let s = self.engine().generate_seal(block.block(), match self.accounts {
+				Some(ref x) => Some(&**x),
+				None => None,
+			});
+			if let Some(seal) = s {
+				trace!(target: "miner", "prepare_sealing: managed internal seal. importing...");
+				if let Ok(sealed) = chain.try_seal(block.lock(), seal) {
+					if let Ok(_) = chain.import_block(sealed.rlp_bytes()) {
+						trace!(target: "miner", "prepare_sealing: sealed internally and imported. leaving.");
 					} else {
-						warn!("prepare_sealing: ERROR: try_seal failed when given internally generated seal. WTF?");
+						warn!("prepare_sealing: ERROR: could not import internally sealed block. WTF?");
 					}
-					return;
 				} else {
-					trace!(target: "miner", "prepare_sealing: unable to generate seal internally");
+					warn!("prepare_sealing: ERROR: try_seal failed when given internally generated seal. WTF?");
 				}
-			}
-			if sealing_work.peek_last_ref().map_or(true, |pb| pb.block().fields().header.hash() != block.block().fields().header.hash()) {
-				trace!(target: "miner", "Pushing a new, refreshed or borrowed pending {}...", block.block().fields().header.hash());
-				sealing_work.push(block);
+				return;
+			} else {
+				trace!(target: "miner", "prepare_sealing: unable to generate seal internally");
 			}
 		}
+		if sealing_work.peek_last_ref().map_or(true, |pb| pb.block().fields().header.hash() != block.block().fields().header.hash()) {
+			trace!(target: "miner", "Pushing a new, refreshed or borrowed pending {}...", block.block().fields().header.hash());
+			sealing_work.push(block);
+		}
+
 		trace!(target: "miner", "prepare_sealing: leaving (last={:?})", sealing_work.peek_last_ref().map(|b| b.block().fields().header.hash()));
 	}
 

ethcore/src/tests/client.rs

@@ -137,7 +137,7 @@ fn can_mine() {
 	let client_result = get_test_client_with_blocks(vec![dummy_blocks[0].clone()]);
 	let client = client_result.reference();
 
-	let b = client.prepare_sealing(Address::default(), 31415926.into(), vec![], vec![]).0.unwrap();
+	let b = client.prepare_open_block(Address::default(), 31415926.into(), vec![]).close();
 
 	assert_eq!(*b.block().header().parent_hash(), BlockView::new(&dummy_blocks[0]).header_view().sha3());
 	assert!(client.try_seal(b.lock(), vec![]).is_ok());