Fix wallet import #7873

andresilva · 2018-02-13T00:58:18Z

This PR updates the wallet import to always generate a new account id and handle duplicate key filenames.

parity-cla-bot · 2018-02-13T00:58:21Z

It looks like @andresilva signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

svyatonik · 2018-02-13T07:08:44Z

ethstore/src/accounts_dir/disk.rs

-	pub fn insert_with_filename(&self, account: SafeAccount, filename: String) -> Result<SafeAccount, Error> {
+	/// insert account with given file name. if the filename is a duplicate of any stored account, a number is appended
+	/// to the filename.
+	pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String) -> Result<SafeAccount, Error> {


Could you please check that this comment in update (https://github.com/paritytech/parity/blob/006b2f35640d2b729b77b4c5f93b2e9b6a10c5d4/ethstore/src/accounts_dir/disk.rs#L202) is still valid? I mean - if existing account is updated (the file with the same name already exists), file won't be duplicated (with suffix append).

svyatonik · 2018-02-13T07:14:40Z

ethstore/src/accounts_dir/disk.rs

+	/// to the filename.
+	pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String) -> Result<SafeAccount, Error> {
+		// check for duplicate filenames and append/increment counter
+		let dups: Vec<_> = self.files()?.into_iter().filter_map(|f| {


Also - maybe just check if file exists (fs::metadata(_)is_ok()) and then just add a random suffix, instead of reading all the files list (self.files()) and trying to maintain an order (do not know if an order is important here)? This should increase performance of insert/update - iirc there are cases when there are a lot (>>10000) key files in single dir.
Do not have a strong opinion on this, though

codecov-io · 2018-02-13T07:38:57Z

Codecov Report

Merging #7873 into master will increase coverage by 0.2%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #7873     +/-   ##
=========================================
+ Coverage   81.91%   82.12%   +0.2%     
=========================================
  Files         660      660             
  Lines       83539    83539             
=========================================
+ Hits        68435    68608    +173     
+ Misses      15104    14931    -173

Impacted Files	Coverage Δ
parity/run.rs	`100% <ø> (ø)`	⬆️
dapps/src/tests/api.rs	`86.95% <0%> (-13.05%)`	⬇️
dapps/src/api/api.rs	`55.26% <0%> (-7.9%)`	⬇️
dapps/src/apps/fetcher/mod.rs	`73.44% <0%> (-5.09%)`	⬇️
dapps/src/tests/fetch.rs	`95.43% <0%> (-3.05%)`	⬇️
sync/src/light_sync/sync_round.rs	`74.71% <0%> (-2.3%)`	⬇️
ethcore/src/engines/tendermint/message.rs	`97.53% <0%> (ø)`	⬆️
ethcore/src/miner/miner.rs	`73.07% <0%> (+0.11%)`	⬆️
util/network/src/host.rs	`71.51% <0%> (+0.12%)`	⬆️
ethcore/src/block.rs	`91.13% <0%> (+0.24%)`	⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8579a56...d7cc6a5. Read the comment docs.

andresilva · 2018-02-13T11:06:31Z

@svyatonik I simplified the deduplication according to your suggestion and fixed the update .

svyatonik

Thanks! Looks good. But maybe worth another check by someone familiar with all account id caveats.

debris · 2018-02-14T09:45:10Z

ethstore/src/accounts_dir/disk.rs

+
+	/// insert account with given filename. if the filename is a duplicate of any stored account and dedup is set to
+	/// true, a random suffix is appended to the filename.
+	pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String, dedup: bool) -> Result<SafeAccount, Error> {


why is dedup optional and not a default behaviour?

Afair this function is also used to update the key file (for instance after a name change or meta change).

tomusdrw · 2018-02-14T09:53:19Z

ethstore/src/accounts_dir/disk.rs

+
+	/// insert account with given filename. if the filename is a duplicate of any stored account and dedup is set to
+	/// true, a random suffix is appended to the filename.
+	pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String, dedup: bool) -> Result<SafeAccount, Error> {


Afair this function is also used to update the key file (for instance after a name change or meta change).

tomusdrw · 2018-02-14T09:54:25Z

ethstore/src/accounts_dir/disk.rs

@@ -286,6 +291,14 @@ impl KeyFileManager for DiskKeyFileManager {
 	}
 }

+fn account_filename(account: &SafeAccount) -> String {
+	// build file path
+	account.filename.as_ref().cloned().unwrap_or_else(|| {


.as_ref().cloned() -> .clone()

tomusdrw · 2018-02-14T09:55:01Z

ethstore/src/accounts_dir/disk.rs

+	/// true, a random suffix is appended to the filename.
+	pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String, dedup: bool) -> Result<SafeAccount, Error> {
+		// path to keyfile
+		let mut keyfile_path = self.path.clone();


let keyfile_path = self.path.join(&filename)

andresilva · 2018-02-14T12:01:48Z

Fixed grumbles.

* rpc: generate new account id for imported wallets * ethstore: handle duplicate wallet filenames * ethstore: simplify deduplication of wallet file names * ethstore: do not dedup wallet filenames on update * ethstore: fix minor grumbles

* update back-references more aggressively after answering from cache (#7578) * Add new EF ropstens nodes. (#7824) * Add new EF ropstens nodes. * Fix tests * Add a timeout for light client sync requests (#7848) * Add a timeout for light client sync requests * Adjusting timeout to number of headers * Flush keyfiles. Resolves #7632 (#7868) * Fix wallet import (#7873) * rpc: generate new account id for imported wallets * ethstore: handle duplicate wallet filenames * ethstore: simplify deduplication of wallet file names * ethstore: do not dedup wallet filenames on update * ethstore: fix minor grumbles * [WASM] mem_cmp added to the Wasm runtime (#7539) * mem_cmp added to the Wasm runtime * schedule.wasm.mem_copy to schedule.wasm.mem_cmp for mem_cmp * [Wasm] memcmp fix and test added (#7590) * [Wasm] memcmp fix and test added * [Wasm] use reqrep_test! macro for memcmp test * wasmi interpreter (#7796) * adjust storage update evm-style (#7812) * disable internal memory (#7842)

* update back-references more aggressively after answering from cache (#7578) * Flush keyfiles. Resolves #7632 (#7868) * Fix wallet import (#7873) * rpc: generate new account id for imported wallets * ethstore: handle duplicate wallet filenames * ethstore: simplify deduplication of wallet file names * ethstore: do not dedup wallet filenames on update * ethstore: fix minor grumbles * parity-version pr reopen (#7136) * parity-version module split from util removed unused util deps and features trigger buildbot again only kvdb links rocksdb snappy linker issues * rm snappy * fixed old version imports * Move updater metadata to Cargo.toml of parity-version. (#7832) * Update version. * Bump parity version. * Fix version. * Fix compilation.

rpc: generate new account id for imported wallets

e170c8e

andresilva added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Feb 13, 2018

andresilva assigned tomusdrw Feb 13, 2018

andresilva unassigned tomusdrw Feb 13, 2018

andresilva added the B0-patch label Feb 13, 2018

andresilva force-pushed the fix-wallet-import branch 3 times, most recently from e75b14d to 4c02710 Compare February 13, 2018 02:47

ethstore: handle duplicate wallet filenames

9346d29

andresilva force-pushed the fix-wallet-import branch from 4c02710 to 9346d29 Compare February 13, 2018 02:54

svyatonik reviewed Feb 13, 2018

View reviewed changes

5chdn added this to the 1.10 milestone Feb 13, 2018

andresilva added 2 commits February 13, 2018 10:55

ethstore: simplify deduplication of wallet file names

4bf912d

ethstore: do not dedup wallet filenames on update

7ba0bc4

svyatonik approved these changes Feb 13, 2018

View reviewed changes

svyatonik added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 13, 2018

debris reviewed Feb 14, 2018

View reviewed changes

tomusdrw approved these changes Feb 14, 2018

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Feb 14, 2018

ethstore: fix minor grumbles

d7cc6a5

andresilva added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Feb 14, 2018

5chdn merged commit d1815ee into master Feb 14, 2018

5chdn deleted the fix-wallet-import branch February 14, 2018 13:22

This was referenced Feb 14, 2018

Backport Core PRs to beta #7891

Merged

Backport core PRs to stable #7892

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wallet import #7873

Fix wallet import #7873

andresilva commented Feb 13, 2018

parity-cla-bot commented Feb 13, 2018

svyatonik Feb 13, 2018

svyatonik Feb 13, 2018

codecov-io commented Feb 13, 2018 •

edited

Loading

andresilva commented Feb 13, 2018

svyatonik left a comment •

edited

Loading

debris Feb 14, 2018

tomusdrw Feb 14, 2018

tomusdrw Feb 14, 2018

tomusdrw Feb 14, 2018

tomusdrw Feb 14, 2018

andresilva commented Feb 14, 2018

Fix wallet import #7873

Fix wallet import #7873

Conversation

andresilva commented Feb 13, 2018

parity-cla-bot commented Feb 13, 2018

svyatonik Feb 13, 2018

Choose a reason for hiding this comment

svyatonik Feb 13, 2018

Choose a reason for hiding this comment

codecov-io commented Feb 13, 2018 • edited Loading

Codecov Report

andresilva commented Feb 13, 2018

svyatonik left a comment • edited Loading

Choose a reason for hiding this comment

debris Feb 14, 2018

Choose a reason for hiding this comment

tomusdrw Feb 14, 2018

Choose a reason for hiding this comment

tomusdrw Feb 14, 2018

Choose a reason for hiding this comment

tomusdrw Feb 14, 2018

Choose a reason for hiding this comment

tomusdrw Feb 14, 2018

Choose a reason for hiding this comment

andresilva commented Feb 14, 2018

codecov-io commented Feb 13, 2018 •

edited

Loading

svyatonik left a comment •

edited

Loading