Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Flush keyfiles. Resolves #7632 #7868

Merged
merged 1 commit into from
Feb 12, 2018
Merged

Flush keyfiles. Resolves #7632 #7868

merged 1 commit into from
Feb 12, 2018

Conversation

tomusdrw
Copy link
Collaborator

No description provided.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. B0-patch labels Feb 12, 2018
@tomusdrw tomusdrw requested a review from debris February 12, 2018 14:13
@5chdn 5chdn added this to the 1.10 milestone Feb 12, 2018
}

// Write key content
self.key_manager.write(original_account, &mut file).map_err(|e| Error::Custom(format!("{:?}", e)))?;
Copy link
Contributor

@0x7CFE 0x7CFE Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the simple e.to_string() do the trick here instead of format!()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a custom Error declared as enum. It only has Debug and Display implementations.

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably a good idea never to delete key files (unless explicitly asked). If there's an error while writing/flushing the key files we will try to import these files on next restart (and possibly error). This fixes #7632 by making sure the flush succeeds right? (i.e. less room for corrupted key files).

@tomusdrw
Copy link
Collaborator Author

@andresilva Correct. I'm also very unsure if dropping a file actually flushes it's content to disk.

@andresilva
Copy link
Contributor

@tomusdrw I think it will only close its underlying file descriptor, which does not guarantee that data is synced. So we should leave the explicit sync_data call.

@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 12, 2018
@5chdn 5chdn merged commit 90bd6bf into master Feb 12, 2018
@5chdn 5chdn deleted the td-keyfile-flush branch February 12, 2018 17:03
tomusdrw added a commit that referenced this pull request Feb 14, 2018
tomusdrw added a commit that referenced this pull request Feb 14, 2018
This was referenced Feb 14, 2018
5chdn pushed a commit that referenced this pull request Feb 14, 2018
* 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)
5chdn pushed a commit that referenced this pull request Feb 14, 2018
* 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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants