Insert PROOF messages for some cases in blockchain #9141

sorpaas · 2018-07-17T07:21:06Z

Change non-test code to use expect and avoid unwrap.

ascjones · 2018-07-17T09:07:47Z

ethcore/src/blockchain/blockchain.rs

@@ -571,8 +571,8 @@ impl BlockChain {

 		{
 			// Fetch best block details
-			let best_block_total_difficulty = bc.block_details(&best_block_hash).unwrap().total_difficulty;
-			let best_block_rlp = bc.block(&best_block_hash).unwrap();
+			let best_block_total_difficulty = bc.block_details(&best_block_hash).expect("Best block is from a known block hash; a known block hash always comes with a known block detail; qed").total_difficulty;


This might benefit from breaking across lines, though not sure what our convention is here. There is some precedent of having expect on the following line in other parts of the codebase.

andresilva

Minor comments mostly opinionated. LGTM overall.

andresilva · 2018-07-17T15:05:44Z

ethcore/src/blockchain/blockchain.rs

@@ -536,7 +536,9 @@ impl BlockChain {
 		};

 		// load best block
-		let best_block_hash = match bc.db.key_value().get(db::COL_EXTRA, b"best").unwrap() {
+		let best_block_hash = match bc.db.key_value().get(db::COL_EXTRA, b"best")
+			.expect("Low level database error. Some issue with disk?")


Maybe we could mention that we're trying to get the best block?
"Low-level database error when fetching 'best' block." ?

andresilva · 2018-07-17T15:07:55Z

ethcore/src/blockchain/blockchain.rs

-			let raw_first = bc.db.key_value().get(db::COL_EXTRA, b"first").unwrap().map(|v| v.into_vec());
-			let mut best_ancient = bc.db.key_value().get(db::COL_EXTRA, b"ancient").unwrap().map(|h| H256::from_slice(&h));
+			let raw_first = bc.db.key_value().get(db::COL_EXTRA, b"first")
+				.expect("Low level database error. Some issue with disk?")


Same as above, we can mention that it's the "first" block and the "best ancient" block below.

andresilva · 2018-07-17T15:20:49Z

ethcore/src/blockchain/blockchain.rs

@@ -1350,11 +1359,13 @@ impl BlockChain {
 				}
 			},
 			BlockLocation::BranchBecomingCanonChain(ref data) => {
-				let ancestor_number = self.block_number(&data.ancestor).unwrap();
+				let ancestor_number = self.block_number(&data.ancestor)
+					.expect("Inserted block's ancestor number always exists; qed");


"hash belongs to an ancestor of an inserted block; ancestors of an inserted block are always available; block number of an inserted block is always available; qed"

Actually while writing this I realised that this could fail because the second condition doesn't always hold true (because of warp sync) right?

Ah looks like that's indeed. This function is called by insert_unordered_block which can get unordered?

It seems that this is fine, although all the logic is not really apparent (so we may consider to do some refactorings for this): In wrap sync, the location is always set to BlockLocation::CanonChain so we will never reach this match statement.

Thanks for looking this up. Yes, it's not clear from just looking at this code that invariant holds. But it makes sense that we do consider the warped block to be canon (what else can we do if we already trusted the warp?).

andresilva · 2018-07-17T15:21:46Z

ethcore/src/blockchain/blockchain.rs

 				let start_number = ancestor_number + 1;

 				let mut blooms: Vec<Bloom> = data.enacted.iter()
-					.map(|hash| self.block_header_data(hash).unwrap())
+					.map(|hash| self.block_header_data(hash)
+						 .expect("Inserted block's header data always exists; qed"))


"hash belongs to an inserted block; block header data of an inserted block is always available; qed"

ascjones · 2018-07-18T09:09:28Z

Tests failing: key_server_cluster::cluster::tests::ecdsa_signing_session_completes_if_node_does_not_have_a_share

sorpaas · 2018-07-18T12:54:37Z

Hmm this is weird. We shouldn't have changed anything in key_server_cluster. Even tried to restart the build once. :/

andresilva

LGTM

* '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)

Insert PROOF messages for some cases in blockchain

6413019

sorpaas added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M4-core ⛓ Core client code / Rust. labels Jul 17, 2018

sorpaas added this to the 2.1 milestone Jul 17, 2018

ascjones reviewed Jul 17, 2018

View reviewed changes

Break expect to its own line to avoid things being too long

5a63bbb

ascjones approved these changes Jul 17, 2018

View reviewed changes

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

andresilva reviewed Jul 17, 2018

View reviewed changes

sorpaas added 2 commits July 18, 2018 00:13

Be more specific for all low-level database error cases

b36efe2

Fix BranchBecomingCanonChain expect

7be9f8a

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

ascjones added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jul 18, 2018

5chdn 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 Jul 25, 2018

ethcore: fix typo in expect proof message

0c82fbb

andresilva approved these changes Jul 25, 2018

View reviewed changes

andresilva merged commit 5795d33 into master Jul 25, 2018

5chdn deleted the sp-unwrap branch July 26, 2018 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert PROOF messages for some cases in blockchain #9141

Insert PROOF messages for some cases in blockchain #9141

sorpaas commented Jul 17, 2018

ascjones Jul 17, 2018

andresilva left a comment

andresilva Jul 17, 2018

andresilva Jul 17, 2018

andresilva Jul 17, 2018

sorpaas Jul 17, 2018

sorpaas Jul 17, 2018

andresilva Jul 25, 2018

andresilva Jul 17, 2018

ascjones commented Jul 18, 2018

sorpaas commented Jul 18, 2018

andresilva left a comment

Insert PROOF messages for some cases in blockchain #9141

Insert PROOF messages for some cases in blockchain #9141

Conversation

sorpaas commented Jul 17, 2018

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

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

ascjones commented Jul 18, 2018

sorpaas commented Jul 18, 2018

andresilva left a comment

Choose a reason for hiding this comment