Unify engine error to reject blocks

ethcore/src/block.rs

@@ -409,20 +409,16 @@ impl<'x> OpenBlock<'x> {
 	}
 
 	/// Turn this into a `ClosedBlock`.
-	pub fn close(self) -> ClosedBlock {
+	pub fn close(self) -> Result<ClosedBlock, Error> {
 		let mut s = self;
 
 		let unclosed_state = s.block.state.clone();
 		let unclosed_metadata = s.block.metadata.clone();
 		let unclosed_finalization_state = s.block.is_finalized;
 
-		if let Err(e) = s.engine.on_close_block(&mut s.block) {
-			warn!("Encountered error on closing the block: {}", e);
-		}
+		s.engine.on_close_block(&mut s.block)?;
+		s.block.state.commit()?;
 
-		if let Err(e) = s.block.state.commit() {
-			warn!("Encountered error on state commit: {}", e);
-		}
 		s.block.header.set_transactions_root(ordered_trie_root(s.block.transactions.iter().map(|e| e.rlp_bytes())));
 		let uncle_bytes = encode_list(&s.block.uncles).into_vec();
 		s.block.header.set_uncles_hash(keccak(&uncle_bytes));
@@ -434,26 +430,21 @@ impl<'x> OpenBlock<'x> {
 		}));
 		s.block.header.set_gas_used(s.block.receipts.last().map_or_else(U256::zero, |r| r.gas_used));
 
-		ClosedBlock {
+		Ok(ClosedBlock {
 			block: s.block,
 			uncle_bytes,
 			unclosed_state,
 			unclosed_metadata,
 			unclosed_finalization_state,
-		}
+		})
 	}
 
 	/// Turn this into a `LockedBlock`.
-	pub fn close_and_lock(self) -> LockedBlock {
+	pub fn close_and_lock(self) -> Result<LockedBlock, Error> {
 		let mut s = self;
 
-		if let Err(e) = s.engine.on_close_block(&mut s.block) {
-			warn!("Encountered error on closing the block: {}", e);
-		}
-
-		if let Err(e) = s.block.state.commit() {
-			warn!("Encountered error on state commit: {}", e);
-		}
+		s.engine.on_close_block(&mut s.block)?;
+		s.block.state.commit()?;
 
 		if s.block.header.transactions_root().is_zero() || s.block.header.transactions_root() == &KECCAK_NULL_RLP {
 			s.block.header.set_transactions_root(ordered_trie_root(s.block.transactions.iter().map(|e| e.rlp_bytes())));
@@ -473,10 +464,10 @@ impl<'x> OpenBlock<'x> {
 		}));
 		s.block.header.set_gas_used(s.block.receipts.last().map_or_else(U256::zero, |r| r.gas_used));
 
-		LockedBlock {
+		Ok(LockedBlock {
 			block: s.block,
 			uncle_bytes,
-		}
+		})
 	}
 
 	#[cfg(test)]
@@ -651,7 +642,7 @@ fn enact(
 		b.push_uncle(u)?;
 	}
 
-	Ok(b.close_and_lock())
+	b.close_and_lock()
 }
 
 /// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header
@@ -747,7 +738,7 @@ mod tests {
 			b.push_uncle(u.clone())?;
 		}
 
-		Ok(b.close_and_lock())
+		b.close_and_lock()
 	}
 
 	/// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header. Seal the block aferwards
@@ -772,7 +763,7 @@ mod tests {
 		let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 		let b = OpenBlock::new(&*spec.engine, Default::default(), false, db, &genesis_header, last_hashes, Address::zero(), (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b = b.close_and_lock();
+		let b = b.close_and_lock().unwrap();
 		let _ = b.seal(&*spec.engine, vec![]);
 	}
 
@@ -786,7 +777,7 @@ mod tests {
 		let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 		let b = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes.clone(), Address::zero(), (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap()
-			.close_and_lock().seal(engine, vec![]).unwrap();
+			.close_and_lock().unwrap().seal(engine, vec![]).unwrap();
 		let orig_bytes = b.rlp_bytes();
 		let orig_db = b.drain();
 
@@ -816,7 +807,7 @@ mod tests {
 		uncle2_header.set_extra_data(b"uncle2".to_vec());
 		open_block.push_uncle(uncle1_header).unwrap();
 		open_block.push_uncle(uncle2_header).unwrap();
-		let b = open_block.close_and_lock().seal(engine, vec![]).unwrap();
+		let b = open_block.close_and_lock().unwrap().seal(engine, vec![]).unwrap();
 
 		let orig_bytes = b.rlp_bytes();
 		let orig_db = b.drain();

ethcore/src/client/client.rs

@@ -2135,7 +2135,7 @@ impl ReopenBlock for Client {
 }
 
 impl PrepareOpenBlock for Client {
-	fn prepare_open_block(&self, author: Address, gas_range_target: (U256, U256), extra_data: Bytes) -> OpenBlock {
+	fn prepare_open_block(&self, author: Address, gas_range_target: (U256, U256), extra_data: Bytes) -> Result<OpenBlock, EthcoreError> {
 		let engine = &*self.engine;
 		let chain = self.chain.read();
 		let best_header = chain.best_block_header();
@@ -2154,7 +2154,7 @@ impl PrepareOpenBlock for Client {
 			extra_data,
 			is_epoch_begin,
 			&mut chain.ancestry_with_metadata_iter(best_header.hash()),
-		).expect("OpenBlock::new only fails if parent state root invalid; state root of best block's header is never invalid; qed");
+		)?;
 
 		// Add uncles
 		chain
@@ -2170,7 +2170,7 @@ impl PrepareOpenBlock for Client {
 												qed");
 			});
 
-		open_block
+		Ok(open_block)
 	}
 }
 

ethcore/src/client/test_client.rs

@@ -46,7 +46,7 @@ use header::{Header as BlockHeader, BlockNumber};
 use filter::Filter;
 use log_entry::LocalizedLogEntry;
 use receipt::{Receipt, LocalizedReceipt, TransactionOutcome};
-use error::ImportResult;
+use error::{Error, ImportResult};
 use vm::Schedule;
 use miner::{self, Miner, MinerService};
 use spec::Spec;
@@ -373,7 +373,7 @@ impl ReopenBlock for TestBlockChainClient {
 }
 
 impl PrepareOpenBlock for TestBlockChainClient {
-	fn prepare_open_block(&self, author: Address, gas_range_target: (U256, U256), extra_data: Bytes) -> OpenBlock {
+	fn prepare_open_block(&self, author: Address, gas_range_target: (U256, U256), extra_data: Bytes) -> Result<OpenBlock, Error> {
 		let engine = &*self.spec.engine;
 		let genesis_header = self.spec.genesis_header();
 		let db = self.spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
@@ -391,10 +391,10 @@ impl PrepareOpenBlock for TestBlockChainClient {
 			extra_data,
 			false,
 			&mut Vec::new().into_iter(),
-		).expect("Opening block for tests will not fail.");
+		)?;
 		// TODO [todr] Override timestamp for predictability
 		open_block.set_timestamp(*self.latest_block_timestamp.read());
-		open_block
+		Ok(open_block)
 	}
 }
 

ethcore/src/client/traits.rs

@@ -24,7 +24,7 @@ use blockchain::TreeRoute;
 use client::Mode;
 use encoded;
 use vm::LastHashes;
-use error::{ImportResult, CallError, BlockImportError};
+use error::{Error, ImportResult, CallError, BlockImportError};
 use evm::Schedule;
 use executive::Executed;
 use filter::Filter;
@@ -395,7 +395,7 @@ pub trait PrepareOpenBlock {
 		author: Address,
 		gas_range_target: (U256, U256),
 		extra_data: Bytes
-	) -> OpenBlock;
+	) -> Result<OpenBlock, Error>;
 }
 
 /// Provides methods used for sealing new state

ethcore/src/engines/authority_round/mod.rs

@@ -1403,7 +1403,7 @@ mod tests {
 	use rlp::encode;
 	use block::*;
 	use test_helpers::{
-		generate_dummy_client_with_spec_and_accounts, get_temp_state_db, generate_dummy_client,
+		generate_dummy_client_with_spec_and_accounts, get_temp_state_db,
 		TestNotify
 	};
 	use account_provider::AccountProvider;
@@ -1451,9 +1451,9 @@ mod tests {
 		let db2 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b1 = b1.close_and_lock();
+		let b1 = b1.close_and_lock().unwrap();
 		let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b2 = b2.close_and_lock();
+		let b2 = b2.close_and_lock().unwrap();
 
 		engine.set_signer(tap.clone(), addr1, "1".into());
 		if let Seal::Regular(seal) = engine.generate_seal(b1.block(), &genesis_header) {
@@ -1485,9 +1485,9 @@ mod tests {
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 
 		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b1 = b1.close_and_lock();
+		let b1 = b1.close_and_lock().unwrap();
 		let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b2 = b2.close_and_lock();
+		let b2 = b2.close_and_lock().unwrap();
 
 		engine.set_signer(tap.clone(), addr1, "1".into());
 		match engine.generate_seal(b1.block(), &genesis_header) {
@@ -1745,16 +1745,17 @@ mod tests {
 		let db1 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
 
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
-		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b1 = b1.close_and_lock();
 
-		let client = generate_dummy_client(0);
+		let client = generate_dummy_client_with_spec_and_accounts(Spec::new_test_round_empty_steps, None);
 		let notify = Arc::new(TestNotify::default());
 		client.add_notify(notify.clone());
 		engine.register_client(Arc::downgrade(&client) as _);
 
 		engine.set_signer(tap.clone(), addr1, "1".into());
 
+		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
+		let b1 = b1.close_and_lock().unwrap();
+
 		// the block is empty so we don't seal and instead broadcast an empty step message
 		assert_eq!(engine.generate_seal(b1.block(), &genesis_header), Seal::None);
 
@@ -1779,9 +1780,14 @@ mod tests {
 
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 
+		let client = generate_dummy_client_with_spec_and_accounts(Spec::new_test_round_empty_steps, None);
+		let notify = Arc::new(TestNotify::default());
+		client.add_notify(notify.clone());
+		engine.register_client(Arc::downgrade(&client) as _);
+
 		// step 2
 		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b1 = b1.close_and_lock();
+		let b1 = b1.close_and_lock().unwrap();
 
 		// since the block is empty it isn't sealed and we generate empty steps
 		engine.set_signer(tap.clone(), addr1, "1".into());
@@ -1798,7 +1804,7 @@ mod tests {
 			value: U256::from(1),
 			data: vec![],
 		}.fake_sign(addr2), None).unwrap();
-		let b2 = b2.close_and_lock();
+		let b2 = b2.close_and_lock().unwrap();
 
 		// we will now seal a block with 1tx and include the accumulated empty step message
 		engine.set_signer(tap.clone(), addr2, "0".into());
@@ -1827,9 +1833,14 @@ mod tests {
 
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 
+		let client = generate_dummy_client_with_spec_and_accounts(Spec::new_test_round_empty_steps, None);
+		let notify = Arc::new(TestNotify::default());
+		client.add_notify(notify.clone());
+		engine.register_client(Arc::downgrade(&client) as _);
+
 		// step 2
 		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b1 = b1.close_and_lock();
+		let b1 = b1.close_and_lock().unwrap();
 
 		// since the block is empty it isn't sealed and we generate empty steps
 		engine.set_signer(tap.clone(), addr1, "1".into());
@@ -1838,15 +1849,15 @@ mod tests {
 
 		// step 3
 		let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes.clone(), addr2, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b2 = b2.close_and_lock();
+		let b2 = b2.close_and_lock().unwrap();
 		engine.set_signer(tap.clone(), addr2, "0".into());
 		assert_eq!(engine.generate_seal(b2.block(), &genesis_header), Seal::None);
 		engine.step();
 
 		// step 4
 		// the spec sets the maximum_empty_steps to 2 so we will now seal an empty block and include the empty step messages
 		let b3 = OpenBlock::new(engine, Default::default(), false, db3, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b3 = b3.close_and_lock();
+		let b3 = b3.close_and_lock().unwrap();
 
 		engine.set_signer(tap.clone(), addr1, "1".into());
 		if let Seal::Regular(seal) = engine.generate_seal(b3.block(), &genesis_header) {
@@ -1879,7 +1890,7 @@ mod tests {
 
 		// step 2
 		let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b1 = b1.close_and_lock();
+		let b1 = b1.close_and_lock().unwrap();
 
 		// since the block is empty it isn't sealed and we generate empty steps
 		engine.set_signer(tap.clone(), addr1, "1".into());
@@ -1892,7 +1903,7 @@ mod tests {
 		let addr1_balance = b2.block().state().balance(&addr1).unwrap();
 
 		// after closing the block `addr1` should be reward twice, one for the included empty step message and another for block creation
-		let b2 = b2.close_and_lock();
+		let b2 = b2.close_and_lock().unwrap();
 
 		// the spec sets the block reward to 10
 		assert_eq!(b2.block().state().balance(&addr1).unwrap(), addr1_balance + (10 * 2).into())
@@ -2013,7 +2024,7 @@ mod tests {
 			false,
 			&mut Vec::new().into_iter(),
 		).unwrap();
-		let b1 = b1.close_and_lock();
+		let b1 = b1.close_and_lock().unwrap();
 
 		// since the block is empty it isn't sealed and we generate empty steps
 		engine.set_signer(tap.clone(), addr1, "1".into());
@@ -2039,7 +2050,7 @@ mod tests {
 
 		// after closing the block `addr1` should be reward twice, one for the included empty step
 		// message and another for block creation
-		let b2 = b2.close_and_lock();
+		let b2 = b2.close_and_lock().unwrap();
 
 		// the contract rewards (1000 + kind) for each benefactor/reward kind
 		assert_eq!(

ethcore/src/engines/basic_authority.rs

@@ -252,7 +252,7 @@ mod tests {
 		let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 		let b = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes, addr, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b = b.close_and_lock();
+		let b = b.close_and_lock().unwrap();
 		if let Seal::Regular(seal) = engine.generate_seal(b.block(), &genesis_header) {
 			assert!(b.try_seal(engine, seal).is_ok());
 		}

ethcore/src/engines/block_reward.rs

@@ -170,7 +170,7 @@ mod test {
 				"0000000000000000000000000000000000000001".into(),
 				(3141562.into(), 31415620.into()),
 				vec![],
-			);
+			).unwrap();
 
 			let result = machine.execute_as_system(
 				block.block_mut(),

ethcore/src/engines/instant_seal.rs

@@ -87,7 +87,7 @@ mod tests {
 		let genesis_header = spec.genesis_header();
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 		let b = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes, Address::default(), (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b = b.close_and_lock();
+		let b = b.close_and_lock().unwrap();
 		if let Seal::Regular(seal) = engine.generate_seal(b.block(), &genesis_header) {
 			assert!(b.try_seal(engine, seal).is_ok());
 		}

ethcore/src/engines/tendermint/mod.rs

@@ -804,7 +804,7 @@ mod tests {
 		let genesis_header = spec.genesis_header();
 		let last_hashes = Arc::new(vec![genesis_header.hash()]);
 		let b = OpenBlock::new(spec.engine.as_ref(), Default::default(), false, db.boxed_clone(), &genesis_header, last_hashes, proposer, (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap();
-		let b = b.close();
+		let b = b.close().unwrap();
 		if let Seal::Proposal(seal) = spec.engine.generate_seal(b.block(), &genesis_header) {
 			(b, seal)
 		} else {