Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Insert PROOF messages for some cases in blockchain #9141

Merged
merged 5 commits into from
Jul 25, 2018
Merged

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Jul 17, 2018

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

@sorpaas 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 sorpaas added this to the 2.1 milestone Jul 17, 2018
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

Minor comments mostly opinionated. LGTM overall.

@@ -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?")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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?")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

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

"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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?).

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"))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@5chdn 5chdn modified the milestones: 2.1, 2.2 Jul 17, 2018
@ascjones 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
@ascjones
Copy link
Contributor

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

@sorpaas
Copy link
Collaborator Author

sorpaas commented Jul 18, 2018

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

@5chdn 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
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM

@andresilva andresilva merged commit 5795d33 into master Jul 25, 2018
@5chdn 5chdn deleted the sp-unwrap branch July 26, 2018 20:47
ordian added a commit to ordian/parity that referenced this pull request Jul 31, 2018
* '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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants