block cleanup #9117

debris · 2018-07-13T12:50:24Z

the goal of this pr is to reduce number of times block data is copied or hashed

debris · 2018-07-13T12:51:28Z

ethcore/src/blockchain/blockchain.rs

@@ -1081,11 +1080,11 @@ impl BlockChain {
 			let mut best_block = self.pending_best_block.write();
 			if is_best && update.info.location != BlockLocation::Branch {
 				batch.put(db::COL_EXTRA, b"best", &update.info.hash);
-				let block = encoded::Block::new(update.block.to_vec());


entire block was copied here

debris · 2018-07-15T09:52:23Z

ethcore/src/block.rs

@@ -116,8 +116,6 @@ pub struct ExecutedBlock {
 	pub traces: Tracing,
 	/// Hashes of last 256 blocks.
 	pub last_hashes: Arc<LastHashes>,
-	/// Finalization flag.
-	pub is_finalized: bool,


this flag is completely redundant and is never set to true

I think we're going to use it #9113? cc @andresilva

I think we won't implement Casper again in the short terms, so some of the #8401 refactorings might use a better interface. But I do think allowing finalized block is a feature that we may use in other engines.

if I see correctly, it's not used by #9113

Ah right. Aura only finalize ancestry blocks so it only needs BlockChain::mark_finalized. However, do we want to support instant finalization for some of our engines in the future? That's where we will need this flag.

I'm not sure whether POC-1 engine (which will replace the current Tendermint engine) will need this. The spec do mention instant finality. But I'm not really familiar with the algorithm.

However, do we want to support instant finalization for some of our engines in the future? That's where we will need this flag.

Than we should add this flag in the future. Imo, we shouldn't keep in our codebase anything that is not used right now

I can't say I fully agree with this, but it sounds okay to me. 👍

Based on the above rationale, however, another thing we need to remove is metadata (https://github.com/paritytech/parity/blob/21d3de236005732253dee442cfbcb52ebcbbc1e7/ethcore/src/block.rs#L119 and https://github.com/paritytech/parity/blob/21d3de236005732253dee442cfbcb52ebcbbc1e7/ethcore/src/blockchain/extras.rs#L155). Also introduced by #8401, but not used by current engines.

metadata removed! :)

We can remove this field as well:

https://github.com/paritytech/parity-ethereum/blob/a598be7b6e3b0d35006d6c25d99e966b002749c7/ethcore/src/blockchain/extras.rs#L156

Metadata or finalization has not been set by anyone, so we can just change the !is_short_version to be 5 RLP fields. :)

debris · 2018-07-15T09:52:56Z

ethcore/src/block.rs

-	}
-}
-
-impl ::parity_machine::WithMetadata for ExecutedBlock {


this interface is never used

debris · 2018-07-15T10:01:43Z

ethcore/src/block.rs

@@ -477,8 +447,8 @@ impl<'x> OpenBlock<'x> {
 		if s.block.header.transactions_root().is_zero() || s.block.header.transactions_root() == &KECCAK_NULL_RLP {


I think we should also get rid of these checks, cause they may lead to invalid blocks being produced. e.g

let closed_block = open_block.close(); let reopened_block = closed_block.reopen(engine); reopened_block.push_transactions(transactions); // close_and_lock is not recalculating `transactions_root` let locked_block = reopened_block.close_and_lock();

debris · 2018-07-15T10:05:28Z

ethcore/src/client/client.rs

@@ -298,7 +297,7 @@ impl Importer {

 						let transactions_len = closed_block.transactions().len();

-						let route = self.commit_block(closed_block, &header, &bytes, client);
+						let route = self.commit_block(closed_block, &header, encoded::Block::new(bytes), client);


in future, I'd like this function (and blockchain.rs functions) to take as argument some MetaBlock that contains both plain block and it's encoded version. Currently we are encoding and then decoding stuff that we just encoded. That's not efficient

debris · 2018-07-16T11:57:02Z

ethcore/src/client/client.rs

 			// Final commit to the DB
 			db.write_buffered(batch);
 			chain.commit();
 		}
 		db.flush().expect("DB flush failed.");
-		Ok(hash)
+		Ok(())


this value was never used, therefore not needed

debris · 2018-07-19T13:19:00Z

ethcore/src/blockchain/extras.rs

@@ -180,7 +177,8 @@ impl rlp::Decodable for BlockDetails {
 	fn decode(rlp: &rlp::Rlp) -> Result<Self, rlp::DecoderError> {
 		let use_short_version = match rlp.item_count()? {
 			4 => true,
-			6 => false,
+			// 6, cause we used to store metadata
+			5 | 6 => false,


to keep database backwards compatibility

Looks fine but I think we actually never had any 6 cases up to this point. 6 is possible after the Aura finality PR is merged. Right now we don't have any. It's also not possible to use any custom chain spec to reach the 6 case.

The only case where 6 was used is in #8654, but we're not using that code in the short-term.

Can we set this to 5? This was added recently and was only used in Casper FFG (which was never merged).

sorpaas

LGTM (but it looks like CI is broken right now!).

A final comment is on this:

https://github.com/paritytech/parity-ethereum/blob/10d05d7a1c41adc08550f883a01384fdacffde53/ethcore/src/blockchain/blockchain.rs#L1151

The term "metadata" is used because I used to call two fields is_finalized and metadata with a combined term "metadata". I think we can keep that because we still use is_finalized as a metadata. But just a note in case.

sorpaas · 2018-07-19T13:26:30Z

ethcore/src/blockchain/extras.rs

@@ -180,7 +177,8 @@ impl rlp::Decodable for BlockDetails {
 	fn decode(rlp: &rlp::Rlp) -> Result<Self, rlp::DecoderError> {
 		let use_short_version = match rlp.item_count()? {
 			4 => true,
-			6 => false,
+			// 6, cause we used to store metadata
+			5 | 6 => false,


Looks fine but I think we actually never had any 6 cases up to this point. 6 is possible after the Aura finality PR is merged. Right now we don't have any. It's also not possible to use any custom chain spec to reach the 6 case.

The only case where 6 was used is in #8654, but we're not using that code in the short-term.

andresilva

Good job on the cleanup and removing unnecessary copies. I have some mixed feelings about some of the stuff that was removed regarding finality and metadata that was just recently added by @sorpaas. We will need instant finality support if we want to integrate rhododendron. But I agree that we can re-add this when we need it (and also make it better since it will be the 2nd implementation 😉). LGTM, just a minor nit on BlockDetails rlp decoding.

andresilva · 2018-07-25T15:13:28Z

ethcore/src/blockchain/extras.rs

@@ -180,7 +177,8 @@ impl rlp::Decodable for BlockDetails {
 	fn decode(rlp: &rlp::Rlp) -> Result<Self, rlp::DecoderError> {
 		let use_short_version = match rlp.item_count()? {
 			4 => true,
-			6 => false,
+			// 6, cause we used to store metadata
+			5 | 6 => false,


Can we set this to 5? This was added recently and was only used in Casper FFG (which was never merged).

andresilva · 2018-07-25T15:26:16Z

@debris Rebase/merge master to fix conflicts with #9161.

fixed!

debris · 2018-07-30T09:19:28Z

@andresilva please check the pr again and approve/reject changes

debris · 2018-07-30T11:35:49Z

🎉

* 'master' of https://github.com/paritytech/parity: removed client error (openethereum#9253) Implement EIP-1052 (EXTCODEHASH) and fix several issues in state account cache (openethereum#9234) Improve Tracer documentation (openethereum#9237) Update Dockerfile (openethereum#9242) block cleanup (openethereum#9117) Increase the number of sessions. (openethereum#9203) add changelog for 1.11.8 stable and 2.0.1 beta (openethereum#9230) fix typo (openethereum#9232) Fix potential as_usize overflow when casting from U256 in miner (openethereum#9221) Allow old blocks from peers with lower difficulty (openethereum#9226) Removes duplicate libudev-dev from Dockerfile (openethereum#9220) snap: remove ssl dependencies from snapcraft definition (openethereum#9222) remove ssl from dockerfiles, closes openethereum#8880 (openethereum#9195) Insert PROOF messages for some cases in blockchain (openethereum#9141) [Chain] Add more bootnodes (openethereum#9174) ethcore: update bn version (openethereum#9217) deserialize block only once during verification (openethereum#9161) Simple build instruction fix (openethereum#9215) Added --tx-queue-no-early-reject flag to disable early tx queue rejects (openethereum#9143)

blockchain insert expects owned block instead of block reference

a0610a7

debris added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Jul 13, 2018

debris commented Jul 13, 2018

View reviewed changes

5chdn added this to the 2.1 milestone Jul 13, 2018

reduce a number of times a block is deserialized

b09d311

debris mentioned this pull request Jul 15, 2018

Removed redundant struct bounds and unnecessary data copying #9096

Merged

debris added 5 commits July 15, 2018 11:13

Merge branch 'master' into block_insert

21d429b

removed cached uncle_bytes from block

306bb01

removed is_finalized from OpenBlock

e60d907

removed unused parity_machine::WithMetadata trait

35c63ee

removed commented out code

09d4961

debris commented Jul 15, 2018

View reviewed changes

ethcore/src/block.rs

}

}

impl ::parity_machine::WithMetadata for ExecutedBlock {

Copy link

Collaborator Author

debris Jul 15, 2018

Choose a reason for hiding this comment

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

this interface is never used

debris commented Jul 15, 2018

View reviewed changes

debris added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 15, 2018

debris changed the title ~~blockchain insert expects owned block instead of block reference~~ block cleanup Jul 15, 2018

debris requested a review from sorpaas July 16, 2018 11:55

Merge branch 'master' into block_insert

21d3de2

debris commented Jul 16, 2018

View reviewed changes

5chdn modified the milestones: 2.1, 2.2 Jul 17, 2018

debris added 4 commits July 19, 2018 11:07

Merge branch 'master' into block_insert

461a428

remove unused metadata from block

e64a006

Merge branch 'master' into block_insert

a598be7

remove unused metadata from block

10d05d7

debris commented Jul 19, 2018

View reviewed changes

sorpaas approved these changes Jul 19, 2018

View reviewed changes

ddorgan closed this Jul 19, 2018

ddorgan reopened this Jul 19, 2018

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 22, 2018

andresilva previously requested changes Jul 25, 2018

View reviewed changes

debris added 2 commits July 27, 2018 20:41

Merge branch 'master' into block_insert

8409d11

BlockDetails extras may have at most 5 elements

9e894e2

andresilva approved these changes Jul 30, 2018

View reviewed changes

andresilva merged commit c54beba into master Jul 30, 2018

5chdn deleted the block_insert branch July 30, 2018 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

block cleanup #9117

block cleanup #9117

debris commented Jul 13, 2018

debris Jul 13, 2018

debris Jul 15, 2018

sorpaas Jul 16, 2018

debris Jul 16, 2018

sorpaas Jul 16, 2018

debris Jul 16, 2018

sorpaas Jul 16, 2018

debris Jul 19, 2018

sorpaas Jul 19, 2018

debris Jul 19, 2018

debris Jul 15, 2018

debris Jul 15, 2018 •

edited

Loading

debris Jul 15, 2018

debris Jul 16, 2018

debris Jul 19, 2018

sorpaas Jul 19, 2018

andresilva Jul 25, 2018

sorpaas left a comment

sorpaas Jul 19, 2018

andresilva left a comment

andresilva Jul 25, 2018

andresilva commented Jul 25, 2018

debris commented Jul 30, 2018

debris commented Jul 30, 2018

		@@ -477,8 +447,8 @@ impl<'x> OpenBlock<'x> {
		if s.block.header.transactions_root().is_zero() \|\| s.block.header.transactions_root() == &KECCAK_NULL_RLP {

block cleanup #9117

block cleanup #9117

Conversation

debris commented Jul 13, 2018

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

debris Jul 15, 2018 • 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

sorpaas left a 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

andresilva commented Jul 25, 2018

debris commented Jul 30, 2018

debris commented Jul 30, 2018

debris Jul 15, 2018 •

edited

Loading