Return error if RLP size of transaction exceeds the limit (#8473)

* Return error if RLP size of transaction exceeds the limit

* Review comments fixed

* RLP check moved to verifier, corresponding pool test added

Cargo.lock

@@ -684,6 +684,7 @@ dependencies = [
  "parking_lot 0.5.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "price-info 1.12.0",
  "rayon 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "rlp 0.2.1",
  "rustc-hex 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "trace-time 0.1.0",
  "transaction-pool 1.12.0",

ethcore/src/client/client.rs

@@ -62,7 +62,6 @@ use ethcore_miner::pool::VerifiedTransaction;
 use parking_lot::{Mutex, RwLock};
 use rand::OsRng;
 use receipt::{Receipt, LocalizedReceipt};
-use rlp::Rlp;
 use snapshot::{self, io as snapshot_io};
 use spec::Spec;
 use state_db::StateDB;
@@ -995,7 +994,7 @@ impl Client {
 
 		let txs: Vec<UnverifiedTransaction> = transactions
 			.iter()
-			.filter_map(|bytes| Rlp::new(bytes).as_val().ok())
+			.filter_map(|bytes| self.engine().decode_transaction(bytes).ok())
 			.collect();
 
 		self.notify(|notify| {

ethcore/src/engines/mod.rs

@@ -426,6 +426,11 @@ pub trait EthEngine: Engine<::machine::EthereumMachine> {
 	fn additional_params(&self) -> HashMap<String, String> {
 		self.machine().additional_params()
 	}
+
+	/// Performs pre-validation of RLP decoded transaction before other processing
+	fn decode_transaction(&self, transaction: &[u8]) -> Result<UnverifiedTransaction, transaction::Error> {
+		self.machine().decode_transaction(transaction)
+	}
 }
 
 // convenience wrappers for existing functions.

ethcore/src/machine.rs

@@ -34,6 +34,7 @@ use tx_filter::TransactionFilter;
 
 use ethereum_types::{U256, Address};
 use bytes::BytesRef;
+use rlp::Rlp;
 use vm::{CallType, ActionParams, ActionValue, ParamsType};
 use vm::{EnvInfo, Schedule, CreateContractAddress};
 
@@ -376,6 +377,16 @@ impl EthereumMachine {
 			"registrar".to_owned() => format!("{:x}", self.params.registrar)
 		]
 	}
+
+	/// Performs pre-validation of RLP decoded transaction before other processing
+	pub fn decode_transaction(&self, transaction: &[u8]) -> Result<UnverifiedTransaction, transaction::Error> {
+		let rlp = Rlp::new(&transaction);
+		if rlp.as_raw().len() > self.params().max_transaction_size {
+			debug!("Rejected oversized transaction of {} bytes", rlp.as_raw().len());
+			return Err(transaction::Error::TooBig)
+		}
+		rlp.as_val().map_err(|e| transaction::Error::InvalidRlp(e.to_string()))
+	}
 }
 
 /// Auxiliary data fetcher for an Ethereum machine. In Ethereum-like machines

ethcore/src/miner/pool_client.rs

@@ -145,6 +145,10 @@ impl<'a, C: 'a> pool::client::Client for PoolClient<'a, C> where
 			}
 		}
 	}
+
+	fn decode_transaction(&self, transaction: &[u8]) -> Result<UnverifiedTransaction, transaction::Error> {
+		self.engine.decode_transaction(transaction)
+	}
 }
 
 impl<'a, C: 'a> NonceClient for PoolClient<'a, C> where

ethcore/src/spec/spec.rs

@@ -48,6 +48,8 @@ use trace::{NoopTracer, NoopVMTracer};
 
 pub use ethash::OptimizeFor;
 
+const MAX_TRANSACTION_SIZE: usize = 300 * 1024;
+
 // helper for formatting errors.
 fn fmt_err<F: ::std::fmt::Display>(f: F) -> String {
 	format!("Spec json is invalid: {}", f)
@@ -123,6 +125,8 @@ pub struct CommonParams {
 	pub max_code_size_transition: BlockNumber,
 	/// Transaction permission managing contract address.
 	pub transaction_permission_contract: Option<Address>,
+	/// Maximum size of transaction's RLP payload
+	pub max_transaction_size: usize,
 }
 
 impl CommonParams {
@@ -238,6 +242,7 @@ impl From<ethjson::spec::Params> for CommonParams {
 			registrar: p.registrar.map_or_else(Address::new, Into::into),
 			node_permission_contract: p.node_permission_contract.map(Into::into),
 			max_code_size: p.max_code_size.map_or(u64::max_value(), Into::into),
+			max_transaction_size: p.max_transaction_size.map_or(MAX_TRANSACTION_SIZE, Into::into),
 			max_code_size_transition: p.max_code_size_transition.map_or(0, Into::into),
 			transaction_permission_contract: p.transaction_permission_contract.map(Into::into),
 			wasm_activation_transition: p.wasm_activation_transition.map_or(

ethcore/sync/src/chain.rs

@@ -140,7 +140,6 @@ const MAX_PEERS_PROPAGATION: usize = 128;
 const MAX_PEER_LAG_PROPAGATION: BlockNumber = 20;
 const MAX_NEW_HASHES: usize = 64;
 const MAX_NEW_BLOCK_AGE: BlockNumber = 20;
-const MAX_TRANSACTION_SIZE: usize = 300*1024;
 // maximal packet size with transactions (cannot be greater than 16MB - protocol limitation).
 const MAX_TRANSACTION_PACKET_SIZE: usize = 8 * 1024 * 1024;
 // Maximal number of transactions in sent in single packet.
@@ -1517,10 +1516,6 @@ impl ChainSync {
 		let mut transactions = Vec::with_capacity(item_count);
 		for i in 0 .. item_count {
 			let rlp = r.at(i)?;
-			if rlp.as_raw().len() > MAX_TRANSACTION_SIZE {
-				debug!("Skipped oversized transaction of {} bytes", rlp.as_raw().len());
-				continue;
-			}
 			let tx = rlp.as_raw().to_vec();
 			transactions.push(tx);
 		}

ethcore/transaction/src/error.rs

@@ -18,6 +18,7 @@ use std::{fmt, error};
 
 use ethereum_types::U256;
 use ethkey;
+use rlp;
 use unexpected::OutOfBounds;
 
 #[derive(Debug, PartialEq, Clone)]
@@ -74,6 +75,10 @@ pub enum Error {
 	NotAllowed,
 	/// Signature error
 	InvalidSignature(String),
+	/// Transaction too big
+	TooBig,
+	/// Invalid RLP encoding
+	InvalidRlp(String),
 }
 
 impl From<ethkey::Error> for Error {
@@ -82,6 +87,12 @@ impl From<ethkey::Error> for Error {
 	}
 }
 
+impl From<rlp::DecoderError> for Error {
+	fn from(err: rlp::DecoderError) -> Self {
+		Error::InvalidRlp(format!("{}", err))
+	}
+}
+
 impl fmt::Display for Error {
 	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
 		use self::Error::*;
@@ -106,6 +117,8 @@ impl fmt::Display for Error {
 			InvalidChainId => "Transaction of this chain ID is not allowed on this chain.".into(),
 			InvalidSignature(ref err) => format!("Transaction has invalid signature: {}.", err),
 			NotAllowed => "Sender does not have permissions to execute this type of transction".into(),
+			TooBig => "Transaction too big".into(),
+			InvalidRlp(ref err) => format!("Transaction has invalid RLP structure: {}.", err),
 		};
 
 		f.write_fmt(format_args!("Transaction error ({})", msg))

json/src/spec/params.rs

@@ -113,6 +113,9 @@ pub struct Params {
 	/// See main EthashParams docs.
 	#[serde(rename="maxCodeSize")]
 	pub max_code_size: Option<Uint>,
+	/// Maximum size of transaction RLP payload.
+	#[serde(rename="maxTransactionSize")]
+	pub max_transaction_size: Option<Uint>,
 	/// See main EthashParams docs.
 	#[serde(rename="maxCodeSizeTransition")]
 	pub max_code_size_transition: Option<Uint>,

miner/Cargo.toml

@@ -28,6 +28,7 @@ log = "0.3"
 parking_lot = "0.5"
 price-info = { path = "../price-info" }
 rayon = "1.0"
+rlp = { path = "../util/rlp" }
 trace-time = { path = "../util/trace-time" }
 transaction-pool = { path = "../transaction-pool" }
 

miner/src/lib.rs

@@ -30,6 +30,7 @@ extern crate linked_hash_map;
 extern crate parking_lot;
 extern crate price_info;
 extern crate rayon;
+extern crate rlp;
 extern crate trace_time;
 extern crate transaction_pool as txpool;
 

miner/src/pool/client.rs

@@ -62,6 +62,10 @@ pub trait Client: fmt::Debug + Sync {
 
 	/// Classify transaction (check if transaction is filtered by some contracts).
 	fn transaction_type(&self, tx: &transaction::SignedTransaction) -> TransactionType;
+
+	/// Performs pre-validation of RLP decoded transaction
+	fn decode_transaction(&self, transaction: &[u8])
+		-> Result<transaction::UnverifiedTransaction, transaction::Error>;
 }
 
 /// State nonce client

miner/src/pool/tests/client.rs

@@ -15,17 +15,21 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
 use ethereum_types::{U256, H256, Address};
+use rlp::Rlp;
 use transaction::{self, Transaction, SignedTransaction, UnverifiedTransaction};
 
 use pool;
 use pool::client::AccountDetails;
 
+const MAX_TRANSACTION_SIZE: usize = 15 * 1024;
+
 #[derive(Debug, Clone)]
 pub struct TestClient {
 	account_details: AccountDetails,
 	gas_required: U256,
 	is_service_transaction: bool,
 	local_address: Address,
+	max_transaction_size: usize,
 }
 
 impl Default for TestClient {
@@ -39,6 +43,7 @@ impl Default for TestClient {
 			gas_required: 21_000.into(),
 			is_service_transaction: false,
 			local_address: Default::default(),
+			max_transaction_size: MAX_TRANSACTION_SIZE,
 		}
 	}
 }
@@ -116,6 +121,15 @@ impl pool::client::Client for TestClient {
 			pool::client::TransactionType::Regular
 		}
 	}
+
+	fn decode_transaction(&self, transaction: &[u8]) -> Result<UnverifiedTransaction, transaction::Error> {
+		let rlp = Rlp::new(&transaction);
+		if rlp.as_raw().len() > self.max_transaction_size {
+			return Err(transaction::Error::TooBig)
+		}
+		rlp.as_val().map_err(|e| transaction::Error::InvalidRlp(e.to_string()))
+	}
+
 }
 
 impl pool::client::NonceClient for TestClient {

miner/src/pool/tests/mod.rs

@@ -755,3 +755,13 @@ fn should_clear_cache_after_timeout_for_local() {
 	// then
 	assert_eq!(txq.pending(TestClient::new(), 0, 1002, None).len(), 2);
 }
+
+#[test]
+fn should_reject_big_transaction() {
+	let txq = new_queue();
+	let big_tx = Tx::default().big_one();
+	let res = txq.import(TestClient::new(), vec![
+		verifier::Transaction::Local(PendingTransaction::new(big_tx, transaction::Condition::Timestamp(1000).into()))
+	]);
+	assert_eq!(res, vec![Err(transaction::Error::TooBig)]);
+}

miner/src/pool/tests/tx.rs

@@ -87,6 +87,19 @@ impl Tx {
 			nonce: self.nonce.into()
 		}
 	}
+
+	pub fn big_one(self) -> SignedTransaction {
+		let keypair = Random.generate().unwrap();
+		let tx = Transaction {
+			action: transaction::Action::Create,
+			value: U256::from(100),
+			data: include_str!("../res/big_transaction.data").from_hex().unwrap(),
+			gas: self.gas.into(),
+			gas_price: self.gas_price.into(),
+			nonce: self.nonce.into()
+		};
+		tx.sign(keypair.secret(), None)
+	}
 }
 pub trait TxExt: Sized {
 	type Out;

miner/src/pool/verifier.rs

@@ -27,6 +27,7 @@ use std::sync::Arc;
 use std::sync::atomic::{self, AtomicUsize};
 
 use ethereum_types::{U256, H256};
+use rlp::Encodable;
 use transaction;
 use txpool;
 
@@ -222,6 +223,12 @@ impl<C: Client> txpool::Verifier<Transaction> for Verifier<C> {
 			Transaction::Local(tx) => tx,
 		};
 
+		// Verify RLP payload
+		if let Err(err) = self.client.decode_transaction(&transaction.rlp_bytes()) {
+			debug!(target: "txqueue", "[{:?}] Rejected transaction's rlp payload", err);
+			bail!(err)
+		}
+
 		let sender = transaction.sender();
 		let account_details = self.client.account_details(&sender);
 

rpc/src/v1/helpers/errors.rs

@@ -338,6 +338,8 @@ pub fn transaction_message(error: &TransactionError) -> String {
 		RecipientBanned => "Recipient is banned in local queue.".into(),
 		CodeBanned => "Code is banned in local queue.".into(),
 		NotAllowed => "Transaction is not permitted.".into(),
+		TooBig => "Transaction is too big, see chain specification for the limit.".into(),
+		InvalidRlp(ref descr) => format!("Invalid RLP data: {}", descr),
 	}
 }