Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Deprecated Spec Logic #1138

Merged
merged 7 commits into from
Dec 20, 2018

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Dec 19, 2018

This is part of #781.


Description

Now that our codebase is getting closer to being aligned with the specification here, we can now delete deprecated methods without causing our runtime to panic or causing build failures through Bazel.

This removes:

  • old state transition function logic
  • old state transition helpers
  • old cross link calculations
  • old justification/finalization, rewards, penalties
  • old block validity conditions

// The node has processed its state up to slot, block.slot - 1.
// The Ethereum 1.0 block pointed to by the state.processed_pow_receipt_root has been processed and accepted.
// The node's local clock time is greater than or equal to state.genesis_time + block.slot * SLOT_DURATION.
func IsValidBlock(
Copy link
Member

Choose a reason for hiding this comment

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

Is this new or just moved from somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to core/blocks/validity_conditions.go - it was previously in state

// Pre-Processing Condition 3:
// The block pointed to by the state in state.processed_pow_receipt_root has
// been processed in the ETH 1.0 chain.
if powBlock == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to do something more with this block? Why do we get the whole block rather than some method like

b, err := doesPoWBlockExist(...)
if !b {
// fail
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might - I think the research around receipt processing is still a little in flux, so let's leave this for now or modify in another PR if needed - this one is just focused on removing fully deprecated code

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

lgtm, please double check lint:

beacon-chain/core/state/state_transition.go:72:1:warning: validatorSetRecalculations is unused (deadcode)
beacon-chain/core/state/state_transition.go:97:1:warning: createRandaoMix is unused (deadcode)

@prestonvanloon
Copy link
Member

If those methods in Terence's comment are truly unused, please delete.

Yay for deleting code :)

@codecov
Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #1138 into master will increase coverage by 0.94%.
The diff coverage is 65%.

@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
+ Coverage   72.12%   73.07%   +0.94%     
==========================================
  Files          83       82       -1     
  Lines        5522     5111     -411     
==========================================
- Hits         3983     3735     -248     
+ Misses       1194     1058     -136     
+ Partials      345      318      -27

@rauljordan rauljordan merged commit 6125c30 into prysmaticlabs:master Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High High priority item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants