Block refactoring, stricter RLP.

Fixed #234.
Partially fixes #233 for Blocks.
Fixed #222.

src/block.rs

@@ -5,80 +5,124 @@ use engine::*;
 use state::*;
 use verification::PreVerifiedBlock;
 
-/// A transaction/receipt execution entry.
+/// A block, encoded as it is on the block chain.
-pub struct Entry {
+// TODO: rename to Block
-	transaction: Transaction,
+#[derive(Default, Debug, Clone)]
-	receipt: Receipt,
+pub struct Block {
+	/// The header of this block.
+	pub header: Header,
+	/// The transactions in this block.
+	pub transactions: Vec<Transaction>,
+	/// The uncles of this block.
+	pub uncles: Vec<Header>,
+}
+
+impl Block {
+	/// Returns true iff the given bytes form a valid encoding of a block in RLP.
+	// TODO: implement Decoder for this and have this use that.
+	pub fn is_good(b: &[u8]) -> bool {
+		/*
+		let urlp = UntrustedRlp::new(&b);
+		if !urlp.is_list() || urlp.item_count() != 3 || urlp.size() != b.len() { return false; }
+		if urlp.val_at::<Header>(0).is_err() { return false; }
+
+		if !urlp.at(1).unwrap().is_list() { return false; }
+		if urlp.at(1).unwrap().iter().find(|i| i.as_val::<Transaction>().is_err()).is_some() {
+			return false;
+		}
+		
+		if !urlp.at(2).unwrap().is_list() { return false; }
+		if urlp.at(2).unwrap().iter().find(|i| i.as_val::<Header>().is_err()).is_some() {
+			return false;
+		}
+		true*/
+		UntrustedRlp::new(b).as_val::<Block>().is_ok()
+	}
+}
+
+impl Decodable for Block {
+	fn decode<D>(decoder: &D) -> Result<Self, DecoderError> where D: Decoder {
+		if decoder.as_raw().len() != try!(decoder.as_rlp().payload_info()).total() {
+			return Err(DecoderError::RlpIsTooBig);	
+		}
+		let d = try!(decoder.as_list());
+		if d.len() != 3 {
+			return Err(DecoderError::RlpIncorrectListLen);
+		}
+		Ok(Block {
+			header: try!(Decodable::decode(&d[0])),
+			transactions: try!(Decodable::decode(&d[1])),
+			uncles: try!(Decodable::decode(&d[2])),
+		})
+	}
 }
 
 /// Internal type for a block's common elements.
-pub struct Block {
+// TODO: rename to ExecutedBlock
-	header: Header,
+// TODO: use BareBlock
+#[derive(Debug, Clone)]
+pub struct ExecutedBlock {
+	base: Block,
 
-	/// State is the most final state in the block.
+	receipts: Vec<Receipt>,
+	transactions_set: HashSet<H256>,
 	state: State,
-
-	archive: Vec<Entry>,
-	archive_set: HashSet<H256>,
-
-	uncles: Vec<Header>,
 }
 
-/// A set of references to `Block` fields that are publicly accessible. 
+/// A set of references to `ExecutedBlock` fields that are publicly accessible. 
 pub struct BlockRefMut<'a> {
 	/// TODO [Gav Wood] Please document me
 	pub header: &'a Header,
 	/// TODO [Gav Wood] Please document me
-	pub state: &'a mut State,
+	pub transactions: &'a Vec<Transaction>,
-	/// TODO [Gav Wood] Please document me
-	pub archive: &'a Vec<Entry>,
 	/// TODO [Gav Wood] Please document me
 	pub uncles: &'a Vec<Header>,
+
+	/// TODO [Gav Wood] Please document me
+	pub receipts: &'a Vec<Receipt>,
+	/// TODO [Gav Wood] Please document me
+	pub state: &'a mut State,
 }
 
-impl Block {
+impl ExecutedBlock {
 	/// Create a new block from the given `state`.
-	fn new(state: State) -> Block {
+	fn new(state: State) -> ExecutedBlock { ExecutedBlock { base: Default::default(), receipts: Default::default(), transactions_set: Default::default(), state: state } }
-		Block {
-			header: Header::new(),
-			state: state,
-			archive: Vec::new(),
-			archive_set: HashSet::new(),
-			uncles: Vec::new(),
-		}
-	}
 
 	/// Get a structure containing individual references to all public fields.
 	pub fn fields(&mut self) -> BlockRefMut {
 		BlockRefMut {
-			header: &self.header,
+			header: &self.base.header,
+			transactions: &self.base.transactions,
+			uncles: &self.base.uncles,
 			state: &mut self.state,
-			archive: &self.archive,
+			receipts: &self.receipts,
-			uncles: &self.uncles,
 		}
 	}
 }
 
-/// Trait for a object that is_a `Block`.
+/// Trait for a object that is_a `ExecutedBlock`.
 pub trait IsBlock {
 	/// Get the block associated with this object.
-	fn block(&self) -> &Block;
+	fn block(&self) -> &ExecutedBlock;
 
 	/// Get the header associated with this object's block.
-	fn header(&self) -> &Header { &self.block().header }
+	fn header(&self) -> &Header { &self.block().base.header }
 
 	/// Get the final state associated with this object's block.
 	fn state(&self) -> &State { &self.block().state }
 
 	/// Get all information on transactions in this block.
-	fn archive(&self) -> &Vec<Entry> { &self.block().archive }
+	fn transactions(&self) -> &Vec<Transaction> { &self.block().base.transactions }
+
+	/// Get all information on receipts in this block.
+	fn receipts(&self) -> &Vec<Receipt> { &self.block().receipts }
 
 	/// Get all uncles in this block.
-	fn uncles(&self) -> &Vec<Header> { &self.block().uncles }
+	fn uncles(&self) -> &Vec<Header> { &self.block().base.uncles }
 }
 
-impl IsBlock for Block {
+impl IsBlock for ExecutedBlock {
-	fn block(&self) -> &Block { self }
+	fn block(&self) -> &ExecutedBlock { self }
 }
 
 /// Block that is ready for transactions to be added.
@@ -86,7 +130,7 @@ impl IsBlock for Block {
 /// It's a bit like a Vec<Transaction>, eccept that whenever a transaction is pushed, we execute it and
 /// maintain the system `state()`. We also archive execution receipts in preparation for later block creation.
 pub struct OpenBlock<'x, 'y> {
-	block: Block,
+	block: ExecutedBlock,
 	engine: &'x Engine,
 	last_hashes: &'y LastHashes,
 }
@@ -104,7 +148,7 @@ pub struct ClosedBlock<'x, 'y> {
 ///
 /// The block's header has valid seal arguments. The block cannot be reversed into a ClosedBlock or OpenBlock.
 pub struct SealedBlock {
-	block: Block,
+	block: ExecutedBlock,
 	uncle_bytes: Bytes,
 }
 
@@ -112,42 +156,42 @@ impl<'x, 'y> OpenBlock<'x, 'y> {
 	/// Create a new OpenBlock ready for transaction pushing.
 	pub fn new<'a, 'b>(engine: &'a Engine, db: JournalDB, parent: &Header, last_hashes: &'b LastHashes, author: Address, extra_data: Bytes) -> OpenBlock<'a, 'b> {
 		let mut r = OpenBlock {
-			block: Block::new(State::from_existing(db, parent.state_root().clone(), engine.account_start_nonce())),
+			block: ExecutedBlock::new(State::from_existing(db, parent.state_root().clone(), engine.account_start_nonce())),
 			engine: engine,
 			last_hashes: last_hashes,
 		};
 
-		r.block.header.set_number(parent.number() + 1);
+		r.block.base.header.set_number(parent.number() + 1);
-		r.block.header.set_author(author);
+		r.block.base.header.set_author(author);
-		r.block.header.set_extra_data(extra_data);
+		r.block.base.header.set_extra_data(extra_data);
-		r.block.header.set_timestamp_now();
+		r.block.base.header.set_timestamp_now();
 
-		engine.populate_from_parent(&mut r.block.header, parent);
+		engine.populate_from_parent(&mut r.block.base.header, parent);
 		engine.on_new_block(&mut r.block);
 		r
 	}
 
 	/// Alter the author for the block.
-	pub fn set_author(&mut self, author: Address) { self.block.header.set_author(author); }
+	pub fn set_author(&mut self, author: Address) { self.block.base.header.set_author(author); }
 
 	/// Alter the timestamp of the block.
-	pub fn set_timestamp(&mut self, timestamp: u64) { self.block.header.set_timestamp(timestamp); }
+	pub fn set_timestamp(&mut self, timestamp: u64) { self.block.base.header.set_timestamp(timestamp); }
 
 	/// Alter the difficulty for the block.
-	pub fn set_difficulty(&mut self, a: U256) { self.block.header.set_difficulty(a); }
+	pub fn set_difficulty(&mut self, a: U256) { self.block.base.header.set_difficulty(a); }
 
 	/// Alter the gas limit for the block.
-	pub fn set_gas_limit(&mut self, a: U256) { self.block.header.set_gas_limit(a); }
+	pub fn set_gas_limit(&mut self, a: U256) { self.block.base.header.set_gas_limit(a); }
 
 	/// Alter the gas limit for the block.
-	pub fn set_gas_used(&mut self, a: U256) { self.block.header.set_gas_used(a); }
+	pub fn set_gas_used(&mut self, a: U256) { self.block.base.header.set_gas_used(a); }
 
 	/// Alter the extra_data for the block.
 	pub fn set_extra_data(&mut self, extra_data: Bytes) -> Result<(), BlockError> {
 		if extra_data.len() > self.engine.maximum_extra_data_size() {
 			Err(BlockError::ExtraDataOutOfBounds(OutOfBounds{min: None, max: Some(self.engine.maximum_extra_data_size()), found: extra_data.len()}))
 		} else {
-			self.block.header.set_extra_data(extra_data);
+			self.block.base.header.set_extra_data(extra_data);
 			Ok(())
 		}
 	}
@@ -157,12 +201,12 @@ impl<'x, 'y> OpenBlock<'x, 'y> {
 	/// NOTE Will check chain constraints and the uncle number but will NOT check
 	/// that the header itself is actually valid.
 	pub fn push_uncle(&mut self, valid_uncle_header: Header) -> Result<(), BlockError> {
-		if self.block.uncles.len() >= self.engine.maximum_uncle_count() {
+		if self.block.base.uncles.len() >= self.engine.maximum_uncle_count() {
-			return Err(BlockError::TooManyUncles(OutOfBounds{min: None, max: Some(self.engine.maximum_uncle_count()), found: self.block.uncles.len()}));
+			return Err(BlockError::TooManyUncles(OutOfBounds{min: None, max: Some(self.engine.maximum_uncle_count()), found: self.block.base.uncles.len()}));
 		}
 		// TODO: check number
 		// TODO: check not a direct ancestor (use last_hashes for that)
-		self.block.uncles.push(valid_uncle_header);
+		self.block.base.uncles.push(valid_uncle_header);
 		Ok(())
 	}
 
@@ -170,13 +214,13 @@ impl<'x, 'y> OpenBlock<'x, 'y> {
 	pub fn env_info(&self) -> EnvInfo {
 		// TODO: memoise.
 		EnvInfo {
-			number: self.block.header.number,
+			number: self.block.base.header.number,
-			author: self.block.header.author.clone(),
+			author: self.block.base.header.author.clone(),
-			timestamp: self.block.header.timestamp,
+			timestamp: self.block.base.header.timestamp,
-			difficulty: self.block.header.difficulty.clone(),
+			difficulty: self.block.base.header.difficulty.clone(),
 			last_hashes: self.last_hashes.clone(),		// TODO: should be a reference.
-			gas_used: self.block.archive.last().map_or(U256::zero(), |t| t.receipt.gas_used),
+			gas_used: self.block.receipts.last().map_or(U256::zero(), |r| r.gas_used),
-			gas_limit: self.block.header.gas_limit.clone(),
+			gas_limit: self.block.base.header.gas_limit.clone(),
 		}
 	}
 
@@ -188,9 +232,10 @@ impl<'x, 'y> OpenBlock<'x, 'y> {
 //		info!("env_info says gas_used={}", env_info.gas_used);
 		match self.block.state.apply(&env_info, self.engine, &t) {
 			Ok(receipt) => {
-				self.block.archive_set.insert(h.unwrap_or_else(||t.hash()));
+				self.block.transactions_set.insert(h.unwrap_or_else(||t.hash()));
-				self.block.archive.push(Entry { transaction: t, receipt: receipt });
+				self.block.base.transactions.push(t);
-				Ok(&self.block.archive.last().unwrap().receipt)
+				self.block.receipts.push(receipt);
+				Ok(&self.block.receipts.last().unwrap())
 			}
 			Err(x) => Err(From::from(x))
 		}
@@ -200,25 +245,25 @@ impl<'x, 'y> OpenBlock<'x, 'y> {
 	pub fn close(self) -> ClosedBlock<'x, 'y> {
 		let mut s = self;
 		s.engine.on_close_block(&mut s.block);
-		s.block.header.transactions_root = ordered_trie_root(s.block.archive.iter().map(|ref e| e.transaction.rlp_bytes()).collect());
+		s.block.base.header.transactions_root = ordered_trie_root(s.block.base.transactions.iter().map(|ref e| e.rlp_bytes()).collect());
-		let uncle_bytes = s.block.uncles.iter().fold(RlpStream::new_list(s.block.uncles.len()), |mut s, u| {s.append(&u.rlp(Seal::With)); s} ).out();
+		let uncle_bytes = s.block.base.uncles.iter().fold(RlpStream::new_list(s.block.base.uncles.len()), |mut s, u| {s.append(&u.rlp(Seal::With)); s} ).out();
-		s.block.header.uncles_hash = uncle_bytes.sha3();
+		s.block.base.header.uncles_hash = uncle_bytes.sha3();
-		s.block.header.state_root = s.block.state.root().clone();
+		s.block.base.header.state_root = s.block.state.root().clone();
-		s.block.header.receipts_root = ordered_trie_root(s.block.archive.iter().map(|ref e| e.receipt.rlp_bytes()).collect());
+		s.block.base.header.receipts_root = ordered_trie_root(s.block.receipts.iter().map(|ref r| r.rlp_bytes()).collect());
-		s.block.header.log_bloom = s.block.archive.iter().fold(LogBloom::zero(), |mut b, e| {b |= &e.receipt.log_bloom; b});
+		s.block.base.header.log_bloom = s.block.receipts.iter().fold(LogBloom::zero(), |mut b, r| {b |= &r.log_bloom; b});
-		s.block.header.gas_used = s.block.archive.last().map_or(U256::zero(), |t| t.receipt.gas_used);
+		s.block.base.header.gas_used = s.block.receipts.last().map_or(U256::zero(), |r| r.gas_used);
-		s.block.header.note_dirty();
+		s.block.base.header.note_dirty();
 
 		ClosedBlock::new(s, uncle_bytes)
 	}
 }
 
 impl<'x, 'y> IsBlock for OpenBlock<'x, 'y> {
-	fn block(&self) -> &Block { &self.block }
+	fn block(&self) -> &ExecutedBlock { &self.block }
 }
 
 impl<'x, 'y> IsBlock for ClosedBlock<'x, 'y> {
-	fn block(&self) -> &Block { &self.open_block.block }
+	fn block(&self) -> &ExecutedBlock { &self.open_block.block }
 }
 
 impl<'x, 'y> ClosedBlock<'x, 'y> {
@@ -240,7 +285,7 @@ impl<'x, 'y> ClosedBlock<'x, 'y> {
 		if seal.len() != s.open_block.engine.seal_fields() {
 			return Err(BlockError::InvalidSealArity(Mismatch{expected: s.open_block.engine.seal_fields(), found: seal.len()}));
 		}
-		s.open_block.block.header.set_seal(seal);
+		s.open_block.block.base.header.set_seal(seal);
 		Ok(SealedBlock { block: s.open_block.block, uncle_bytes: s.uncle_bytes })
 	}
 
@@ -255,9 +300,9 @@ impl SealedBlock {
 	/// Get the RLP-encoding of the block.
 	pub fn rlp_bytes(&self) -> Bytes {
 		let mut block_rlp = RlpStream::new_list(3);
-		self.block.header.stream_rlp(&mut block_rlp, Seal::With);
+		self.block.base.header.stream_rlp(&mut block_rlp, Seal::With);
-		block_rlp.append_list(self.block.archive.len());
+		block_rlp.append_list(self.block.receipts.len());
-		for e in &self.block.archive { e.transaction.rlp_append(&mut block_rlp); }
+		for t in &self.block.base.transactions { t.rlp_append(&mut block_rlp); }
 		block_rlp.append_raw(&self.uncle_bytes, 1);
 		block_rlp.out()
 	}
@@ -267,7 +312,7 @@ impl SealedBlock {
 }
 
 impl IsBlock for SealedBlock {
-	fn block(&self) -> &Block { &self.block }
+	fn block(&self) -> &ExecutedBlock { &self.block }
 }
 
 /// Enact the block given by block header, transactions and uncles

src/engine.rs

@@ -1,5 +1,5 @@
 use common::*;
-use block::Block;
+use block::ExecutedBlock;
 use spec::Spec;
 use evm::Schedule;
 use evm::Factory;
@@ -37,9 +37,9 @@ pub trait Engine : Sync + Send {
 	fn account_start_nonce(&self) -> U256 { decode(&self.spec().engine_params.get("accountStartNonce").unwrap()) }
 
 	/// Block transformation functions, before and after the transactions.
-	fn on_new_block(&self, _block: &mut Block) {}
+	fn on_new_block(&self, _block: &mut ExecutedBlock) {}
 	/// TODO [Gav Wood] Please document me
-	fn on_close_block(&self, _block: &mut Block) {}
+	fn on_close_block(&self, _block: &mut ExecutedBlock) {}
 
 	// TODO: consider including State in the params for verification functions.
 	/// Phase 1 quick block verification. Only does checks that are cheap. `block` (the header's full block) 

src/ethereum/ethash.rs

@@ -83,7 +83,7 @@ impl Engine for Ethash {
 
 	/// Apply the block reward on finalisation of the block.
 	/// This assumes that all uncles are valid uncles (i.e. of at least one generation before the current).
-	fn on_close_block(&self, block: &mut Block) {
+	fn on_close_block(&self, block: &mut ExecutedBlock) {
 		let reward = self.spec().engine_params.get("blockReward").map_or(U256::from(0u64), |a| decode(&a));
 		let fields = block.fields();
 

src/header.rs

@@ -11,7 +11,7 @@ pub type BlockNumber = u64;
 /// which is non-specific.
 ///
 /// Doesn't do all that much on its own.
-#[derive(Debug, Clone)]
+#[derive(Default, Debug, Clone)]
 pub struct Header {
 	// TODO: make all private.
 	/// TODO [Gav Wood] Please document me

src/log_entry.rs

@@ -2,7 +2,7 @@ use util::*;
 use basic_types::LogBloom;
 
 /// A single log's entry.
-#[derive(Debug,PartialEq,Eq)]
+#[derive(Default, Debug, Clone, PartialEq, Eq)]
 pub struct LogEntry {
 	/// TODO [Gav Wood] Please document me
 	pub address: Address,

src/receipt.rs

@@ -3,7 +3,7 @@ use basic_types::LogBloom;
 use log_entry::LogEntry;
 
 /// Information describing execution of a transaction.
-#[derive(Debug)]
+#[derive(Default, Debug, Clone)]
 pub struct Receipt {
 	/// TODO [Gav Wood] Please document me
 	pub state_root: H256,

src/tests/chain.rs

@@ -2,6 +2,7 @@ use std::env;
 use super::test_common::*;
 use client::{BlockChainClient,Client};
 use pod_state::*;
+use block::Block;
 use ethereum;
 
 fn do_json_test(json_data: &[u8]) -> Vec<String> {
@@ -31,20 +32,7 @@ fn do_json_test(json_data: &[u8]) -> Vec<String> {
 			dir.push(H32::random().hex());
 			{
 				let client = Client::new(spec, &dir, IoChannel::disconnected()).unwrap();
-				for b in blocks.into_iter() {
+				for b in blocks.into_iter().filter(|ref b| Block::is_good(b)) {
-					{
-						let urlp = UntrustedRlp::new(&b);
-						if !urlp.is_list() || urlp.item_count() != 3 || urlp.size() != b.len() { continue; }
-						if urlp.val_at::<Header>(0).is_err() { continue; }
-						if !urlp.at(1).unwrap().is_list() { continue; }
-						if urlp.at(1).unwrap().iter().find(|i| i.as_val::<Transaction>().is_err()).is_some() {
-							continue;
-						}
-						if !urlp.at(2).unwrap().is_list() { continue; }
-						if urlp.at(2).unwrap().iter().find(|i| i.as_val::<Header>().is_err()).is_some() {
-							continue;
-						}
-					}
 					client.import_block(b).unwrap();
 				}
 				client.flush_queue();

src/transaction.rs

@@ -12,9 +12,13 @@ pub enum Action {
 	Call(Address),
 }
 
+impl Default for Action {
+	fn default() -> Action { Action::Create }
+}
+
 /// A set of information describing an externally-originating message call
 /// or contract creation operation.
-#[derive(Debug,Clone)]
+#[derive(Default, Debug, Clone)]
 pub struct Transaction {
 	/// TODO [debris] Please document me
 	pub nonce: U256,

util/src/bytes.rs

@@ -273,7 +273,9 @@ pub enum FromBytesError {
 	/// TODO [debris] Please document me
 	DataIsTooShort,
 	/// TODO [debris] Please document me
-	DataIsTooLong
+	DataIsTooLong,
+	/// Integer-representation is non-canonically prefixed with zero byte(s).
+	ZeroPrefixedInt,
 }
 
 impl StdError for FromBytesError {
@@ -310,6 +312,9 @@ macro_rules! impl_uint_from_bytes {
 				match bytes.len() {
 					0 => Ok(0),
 					l if l <= mem::size_of::<$to>() => {
+						if bytes[0] == 0 {
+							return Err(FromBytesError::ZeroPrefixedInt)
+						}
 						let mut res = 0 as $to;
 						for i in 0..l {
 							let shift = (l - 1 - i) * 8;
@@ -344,7 +349,9 @@ macro_rules! impl_uint_from_bytes {
 	($name: ident) => {
 		impl FromBytes for $name {
 			fn from_bytes(bytes: &[u8]) -> FromBytesResult<$name> {
-				if bytes.len() <= $name::SIZE {
+				if !bytes.is_empty() && bytes[0] == 0 {
+					Err(FromBytesError::ZeroPrefixedInt)
+				} else if bytes.len() <= $name::SIZE {
 					Ok($name::from(bytes))
 				} else {
 					Err(FromBytesError::DataIsTooLong)

util/src/rlp/rlperrors.rs

@@ -7,6 +7,8 @@ use bytes::FromBytesError;
 pub enum DecoderError {
 	/// TODO [debris] Please document me
 	FromBytesError(FromBytesError),
+	/// Given data has additional bytes at the end of the valid RLP fragment.
+	RlpIsTooBig,
 	/// TODO [debris] Please document me
 	RlpIsTooShort,
 	/// TODO [debris] Please document me

util/src/rlp/untrusted_rlp.rs

@@ -46,6 +46,9 @@ impl PayloadInfo {
 			value_len: value_len,
 		}
 	}
+
+	/// Total size of the RLP.
+	pub fn total(&self) -> usize { self.header_len + self.value_len }
 }
 
 /// Data-oriented view onto rlp-slice.