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 1 commit
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
31 changes: 26 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,41 @@ impl<V> BlockChainClient for Client<V> where V: Verifier {

// Add transactions
let block_number = b.block().header().number();
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.

let hash = tx.hash();
let gas = tx.gas;
// TODO [todr] It seems that calculating gas_left here doesn't really look nice. After moving this function
// to miner crate we should consider rewriting this logic in some better way.
if gas > gas_left {
trace!(target: "miner", "Skipping adding transaction to block because of gas limit: {:?}", hash);
continue;
}

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) => {
trace!(target: "miner", "Error adding transaction to block: number={}. transaction_hash={:?}, Error: {:?}",
block_number, hash, e);
invalid_transactions.insert(hash);
},
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