Accounts bloom by NikVolf · Pull Request #2357 · openethereum/parity-ethereum

NikVolf · 2016-09-27T12:06:04Z

still missing:

~~snapshot restoration bloom update~~
~~miration~~

rphmeier · 2016-09-27T12:10:26Z

could we put the source for bloom filter in this repo? /util/bloomfilter would be a good place. It's hard to review code in multiple places at once.

NikVolf · 2016-09-27T12:15:18Z

@rphmeier it's a fork from other guy crate

gavofyork · 2016-09-27T16:39:41Z

can be made against master now.

arkpar · 2016-09-27T12:15:39Z

+pub const ACCOUNT_BLOOM_SPACE: usize = 1048576;
+pub const DEFAULT_ACCOUNT_PRESET: usize = 1000000;
+
+pub const ACCOUNT_BLOOM_SPACE_COLUMN: &'static[u8] = b"accounts_bloom";


Should be called ACCOUNT_BLOOM_SPACE_KEY, Column is something else in rocksdb terms.

coveralls · 2016-09-27T21:43:24Z

Coverage decreased (-0.4%) to 87.051% when pulling 5c29d1e on state-cache-bloom into facab31 on beta.

coveralls · 2016-09-27T21:48:07Z

Coverage decreased (-0.4%) to 87.05% when pulling 5c29d1e on state-cache-bloom into facab31 on beta.

coveralls · 2016-09-27T22:47:14Z

Coverage decreased (-0.4%) to 87.037% when pulling 33e181a on state-cache-bloom into facab31 on beta.

coveralls · 2016-09-27T23:35:45Z

Coverage decreased (-0.4%) to 87.039% when pulling c2022fa on state-cache-bloom into facab31 on beta.

coveralls · 2016-09-28T00:29:35Z

Coverage decreased (-0.4%) to 87.036% when pulling f3b094a on state-cache-bloom into facab31 on beta.

coveralls · 2016-09-28T00:44:27Z

Coverage decreased (-0.4%) to 87.039% when pulling f3b094a on state-cache-bloom into facab31 on beta.

coveralls · 2016-09-28T02:05:41Z

Coverage decreased (-0.04%) to 87.375% when pulling ee36df0 on state-cache-bloom into facab31 on beta.

arkpar · 2016-09-28T10:27:25Z

+		let account_trie = try!(TrieDB::new(state_db.as_hashdb(), &state_root).map_err(|e| Error::Custom(format!("Cannot open trie: {:?}", e))));
+		for (ref account_key, _) in account_trie.iter() {
+			let account_key_hash = H256::from_slice(&account_key);
+			bloom.set(account_key_hash.sha3().as_slice());


account_key_hash is an address hash already, no need to sha3() it here

arkpar · 2016-09-28T10:27:57Z

+	try!(db.write(batch));
+
+	trace!(target: "migration", "Finished bloom update");
+	println!("Done.");


should be removed

arkpar · 2016-09-28T10:32:13Z

+		}
+
+		let bloom = Bloom::from_parts(&bloom_parts, hash_count as u32);
+		trace!(target: "account_bloom", "Bloom is {:?} full, hash functions number = {:?}", bloom.how_full(), hash_count);


"hash functions number" -> "hash function count" or "number of hash functions"

NikVolf · 2016-09-28T10:33:24Z

+
+	trace!(target: "migration", "Generated {} bloom updates", bloom_journal.entries.len());
+
+	let batch = DBTransaction::new(&db);


Unsure if this can or can not happend concurrently
would be nice if @rphmeier review this

Becomes a non-issue if you switch the strategy to the one described in my other comment.
DBTransaction exists entirely outside of the rocksdb layer so it's safe itself, and then db.write may race against any other writes. Afaik the migrations are single-threaded so it probably shouldn't.

coveralls · 2016-09-28T11:36:18Z

Coverage decreased (-0.03%) to 87.377% when pulling 039c345 on state-cache-bloom into facab31 on beta.

arkpar · 2016-09-28T12:02:44Z

+		return Ok(())
+	}
+
+	println!("Adding accounts bloom (one-time upgrade). Please don't close parity.");


use info! instead of println!
"Please don't close" should be irrelevant. The whole upgrade should be one transaction or pa process that can be canceled

All migration messages are displayed using println!

It is indeed in one transaction, so i will remove this pray

gavofyork · 2016-09-28T12:27:09Z

@rphmeier should sign off, too.

rphmeier

couple issues with the migration, haven't looked at the bloom filter crate itself but the API calls look right.

rphmeier · 2016-09-28T12:48:59Z

+// in-place upgrades that do nothing when called repeatedly
+fn run_inplace_upgrades(path: &Path) -> Result<(), Error> {
+	try!(migrations::upgrade_account_bloom(path));
+	Ok(())


I don't think this strategy will work once we add more migrations on top of this one, since we need this account bloom migration to happen before others which may affect it.

Why not change Migration to supply Arc<Database> as source, and copy everything over for each column, and then additionally if column == COL_STATE, you also do the same stuff as upgrade_account_bloom would?

I think this would also require backporting my pre_columns fixes from master as consolidated db migrations are currently broken.

it's perfectly safe to run this migration any time on the upgraded database - it just won't do anything

any further upgrades should just go after this line

i don't want to introduce any new logic to the migration in beta and at this pr

rphmeier · 2016-09-28T12:50:35Z

+
+	trace!(target: "migration", "Generated {} bloom updates", bloom_journal.entries.len());
+
+	let batch = DBTransaction::new(&db);


Becomes a non-issue if you switch the strategy to the one described in my other comment.
DBTransaction exists entirely outside of the rocksdb layer so it's safe itself, and then db.write may race against any other writes. Afaik the migrations are single-threaded so it probably shouldn't.

rphmeier · 2016-09-28T12:51:40Z

+//! Bloom upgrade
+
+use client::{DB_COL_EXTRA, DB_COL_HEADERS, DB_NO_OF_COLUMNS, DB_COL_STATE};
+use state_db::{ACCOUNT_BLOOM_SPACE, DEFAULT_ACCOUNT_PRESET, StateDB};


all constants should be reproduced in the migration implementation for backwards compatibility -- there is no guarantee that they will remain the same as they were at the database version you're migrating.

it's not a migration strictly, it's inplace upgrade
it's perfectly guaranteed that the columns should be the same otherwise all other logic will instantly fail

rphmeier · 2016-09-28T12:54:17Z

-						Ok(Some(acc)) => AccountEntry::Cached(Account::from_rlp(acc)),
-						Ok(None) => AccountEntry::Missing,
-						Err(e) => panic!("Potential DB corruption encountered: {}", e),
+					let maybe_acc = if self.db.check_account_bloom(a) {


this check can be saved just by caching the value of the previous query

can't see how

rphmeier · 2016-09-28T12:54:58Z

+pub const ACCOUNT_BLOOM_SPACE: usize = 1048576;
+pub const DEFAULT_ACCOUNT_PRESET: usize = 1000000;
+
+pub const ACCOUNT_BLOOM_HASHCOUNT_KEY: &'static[u8] = b"account_hash_count";


style: &'static [u8]

rphmeier · 2016-09-28T12:56:11Z

+			.expect("Low-level database error");
+
+		hash_count_entry.is_some()
+	}


this function is only used for the migration; maybe it should be implemented there? (especially as it relies on external constants)

coveralls · 2016-09-28T13:21:14Z

Coverage decreased (-0.03%) to 87.377% when pulling e25441d on state-cache-bloom into facab31 on beta.

coveralls · 2016-09-28T15:01:53Z

Coverage decreased (-0.02%) to 87.392% when pulling def6343 on state-cache-bloom into facab31 on beta.

NikVolf added the A0-pleasereview 🤓 Pull request needs code review. label Sep 27, 2016

NikVolf added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 27, 2016

gavofyork added this to the 1.3.2 milestone Sep 27, 2016

gavofyork added B0-patch M4-core ⛓ Core client code / Rust. labels Sep 27, 2016

arkpar reviewed Sep 27, 2016

View reviewed changes

NikVolf changed the base branch from state-cache to beta September 27, 2016 19:41

NikVolf and others added 8 commits September 27, 2016 22:46

proper bloom

5207927

incremental bloom updates

5efc137

crate update

d2217f7

return of the column

dc95ee2

fix n^2 byteorder write

82c08bd

add notes to funs

1f7036e

working bloom commits

12abc31

Optimizations

c33e442

NikVolf force-pushed the state-cache-bloom branch from d6e3b15 to 67e68e9 Compare September 27, 2016 20:57

bloom diag

c45e992

arkpar force-pushed the state-cache-bloom branch from 33e181a to c45e992 Compare September 27, 2016 22:52

migration basic

260e75f

arkpar force-pushed the state-cache-bloom branch from b73c7ce to 260e75f Compare September 27, 2016 23:12

migration ongoing

c2022fa

migration finalizing

f3b094a

mingration api workarounds

a55e108

NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 28, 2016

fix test_client setups

ee36df0

snapshot bloom update

ee34d0e

arkpar suggested changes Sep 28, 2016

View reviewed changes

NikVolf commented Sep 28, 2016

View reviewed changes

NikVolf added 2 commits September 28, 2016 13:39

review fixes

2db6e7e

just forward keys in the migration

039c345

arkpar approved these changes Sep 28, 2016

View reviewed changes

migration extra tracing

1bf7f46

arkpar reviewed Sep 28, 2016

View reviewed changes

fix migration path

f4b5de8

remove close pray

e25441d

rphmeier suggested changes Sep 28, 2016

View reviewed changes

review issues

def6343

arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 29, 2016

arkpar merged commit 4d11598 into beta Sep 29, 2016

arkpar deleted the state-cache-bloom branch October 3, 2016 15:34


		trace!(target: "migration", "Generated {} bloom updates", bloom_journal.entries.len());

		let batch = DBTransaction::new(&db);

Conversation

NikVolf commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rphmeier commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NikVolf commented Sep 27, 2016

Uh oh!

gavofyork commented Sep 27, 2016

Uh oh!

arkpar Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 27, 2016

Uh oh!

coveralls commented Sep 27, 2016

Uh oh!

coveralls commented Sep 27, 2016

Uh oh!

coveralls commented Sep 27, 2016

Uh oh!

coveralls commented Sep 28, 2016

Uh oh!

coveralls commented Sep 28, 2016

Uh oh!

coveralls commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikVolf Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Sep 28, 2016

Uh oh!

rphmeier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikVolf Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 28, 2016

Uh oh!

coveralls commented Sep 28, 2016

Uh oh!

NikVolf commented Sep 27, 2016 •

edited

Loading

rphmeier commented Sep 27, 2016 •

edited

Loading

arkpar Sep 27, 2016 •

edited

Loading

NikVolf Sep 28, 2016 •

edited

Loading

NikVolf Sep 28, 2016 •

edited

Loading