Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Removing transactions that failed to be pushed to block. #752

Merged
merged 6 commits into from
Mar 18, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 33 additions & 5 deletions ethcore/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,8 @@ impl<V> BlockChainClient for Client<V> where V: Verifier {
}

// TODO [todr] Should be moved to miner crate eventually.
fn prepare_sealing(&self, author: Address, gas_floor_target: U256, extra_data: Bytes, transactions: Vec<SignedTransaction>) -> Option<ClosedBlock> {
fn prepare_sealing(&self, author: Address, gas_floor_target: U256, extra_data: Bytes, transactions: Vec<SignedTransaction>)
-> Option<(ClosedBlock, HashSet<H256>)> {
let engine = self.engine.deref().deref();
let h = self.chain.best_block_hash();

Expand All @@ -417,21 +418,48 @@ impl<V> BlockChainClient for Client<V> where V: Verifier {

// Add transactions
let block_number = b.block().header().number();
let min_tx_gas = U256::from(self.engine.schedule(&b.env_info()).tx_gas);
let gas_limit = *b.block().header().gas_limit();
let mut gas_left = gas_limit;
let mut invalid_transactions = HashSet::new();

for tx in transactions {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stop when gas_left < min transaction gas (21000). There should be a constant for it in the Schedule I believe

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// Stop early if we are sure that no other transaction will be included
if gas_left < min_tx_gas {
break;
}

// TODO [todr] It seems that calculating gas_left here doesn't look nice. After moving this function
// to miner crate we should consider rewriting this logic in some better way.
if tx.gas > gas_left {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to duplicate this check here rather than just checking import later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I guess it would be possible to unwrap error, check if it's BlockGasLimitReached, check minimal transaction gas.

Will refactor, because calculating gas here is not particularly appealing.

trace!(target: "miner", "Skipping adding transaction to block because of gas limit: {:?}", tx.hash());
continue;
}

// Push transaction to block
let hash = tx.hash();
let import = b.push_transaction(tx, None);
if let Err(e) = import {
trace!("Error adding transaction to block: number={}. Error: {:?}", block_number, e);
match import {
Err(e) => {
invalid_transactions.insert(hash);
trace!(target: "miner",
"Error adding transaction to block: number={}. transaction_hash={:?}, Error: {:?}",
block_number, hash, e);
},
Ok(receipt) => {
gas_left = gas_limit - receipt.gas_used;
}
}
}

// And close
let b = b.close();
trace!("Sealing: number={}, hash={}, diff={}",
trace!(target: "miner", "Sealing: number={}, hash={}, diff={}",
b.block().header().number(),
b.hash(),
b.block().header().difficulty()
);
Some(b)
Some((b, invalid_transactions))
}

fn block_header(&self, id: BlockId) -> Option<Bytes> {
Expand Down
4 changes: 3 additions & 1 deletion ethcore/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub use self::config::{ClientConfig, BlockQueueConfig, BlockChainConfig};
pub use self::ids::{BlockId, TransactionId};
pub use self::test_client::{TestBlockChainClient, EachBlockWith};

use std::collections::HashSet;
use util::bytes::Bytes;
use util::hash::{Address, H256, H2048};
use util::numbers::U256;
Expand Down Expand Up @@ -110,7 +111,8 @@ pub trait BlockChainClient : Sync + Send {

// TODO [todr] Should be moved to miner crate eventually.
/// Returns ClosedBlock prepared for sealing.
fn prepare_sealing(&self, author: Address, gas_floor_target: U256, extra_data: Bytes, transactions: Vec<SignedTransaction>) -> Option<ClosedBlock>;
fn prepare_sealing(&self, author: Address, gas_floor_target: U256, extra_data: Bytes, transactions: Vec<SignedTransaction>)
-> Option<(ClosedBlock, HashSet<H256>)>;

// TODO [todr] Should be moved to miner crate eventually.
/// Attempts to seal given block. Returns `SealedBlock` on success and the same block in case of error.
Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/client/test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ impl BlockChainClient for TestBlockChainClient {
unimplemented!();
}

fn prepare_sealing(&self, _author: Address, _gas_floor_target: U256, _extra_data: Bytes, _transactions: Vec<SignedTransaction>) -> Option<ClosedBlock> {
fn prepare_sealing(&self, _author: Address, _gas_floor_target: U256, _extra_data: Bytes, _transactions: Vec<SignedTransaction>) -> Option<(ClosedBlock, HashSet<H256>)> {
unimplemented!()
}

Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/tests/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,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(), x!(31415926), vec![], vec![]).unwrap();
let b = client.prepare_sealing(Address::default(), x!(31415926), vec![], vec![]).unwrap().0;

assert_eq!(*b.block().header().parent_hash(), BlockView::new(&dummy_blocks[0]).header_view().sha3());
assert!(client.try_seal(b, vec![]).is_ok());
Expand Down
15 changes: 14 additions & 1 deletion miner/src/miner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,20 @@ impl MinerService for Miner {
self.extra_data(),
transactions,
);
*self.sealing_block.lock().unwrap() = b;

match b {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be done more succinctly as:

*self.sealing_block.lock().unwrap() = b.map(|x| {
    let (block, invalid_transactions) = x;
    let mut queue ...
    block
});

no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True - in this case it looks much better. However in general I'm trying to avoid side effects in map.

None => {
*self.sealing_block.lock().unwrap() = None
},
Some((block, invalid_transactions)) => {
let mut queue = self.transaction_queue.lock().unwrap();
queue.remove_all(
&invalid_transactions.into_iter().collect::<Vec<H256>>(),
|a: &Address| chain.nonce(a)
);
*self.sealing_block.lock().unwrap() = Some(block)
}
}
}

fn sealing_block(&self, chain: &BlockChainClient) -> &Mutex<Option<ClosedBlock>> {
Expand Down