Handle reorganizations in the state cache #2490

arkpar · 2016-10-06T08:37:53Z

Tests pending

coveralls · 2016-10-06T10:04:26Z

Coverage increased (+0.05%) to 86.464% when pulling abbf0ef on state-cache-reorg into ecf098e on master.

rphmeier · 2016-10-06T10:49:27Z

ethcore/src/state_db.rs

+
+	// Check if the account can be returned from cache by matching current block parent hash against canonical
+	// state and filtering out account modified in later blocks.
+	fn is_allowed(addr: &Address, parent_hash: &Option<H256>, modifications: &VecDeque<BlockChanges>) -> bool {


isn't this a relatively expensive check to do on each cache lookup? Can it be made more efficient by creating a view over a specific subset of the cache (excluding non-canonical entries based on a given block hash) which can be queried in constant time?

It's not expensive at all compared to the database or even HashMap lookup. In most cases it will break out on the first loop iteration. Current implementation support sharing the cache with other StateDB objects, which can commit independently. Creating a "view" would make it impossible.

gavofyork · 2016-10-06T11:50:23Z

ethcore/src/client/client.rs

@@ -459,6 +458,8 @@ impl Client {
 			enacted: route.enacted.clone(),
 			retracted: route.retracted.len()
 		});
+		let is_canon = route.omitted.len() == 0;


weird way of determining canonicality - do you really mean is_canon? what's wrong with the method above? probably more direct to check whether this block is the last enacted.

gavofyork · 2016-10-06T11:55:25Z

ethcore/src/state/mod.rs

@@ -486,7 +486,7 @@ impl State {
 		let mut addresses = self.cache.borrow_mut();


commit_cache -> what does this even mean? a "cache" cannot be committed - it is simply a speedily accessible version of what is on a secondary storage device (i.e. disk). if it's something that has changed without its backing store then it's a "buffer".

gavofyork · 2016-10-06T11:56:31Z

ethcore/src/state/mod.rs

@@ -486,7 +486,7 @@ impl State {
 		let mut addresses = self.cache.borrow_mut();
 		trace!("Committing cache {:?} entries", addresses.len());
 		for (address, a) in addresses.drain().filter(|&(_, ref a)| !a.is_dirty()) {
-			self.db.cache_account(address, a.account);
+			self.db.cache_account(address, a.account, a.state == AccountState::Commited);


cache_account doesn't particularly follow as an operation i might want to do as part of commit_cache. one suggests loading from disk into memory and the other suggests propagating from memory back to disk.

gavofyork · 2016-10-06T12:03:57Z

ethcore/src/state_db.rs

+	modifications: VecDeque<BlockChanges>,
+}
+
+struct CacheQueueItem {


docs needed.

gavofyork · 2016-10-06T12:04:04Z

ethcore/src/state_db.rs

+#[derive(Debug)]
+// Accumulates a list of accounts changed in a block.
+struct BlockChanges {
+	number: BlockNumber,


more docs please

gavofyork · 2016-10-06T12:04:24Z

ethcore/src/state_db.rs

+	parent_hash: Option<H256>,
+	// Hash of the committing block
+	commit_hash: Option<H256>,
+	// Number of the committing block


explain what None means, too.

gavofyork · 2016-10-06T12:04:34Z

ethcore/src/state_db.rs

 	account_bloom: Arc<Mutex<Bloom>>,
+	// Hash of the block on top of which this instance was created
+	parent_hash: Option<H256>,


explain what None means, too.

gavofyork · 2016-10-06T12:04:39Z

ethcore/src/state_db.rs

+	// Hash of the block on top of which this instance was created
+	parent_hash: Option<H256>,
+	// Hash of the committing block
+	commit_hash: Option<H256>,


explain what None means, too.

gavofyork · 2016-10-06T12:06:18Z

ethcore/src/state_db.rs

@@ -34,6 +38,24 @@ pub const ACCOUNT_BLOOM_HASHCOUNT_KEY: &'static [u8] = b"account_hash_count";
 struct AccountCache {
 	/// DB Account cache. `None` indicates that account is known to be missing.
 	accounts: LruCache<Address, Option<Account>>,
+	/// Accounts changed in recent blocks. Ordered by block number.


are they all strictly parent-child?

no, there can be multiple blocks with the same number

gavofyork · 2016-10-06T12:14:22Z

ethcore/src/state_db.rs

 		Ok(records)
 	}

+	/// Update canonical cache. This should be called after the block has been commited and the


"Update canonical cache" not a particularly enlightening description of what this mammoth function is meant to be doing. A lot more detail needed. What is its purpose and how does it achieve that?

arkpar · 2016-10-06T13:38:13Z

Renamed and documented a few methods

gavofyork · 2016-10-06T14:00:16Z

ethcore/src/state_db.rs

 		Ok(records)
 	}

+	/// Apply pending cache changes and synchronize canonical


"pending cache changes" -> is it fair to say these are "cache changes" but "buffered changes" or "uncommitted changes"?

All the changes are already committed to the disk at this point. "Buffered" works

maybe "recently committed cache"? if they are on disk then it's completely fair to call it a cache.

again, most of these don't get "committed" anywhere. Thy are just transferred from State cache to StateDB cache in atomic manner.

gavofyork · 2016-10-06T14:06:29Z

ethcore/src/state_db.rs

-		self.cache_overlay.clear();
-		let mut cache = self.account_cache.lock();
-		cache.accounts.clear();
+	/// Add pending cache change.


would prefer to avoid "cache changes" or even "cache modifications". "buffered changes" or "uncommitted changes" are better. if we really are mixing caching and buffering into the same data structure then stlil prefer to rename "cache changes" to "dirty cache entries".

Not sure what you mean by "dirty". Some of these changes are just accounts cached in the State locally and were not modified at all.

"Add pending cache change" suggests they were all modified.

Cache change means here that cache is modified, not the data that is cached. I.e. something is read from disk and added to the cache: cache is modified.

i see - in that case call it cache_populated or more_data_cached?

gavofyork · 2016-10-06T14:16:19Z

ethcore/src/state_db.rs

+		// all its parent until we hit the canonical block,
+		// checking against all the intermediate modifications.
+		let mut iter = modifications.iter();
+		while let Some(ref m) = iter.next() {


what if they're the same number and containing the account, but not the block in question? shouldn't it ignore it?

It should check against changes made in all children of the parent block and canonical children of the first canonical parent of the parent block. Completely correct check would be to complicated and error-prone gaining little. This implementation is more simple although it may result in some of the accounts being re-cached in stale blocks. This is a rare case and it does not affect performance because most of the blocks access a lot of accounts, but only modify a few.

coveralls · 2016-10-06T14:30:47Z

Coverage increased (+0.05%) to 86.457% when pulling aab0a59 on state-cache-reorg into ecf098e on master.

arkpar · 2016-10-06T15:48:24Z

Renamed "pending" to buffered and added a baic test.

coveralls · 2016-10-06T16:23:07Z

Coverage increased (+0.02%) to 86.434% when pulling 666b0be on state-cache-reorg into 6c1b2fb on master.

coveralls · 2016-10-06T19:51:07Z

Coverage increased (+0.05%) to 86.488% when pulling 79bbc26 on state-cache-reorg into 0c7a287 on master.

gavofyork · 2016-10-07T09:53:33Z

ethcore/src/state/mod.rs

@@ -523,11 +523,12 @@ impl State {
 		Ok(())
 	}

-	fn commit_cache(&mut self) {
+	/// Merge local cache into shared canonical state cache.
+	fn update_shared_cache(&mut self) {


propagate_to_global_cache

gavofyork · 2016-10-07T09:56:17Z

ethcore/src/state_db.rs

 		Ok(records)
 	}

+	/// Apply buffered cache changes and synchronize canonical


"buffered cache changes" what does this mean? "cache change" here presumably doesn't mean "population form disk"? "Apply" to what?

coveralls · 2016-10-07T10:26:01Z

Coverage increased (+0.06%) to 86.494% when pulling d6480e3 on state-cache-reorg into 0c7a287 on master.

gavofyork · 2016-10-07T10:26:36Z

ethcore/src/state_db.rs

-		let mut cache = self.account_cache.lock();
-		cache.accounts.clear();
+	/// Add pending cache change.
+	/// The change is queued to be applied in `commit`.


if it's a "change", why allow modified == false?

gavofyork · 2016-10-07T10:43:26Z

ethcore/src/state_db.rs

 struct AccountCache {
 	/// DB Account cache. `None` indicates that account is known to be missing.
 	accounts: LruCache<Address, Option<Account>>,
+	/// Accounts changed in recently committed blocks. Ordered by block number.


"Information on the modifications in recently committed blocks; specifically which addresses
changed in which block."

gavofyork · 2016-10-07T10:43:43Z

ethcore/src/state_db.rs

@@ -42,28 +74,32 @@ struct AccountCache {
 /// For canonical clones cache changes are accumulated and applied


"cache changes"

gavofyork · 2016-10-07T10:44:36Z

ethcore/src/state_db.rs

@@ -42,28 +74,32 @@ struct AccountCache {
 /// For canonical clones cache changes are accumulated and applied
 /// on commit.
 /// For non-canonical clones cache is cleared on commit.


"For non-canonical clones, any dirty entries are dropped on commit."

coveralls · 2016-10-07T11:51:32Z

Coverage increased (+0.05%) to 86.44% when pulling 59167f3 on state-cache-reorg into 4655fd0 on master.

* commit '72ec9366ad45526d16f6c78b5dfb32d16ba5aa6c': Handle reorganizations in the state cache (#2490) terminate after 30 seconds (#2513)

* js: (228 commits) registration in place Backports to master (#2530) lookup hash ethcore_hashContent call single input for commit/filename basic githubhint layout Handle reorganizations in the state cache (#2490) terminate after 30 seconds (#2513) allow updates of the secure token Using pending block only if not old (#2514) Caching optimizations (#2505) rework connection display basic test for manual token Fixed overflow panic in handshake_panic (#2495) Trim password from file (#2503) Fixing RPC Filter conversion to EthFilter (#2500) init token updates take place initial token connection - WIP Fixing error message for transactions (#2496) Adjustable stack size for EVM (#2483) ... # Conflicts: # js/src/dapps/registry/Application/application.js # js/src/dapps/registry/Container.js # js/src/dapps/registry/actions.js # js/src/dapps/registry/reducers.js

* js: (23 commits) alignment back to normal Print backtrace on panic (#2535) NEVER export class instance functions stop wrongly using main app IdentityIcon pass value through as-is events is using a proper table fixes registration in place Backports to master (#2530) lookup hash ethcore_hashContent call single input for commit/filename basic githubhint layout Handle reorganizations in the state cache (#2490) terminate after 30 seconds (#2513) registry: fix setting A records registry: reducer returned invalid state registry: don't hash for A records registry: sha3 value first registry: connect record management component ...

arkpar added A0-pleasereview 🤓 Pull request needs code review. B0-patch labels Oct 6, 2016

rphmeier reviewed Oct 6, 2016

View reviewed changes

gavofyork reviewed Oct 6, 2016

View reviewed changes

ethcore/src/state_db.rs

modifications: VecDeque<BlockChanges>,

}

struct CacheQueueItem {

Copy link

Contributor

gavofyork Oct 6, 2016

Choose a reason for hiding this comment

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

docs needed.

gavofyork reviewed Oct 6, 2016

View reviewed changes

arkpar force-pushed the state-cache-reorg branch from aab0a59 to 666b0be Compare October 6, 2016 15:48

arkpar force-pushed the state-cache-reorg branch from 666b0be to 79bbc26 Compare October 6, 2016 19:16

gavofyork reviewed Oct 7, 2016

View reviewed changes

arkpar added 2 commits October 7, 2016 12:37

Handle reorganizations in the state cache

5d7e72f

Renamed and documented a few methods

ab309e6

arkpar added 4 commits October 7, 2016 12:37

Basic test

fd37b0f

Renamed pending to buffered

c8f37c5

Updated cache on sealed block

455279d

More renames and updated documentation

0f80b7b

arkpar force-pushed the state-cache-reorg branch from d6480e3 to 0f80b7b Compare October 7, 2016 10:40

gavofyork reviewed Oct 7, 2016

View reviewed changes

gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 7, 2016

Minor doc tweaks

59167f3

arkpar force-pushed the state-cache-reorg branch from 3ce5988 to 59167f3 Compare October 7, 2016 11:00

gavofyork merged commit 72ec936 into master Oct 7, 2016

gavofyork deleted the state-cache-reorg branch October 7, 2016 11:34

jacogr added a commit that referenced this pull request Oct 7, 2016

Merge commit '72ec9366ad45526d16f6c78b5dfb32d16ba5aa6c' into js

6c6c97e

* commit '72ec9366ad45526d16f6c78b5dfb32d16ba5aa6c': Handle reorganizations in the state cache (#2490) terminate after 30 seconds (#2513)

		@@ -486,7 +486,7 @@ impl State {
		let mut addresses = self.cache.borrow_mut();

		@@ -42,28 +74,32 @@ struct AccountCache {
		/// For canonical clones cache changes are accumulated and applied

Handle reorganizations in the state cache #2490

Handle reorganizations in the state cache #2490

Conversation

arkpar commented Oct 6, 2016 • edited Loading

coveralls commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork Oct 6, 2016 • edited Loading

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

arkpar commented Oct 6, 2016

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

gavofyork Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork Oct 6, 2016 • edited Loading

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

coveralls commented Oct 6, 2016

arkpar commented Oct 6, 2016

coveralls commented Oct 6, 2016

coveralls commented Oct 6, 2016

gavofyork Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 7, 2016

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

coveralls commented Oct 7, 2016

arkpar commented Oct 6, 2016 •

edited

Loading

gavofyork Oct 6, 2016 •

edited

Loading

gavofyork Oct 6, 2016 •

edited

Loading

gavofyork Oct 6, 2016 •

edited

Loading

gavofyork Oct 7, 2016 •

edited

Loading