get rid of hidden mutability of Spec (#10904)

* get rid of hidden mutabilityof Spec

* small cleanups

* fixed SpecHardcodedSync printing

ethcore/pod/src/state.rs

@@ -31,7 +31,9 @@ pub struct PodState(BTreeMap<Address, PodAccount>);
 
 impl PodState {
 	/// Get the underlying map.
-	pub fn get(&self) -> &BTreeMap<Address, PodAccount> { &self.0 }
+	pub fn get(&self) -> &BTreeMap<Address, PodAccount> {
+		&self.0
+	}
 
 	/// Get the root hash of the trie of the RLP of this.
 	pub fn root(&self) -> H256 {
@@ -39,7 +41,9 @@ impl PodState {
 	}
 
 	/// Drain object to get the underlying map.
-	pub fn drain(self) -> BTreeMap<Address, PodAccount> { self.0 }
+	pub fn drain(self) -> BTreeMap<Address, PodAccount> {
+		self.0
+	}
 }
 
 impl From<ethjson::blockchain::State> for PodState {

ethcore/src/json_tests/chain.rs

@@ -77,7 +77,6 @@ pub fn json_chain_test<H: FnMut(&str, HookType)>(json_data: &[u8], start_stop_ho
 				let state = From::from(blockchain.pre_state.clone());
 				spec.set_genesis_state(state).expect("Failed to overwrite genesis state");
 				spec.overwrite_genesis_params(genesis);
-				assert!(spec.is_state_root_valid());
 				spec
 			};
 

ethcore/src/spec/spec.rs

@@ -16,18 +16,19 @@
 
 //! Parameters for a block chain.
 
-use std::collections::BTreeMap;
+use std::{
-use std::io::Read;
+	collections::BTreeMap,
-use std::path::Path;
+	fmt,
-use std::sync::Arc;
+	io::Read,
+	path::Path,
+	sync::Arc,
+};
 
 use bytes::Bytes;
 use ethereum_types::{H256, Bloom, U256, Address};
 use ethjson;
 use hash::{KECCAK_NULL_RLP, keccak};
-use parking_lot::RwLock;
 use rlp::{Rlp, RlpStream};
-use rustc_hex::{FromHex, ToHex};
 use types::{
 	BlockNumber,
 	header::Header,
@@ -96,6 +97,93 @@ impl<'a, T: AsRef<Path>> From<&'a T> for SpecParams<'a> {
 	}
 }
 
+/// given a pre-constructor state, run all the given constructors and produce a new state and
+/// state root.
+fn run_constructors<T: Backend>(
+	genesis_state: &PodState,
+	constructors: &[(Address, Bytes)],
+	engine: &Engine,
+	author: Address,
+	timestamp: u64,
+	difficulty: U256,
+	factories: &Factories,
+	mut db: T
+) -> Result<(H256, T), Error> {
+	let mut root = KECCAK_NULL_RLP;
+
+	// basic accounts in spec.
+	{
+		let mut t = factories.trie.create(db.as_hash_db_mut(), &mut root);
+
+		for (address, account) in genesis_state.get().iter() {
+			t.insert(address.as_bytes(), &account.rlp())?;
+		}
+	}
+
+	for (address, account) in genesis_state.get().iter() {
+		db.note_non_null_account(address);
+		account.insert_additional(
+			&mut *factories.accountdb.create(
+				db.as_hash_db_mut(),
+				keccak(address),
+			),
+			&factories.trie,
+		);
+	}
+
+	let start_nonce = engine.account_start_nonce(0);
+
+	let mut state = State::from_existing(db, root, start_nonce, factories.clone())?;
+
+	// Execute contract constructors.
+	let env_info = EnvInfo {
+		number: 0,
+		author,
+		timestamp,
+		difficulty,
+		last_hashes: Default::default(),
+		gas_used: U256::zero(),
+		gas_limit: U256::max_value(),
+	};
+
+	let from = Address::zero();
+	for &(ref address, ref constructor) in constructors.iter() {
+		trace!(target: "spec", "run_constructors: Creating a contract at {}.", address);
+		trace!(target: "spec", "  .. root before = {}", state.root());
+		let params = ActionParams {
+			code_address: address.clone(),
+			code_hash: Some(keccak(constructor)),
+			code_version: U256::zero(),
+			address: address.clone(),
+			sender: from.clone(),
+			origin: from.clone(),
+			gas: U256::max_value(),
+			gas_price: Default::default(),
+			value: ActionValue::Transfer(Default::default()),
+			code: Some(Arc::new(constructor.clone())),
+			data: None,
+			call_type: CallType::None,
+			params_type: ParamsType::Embedded,
+		};
+
+		let mut substate = Substate::new();
+
+		{
+			let machine = engine.machine();
+			let schedule = machine.schedule(env_info.number);
+			let mut exec = Executive::new(&mut state, &env_info, &machine, &schedule);
+			// failing create is not a bug
+			if let Err(e) = exec.create(params, &mut substate, &mut NoopTracer, &mut NoopVMTracer) {
+				warn!(target: "spec", "Genesis constructor execution at {} failed: {}.", address, e);
+			}
+		}
+
+		let _ = state.commit()?;
+	}
+
+	Ok(state.drop())
+}
+
 /// Parameters for a block chain; includes both those intrinsic to the design of the
 /// chain and those to be interpreted by the active chain engine.
 pub struct Spec {
@@ -137,7 +225,7 @@ pub struct Spec {
 	constructors: Vec<(Address, Bytes)>,
 
 	/// May be prepopulated if we know this in advance.
-	state_root_memo: RwLock<H256>,
+	state_root_memo: H256,
 
 	/// Genesis state as plain old data.
 	genesis_state: PodState,
@@ -163,13 +251,14 @@ impl Clone for Spec {
 			seal_rlp: self.seal_rlp.clone(),
 			hardcoded_sync: self.hardcoded_sync.clone(),
 			constructors: self.constructors.clone(),
-			state_root_memo: RwLock::new(*self.state_root_memo.read()),
+			state_root_memo: self.state_root_memo,
 			genesis_state: self.genesis_state.clone(),
 		}
 	}
 }
 
 /// Part of `Spec`. Describes the hardcoded synchronization parameters.
+#[derive(Debug, Clone)]
 pub struct SpecHardcodedSync {
 	/// Header of the block to jump to for hardcoded sync, and total difficulty.
 	pub header: encoded::Header,
@@ -180,34 +269,26 @@ pub struct SpecHardcodedSync {
 	pub chts: Vec<H256>,
 }
 
-impl SpecHardcodedSync {
+impl From<ethjson::spec::HardcodedSync> for SpecHardcodedSync {
-	/// Turns this specifications back into JSON. Useful for pretty printing.
+	fn from(sync: ethjson::spec::HardcodedSync) -> Self {
-	pub fn to_json(self) -> ethjson::spec::HardcodedSync {
-		self.into()
-	}
-}
-
-#[cfg(test)]
-impl Clone for SpecHardcodedSync {
-	fn clone(&self) -> SpecHardcodedSync {
 		SpecHardcodedSync {
-			header: self.header.clone(),
+			header: encoded::Header::new(sync.header.into()),
-			total_difficulty: self.total_difficulty.clone(),
+			total_difficulty: sync.total_difficulty.into(),
-			chts: self.chts.clone(),
-		}
-	}
-}
-
-impl From<SpecHardcodedSync> for ethjson::spec::HardcodedSync {
-	fn from(sync: SpecHardcodedSync) -> ethjson::spec::HardcodedSync {
-		ethjson::spec::HardcodedSync {
-			header: sync.header.into_inner().to_hex(),
-			total_difficulty: ethjson::uint::Uint(sync.total_difficulty),
 			chts: sync.chts.into_iter().map(Into::into).collect(),
 		}
 	}
 }
 
+impl fmt::Display for SpecHardcodedSync {
+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		writeln!(f, "{{")?;
+		writeln!(f, r#"header": "{:?},"#, self.header)?;
+		writeln!(f, r#"total_difficulty": "{:?},"#, self.total_difficulty)?;
+		writeln!(f, r#"chts": {:#?}"#, self.chts.iter().map(|x| format!(r#"{}"#, x)).collect::<Vec<_>>())?;
+		writeln!(f, "}}")
+	}
+}
+
 fn load_machine_from(s: ethjson::spec::Spec) -> Machine {
 	let builtins = s.accounts.builtins().into_iter().map(|p| (p.0.into(), From::from(p.1))).collect();
 	let params = CommonParams::from(s.params);
@@ -226,59 +307,51 @@ fn load_from(spec_params: SpecParams, s: ethjson::spec::Spec) -> Result<Spec, Er
 	let GenericSeal(seal_rlp) = g.seal.into();
 	let params = CommonParams::from(s.params);
 
-	let hardcoded_sync = if let Some(ref hs) = s.hardcoded_sync {
+	let hardcoded_sync = s.hardcoded_sync.map(Into::into);
-		if let Ok(header) = hs.header.from_hex() {
-			Some(SpecHardcodedSync {
-				header: encoded::Header::new(header),
-				total_difficulty: hs.total_difficulty.into(),
-				chts: s.hardcoded_sync
-					.as_ref()
-					.map(|s| s.chts.iter().map(|c| c.clone().into()).collect())
-					.unwrap_or_default()
-			})
-		} else {
-			None
-		}
-	} else {
-		None
-	};
 
-	let mut s = Spec {
+	let engine = Spec::engine(spec_params, s.engine, params, builtins);
+	let author = g.author;
+	let timestamp = g.timestamp;
+	let difficulty = g.difficulty;
+	let constructors: Vec<_> = s.accounts
+		.constructors()
+		.into_iter()
+		.map(|(a, c)| (a.into(), c.into()))
+		.collect();
+	let genesis_state: PodState = s.accounts.into();
+
+	let (state_root_memo, _) = run_constructors(
+		&genesis_state,
+		&constructors,
+		&*engine,
+		author,
+		timestamp,
+		difficulty,
+		&Default::default(),
+		BasicBackend(journaldb::new_memory_db()),
+	)?;
+
+	let s = Spec {
+		engine,
 		name: s.name.clone().into(),
-		engine: Spec::engine(spec_params, s.engine, params, builtins),
 		data_dir: s.data_dir.unwrap_or(s.name).into(),
 		nodes: s.nodes.unwrap_or_else(Vec::new),
 		parent_hash: g.parent_hash,
 		transactions_root: g.transactions_root,
 		receipts_root: g.receipts_root,
-		author: g.author,
+		author,
-		difficulty: g.difficulty,
+		difficulty,
 		gas_limit: g.gas_limit,
 		gas_used: g.gas_used,
-		timestamp: g.timestamp,
+		timestamp,
 		extra_data: g.extra_data,
 		seal_rlp,
 		hardcoded_sync,
-		constructors: s.accounts
+		constructors,
-			.constructors()
+		genesis_state,
-			.into_iter()
+		state_root_memo,
-			.map(|(a, c)| (a.into(), c.into()))
-			.collect(),
-		state_root_memo: RwLock::new(Default::default()), // will be overwritten right after.
-		genesis_state: s.accounts.into(),
 	};
 
-	// use memoized state root if provided.
-	match g.state_root {
-		Some(root) => *s.state_root_memo.get_mut() = root,
-		None => {
-			let _ = s.run_constructors(
-				&Default::default(),
-				BasicBackend(journaldb::new_memory_db()),
-			)?;
-		}
-	}
-
 	Ok(s)
 }
 
@@ -337,95 +410,9 @@ impl Spec {
 		}
 	}
 
-	// given a pre-constructor state, run all the given constructors and produce a new state and
-	// state root.
-	fn run_constructors<T: Backend>(&self, factories: &Factories, mut db: T) -> Result<T, Error> {
-		let mut root = KECCAK_NULL_RLP;
-
-		// basic accounts in spec.
-		{
-			let mut t = factories.trie.create(db.as_hash_db_mut(), &mut root);
-
-			for (address, account) in self.genesis_state.get().iter() {
-				t.insert(address.as_bytes(), &account.rlp())?;
-			}
-		}
-
-		for (address, account) in self.genesis_state.get().iter() {
-			db.note_non_null_account(address);
-			account.insert_additional(
-				&mut *factories.accountdb.create(
-					db.as_hash_db_mut(),
-					keccak(address),
-				),
-				&factories.trie,
-			);
-		}
-
-		let start_nonce = self.engine.account_start_nonce(0);
-
-		let (root, db) = {
-			let mut state = State::from_existing(db, root, start_nonce, factories.clone())?;
-
-			// Execute contract constructors.
-			let env_info = EnvInfo {
-				number: 0,
-				author: self.author,
-				timestamp: self.timestamp,
-				difficulty: self.difficulty,
-				last_hashes: Default::default(),
-				gas_used: U256::zero(),
-				gas_limit: U256::max_value(),
-			};
-
-			let from = Address::zero();
-			for &(ref address, ref constructor) in self.constructors.iter() {
-				trace!(target: "spec", "run_constructors: Creating a contract at {}.", address);
-				trace!(target: "spec", "  .. root before = {}", state.root());
-				let params = ActionParams {
-					code_address: address.clone(),
-					code_hash: Some(keccak(constructor)),
-					code_version: U256::zero(),
-					address: address.clone(),
-					sender: from.clone(),
-					origin: from.clone(),
-					gas: U256::max_value(),
-					gas_price: Default::default(),
-					value: ActionValue::Transfer(Default::default()),
-					code: Some(Arc::new(constructor.clone())),
-					data: None,
-					call_type: CallType::None,
-					params_type: ParamsType::Embedded,
-				};
-
-				let mut substate = Substate::new();
-
-				{
-					let machine = self.engine.machine();
-					let schedule = machine.schedule(env_info.number);
-					let mut exec = Executive::new(&mut state, &env_info, &machine, &schedule);
-					if let Err(e) = exec.create(params, &mut substate, &mut NoopTracer, &mut NoopVMTracer) {
-						warn!(target: "spec", "Genesis constructor execution at {} failed: {}.", address, e);
-					}
-				}
-
-				if let Err(e) = state.commit() {
-					warn!(target: "spec", "Genesis constructor trie commit at {} failed: {}.", address, e);
-				}
-
-				trace!(target: "spec", "  .. root after = {}", state.root());
-			}
-
-			state.drop()
-		};
-
-		*self.state_root_memo.write() = root;
-		Ok(db)
-	}
-
 	/// Return the state root for the genesis state, memoising accordingly.
 	pub fn state_root(&self) -> H256 {
-		self.state_root_memo.read().clone()
+		self.state_root_memo
 	}
 
 	/// Get common blockchain parameters.
@@ -511,11 +498,18 @@ impl Spec {
 	/// Alter the value of the genesis state.
 	pub fn set_genesis_state(&mut self, s: PodState) -> Result<(), Error> {
 		self.genesis_state = s;
-		let _ = self.run_constructors(
+		let (root, _) = run_constructors(
+			&self.genesis_state,
+			&self.constructors,
+			&*self.engine,
+			self.author,
+			self.timestamp,
+			self.difficulty,
 			&Default::default(),
 			BasicBackend(journaldb::new_memory_db()),
 		)?;
 
+		self.state_root_memo = root;
 		Ok(())
 	}
 
@@ -524,14 +518,6 @@ impl Spec {
 		&self.genesis_state
 	}
 
-	/// Returns `false` if the memoized state root is invalid. `true` otherwise.
-	pub fn is_state_root_valid(&self) -> bool {
-		// TODO: get rid of this function and ensure state root always is valid.
-		// we're mostly there, but `self.genesis_state.root()` doesn't encompass
-		// post-constructor state.
-		*self.state_root_memo.read() == self.genesis_state.root()
-	}
-
 	/// Ensure that the given state DB has the trie nodes in for the genesis state.
 	pub fn ensure_db_good<T: Backend>(&self, db: T, factories: &Factories) -> Result<T, Error> {
 		if db.as_hash_db().contains(&self.state_root(), hash_db::EMPTY_PREFIX) {
@@ -540,7 +526,18 @@ impl Spec {
 
 		// TODO: could optimize so we don't re-run, but `ensure_db_good` is barely ever
 		// called anyway.
-		let db = self.run_constructors(factories, db)?;
+		let (root, db) = run_constructors(
+			&self.genesis_state,
+			&self.constructors,
+			&*self.engine,
+			self.author,
+			self.timestamp,
+			self.difficulty,
+			factories,
+			db
+		)?;
+
+		assert_eq!(root, self.state_root(), "Spec's state root has not been precomputed correctly.");
 		Ok(db)
 	}
 

json/src/bytes.rs

@@ -34,9 +34,15 @@ impl Bytes {
 	}
 }
 
-impl Into<Vec<u8>> for Bytes {
+impl From<Bytes> for Vec<u8> {
-	fn into(self) -> Vec<u8> {
+	fn from(bytes: Bytes) -> Self {
-		self.0
+		bytes.0
+	}
+}
+
+impl From<Vec<u8>> for Bytes {
+	fn from(bytes: Vec<u8>) -> Self {
+		Bytes(bytes)
 	}
 }
 

json/src/spec/hardcoded_sync.rs

@@ -18,14 +18,15 @@
 
 use hash::H256;
 use uint::Uint;
+use bytes::Bytes;
 
 /// Spec hardcoded sync.
-#[derive(Debug, PartialEq, Serialize, Deserialize)]
+#[derive(Debug, PartialEq, Deserialize)]
 #[serde(deny_unknown_fields)]
 #[serde(rename_all = "camelCase")]
 pub struct HardcodedSync {
 	/// Hexadecimal of the RLP encoding of the header of the block to start synchronization from.
-	pub header: String,
+	pub header: Bytes,
 	/// Total difficulty including the block of `header`.
 	pub total_difficulty: Uint,
 	/// Ordered trie roots of blocks before and including `header`.
@@ -54,7 +55,7 @@ mod tests {
 		}"#;
 		let deserialized: HardcodedSync = serde_json::from_str(s).unwrap();
 		assert_eq!(deserialized, HardcodedSync {
-			header: String::from("f901f9a0d405da4e66f1445d455195229624e133f5baafe72b5cf7b3c36c12c8146e98b7a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347948888f1f195afa192cfee860698584c030f4c9db1a05fb2b4bfdef7b314451cb138a534d225c922fc0e5fbe25e451142732c3e25c25a088d2ec6b9860aae1a2c3b299f72b6a5d70d7f7ba4722c78f2c49ba96273c2158a007c6fdfa8eea7e86b81f5b0fc0f78f90cc19f4aa60d323151e0cac660199e9a1b90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302008003832fefba82524d84568e932a80a0a0349d8c3df71f1a48a9df7d03fd5f14aeee7d91332c009ecaff0a71ead405bd88ab4e252a7e8c2a23"),
+			header: "f901f9a0d405da4e66f1445d455195229624e133f5baafe72b5cf7b3c36c12c8146e98b7a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347948888f1f195afa192cfee860698584c030f4c9db1a05fb2b4bfdef7b314451cb138a534d225c922fc0e5fbe25e451142732c3e25c25a088d2ec6b9860aae1a2c3b299f72b6a5d70d7f7ba4722c78f2c49ba96273c2158a007c6fdfa8eea7e86b81f5b0fc0f78f90cc19f4aa60d323151e0cac660199e9a1b90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302008003832fefba82524d84568e932a80a0a0349d8c3df71f1a48a9df7d03fd5f14aeee7d91332c009ecaff0a71ead405bd88ab4e252a7e8c2a23".parse().unwrap(),
 			total_difficulty: Uint(U256::from(0x400000000u64)),
 			chts: vec![
 				H256(Eth256::from_str("11bbe8db4e347b4e8c937c1c8370e4b5ed33adb3db69cbdb7a38e1e50b1b82fa").unwrap()),

parity/export_hardcoded_sync.rs

@@ -96,7 +96,7 @@ pub fn execute(cmd: ExportHsyncCmd) -> Result<String, String> {
 	let hs = service.client().read_hardcoded_sync()
 		.map_err(|e| format!("Error reading hardcoded sync: {}", e))?;
 	if let Some(hs) = hs {
-		Ok(::serde_json::to_string_pretty(&hs.to_json()).expect("generated JSON is always valid"))
+		Ok(format!("{}", hs))
 	} else {
 		Err("Error: cannot generate hardcoded sync because the database is empty.".into())
 	}

rpc/src/v1/tests/eth.rs

@@ -68,7 +68,6 @@ fn make_spec(chain: &BlockChain) -> Spec {
 	let state = chain.pre_state.clone().into();
 	spec.set_genesis_state(state).expect("unable to set genesis state");
 	spec.overwrite_genesis_params(genesis);
-	assert!(spec.is_state_root_valid());
 	spec
 }