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

Shortcut SF condition when canon known #1401

Merged
merged 5 commits into from
Jun 23, 2016
Merged

Shortcut SF condition when canon known #1401

merged 5 commits into from
Jun 23, 2016

Conversation

gavofyork
Copy link
Contributor

No description provided.

@gavofyork gavofyork added the A0-pleasereview 🤓 Pull request needs code review. label Jun 22, 2016
if self.chain_info().best_block_number > 1_761_000 {
return self.block_header(BlockID::Number(1_760_000)).map(|header| HeaderView::new(&header).gas_limit());
}
// otherwise check according to `chain_hash`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just?

fn dao_rescue_block_gas_limit(&self) -> Option<U256> {
  self.block_header(BlockID::Number(1_760_000)).map(|header| HeaderView::new(&header).gas_limit())
}

the code below is doing exactly the same thing. It's also less efficient and more complex

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your code will look only in canon chain. The code below is looking for the block in other possible chains.

Copy link
Collaborator

@debris debris Jun 23, 2016

Choose a reason for hiding this comment

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

so this makes the pr code invalid, cause it cares about the canon chain

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? The first part is shortcutting if canon is more then 1000 blocks after the trigger. The code below will be run between 1_760_000 and 1_761_000.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My bad. 1_761_000 looks almost the same as 1_760_000. It would be good to make constants from these numbers.

@gavofyork
Copy link
Contributor Author

the code will be removed in 2 days; i think we all understand what it does now :)

@gavofyork
Copy link
Contributor Author

builds & tests fine locally.

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 23, 2016
@arkpar arkpar merged commit f3486c4 into master Jun 23, 2016
@debris debris deleted the sfedgecase branch June 23, 2016 11:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants