Updater verification (#8787)

* getting started

* refactor main

* unwrap_or -> unwrap_or_else

* force parity to lower version number to trigger update

* Fix typos

* formating

* some minor refactoring

* enable lints and fix some warnings

* make it compile

* minor tweaks to make it work

* address review comments

* Rename exe to exe_path and minor import changes

* updater: unreleased -> unknown

* Add `debug` configuration to force parity-updater

* Introduce a new feature `test-updater` in order conditionally hardcode
the version number in parity in order to force an update
* This should only be used for debug/dev purposes

* nits

* Pulled latest submodule of `wasm-tests`

Cargo.toml

@@ -106,6 +106,10 @@ deadlock_detection = ["parking_lot/deadlock_detection"]
 # `valgrind --tool=massif /path/to/parity <parity params>`
 # and `massif-visualizer` for visualization
 memory_profiling = []
+# hardcode version number 1.3.7 of parity to force an update
+# in order to manually test that parity fall-over to the local version
+# in case of invalid or deprecated command line arguments are entered
+test-updater = ["parity-updater/test-updater"]
 
 [lib]
 path = "parity/lib.rs"

ethcore/src/client/client.rs

@@ -2380,7 +2380,7 @@ mod tests {
 		let (new_hash, new_block) = get_good_dummy_block_hash();
 
 		let go = {
-			// Separate thread uncommited transaction
+			// Separate thread uncommitted transaction
 			let go = Arc::new(AtomicBool::new(false));
 			let go_thread = go.clone();
 			let another_client = client.clone();

parity/configuration.rs

@@ -103,7 +103,7 @@ impl Configuration {
 	/// # Example
 	///
 	/// ```
-	/// let _cfg = parity::Configuration::parse_cli(&["--light", "--chain", "koven"]).unwrap();
+	/// let _cfg = parity::Configuration::parse_cli(&["--light", "--chain", "kovan"]).unwrap();
 	/// ```
 	pub fn parse_cli<S: AsRef<str>>(command: &[S]) -> Result<Self, ArgsError> {
 		let config = Configuration {

parity/main.rs

@@ -18,54 +18,90 @@
 
 #![warn(missing_docs)]
 
-extern crate parity;
-
 extern crate ctrlc;
 extern crate dir;
 extern crate fdlimit;
 #[macro_use]
 extern crate log;
 extern crate panic_hook;
+extern crate parity;
 extern crate parking_lot;
 
 #[cfg(windows)] extern crate winapi;
 
-use std::{process, env};
+use std::ffi::OsString;
-use std::sync::atomic::{AtomicBool, Ordering};
-use std::io::{self as stdio, Read, Write};
 use std::fs::{remove_file, metadata, File, create_dir_all};
+use std::io::{self as stdio, Read, Write};
 use std::path::PathBuf;
 use std::sync::Arc;
+use std::sync::atomic::{AtomicBool, Ordering};
+use std::{process, env};
+
 use ctrlc::CtrlC;
 use dir::default_hypervisor_path;
 use fdlimit::raise_fd_limit;
 use parity::{start, ExecutionAction};
 use parking_lot::{Condvar, Mutex};
 
-fn updates_path(name: &str) -> PathBuf {
+const PLEASE_RESTART_EXIT_CODE: i32 = 69;
-	let mut dest = PathBuf::from(default_hypervisor_path());
+const PARITY_EXECUTABLE_NAME: &str = "parity";
+
+#[derive(Debug)]
+enum Error {
+	BinaryNotFound,
+	ExitCode(i32),
+	Restart,
+	Unknown
+}
+
+fn update_path(name: &str) -> PathBuf {
+	let mut dest = default_hypervisor_path();
 	dest.push(name);
 	dest
 }
 
-fn latest_exe_path() -> Option<PathBuf> {
+fn latest_exe_path() -> Result<PathBuf, Error> {
-	File::open(updates_path("latest")).ok()
+	File::open(update_path("latest")).and_then(|mut f| { 
-		.and_then(|mut f| { let mut exe = String::new(); f.read_to_string(&mut exe).ok().map(|_| updates_path(&exe)) })
+			let mut exe_path = String::new(); 
+			trace!(target: "updater", "latest binary path: {:?}", f); 
+			f.read_to_string(&mut exe_path).map(|_| update_path(&exe_path))
+	})
+	.or(Err(Error::BinaryNotFound))
 }
 
-fn set_spec_name_override(spec_name: String) {
+fn latest_binary_is_newer(current_binary: &Option<PathBuf>, latest_binary: &Option<PathBuf>) -> bool {
+	match (
+		current_binary
+			.as_ref()
+			.and_then(|p| metadata(p.as_path()).ok())
+			.and_then(|m| m.modified().ok()),
+		latest_binary
+			.as_ref()
+			.and_then(|p| metadata(p.as_path()).ok())
+			.and_then(|m| m.modified().ok())
+	) {
+			(Some(latest_exe_time), Some(this_exe_time)) if latest_exe_time > this_exe_time => true,
+			_ => false,
+	}
+}
+
+fn set_spec_name_override(spec_name: &str) {
 	if let Err(e) = create_dir_all(default_hypervisor_path())
-		.and_then(|_| File::create(updates_path("spec_name_override"))
+		.and_then(|_| File::create(update_path("spec_name_override"))
 		.and_then(|mut f| f.write_all(spec_name.as_bytes())))
 	{
-		warn!("Couldn't override chain spec: {} at {:?}", e, updates_path("spec_name_override"));
+		warn!("Couldn't override chain spec: {} at {:?}", e, update_path("spec_name_override"));
 	}
 }
 
 fn take_spec_name_override() -> Option<String> {
-	let p = updates_path("spec_name_override");
+	let p = update_path("spec_name_override");
-	let r = File::open(p.clone()).ok()
+	let r = File::open(p.clone())
-		.and_then(|mut f| { let mut spec_name = String::new(); f.read_to_string(&mut spec_name).ok().map(|_| spec_name) });
+		.ok()
+		.and_then(|mut f| { 
+			let mut spec_name = String::new(); 
+			f.read_to_string(&mut spec_name).ok().map(|_| spec_name) 
+		});
 	let _ = remove_file(p);
 	r
 }
@@ -96,23 +132,35 @@ fn global_init() {
 #[cfg(not(windows))]
 fn global_cleanup() {}
 
-// Starts ~/.parity-updates/parity and returns the code it exits with.
+// Starts parity binary installed via `parity-updater` and returns the code it exits with.
-fn run_parity() -> Option<i32> {
+fn run_parity() -> Result<(), Error> {
 	global_init();
-	use ::std::ffi::OsString;
+
 	let prefix = vec![OsString::from("--can-restart"), OsString::from("--force-direct")];
-	let res = latest_exe_path().and_then(|exe| process::Command::new(exe)
+	
+	let res: Result<(), Error> = latest_exe_path()
+		.and_then(|exe| process::Command::new(exe)
 		.args(&(env::args_os().skip(1).chain(prefix.into_iter()).collect::<Vec<_>>()))
 		.status()
-		.map(|es| es.code().unwrap_or(128))
 		.ok()
+		.map_or(Err(Error::Unknown), |es| {
+			match es.code() {
+				// Process success
+				Some(0) => Ok(()),
+				// Please restart
+				Some(PLEASE_RESTART_EXIT_CODE) => Err(Error::Restart),
+				// Process error code `c`
+				Some(c) => Err(Error::ExitCode(c)),
+				// Unknown error, couldn't determine error code
+				_ => Err(Error::Unknown),
+			}
+		})
 	);	
+
 	global_cleanup();
 	res
 }
 
-const PLEASE_RESTART_EXIT_CODE: i32 = 69;
-
 #[derive(Debug)]
 /// Status used to exit or restart the program.
 struct ExitStatus {
@@ -126,7 +174,7 @@ struct ExitStatus {
 	spec_name_override: Option<String>,
 }
 
-// Run our version of parity.
+// Run `locally installed version` of parity (i.e, not installed via `parity-updater`)
 // Returns the exit error code.
 fn main_direct(force_can_restart: bool) -> i32 {
 	global_init();
@@ -244,7 +292,7 @@ fn main_direct(force_can_restart: bool) -> i32 {
 
 				if lock.should_restart {
 					if let Some(ref spec_name) = lock.spec_name_override {
-						set_spec_name_override(spec_name.clone());
+						set_spec_name_override(&spec_name.clone());
 					}
 					PLEASE_RESTART_EXIT_CODE
 				} else {
@@ -281,46 +329,80 @@ macro_rules! trace_main {
 fn main() {
 	panic_hook::set_abort();
 
-	// assuming the user is not running with `--force-direct`, then:
+	// the user has specified to run its originally installed binary (not via `parity-updater`)
-	// if argv[0] == "parity" and this executable != ~/.parity-updates/parity, run that instead.
 	let force_direct = std::env::args().any(|arg| arg == "--force-direct");
-	let exe = std::env::current_exe().ok();
+	
-	let development = exe.as_ref().and_then(|p| p.parent().and_then(|p| p.parent()).and_then(|p| p.file_name()).map(|n| n == "target")).unwrap_or(false);
+	// absolute path to the current `binary`
-	let same_name = exe.as_ref().map(|p| p.file_stem().map_or(false, |s| s == "parity") && p.extension().map_or(true, |x| x == "exe")).unwrap_or(false);
+	let exe_path = std::env::current_exe().ok();
-	trace_main!("Starting up {} (force-direct: {}, development: {}, same-name: {})", std::env::current_exe().map(|x| format!("{}", x.display())).unwrap_or("<unknown>".to_owned()), force_direct, development, same_name);
+	
+	// the binary is named `target/xx/yy`
+	let development = exe_path
+		.as_ref()
+		.and_then(|p| {
+			p.parent()
+				.and_then(|p| p.parent())
+				.and_then(|p| p.file_name())
+				.map(|n| n == "target")
+		})
+		.unwrap_or(false);
+	
+	// the binary is named `parity`
+	let same_name = exe_path
+		.as_ref()
+		.map_or(false, |p| { 
+			p.file_stem().map_or(false, |n| n == PARITY_EXECUTABLE_NAME) 
+		});
+
+	trace_main!("Starting up {} (force-direct: {}, development: {}, same-name: {})", 
+				std::env::current_exe().ok().map_or_else(|| "<unknown>".into(), |x| format!("{}", x.display())), 
+				force_direct, 
+				development, 
+				same_name);
+
 	if !force_direct && !development && same_name {
-		// looks like we're not running ~/.parity-updates/parity when the user is expecting otherwise.
+		// Try to run the latest installed version of `parity`, 
+		// Upon failure it falls back to the locally installed version of `parity`
 		// Everything run inside a loop, so we'll be able to restart from the child into a new version seamlessly.
 		loop {
-			// If we fail to run the updated parity then fallback to local version.
+			// `Path` to the latest downloaded binary
-			let latest_exe = latest_exe_path();
+			let latest_exe = latest_exe_path().ok();
+			
+			// `Latest´ binary exist
 			let have_update = latest_exe.as_ref().map_or(false, |p| p.exists());
-			let is_non_updated_current = exe.as_ref().map_or(false, |exe| latest_exe.as_ref().map_or(false, |lexe| exe.canonicalize().ok() != lexe.canonicalize().ok()));
+			
-			let update_is_newer = match (
+			// Canonicalized path to the current binary is not the same as to latest binary
-				latest_exe.as_ref()
+			let canonicalized_path_not_same = exe_path
-					.and_then(|p| metadata(p.as_path()).ok())
+				.as_ref()
-					.and_then(|m| m.modified().ok()),
+				.map_or(false, |exe| latest_exe.as_ref()
-				exe.as_ref()
+				.map_or(false, |lexe| exe.canonicalize().ok() != lexe.canonicalize().ok()));
-					.and_then(|p| metadata(p.as_path()).ok())
+
-					.and_then(|m| m.modified().ok())
+			// Downloaded `binary` is newer
-			) {
+			let update_is_newer = latest_binary_is_newer(&latest_exe, &exe_path);
-				(Some(latest_exe_time), Some(this_exe_time)) if latest_exe_time > this_exe_time => true,
+			trace_main!("Starting... (have-update: {}, non-updated-current: {}, update-is-newer: {})", have_update, canonicalized_path_not_same, update_is_newer);
-				_ => false,
+
-			};
+			let exit_code = if have_update && canonicalized_path_not_same && update_is_newer {
-			trace_main!("Starting... (have-update: {}, non-updated-current: {}, update-is-newer: {})", have_update, is_non_updated_current, update_is_newer);
+				trace_main!("Attempting to run latest update ({})...", 
-			let exit_code = if have_update && is_non_updated_current && update_is_newer {
+							latest_exe.as_ref().expect("guarded by have_update; latest_exe must exist for have_update; qed").display());
-				trace_main!("Attempting to run latest update ({})...", latest_exe.as_ref().expect("guarded by have_update; latest_exe must exist for have_update; qed").display());
+				match run_parity() {
-				run_parity().unwrap_or_else(|| { trace_main!("Falling back to local..."); main_direct(true) })
+					Ok(_) => 0,
+					// Restart parity
+					Err(Error::Restart) => PLEASE_RESTART_EXIT_CODE,
+					// Fall back to local version
+					Err(e) => {
+						error!(target: "updater", "Updated binary could not be executed error: {:?}. Falling back to local version", e); 
+						main_direct(true)
+					}
+				}
 			} else {
 				trace_main!("No latest update. Attempting to direct...");
 				main_direct(true)
 			};
-			trace_main!("Latest exited with {}", exit_code);
+			trace_main!("Latest binary exited with exit code: {}", exit_code);
 			if exit_code != PLEASE_RESTART_EXIT_CODE {
 				trace_main!("Quitting...");
 				process::exit(exit_code);
 			}
-			trace_main!("Rerunning...");
+			trace!(target: "updater", "Re-running updater loop");
 		}
 	} else {
 		trace_main!("Running direct");

parity/run.rs

@@ -717,8 +717,8 @@ fn execute_impl<Cr, Rr>(cmd: RunCmd, logger: Arc<RotatingLogger>, on_client_rq:
 	// the updater service
 	let updater_fetch = fetch.clone();
 	let updater = Updater::new(
-		Arc::downgrade(&(service.client() as Arc<BlockChainClient>)),
+		&Arc::downgrade(&(service.client() as Arc<BlockChainClient>)),
-		Arc::downgrade(&sync_provider),
+		&Arc::downgrade(&sync_provider),
 		update_policy,
 		hash_fetch::Client::with_fetch(contract_client.clone(), cpu_pool.clone(), updater_fetch, event_loop.remote())
 	);

updater/Cargo.toml

@@ -28,3 +28,9 @@ rand = "0.4"
 ethcore = { path = "../ethcore", features = ["test-helpers"] }
 tempdir = "0.3"
 matches = "0.1"
+
+[features]
+# hardcode version number 1.3.7 of parity to force an update
+# in order to manually test that parity fall-over to the local version
+# in case of invalid or deprecated command line arguments are entered
+test-updater = []

updater/src/lib.rs

@@ -16,6 +16,8 @@
 
 //! Updater for Parity executables
 
+#![warn(missing_docs)]
+
 extern crate ethabi;
 extern crate ethcore;
 extern crate ethcore_bytes as bytes;

updater/src/service.rs

@@ -16,6 +16,7 @@
 
 use types::{CapState, ReleaseInfo, OperationsInfo, VersionInfo};
 
+/// Parity updater service trait
 pub trait Service: Send + Sync {
 	/// Is the currently running client capable of supporting the current chain?
 	/// We default to true if there's no clear information.

updater/src/types/version_info.rs

@@ -55,14 +55,14 @@ impl VersionInfo {
 		let t = track.into();
 		VersionInfo {
 			version: Version {
-				major: (semver >> 16) as u64,
+				major: u64::from(semver >> 16),
-				minor: ((semver >> 8) & 0xff) as u64,
+				minor: u64::from((semver >> 8) & 0xff),
-				patch: (semver & 0xff) as u64,
+				patch: u64::from(semver & 0xff),
 				build: vec![],
 				pre: vec![],
 			},
 			track: t,
-			hash: hash,
+			hash,
 		}
 	}
 }

updater/src/updater.rs

@@ -27,15 +27,16 @@ use target_info::Target;
 
 use bytes::Bytes;
 use ethcore::BlockNumber;
-use ethcore::filter::Filter;
 use ethcore::client::{BlockId, BlockChainClient, ChainNotify, ChainRoute};
+use ethcore::filter::Filter;
 use ethereum_types::H256;
-use sync::{SyncProvider};
 use hash_fetch::{self as fetch, HashFetch};
 use path::restrict_permissions_owner;
 use service::Service;
+use sync::{SyncProvider};
 use types::{ReleaseInfo, OperationsInfo, CapState, VersionInfo, ReleaseTrack};
 use version;
+use semver::Version;
 
 use_contract!(operations_contract, "Operations", "res/operations.json");
 
@@ -155,7 +156,7 @@ pub struct Updater<O = OperationsContractClient, F = fetch::Client, T = StdTimeP
 	state: Mutex<UpdaterState>,
 }
 
-const CLIENT_ID: &'static str = "parity";
+const CLIENT_ID: &str = "parity";
 
 lazy_static! {
 	static ref CLIENT_ID_HASH: H256 = CLIENT_ID.as_bytes().into();
@@ -189,7 +190,7 @@ pub trait OperationsClient: Send + Sync + 'static {
 	fn release_block_number(&self, from: BlockNumber, release: &ReleaseInfo) -> Option<BlockNumber>;
 }
 
-/// OperationsClient that delegates calls to the operations contract.
+/// `OperationsClient` that delegates calls to the operations contract.
 pub struct OperationsContractClient {
 	operations_contract: operations_contract::Operations,
 	client: Weak<BlockChainClient>,
@@ -267,7 +268,7 @@ impl OperationsClient for OperationsContractClient {
 		// get the release info for the latest version in track
 		let in_track = self.release_info(latest_in_track, &do_call)?;
 		let mut in_minor = Some(in_track.clone());
-		const PROOF: &'static str = "in_minor initialised and assigned with Some; loop breaks if None assigned; qed";
+		const PROOF: &str = "in_minor initialized and assigned with Some; loop breaks if None assigned; qed";
 
 		// if the minor version has changed, let's check the minor version on a different track
 		while in_minor.as_ref().expect(PROOF).version.version.minor != this.version.minor {
@@ -308,7 +309,7 @@ impl OperationsClient for OperationsContractClient {
 			from_block: BlockId::Number(from),
 			to_block: BlockId::Latest,
 			address: Some(vec![address]),
-			topics: topics,
+			topics,
 			limit: None,
 		};
 
@@ -333,7 +334,7 @@ pub trait TimeProvider: Send + Sync + 'static {
 	fn now(&self) -> Instant;
 }
 
-/// TimeProvider implementation that delegates calls to std::time.
+/// `TimeProvider` implementation that delegates calls to `std::time`.
 pub struct StdTimeProvider;
 
 impl TimeProvider for StdTimeProvider {
@@ -349,7 +350,7 @@ pub trait GenRange: Send + Sync + 'static {
 	fn gen_range(&self, low: u64, high: u64) -> u64;
 }
 
-/// GenRange implementation that uses a rand::thread_rng for randomness.
+/// `GenRange` implementation that uses a `rand::thread_rng` for randomness.
 pub struct ThreadRngGenRange;
 
 impl GenRange for ThreadRngGenRange {
@@ -359,14 +360,15 @@ impl GenRange for ThreadRngGenRange {
 }
 
 impl Updater {
+	/// `Updater` constructor
 	pub fn new(
-		client: Weak<BlockChainClient>,
+		client: &Weak<BlockChainClient>,
-		sync: Weak<SyncProvider>,
+		sync: &Weak<SyncProvider>,
 		update_policy: UpdatePolicy,
 		fetcher: fetch::Client,
 	) -> Arc<Updater> {
 		let r = Arc::new(Updater {
-			update_policy: update_policy,
+			update_policy,
 			weak_self: Mutex::new(Default::default()),
 			client: client.clone(),
 			sync: Some(sync.clone()),
@@ -375,12 +377,21 @@ impl Updater {
 				operations_contract::Operations::default(),
 				client.clone()),
 			exit_handler: Mutex::new(None),
-			this: VersionInfo::this(),
+			this: if cfg!(feature = "test-updater") {
+				VersionInfo {
+					track: ReleaseTrack::Stable,
+					version: Version::new(1, 3, 7),
+					hash: 0.into(),
+				}
+			} else {
+				VersionInfo::this()
+			},
 			time_provider: StdTimeProvider,
 			rng: ThreadRngGenRange,
 			state: Mutex::new(Default::default()),
 		});
 		*r.weak_self.lock() = Arc::downgrade(&r);
+
 		r.poll();
 		r
 	}
@@ -447,7 +458,7 @@ impl<O: OperationsClient, F: HashFetch, T: TimeProvider, R: GenRange> Updater<O,
 				},
 				// There was an error fetching the update, apply a backoff delay before retrying
 				Err(err) => {
-					let delay = 2usize.pow(retries) as u64;
+					let delay = 2_usize.pow(retries) as u64;
 					// cap maximum backoff to 1 day
 					let delay = cmp::min(delay, 24 * 60 * 60);
 					let backoff = (retries, self.time_provider.now() + Duration::from_secs(delay));
@@ -599,14 +610,19 @@ impl<O: OperationsClient, F: HashFetch, T: TimeProvider, R: GenRange> Updater<O,
 		trace!(target: "updater", "Current release is {} ({:?})", self.this, self.this.hash);
 
 		// We rely on a secure state. Bail if we're unsure about it.
-		if self.client.upgrade().map_or(true, |c| !c.chain_info().security_level().is_full()) {
+		if !cfg!(feature = "test-updater") {
-			return;
+			if self.client.upgrade().map_or(true, |c| !c.chain_info().security_level().is_full()) {
+			    return;
+			}
 		}
 
 		// Only check for updates every n blocks
 		let current_block_number = self.client.upgrade().map_or(0, |c| c.block_number(BlockId::Latest).unwrap_or(0));
-		if current_block_number % cmp::max(self.update_policy.frequency, 1) != 0 {
+		
-			return;
+		if !cfg!(feature = "test-updater") {
+			if current_block_number % cmp::max(self.update_policy.frequency, 1) != 0 {
+				return;
+			}
 		}
 
 		let mut state = self.state.lock();
@@ -639,16 +655,16 @@ impl<O: OperationsClient, F: HashFetch, T: TimeProvider, R: GenRange> Updater<O,
 			// There's a new release available
 			if state.latest.as_ref() != Some(&latest) {
 				trace!(target: "updater", "Latest release in our track is v{} it is {}critical ({} binary is {})",
-					   latest.track.version,
+					latest.track.version,
-					   if latest.track.is_critical {""} else {"non-"},
+					if latest.track.is_critical {""} else {"non-"},
-					   *PLATFORM,
+					*PLATFORM,
-					   latest.track.binary.map(|b| format!("{}", b)).unwrap_or("unreleased".into()));
+					latest.track.binary.map_or_else(|| "unreleased".into(), |b| format!("{}", b)));
 
 				trace!(target: "updater", "Fork: this/current/latest/latest-known: {}/#{}/#{}/#{}",
-					   latest.this_fork.map(|f| format!("#{}", f)).unwrap_or("unknown".into()),
+					latest.this_fork.map_or_else(|| "unknown".into(), |f| format!("#{}", f)),
-					   current_block_number,
+					current_block_number,
-					   latest.track.fork,
+					latest.track.fork,
-					   latest.fork);
+					latest.fork);
 
 				// Update latest release
 				state.latest = Some(latest);