Blockchain module cleanup #524

debris · 2016-02-27T01:28:29Z

changes:

split block_to_extras_update function into 6 smaller functions. Code is cleaner, more maintainable and readable.
fully separated preparing extas update from applying it to a batch.
split blockchain.rs into few separate files. This file was becoming a behemoth.
fixed cleaning up blockchain caches.

…rocess

debris · 2016-02-27T09:29:20Z

I recommend opening blockchain.rs file in new tab. Git diff is hardly readable.

gavofyork · 2016-02-27T14:55:10Z

needs a rebase. also it's quite big - i presume no logic has been added in this PR?

debris · 2016-02-27T14:56:17Z

no, I will rebase

gavofyork · 2016-02-27T14:58:49Z

ethcore/src/blockchain/blockchain.rs

-		let mut to_details = to.0.clone();
-		let mut current_from = from.1.clone();
-		let mut current_to = to.1.clone();
+		let mut from_details = self.block_details(&from).expect(&format!("0. Expected to find details for block {:?}", from));


this probably shouldn't panic.

Actually here it should. Missing block details means one of two things:

our db is malformed (or not complete).

this function is being used incorrectly.

asserting on an external factor such as what is or is not in some particular portion of the database is not particularly defensive coding.

the only proper use of panics/assertions in live code is for specifying logical certainties (assert!(1 != 0);) or absolute assertions over the high-level design that are themselves well tested (e.g. TrieDBMut asserts that nodes are in the database, but the trie has comprehensive tests and is written to in only two places in the codebase, with no forseeable alterations, so it's possible to convince oneself that we've structured things so this panic cannot happen; similarly, all data is checked first with UntrustedRLP before it is allowed to be interpreted with the panicking RLP).

it is sometimes reasonable to use them to specify a function's preconditions, this in this case they should be simple enough that it can be done up front in the code prior to the implementation proper, functioning essentially as symbolic documentation (e.g. fn sqrt(x: int) -> int { assert!(x >= 0); ... })

if you cannot point to comprehensive tests and, from the structure of the software's design see that your assertion can never be false, then it should not panic, but rather fail-over at a higher level.

in this case, there is a mere "expectation" that the database happens to have all entries in it that the algorithm decides it needs given the argument of unknown provenance. this is not a trivial precondition and could therefore not function as symbolic documentation.

in short, unless you are willing to bet the farm on this function never being used "improperly" (not to mention any DB manipulation code failing), and can prove that by design it cannot happen now or in the future, then a panic is unacceptable in this position.

I see. I will fix it in next prs.

…chain_cleanup

gavofyork · 2016-02-27T15:08:33Z

it's not especially clear that no logic has been added/changed. maybe add a commentary so we know which functions have been moved where?

given that the tests pass, i'm inclined to let it go this time, but i think for complex refactoring, when it's not clear that it is just repotting, either break down the PRs into two or more smaller refactorings or give clear commentary about what's going on in the PR (e.g. line comments seem to work quite well to guide the reviewer).

…chain_cleanup

arkpar · 2016-02-27T16:10:33Z

There should be a test that checks if the blockchain cache has been updated correctly after a fork

…chain_cleanup

gavofyork · 2016-02-27T17:44:42Z

can "fixed cleaning up blockchain caches" be placed in a separate PR for ease of review? the other 3 changes this PR makes change no logic.

debris · 2016-02-27T19:59:39Z

@gavofyork

it's not especially clear that no logic has been added/changed. maybe add a commentary so we know which functions have been moved where?

moved blockchain.rs to blockchain/blockchain.rs
added BlockInfo and BlockLocation structs (blockchain/block_info.rs). They replaced unreadable if statements and comments
split block_to_extras_update into 6 functions. Each of this functions updates only one extras type.
- block_info
- prepare_block_hashes_update
- prepare_block_details_update
- prepare_block_receipts_update
- prepare_transaction_addresses_update
- prepare_block_blooms_update
moved BloomIndexer and it's tests to blockchain/bloom_indexer.rs
moved BestBlock to blockchain/best_block.rs
moved CacheSize to blockchain/cache.rs
moved TreeRoute to blockchain/tree_route.rs`
moved ExtrasUpdate to blockchain/update.rs
removed tree_route_aux function which was not needed (previously used in block_to_extras_update function)

@gavofyork

can "fixed cleaning up blockchain caches" be placed in a separate PR for ease of review? the other 3 changes this PR makes change no logic.

These are just two lines. block hashes and transaction addresses.

@arkpar

There should be a test that checks if the blockchain cache has been updated correctly after a fork

There are already few tests which check updating block_details && blocks_blooms after a fork. I think it's best to add more test for forking in next prs, cause these tests will make this pr significantly bigger.

gavofyork · 2016-02-27T21:09:15Z

ok to merge on my end, though i'd like to see the fork tests and removal of asserts in the not-too-distant future. perhaps introduce an issue to track?

debris · 2016-02-27T21:45:59Z

I've added #529 and #530

debris · 2016-02-27T22:36:00Z

Started working on sweeping panics in the other branch. Does tree_route function look better? ethcore/parity@8c61131#diff-422bab2793f9fb234587f1dadcf690d3R350

Blockchain module cleanup

debris added 3 commits February 27, 2016 01:37

blockchain split into few separate submodules, cleanup insert_block p…

53e8d99

…rocess

added docs and license headers

cd43e32

Merge branch 'master' into blockchain_cleanup

d57518d

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

debris added 2 commits February 27, 2016 10:19

moved creation of blockchains db transaction to apply_update function

01f69ca

updated ExtrasUpdate function

7b3613e

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 Feb 27, 2016

gavofyork reviewed Feb 27, 2016
View reviewed changes

Merge branch 'master' of https://github.com/ethcore/parity into block…

53b19e9

…chain_cleanup

Merge branch 'master' of https://github.com/ethcore/parity into block…

fe13d28

…chain_cleanup

Merge branch 'master' of https://github.com/ethcore/parity into block…

1d19196

…chain_cleanup

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

description for prepare_block_blooms_update function

1cc719d

replaced match with if to shorten the code

1481f3f

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

gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 27, 2016

NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Feb 29, 2016

NikVolf added a commit that referenced this pull request Feb 29, 2016

Merge pull request #524 from ethcore/blockchain_cleanup

f0c11fa

Blockchain module cleanup

NikVolf merged commit f0c11fa into master Feb 29, 2016

debris deleted the blockchain_cleanup branch March 10, 2016 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blockchain module cleanup #524

Blockchain module cleanup #524

debris commented Feb 27, 2016

debris commented Feb 27, 2016

gavofyork commented Feb 27, 2016

debris commented Feb 27, 2016

gavofyork Feb 27, 2016

debris Feb 27, 2016

gavofyork Feb 27, 2016

debris Feb 27, 2016

gavofyork commented Feb 27, 2016

arkpar commented Feb 27, 2016

gavofyork commented Feb 27, 2016

debris commented Feb 27, 2016

gavofyork commented Feb 27, 2016

debris commented Feb 27, 2016

debris commented Feb 27, 2016

Blockchain module cleanup #524

Blockchain module cleanup #524

Conversation

debris commented Feb 27, 2016

debris commented Feb 27, 2016

gavofyork commented Feb 27, 2016

debris commented Feb 27, 2016

gavofyork Feb 27, 2016

Choose a reason for hiding this comment

debris Feb 27, 2016

Choose a reason for hiding this comment

gavofyork Feb 27, 2016

Choose a reason for hiding this comment

debris Feb 27, 2016

Choose a reason for hiding this comment

gavofyork commented Feb 27, 2016

arkpar commented Feb 27, 2016

gavofyork commented Feb 27, 2016

debris commented Feb 27, 2016

gavofyork commented Feb 27, 2016

debris commented Feb 27, 2016

debris commented Feb 27, 2016