Configurable history size in beta #2587

arkpar · 2016-10-12T11:41:54Z

No description provided.

rphmeier

There should also be a mechanism for detecting a reorganization which is too large and rebuilding old states in the journal so it's not a fatal error which ruins the database forever. (now that tx verification happens after block import, we could just check if route.retracted > self.history). Maybe not for a hotfix release, but this is important for the future.

rphmeier · 2016-10-12T11:52:12Z

ethcore/src/client/client.rs

+				while era <= latest - config.history {
+					trace!("Removing era {}", era);
+					let batch = DBTransaction::new(&db);
+					try!(state_db.journal_db_mut().commit_old(&batch, era, &chain.block_hash(era).expect("Old block not found in the database")));


This is exactly why I made https://github.com/ethcore/parity/pull/2329. There is no need for commit, commit_old since they work the same way.

It should just mark_canonical in a loop.

I understand this is beta, but I would strongly suggest using that mechanism for the version that lands on master.

rphmeier · 2016-10-12T11:53:30Z

ethcore/src/client/client.rs

+		if let (Some(earliest), Some(latest)) = (state_db.journal_db().earliest_era(), state_db.journal_db().latest_era()) {
+			if latest > earliest && latest - earliest > config.history {
+				let mut era = earliest;
+				while era <= latest - config.history {


for era in earliest..(latest - config.history + 1) { is a bit less error-prone.

rphmeier · 2016-10-12T11:56:05Z

parity/cli.rs

@@ -219,6 +219,8 @@ Footprint Options:
                           fast - maintain journal overlay. Fast but 50MB used.
                           auto - use the method most recently synced or
                           default to fast if none synced [default: auto].
+  --pruning-history NUM    Set a number of recent states to keep when pruning
+                           is active. [default: 64].


Isn't ~128 a bit safer? Although the mainnet isn't often susceptible to reorgs beyond 5 or so, the testnet, ETC, expanse, or private chains don't have quite the same level of stability.

They are more stable than ETH currently. ETH has higher uncle rate because of the attack.

rphmeier · 2016-10-12T12:00:04Z

parity/configuration.rs

@@ -73,6 +73,7 @@ impl Configuration {
 	pub fn into_command(self) -> Result<Cmd, String> {
 		let dirs = self.directories();
 		let pruning = try!(self.args.flag_pruning.parse());
+		let pruning_history = self.args.flag_pruning_history;


History below a certain amount shouldn't be allowed until we have a way to recover from a prehistoric reorganization.

rphmeier · 2016-10-12T13:15:18Z

ethcore/src/client/client.rs

@@ -187,6 +188,19 @@ impl Client {
 			try!(db.write(batch).map_err(ClientError::Database));
 		}

+		trace!("Cleanup journal: DB Earliest = {:?}, Latest = {:?}", state_db.journal_db().earliest_era(), state_db.journal_db().latest_era());
+		let history = cmp::max(MIN_HISTORY_SIZE, config.history);


an issue with this approach is that the user has no idea that their pruning argument is ignored.

rphmeier · 2016-10-12T13:15:53Z

ethcore/src/client/client.rs

+		if let (Some(earliest), Some(latest)) = (state_db.journal_db().earliest_era(), state_db.journal_db().latest_era()) {
+			if latest > earliest && latest - earliest > history {
+				for era in earliest..(latest - history + 1) {
+					trace!("Removing era {}", era);


The new traces here have no target.

coveralls · 2016-10-12T13:46:15Z

Coverage decreased (-0.03%) to 87.213% when pulling 4b04d6b on history-config into 66f6845 on beta.

coveralls · 2016-10-12T14:29:42Z

Coverage increased (+0.02%) to 87.267% when pulling 3784c6c on history-config into 8fba01a on beta.

gavofyork · 2016-10-12T16:32:35Z

ethcore/src/client/client.rs

@@ -73,6 +74,7 @@ pub use blockchain::CacheSize as BlockChainCacheSize;

 const MAX_TX_QUEUE_SIZE: usize = 4096;
 const MAX_QUEUE_SIZE_TO_SLEEP_ON: usize = 2;
+const MIN_HISTORY_SIZE: u64 = 64;


why so high for the limit? seems a bit odd when default == min limit

gavofyork · 2016-10-12T16:34:51Z

would have liked to see at least one test.

coveralls · 2016-10-12T18:21:30Z

Coverage increased (+0.1%) to 87.342% when pulling 03f9a32 on history-config into 8fba01a on beta.

coveralls · 2016-10-12T18:33:04Z

Coverage increased (+0.1%) to 87.354% when pulling 03f9a32 on history-config into 8fba01a on beta.

rphmeier · 2016-10-12T18:43:10Z

ethcore/src/block.rs

@@ -395,6 +395,10 @@ impl<'x> OpenBlock<'x> {
 			uncle_bytes: uncle_bytes,
 		}
 	}
+
+	#[cfg(test)]
+	/// Return mutable block reference. To be used in tests only.


attributes under doc

arkpar added 2 commits October 12, 2016 01:44

Reduce DB history

3024043

Configurable history size

37da591

arkpar added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 12, 2016

rphmeier reviewed Oct 12, 2016

View reviewed changes

rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 12, 2016

rphmeier reviewed Oct 12, 2016

View reviewed changes

Set min history size

3784c6c

arkpar force-pushed the history-config branch from 4b04d6b to 3784c6c Compare October 12, 2016 13:54

arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 12, 2016

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

gavofyork reviewed Oct 12, 2016

View reviewed changes

gavofyork 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 Oct 12, 2016

arkpar added 2 commits October 12, 2016 19:24

Test

2f22442

Fixed a test and reduced the limit

03f9a32

arkpar 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 Oct 12, 2016

gavofyork added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 12, 2016

rphmeier reviewed Oct 12, 2016

View reviewed changes

arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Oct 12, 2016

arkpar merged commit 6b4d0ce into beta Oct 12, 2016

gavofyork deleted the history-config branch October 13, 2016 08:41

rphmeier mentioned this pull request Oct 13, 2016

Configurable history size in master #2606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable history size in beta #2587

Configurable history size in beta #2587

arkpar commented Oct 12, 2016

rphmeier left a comment

rphmeier Oct 12, 2016

rphmeier Oct 12, 2016

arkpar Oct 12, 2016

rphmeier Oct 12, 2016

rphmeier Oct 12, 2016

arkpar Oct 12, 2016

rphmeier Oct 12, 2016 •

edited

rphmeier Oct 12, 2016

rphmeier Oct 12, 2016

coveralls commented Oct 12, 2016

coveralls commented Oct 12, 2016

gavofyork Oct 12, 2016 •

edited

gavofyork commented Oct 12, 2016

coveralls commented Oct 12, 2016

coveralls commented Oct 12, 2016

rphmeier Oct 12, 2016

Configurable history size in beta #2587

Configurable history size in beta #2587

Conversation

arkpar commented Oct 12, 2016

rphmeier 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

rphmeier Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 12, 2016

coveralls commented Oct 12, 2016

gavofyork Oct 12, 2016 • edited

Choose a reason for hiding this comment

gavofyork commented Oct 12, 2016

coveralls commented Oct 12, 2016

coveralls commented Oct 12, 2016

Choose a reason for hiding this comment

rphmeier Oct 12, 2016 •

edited

gavofyork Oct 12, 2016 •

edited