From 5059619947c5af12ecd4adc457714ec09d901573 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 16 Jul 2018 19:53:55 +0800 Subject: [PATCH] Unify engine error to reject blocks (#9085) * Reject if Engine::on_close_block returns error * Unify open block behaviors * Fix tests in ethcore * Fix Aura tests * Fix RPC test * Print a warning if open block failed * Print the actual error when closing the block * Update comments for prepare_pending_block * Add BlockPreparationStatus to distingish three different state after prepare_pending_block --- ethcore/src/block.rs | 39 +++++------- ethcore/src/client/client.rs | 6 +- ethcore/src/client/test_client.rs | 8 +-- ethcore/src/client/traits.rs | 4 +- ethcore/src/engines/authority_round/mod.rs | 45 ++++++++----- ethcore/src/engines/basic_authority.rs | 2 +- ethcore/src/engines/block_reward.rs | 2 +- ethcore/src/engines/instant_seal.rs | 2 +- ethcore/src/engines/tendermint/mod.rs | 2 +- ethcore/src/ethereum/ethash.rs | 6 +- ethcore/src/miner/miner.rs | 73 ++++++++++++++++------ ethcore/src/test_helpers.rs | 6 +- ethcore/src/tests/client.rs | 10 +-- ethcore/src/tests/trace.rs | 6 +- rpc/src/v1/tests/helpers/miner_service.rs | 4 +- 15 files changed, 125 insertions(+), 90 deletions(-) diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index 43400895f..2cd2fef70 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -428,20 +428,16 @@ impl<'x> OpenBlock<'x> { } /// Turn this into a `ClosedBlock`. - pub fn close(self) -> ClosedBlock { + pub fn close(self) -> Result { 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)); @@ -453,26 +449,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 { 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()))); @@ -492,10 +483,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)] @@ -668,7 +659,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 @@ -764,7 +755,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 @@ -789,7 +780,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![]); } @@ -803,7 +794,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().state.drop().1; @@ -833,7 +824,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().state.drop().1; diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 3a992fcb2..56c71a86e 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2131,7 +2131,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 { let engine = &*self.engine; let chain = self.chain.read(); let best_header = chain.best_block_header(); @@ -2150,7 +2150,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 @@ -2166,7 +2166,7 @@ impl PrepareOpenBlock for Client { qed"); }); - open_block + Ok(open_block) } } diff --git a/ethcore/src/client/test_client.rs b/ethcore/src/client/test_client.rs index 817039611..7a3dfcd00 100644 --- a/ethcore/src/client/test_client.rs +++ b/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 { 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) } } diff --git a/ethcore/src/client/traits.rs b/ethcore/src/client/traits.rs index e633ae7c5..65bf00921 100644 --- a/ethcore/src/client/traits.rs +++ b/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; } /// Provides methods used for sealing new state diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index e927f77b7..f3f4504ec 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/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,7 +1849,7 @@ 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(); @@ -1846,7 +1857,7 @@ mod tests { // 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!( diff --git a/ethcore/src/engines/basic_authority.rs b/ethcore/src/engines/basic_authority.rs index f535399e5..e73b10461 100644 --- a/ethcore/src/engines/basic_authority.rs +++ b/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()); } diff --git a/ethcore/src/engines/block_reward.rs b/ethcore/src/engines/block_reward.rs index 9a9d54e4a..7144ed78d 100644 --- a/ethcore/src/engines/block_reward.rs +++ b/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(), diff --git a/ethcore/src/engines/instant_seal.rs b/ethcore/src/engines/instant_seal.rs index a35dea521..5719e11b1 100644 --- a/ethcore/src/engines/instant_seal.rs +++ b/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()); } diff --git a/ethcore/src/engines/tendermint/mod.rs b/ethcore/src/engines/tendermint/mod.rs index c95d61aa7..cc2325bed 100644 --- a/ethcore/src/engines/tendermint/mod.rs +++ b/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 { diff --git a/ethcore/src/ethereum/ethash.rs b/ethcore/src/ethereum/ethash.rs index 17268fee1..ca19983d3 100644 --- a/ethcore/src/ethereum/ethash.rs +++ b/ethcore/src/ethereum/ethash.rs @@ -542,7 +542,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, Address::zero(), (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap(); - let b = b.close(); + let b = b.close().unwrap(); assert_eq!(b.state().balance(&Address::zero()).unwrap(), U256::from_str("4563918244f40000").unwrap()); } @@ -596,7 +596,7 @@ mod tests { uncle.set_author(uncle_author); b.push_uncle(uncle).unwrap(); - let b = b.close(); + let b = b.close().unwrap(); assert_eq!(b.state().balance(&Address::zero()).unwrap(), "478eae0e571ba000".into()); assert_eq!(b.state().balance(&uncle_author).unwrap(), "3cb71f51fc558000".into()); } @@ -609,7 +609,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, Address::zero(), (3141562.into(), 31415620.into()), vec![], false, &mut Vec::new().into_iter()).unwrap(); - let b = b.close(); + let b = b.close().unwrap(); let ubi_contract: Address = "00efdd5883ec628983e9063c7d969fe268bbf310".into(); let dev_contract: Address = "00756cf8159095948496617f5fb17ed95059f536".into(); diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 16a20f8a0..167a58d23 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -82,6 +82,17 @@ pub enum Penalization { }, } +/// Pending block preparation status. +#[derive(Debug, PartialEq)] +pub enum BlockPreparationStatus { + /// We had to prepare new pending block and the preparation succeeded. + Succeeded, + /// We had to prepare new pending block but the preparation failed. + Failed, + /// We didn't have to prepare a new block. + NotPrepared, +} + /// Initial minimal gas price. /// /// Gas price should be later overwritten externally @@ -334,7 +345,7 @@ impl Miner { } /// Prepares new block for sealing including top transactions from queue. - fn prepare_block(&self, chain: &C) -> (ClosedBlock, Option) where + fn prepare_block(&self, chain: &C) -> Option<(ClosedBlock, Option)> where C: BlockChain + CallContract + BlockProducer + Nonce + Sync, { trace_time!("prepare_block"); @@ -362,11 +373,18 @@ impl Miner { // block not found - create it. trace!(target: "miner", "prepare_block: No existing work - making new block"); let params = self.params.read().clone(); - chain.prepare_open_block( + + match chain.prepare_open_block( params.author, params.gas_range_target, params.extra_data, - ) + ) { + Ok(block) => block, + Err(err) => { + warn!(target: "miner", "Open new block failed with error {:?}. This is likely an error in chain specificiations or on-chain consensus smart contracts.", err); + return None; + } + } } }; @@ -493,7 +511,13 @@ impl Miner { let elapsed = block_start.elapsed(); debug!(target: "miner", "Pushed {} transactions in {} ms", tx_count, took_ms(&elapsed)); - let block = open_block.close(); + let block = match open_block.close() { + Ok(block) => block, + Err(err) => { + warn!(target: "miner", "Closing the block failed with error {:?}. This is likely an error in chain specificiations or on-chain consensus smart contracts.", err); + return None; + } + }; { self.transaction_queue.remove(invalid_transactions.iter(), true); @@ -501,7 +525,7 @@ impl Miner { self.transaction_queue.penalize(senders_to_penalize.iter()); } - (block, original_work_hash) + Some((block, original_work_hash)) } /// Returns `true` if we should create pending block even if some other conditions are not met. @@ -705,8 +729,8 @@ impl Miner { } } - /// Returns true if we had to prepare new pending block. - fn prepare_pending_block(&self, client: &C) -> bool where + /// Prepare a pending block. Returns the preparation status. + fn prepare_pending_block(&self, client: &C) -> BlockPreparationStatus where C: BlockChain + CallContract + BlockProducer + SealedBlockImporter + Nonce + Sync, { trace!(target: "miner", "prepare_pending_block: entering"); @@ -722,14 +746,21 @@ impl Miner { } }; - if prepare_new { + let preparation_status = if prepare_new { // -------------------------------------------------------------------------- // | NOTE Code below requires sealing locks. | // | Make sure to release the locks before calling that method. | // -------------------------------------------------------------------------- - let (block, original_work_hash) = self.prepare_block(client); - self.prepare_work(block, original_work_hash); - } + match self.prepare_block(client) { + Some((block, original_work_hash)) => { + self.prepare_work(block, original_work_hash); + BlockPreparationStatus::Succeeded + }, + None => BlockPreparationStatus::Failed, + } + } else { + BlockPreparationStatus::NotPrepared + }; let best_number = client.chain_info().best_block_number; let mut sealing = self.sealing.lock(); @@ -742,8 +773,7 @@ impl Miner { sealing.last_request = Some(best_number); } - // Return if we restarted - prepare_new + preparation_status } /// Prepare pending block, check whether sealing is needed, and then update sealing. @@ -752,7 +782,7 @@ impl Miner { // Make sure to do it after transaction is imported and lock is dropped. // We need to create pending block and enable sealing. - if self.engine.seals_internally().unwrap_or(false) || !self.prepare_pending_block(chain) { + if self.engine.seals_internally().unwrap_or(false) || self.prepare_pending_block(chain) == BlockPreparationStatus::NotPrepared { // If new block has not been prepared (means we already had one) // or Engine might be able to seal internally, // we need to update sealing. @@ -1051,7 +1081,10 @@ impl miner::MinerService for Miner { // | Make sure to release the locks before calling that method. | // -------------------------------------------------------------------------- trace!(target: "miner", "update_sealing: preparing a block"); - let (block, original_work_hash) = self.prepare_block(chain); + let (block, original_work_hash) = match self.prepare_block(chain) { + Some((block, original_work_hash)) => (block, original_work_hash), + None => return, + }; // refuse to seal the first block of the chain if it contains hard forks // which should be on by default. @@ -1345,7 +1378,7 @@ mod tests { assert_eq!(miner.pending_receipts(best_block).unwrap().len(), 1); assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1); // This method will let us know if pending block was created (before calling that method) - assert!(!miner.prepare_pending_block(&client)); + assert_eq!(miner.prepare_pending_block(&client), BlockPreparationStatus::NotPrepared); } #[test] @@ -1383,7 +1416,7 @@ mod tests { // By default we use PendingSet::AlwaysSealing, so no transactions yet. assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 0); // This method will let us know if pending block was created (before calling that method) - assert!(miner.prepare_pending_block(&client)); + assert_eq!(miner.prepare_pending_block(&client), BlockPreparationStatus::Succeeded); // After pending block is created we should see a transaction. assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1); } @@ -1418,7 +1451,7 @@ mod tests { assert_eq!(miner.pending_transactions(best_block), None); assert_eq!(miner.pending_receipts(best_block), None); assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 0); - assert!(miner.prepare_pending_block(&client)); + assert_eq!(miner.prepare_pending_block(&client), BlockPreparationStatus::Succeeded); assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1); // when - 2nd part: create a local transaction from account_provider. @@ -1432,7 +1465,7 @@ mod tests { assert_eq!(miner.pending_transactions(best_block).unwrap().len(), 2); assert_eq!(miner.pending_receipts(best_block).unwrap().len(), 2); assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 2); - assert!(!miner.prepare_pending_block(&client)); + assert_eq!(miner.prepare_pending_block(&client), BlockPreparationStatus::NotPrepared); } #[test] @@ -1443,7 +1476,7 @@ mod tests { assert!(!miner.requires_reseal(1u8.into())); miner.import_external_transactions(&client, vec![transaction().into()]).pop().unwrap().unwrap(); - assert!(miner.prepare_pending_block(&client)); + assert_eq!(miner.prepare_pending_block(&client), BlockPreparationStatus::Succeeded); // Unless asked to prepare work. assert!(miner.requires_reseal(1u8.into())); } diff --git a/ethcore/src/test_helpers.rs b/ethcore/src/test_helpers.rs index 90ab15598..443e0418b 100644 --- a/ethcore/src/test_helpers.rs +++ b/ethcore/src/test_helpers.rs @@ -172,7 +172,7 @@ pub fn generate_dummy_client_with_spec_accounts_and_data(test_spec: F, accoun n += 1; } - let b = b.close_and_lock().seal(test_engine, vec![]).unwrap(); + let b = b.close_and_lock().unwrap().seal(test_engine, vec![]).unwrap(); if let Err(e) = client.import_block(b.rlp_bytes()) { panic!("error importing block which is valid by definition: {:?}", e); @@ -222,13 +222,13 @@ pub fn push_block_with_transactions(client: &Arc, transactions: &[Signed let test_engine = &*test_spec.engine; let block_number = client.chain_info().best_block_number as u64 + 1; - let mut b = client.prepare_open_block(Address::default(), (0.into(), 5000000.into()), Bytes::new()); + let mut b = client.prepare_open_block(Address::default(), (0.into(), 5000000.into()), Bytes::new()).unwrap(); b.set_timestamp(block_number * 10); for t in transactions { b.push_transaction(t.clone(), None).unwrap(); } - let b = b.close_and_lock().seal(test_engine, vec![]).unwrap(); + let b = b.close_and_lock().unwrap().seal(test_engine, vec![]).unwrap(); if let Err(e) = client.import_block(b.rlp_bytes()) { panic!("error importing block which is valid by definition: {:?}", e); diff --git a/ethcore/src/tests/client.rs b/ethcore/src/tests/client.rs index 409248a2e..0b8f2c4fa 100644 --- a/ethcore/src/tests/client.rs +++ b/ethcore/src/tests/client.rs @@ -268,7 +268,7 @@ fn can_mine() { let dummy_blocks = get_good_dummy_block_seq(2); let client = get_test_client_with_blocks(vec![dummy_blocks[0].clone()]); - let b = client.prepare_open_block(Address::default(), (3141562.into(), 31415620.into()), vec![]).close(); + let b = client.prepare_open_block(Address::default(), (3141562.into(), 31415620.into()), vec![]).unwrap().close().unwrap(); assert_eq!(*b.block().header().parent_hash(), view!(BlockView, &dummy_blocks[0]).header_view().hash()); } @@ -291,10 +291,10 @@ fn change_history_size() { ).unwrap(); for _ in 0..20 { - let mut b = client.prepare_open_block(Address::default(), (3141562.into(), 31415620.into()), vec![]); + let mut b = client.prepare_open_block(Address::default(), (3141562.into(), 31415620.into()), vec![]).unwrap(); b.block_mut().state_mut().add_balance(&address, &5.into(), CleanupMode::NoEmpty).unwrap(); b.block_mut().state_mut().commit().unwrap(); - let b = b.close_and_lock().seal(&*test_spec.engine, vec![]).unwrap(); + let b = b.close_and_lock().unwrap().seal(&*test_spec.engine, vec![]).unwrap(); client.import_sealed_block(b).unwrap(); // account change is in the journal overlay } } @@ -350,10 +350,10 @@ fn transaction_proof() { let address = Address::random(); let test_spec = Spec::new_test(); for _ in 0..20 { - let mut b = client.prepare_open_block(Address::default(), (3141562.into(), 31415620.into()), vec![]); + let mut b = client.prepare_open_block(Address::default(), (3141562.into(), 31415620.into()), vec![]).unwrap(); b.block_mut().state_mut().add_balance(&address, &5.into(), CleanupMode::NoEmpty).unwrap(); b.block_mut().state_mut().commit().unwrap(); - let b = b.close_and_lock().seal(&*test_spec.engine, vec![]).unwrap(); + let b = b.close_and_lock().unwrap().seal(&*test_spec.engine, vec![]).unwrap(); client.import_sealed_block(b).unwrap(); // account change is in the journal overlay } diff --git a/ethcore/src/tests/trace.rs b/ethcore/src/tests/trace.rs index fd0604cf5..fd7c932cb 100644 --- a/ethcore/src/tests/trace.rs +++ b/ethcore/src/tests/trace.rs @@ -89,7 +89,7 @@ fn can_trace_block_and_uncle_reward() { rolling_timestamp += 10; root_block.set_timestamp(rolling_timestamp); - let root_block = root_block.close_and_lock().seal(engine, vec![]).unwrap(); + let root_block = root_block.close_and_lock().unwrap().seal(engine, vec![]).unwrap(); if let Err(e) = client.import_block(root_block.rlp_bytes()) { panic!("error importing block which is valid by definition: {:?}", e); @@ -118,7 +118,7 @@ fn can_trace_block_and_uncle_reward() { rolling_timestamp += 10; parent_block.set_timestamp(rolling_timestamp); - let parent_block = parent_block.close_and_lock().seal(engine, vec![]).unwrap(); + let parent_block = parent_block.close_and_lock().unwrap().seal(engine, vec![]).unwrap(); if let Err(e) = client.import_block(parent_block.rlp_bytes()) { panic!("error importing block which is valid by definition: {:?}", e); @@ -168,7 +168,7 @@ fn can_trace_block_and_uncle_reward() { uncle.set_timestamp(rolling_timestamp); block.push_uncle(uncle).unwrap(); - let block = block.close_and_lock().seal(engine, vec![]).unwrap(); + let block = block.close_and_lock().unwrap().seal(engine, vec![]).unwrap(); let res = client.import_block(block.rlp_bytes()); if res.is_err() { diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index fa9f22b24..7ecd2a474 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -189,8 +189,8 @@ impl MinerService for TestMinerService { fn work_package(&self, chain: &C) -> Option<(H256, BlockNumber, u64, U256)> { let params = self.authoring_params(); - let open_block = chain.prepare_open_block(params.author, params.gas_range_target, params.extra_data); - let closed = open_block.close(); + let open_block = chain.prepare_open_block(params.author, params.gas_range_target, params.extra_data).unwrap(); + let closed = open_block.close().unwrap(); let header = closed.header(); Some((header.hash(), header.number(), header.timestamp(), *header.difficulty()))