Updater verification #8787
Updater verification #8787
Conversation
5chdn
added
A3-inprogress ⏳
niklasad1
added
A0-pleasereview 🤓
updater/src/updater.rs
Outdated
| @@ -375,12 +377,21 @@ impl Updater { | |||
| operations_contract::Operations::default(), | |||
| client.clone()), | |||
| exit_handler: Mutex::new(None), | |||
| this: VersionInfo::this(), | |||
| // this: VersionInfo::this(), | |||
| // TODO: Remove hardcoded dummy version for this | |||
updater/src/updater.rs
Outdated
| @@ -600,13 +611,15 @@ impl<O: OperationsClient, F: HashFetch, T: TimeProvider, R: GenRange> Updater<O, | |||
|
|
|||
| // 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()) { | |||
| return; | |||
| info!("The `security_level` check is DISABLED!!!!! It is just diabled to test the updater-verification and should be removed later, {}:{}", file!(), line!()); | |||
| // return; | |||
There was a problem hiding this comment.
Yepp, only for manual testing
updater/src/updater.rs
Outdated
| } | ||
|
|
||
| // 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; | ||
| info!("The `updater_policy frequency` check is DISABLED!!!!!. This check is just diabled to test the updater and should be removed later, {}:{}", file!(), line!()); | ||
| // return; |
There was a problem hiding this comment.
Yepp, only for manual testing
parity/main.rs
Outdated
| let same_name = exe_path | ||
| .as_ref() | ||
| .map_or(false, |p| { | ||
| p.file_stem().map_or(false, |n| n == "parity") |
There was a problem hiding this comment.
I'd make the executable name a const.
parity/main.rs
Outdated
| 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 fails fall back into the locally installed version of `parity` |
There was a problem hiding this comment.
s/upon failure it fails fall back/upon failure it falls back/
| // If we fail to run the updated parity then fallback to local version. | ||
| let latest_exe = latest_exe_path(); | ||
| // `Path` to the latest downloaded binary | ||
| let latest_exe = latest_exe_path().ok(); |
There was a problem hiding this comment.
Aren't we swallowing the error here? Is that what we want or do we want to print something useful?
There was a problem hiding this comment.
Basically, it checks if we have a file named latest which in turn contains the latest known version of parity if the local version is not the latest. If latest doesn´t exist (it is None) then we run our locally installed version of parity!
It is used for this condition also traces before and after that already exist which should be sufficient IMO!
| 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); | ||
|
|
||
| // absolute path to the current `binary` | ||
| let exe_path = std::env::current_exe().ok(); |
There was a problem hiding this comment.
I think we should handle the error here and be very careful not to introduce vulnerabilities.
There was a problem hiding this comment.
This "shouldn't" fail it's a wrapper on top of the underlying OS.
Do you want unwrap/expect here?
However, it's safe because if this fails we fall back to the locally installed version (or in other words don't spawn a child process)
parity/main.rs
Outdated
| File::open(update_path("latest")).and_then(|mut f| { | ||
| let mut exe = String::new(); | ||
| trace!(target: "updater", "latest binary path: {:?}", f); | ||
| f.read_to_string(&mut exe).map(|_| update_path(&exe)) |
There was a problem hiding this comment.
Am I reading this right? Are we reading the whole binary into a String? Can you help me understand why that is?
There was a problem hiding this comment.
There are two files in .local/share/io.parity.ethereum-updates (path for *NIX-based systems):
- latest - contains the name of the latest binary
- the actual binary - some like
parity-1.9.5-ff821daf1da42865f229aee35f2e74e7b2dd8db2
So if .local/share/io.parity.ethereum-updates/latest exists it will return the path the newest binary e.g, .local/share/io.parity.ethereum-updates/parity-1.9.5-ff821daf1da42865f229aee35f2e74e7b2dd8db2
There was a problem hiding this comment.
Changing the name of the variable from exe to exe_path would make it simple to understand :)
niklasad1
force-pushed
the
na-updater-verification
branch
from
b42f9b6
to
8e3ea36
Compare
|
Test |
|
@5chdn It is expected because I have hardcoded the version number in order to force an update! |
|
So at what point this PR is reviewable/mergable? |
|
@5chdn So the hard-coded stuff will be reverted if @tomusdrw approved this approach otherwise I need to restructure |
parity/main.rs
Outdated
| File::open(update_path("latest")).and_then(|mut f| { | ||
| let mut exe = String::new(); | ||
| trace!(target: "updater", "latest binary path: {:?}", f); | ||
| f.read_to_string(&mut exe).map(|_| update_path(&exe)) |
There was a problem hiding this comment.
Changing the name of the variable from exe to exe_path would make it simple to understand :)
updater/src/updater.rs
Outdated
| @@ -600,13 +611,15 @@ impl<O: OperationsClient, F: HashFetch, T: TimeProvider, R: GenRange> Updater<O, | |||
|
|
|||
| // 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()) { | |||
| return; | |||
| info!("The `security_level` check is DISABLED!!!!! It is just diabled to test the updater-verification and should be removed later, {}:{}", file!(), line!()); | |||
There was a problem hiding this comment.
Should we have a const DEBUG: bool = for testing? Then such code could be committed and would help others working on this in the future.
updater/src/updater.rs
Outdated
| current_block_number, | ||
| latest.track.fork, | ||
| latest.fork); | ||
| latest.this_fork.map_or_else(|| "unreleased".into(), |f| format!("#{}", f)), |
There was a problem hiding this comment.
Should be unknown not unreleased since it's the fork name.
5chdn
added
A6-mustntgrumble 💦
niklasad1
force-pushed
the
na-updater-verification
branch
from
8e3ea36
to
8048915
Compare
Cargo.toml
Outdated
| @@ -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 = [] | |||
| # use harcoded version 1.3.7 of parity to force an update | |||
| # and to manually test that parity fall-over the local version | |||
There was a problem hiding this comment.
"…that parity reverts to the local version…"?
Cargo.toml
Outdated
| @@ -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 = [] | |||
| # use harcoded version 1.3.7 of parity to force an update | |||
| # and to manually test that parity fall-over the local version | |||
| # in case of invalid or depricated command line arguments | |||
There was a problem hiding this comment.
Sorry, this was sloppy of me!
updater/Cargo.toml
Outdated
| [features] | ||
| # use harcoded version 1.3.7 of parity to force an update | ||
| # and to manually test that parity fall-over the local version | ||
| # in case of invalid or depricated command line arguments |
updater/Cargo.toml
Outdated
| @@ -28,3 +28,9 @@ rand = "0.4" | |||
| ethcore = { path = "../ethcore", features = ["test-helpers"] } | |||
| tempdir = "0.3" | |||
| matches = "0.1" | |||
|
|
|||
| [features] | |||
| # use harcoded version 1.3.7 of parity to force an update | |||
There was a problem hiding this comment.
What is "magic" about v1.3.7? Might be worth mentioning in the comment?
There was a problem hiding this comment.
Nothing, just old enough to be regarded as a critical update!
There was a problem hiding this comment.
I'd add a parenthesis like "(a version old enough to require a critical update, nothing special about it per se)" or something along those line. But up to you!
niklasad1
force-pushed
the
na-updater-verification
branch
from
2d74e86
to
ac74c1d
Compare
|
Could not compile Why is that happening? Restarting CI |
|
@5chdn I think I have to update the submodule in this PR, can check later! |
* 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
niklasad1
force-pushed
the
na-updater-verification
branch
from
ac74c1d
to
dd9b477
Compare
|
Restarted CI, seems unrelated. |
5chdn
added
A8-looksgood 🦄
| let prefix = vec![OsString::from("--can-restart"), OsString::from("--force-direct")]; | ||
| let res = latest_exe_path().and_then(|exe| process::Command::new(exe) | ||
|
|
There was a problem hiding this comment.
Please make sure your editor trims whitespaces automatically
Attempt to close #8178
Abstract
In the entry point in
parity/mainwe first try run the latest installed version of parity if not--force-restartis explicitly specified. If we try to run the latest version of parity(run_parity) it will return Some(ErrorCode) unless the downloaded binary is not found. Thus, if unsupported CLI args are entered for example it will terminate (return the exit code) instead of falling back to local version.In other words, only if the binary is not found we fall back to the local version!
So, high-level this PR changes so
run_parity()returnResult<(), ErrorCode>and if an error received while running parity we fall back to the local version! That's it still, I think it's okay still store the newer version try it if, for example, other CLI args may be valid. Otherwise, it is easy to add a flag for that or change the timestamp of binary to disable it the entering theupdater loop. But, the downloaded binary should be probably not be removed otherwise the updater will fetch the same binary over and over again!Manual tests
Incompatible CLI args
Compatible CLI args
/cc @tomusdrw @andresilva