Fix Snapshot restoration failure on Windows #9491

ngotchac · 2018-09-07T09:15:12Z

The issue would only arise on Windows: the files used for the Blooms database were not properly closed before restoring the snapshot DB. Thus windows would fail to move the directories around.

In addition to this PR, having paritytech/parity-common#53 and debris/fs-swap#8 would be great!

ForauDNB · 2018-09-07T15:46:59Z

Thankyou for bringing up this issue for investigation. Is anybody aware of any manual workarounds in the mean time?

ngotchac · 2018-09-07T15:50:49Z

Sadly there are no manual fixes other than not using Windows or using an older Parity version :/

ForauDNB · 2018-09-07T15:56:42Z

Ok thanks. I'm not too worried as for me this only affects mainnet, ropsten is syncing fine and that's all I need at the moment for solidity development purposes :). Although I may just end up dual booting my new machine. Thanks for your help, keep up the great work 👍

dvdplm

lgtm modulo spelling nits

dvdplm · 2018-09-10T08:43:19Z

ethcore/src/blockchain/blockchain.rs

+		self.blooms().close()?;
+		self.trace_blooms().close()?;
+
+		// Restore the key_value BD


dvdplm · 2018-09-10T08:45:21Z

util/blooms-db/src/db.rs

+impl DatabaseFiles {
+	/// Open the blooms db files
+	pub fn open(path: &PathBuf) -> io::Result<DatabaseFiles> {
+		// let path = path.as_ref();


dvdplm · 2018-09-10T08:45:44Z

util/blooms-db/src/db.rs

+}
+
+impl DatabaseFiles {
+	/// Open the blooms db files


s/blooms db files/bloom db files/ ?

Well it's called blooms-db :p

dvdplm · 2018-09-10T08:52:36Z

util/blooms-db/src/db.rs

+		})
+	}
+
+	pub fn flush(&mut self) -> io::Result<()> {


Does this have to be pub? It'd be nice if flushing was taken care of internally if&when needed.

We should probably flush on drop, but also keep the explicit flushing.

dvdplm · 2018-09-10T08:56:07Z

util/blooms-db/src/db.rs

-		self.top = File::open(self.path.join("top.bdb"))?;
-		self.mid = File::open(self.path.join("mid.bdb"))?;
-		self.bot = File::open(self.path.join("bot.bdb"))?;
+		self.db_files = Some(DatabaseFiles::open(&self.path)?);
 		Ok(())
 	}

 	/// Insert consecutive blooms into database starting with positon from.


s/…starting with position from/…starting at the given position/

dvdplm · 2018-09-10T09:03:44Z

util/blooms-db/src/db.rs

+		Ok(())
+	}
+
+	pub fn iterator_from(&mut self, pos: Positions) -> io::Result<DatabaseFilesIterator> {


Just a curiosity: This method looks a little like the std::io::Seek trait. Would it be possible to refactor things so that Positions become Position and consist of single u64 (which is then shifted properly to form the three variants needed for top, middle and bottom, kind of what happens in the insert_blooms method) and then impl seek for DatabaseFilesIterator? Not sure what we'd gain, and not in scope for this bug-fix PR but interested in your thoughts! :)

I'm not actually quite sure how this blooms-db work so I'd say it's (as you said) out-of-scop for this PR, but @debris might have an opinion on this

dvdplm · 2018-09-10T09:06:35Z

util/blooms-db/src/db.rs

+				for (index, bloom) in (from..).into_iter().zip(blooms.map(Into::into)) {
+					let pos = Positions::from_index(index);
+
+					// constant forks make lead to increased ration of false positives in bloom filters


s/constant forks make lead to increased ration of /Constant forks may lead to increased ratio of/

dvdplm · 2018-09-10T09:07:42Z

util/blooms-db/src/db.rs

+
+					// constant forks make lead to increased ration of false positives in bloom filters
+					// since we do not rebuild top or mid level, but we should not be worried about that
+					// most of the time events at block n(a) occur also on block n(b) or n+1(b)


s/most of the times/because most of the time/

andresilva

LGTM. Nice find :)

andresilva · 2018-09-10T10:30:11Z

util/blooms-db/src/db.rs

+		})
+	}
+
+	pub fn flush(&mut self) -> io::Result<()> {


We should probably flush on drop, but also keep the explicit flushing.

andresilva · 2018-09-10T10:45:02Z

util/blooms-db/src/db.rs

+		if let Some(ref mut db_files) = self.db_files {
+			db_files.flush()?;
+		}
+		self.db_files = None;


If we impl Drop for DatabaseFiles we can avoid the explicit flush above right?

sorpaas

Few grumbles/questions:

Rust's std::fs::File design is to automatically close the file on Drop. Would we want to remove the Option/reopen here -- just drop the value and then recreate a new handle?
I think there may be a simpler fix for this, in https://github.com/paritytech/parity-ethereum/pull/9491/files#diff-9d32ce4d31852f1010ba2899d38f245dL75, we can just first drop all self.top, self.mid, self.bot handles before open files again. I think the issue is just that Rust tries to call drop after File::open, but we should call drop before it.

sorpaas · 2018-09-10T13:13:20Z

util/blooms-db/src/db.rs

 	/// Database path
 	path: PathBuf,
 }

 impl Database {
 	/// Opens blooms database.
 	pub fn open<P>(path: P) -> io::Result<Database> where P: AsRef<Path> {
-		let path = path.as_ref();
+		let path: PathBuf = path.as_ref().to_owned();


Why is this changed to take owned value? Path also has join which may just be enough.

ngotchac · 2018-09-10T13:43:31Z

@sorpaas So the issue is that (on Windows) we need to first close the handles to the blooms-db files before being able to move the key_value DB (by calling the restore method): so first close the blooms handles, call restore on the kvdb and then re-open the handles. So you can't just drop the values and re-open them, since we need to do something between the two.
I found that the minimal changes would be to add this close method to the blooms-db, which already has the path value stored in struct Database.
Does this answer your comment?

util/blooms-db/src/db.rs

-			mid: File::open(path.join("mid.bdb"))?,
-			bot: File::open(path.join("bot.bdb"))?,
-			path: path.to_owned(),
+			db_files: Some(DatabaseFiles::open(&path)?),


sorpaas · 2018-09-10T14:26:52Z

util/blooms-db/src/db.rs

+	}
+
+	pub fn iterator_from(&mut self, pos: Positions) -> io::Result<DatabaseFilesIterator> {
+		Ok(DatabaseFilesIterator{


Missing a whitespace between DatabaseFilesIterator and {.

sorpaas · 2018-09-10T14:27:41Z

Understood. Indeed we need close. Have two final grumbles!

* Close Blooms DB files before DB restoration * PR Grumbles I * PR Grumble * Grumble

* parity-version: bump beta to 2.0.4 * [light/jsonrpc] Provide the actual account for `eth_coinbase` RPC and unify error handeling for light and full client (#9383) * Provide the actual `account` for eth_coinbase The previous implementation always provided the `zero address` on `eth_coinbase` RPC. Now, instead the actual address is returned on success or an error when no account(s) is found! * full client `eth_coinbase` return err In the full-client return an error when no account is found instead of returning the `zero address` * Remove needless blocks on single import * Remove needless `static` lifetime on const * Fix `rpc_eth_author` test * parity: print correct keys path on startup (#9501) * aura: don't report skipped primaries when empty steps are enabled (#9435) * Only check warp syncing for eth_getWorks (#9484) * Only check warp syncing for eth_getWorks * Use SyncStatus::is_snapshot_syncing * Fix Snapshot restoration failure on Windows (#9491) * Close Blooms DB files before DB restoration * PR Grumbles I * PR Grumble * Grumble

Close Blooms DB files before DB restoration

8f83920

ngotchac added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 7, 2018

5chdn added this to the 2.1 milestone Sep 8, 2018

5chdn added the B0-patchthis 🕷 label Sep 8, 2018

5chdn mentioned this pull request Sep 8, 2018

Beta backports to 2.0.4 #9452

Merged

8 tasks

dvdplm approved these changes Sep 10, 2018

View reviewed changes

andresilva approved these changes Sep 10, 2018

View reviewed changes

PR Grumbles I

c73975b

sorpaas suggested changes Sep 10, 2018

View reviewed changes

sorpaas reviewed Sep 10, 2018

View reviewed changes

PR Grumble

370b150

sorpaas reviewed Sep 10, 2018

View reviewed changes

util/blooms-db/src/db.rs

mid: File::open(path.join("mid.bdb"))?,

bot: File::open(path.join("bot.bdb"))?,

path: path.to_owned(),

db_files: Some(DatabaseFiles::open(&path)?),

This comment was marked as resolved.

Sign in to view

sorpaas reviewed Sep 10, 2018

View reviewed changes

sorpaas approved these changes Sep 10, 2018

View reviewed changes

Grumble

19b963e

sorpaas added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 10, 2018

ordian merged commit baf5be0 into master Sep 10, 2018

ordian deleted the ng-fix-restore branch September 10, 2018 15:22

sorpaas pushed a commit that referenced this pull request Sep 10, 2018

Fix Snapshot restoration failure on Windows (#9491)

98f6d6c

* Close Blooms DB files before DB restoration * PR Grumbles I * PR Grumble * Grumble

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Snapshot restoration failure on Windows #9491

Fix Snapshot restoration failure on Windows #9491

ngotchac commented Sep 7, 2018

ForauDNB commented Sep 7, 2018

ngotchac commented Sep 7, 2018

ForauDNB commented Sep 7, 2018

dvdplm left a comment

dvdplm Sep 10, 2018

dvdplm Sep 10, 2018

dvdplm Sep 10, 2018

ngotchac Sep 10, 2018

dvdplm Sep 10, 2018

andresilva Sep 10, 2018

dvdplm Sep 10, 2018

dvdplm Sep 10, 2018

ngotchac Sep 10, 2018

dvdplm Sep 10, 2018

dvdplm Sep 10, 2018

andresilva left a comment

andresilva Sep 10, 2018

andresilva Sep 10, 2018

ngotchac Sep 10, 2018

sorpaas left a comment •

edited

Loading

sorpaas Sep 10, 2018

ngotchac commented Sep 10, 2018

This comment was marked as resolved.

sorpaas Sep 10, 2018

sorpaas commented Sep 10, 2018

Fix Snapshot restoration failure on Windows #9491

Fix Snapshot restoration failure on Windows #9491

Conversation

ngotchac commented Sep 7, 2018

ForauDNB commented Sep 7, 2018

ngotchac commented Sep 7, 2018

ForauDNB commented Sep 7, 2018

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorpaas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngotchac commented Sep 10, 2018

This comment was marked as resolved.

Choose a reason for hiding this comment

sorpaas commented Sep 10, 2018

sorpaas left a comment •

edited

Loading