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

Avoid importing transactions with gas above 1.1*block_gas_limit to transaction queue #760

Merged
merged 3 commits into from
Mar 18, 2016
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions ethcore/src/client/test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ impl TestBlockChainClient {
header.difficulty = From::from(n);
header.parent_hash = self.last_hash.read().unwrap().clone();
header.number = n as BlockNumber;
header.gas_limit = U256::from(1_000_000);
let uncles = match with {
EachBlockWith::Uncle | EachBlockWith::UncleAndTransaction => {
let mut uncles = RlpStream::new_list(1);
Expand Down
7 changes: 7 additions & 0 deletions ethcore/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ pub enum TransactionError {
/// Transaction cost
cost: U256,
},
/// Transactions gas is higher then current gas limit
GasLimitExceeded {
/// Current gas limit
limit: U256,
/// Declared transaction gas
got: U256,
},
/// Transaction's gas limit (aka gas) is invalid.
InvalidGasLimit(OutOfBounds<U256>),
}
Expand Down
17 changes: 15 additions & 2 deletions miner/src/miner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::sync::atomic::AtomicBool;
use std::collections::HashSet;

use util::{H256, U256, Address, Bytes, Uint};
use ethcore::views::{BlockView};
use ethcore::views::{BlockView, HeaderView};
use ethcore::client::{BlockChainClient, BlockId};
use ethcore::block::{ClosedBlock, IsBlock};
use ethcore::error::{Error};
Expand Down Expand Up @@ -94,6 +94,12 @@ impl Miner {
pub fn set_minimal_gas_price(&self, min_gas_price: U256) {
self.transaction_queue.lock().unwrap().set_minimal_gas_price(min_gas_price);
}

fn update_gas_limit(&self, chain: &BlockChainClient) {
let gas_limit = HeaderView::new(&chain.best_block_header()).gas_limit();
let mut queue = self.transaction_queue.lock().unwrap();
queue.set_gas_limit(gas_limit);
}
}

impl MinerService for Miner {
Expand Down Expand Up @@ -174,6 +180,11 @@ impl MinerService for Miner {
let block = BlockView::new(&block);
block.transactions()
}

// First update gas limit in transaction queue
self.update_gas_limit(chain);

// Then import all transactions...
{
let out_of_chain = retracted
.par_iter()
Expand All @@ -190,7 +201,8 @@ impl MinerService for Miner {
});
});
}
// First import all transactions and after that remove old ones

// ...and after that remove old ones
{
let in_chain = {
let mut in_chain = HashSet::new();
Expand All @@ -216,6 +228,7 @@ impl MinerService for Miner {
});
}

// Update mined block
if self.sealing_enabled.load(atomic::Ordering::Relaxed) {
self.prepare_sealing(chain);
}
Expand Down
65 changes: 63 additions & 2 deletions miner/src/transaction_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,16 @@ pub struct AccountDetails {
pub balance: U256,
}


/// Transactions with `gas > (gas_limit + gas_limit * Factor(in percents))` are not imported to the queue.
const GAS_LIMIT_HYSTERESIS: usize = 10; // %

/// TransactionQueue implementation
pub struct TransactionQueue {
/// Gas Price threshold for transactions that can be imported to this queue (defaults to 0)
minimal_gas_price: U256,
/// Current gas limit (block gas limit * factor). Transactions above the limit will not be accepted (default to !0)
gas_limit: U256,
/// Priority queue for transactions that can go to block
current: TransactionSet,
/// Priority queue for transactions that has been received but are not yet valid to go to block
Expand Down Expand Up @@ -293,6 +299,7 @@ impl TransactionQueue {

TransactionQueue {
minimal_gas_price: U256::zero(),
gas_limit: !U256::zero(),
current: current,
future: future,
by_hash: HashMap::new(),
Expand All @@ -301,11 +308,22 @@ impl TransactionQueue {
}

/// Sets new gas price threshold for incoming transactions.
/// Any transactions already imported to the queue are not affected.
/// Any transaction already imported to the queue is not affected.
pub fn set_minimal_gas_price(&mut self, min_gas_price: U256) {
self.minimal_gas_price = min_gas_price;
}

/// Sets new gas limit. Transactions with gas slightly (`GAS_LIMIT_HYSTERESIS`) above the limit won't be imported.
/// Any transaction already imported to the queue is not affected.
pub fn set_gas_limit(&mut self, gas_limit: U256) {
let extra = gas_limit / U256::from(GAS_LIMIT_HYSTERESIS);

self.gas_limit = match gas_limit.overflowing_add(extra) {
(_, true) => !U256::zero(),
(val, false) => val,
};
}

/// Returns current status for this queue
pub fn status(&self) -> TransactionQueueStatus {
TransactionQueueStatus {
Expand Down Expand Up @@ -335,12 +353,24 @@ impl TransactionQueue {
tx.hash(), tx.gas_price, self.minimal_gas_price
);

return Err(Error::Transaction(TransactionError::InsufficientGasPrice{
return Err(Error::Transaction(TransactionError::InsufficientGasPrice {
minimal: self.minimal_gas_price,
got: tx.gas_price,
}));
}

if tx.gas > self.gas_limit {
Copy link
Contributor

Choose a reason for hiding this comment

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

this check should probably be moved be checked OpenBlock::push_transaction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But there already is similar check (or executive will just fail with BlockGasLimitReached).
In this PR we want to avoid even storing such transactions in TransactionQueue (`self.gas_limit here is 1.1*best_block_gas_limit).

trace!(target: "miner",
"Dropping transaction above gas limit: {:?} ({} > {})",
tx.hash(), tx.gas, self.gas_limit
);

return Err(Error::Transaction(TransactionError::GasLimitExceeded {
limit: self.gas_limit,
got: tx.gas,
}));
}


let vtx = try!(VerifiedTransaction::new(tx));
let account = fetch_account(&vtx.sender());
Expand Down Expand Up @@ -677,6 +707,37 @@ mod test {
assert_eq!(stats.pending, 1);
}

#[test]
fn gas_limit_should_never_overflow() {
// given
let mut txq = TransactionQueue::new();
txq.set_gas_limit(U256::zero());
assert_eq!(txq.gas_limit, U256::zero());

// when
txq.set_gas_limit(!U256::zero());

// then
assert_eq!(txq.gas_limit, !U256::zero());
}

#[test]
fn should_not_import_transaction_above_gas_limit() {
// given
let mut txq = TransactionQueue::new();
let tx = new_tx();
txq.set_gas_limit(tx.gas / U256::from(2));

// when
txq.add(tx, &default_nonce).unwrap_err();

// then
let stats = txq.status();
assert_eq!(stats.pending, 0);
assert_eq!(stats.future, 0);
}


#[test]
fn should_drop_transactions_from_senders_without_balance() {
// given
Expand Down