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

Fix deposit block slot before genesis state #4495

Merged

Conversation

princesinha19
Copy link
Contributor

This PR fixes the incorrect result for depositBlockSlot() before the genesis state.

[Resolves] #4465

@claassistantio
Copy link

claassistantio commented Jan 11, 2020

CLA assistant check
All committers have signed the CLA.

@mcdee
Copy link
Contributor

mcdee commented Jan 11, 2020

There are some additional nuances that need to be considered here:

  • there will be a number of validators processed at genesis, but not all of them. The actual number will be params.BeaconConfig().MinGenesisActiveValidatorCount plus any additional validators in the Ethereum 1block this number was achieved (note that it is possible for there to be multiple deposits for the same validator)
  • post-genesis there can be a maximum of params.BeaconConfig().MaxDeposits deposits per slot
    • as such, the deposit inclusion slot can increase if a block is either not proposed or proposed without the maximum number of deposits

That said, the function should remain relatively simple because it can be called many times. If will result in too high a CPU load it may make sense for the pre-genesis version the calculation to ignore the additional validators in the Ethereum 1 block after MinGenesisActiveValidatorCount (although in this case a note should be provided explaining why).

@codecov
Copy link

codecov bot commented Jan 12, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@7a31c84). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4495   +/-   ##
=========================================
  Coverage          ?   19.02%           
=========================================
  Files             ?      165           
  Lines             ?    10623           
  Branches          ?        0           
=========================================
  Hits              ?     2021           
  Misses            ?     8249           
  Partials          ?      353

@princesinha19
Copy link
Contributor Author

@terencechain Can you please review this?

@terencechain
Copy link
Member

@princesinha19 thanks for adding the test, it looks good.

e2e tests failed, something to look into

@princesinha19
Copy link
Contributor Author

@terencechain It's working now. Actually it was not an issue. I don't know exactly why the e2e test failed with error:-

eth1.go:67: mining log not found, this means the eth1 chain had issues starting: could not find requested text "Commit new mining work" in logs:  

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.

Great. Thanks 👍🏼

@princesinha19
Copy link
Contributor Author

@terencechain Should we merge this?

@prylabs-bulldozer prylabs-bulldozer bot merged commit 63d692a into prysmaticlabs:master Jan 16, 2020
@princesinha19 princesinha19 deleted the deposit-block-slot branch February 10, 2020 06:03
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 24, 2020
* before genesis state commit
* Merge branch 'master' into deposit-block-slot
* Merge branch 'master' into deposit-block-slot
* depositBlockSlot test added
* Merge branch 'deposit-block-slot' of https://github.com/princesinha19/prysm into deposit-block-slot
* Merge branch 'master' into deposit-block-slot
* Merge branch 'master' into deposit-block-slot
* resolve conflict
* status test commit
* Merge branch 'master' into deposit-block-slot
* Merge branch 'master' into deposit-block-slot
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 28, 2020
* before genesis state commit
* Merge branch 'master' into deposit-block-slot
* Merge branch 'master' into deposit-block-slot
* depositBlockSlot test added
* Merge branch 'deposit-block-slot' of https://github.com/princesinha19/prysm into deposit-block-slot
* Merge branch 'master' into deposit-block-slot
* Merge branch 'master' into deposit-block-slot
* resolve conflict
* status test commit
* Merge branch 'master' into deposit-block-slot
* Merge branch 'master' into deposit-block-slot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants