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 Bug On Startup Using Genesis.json #1258

Merged
merged 13 commits into from Jan 9, 2019
Merged

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Jan 6, 2019

This fixes a bug, when starting up and running the beacon-chain, by providing the initial validators from genesis.json as an argument to initialize state.

Due to validators not being initialized from genesis.json , it leads to there being no validators stored in the beacon state. After shuffling , ActiveValidatorIndices returns an empty slice, which leads there to being no committees so we get a divide by zero error. This PR will be fixing this issue.

@nisdas nisdas added the Ready For Review A pull request ready for code review label Jan 6, 2019
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.

Thanks for fixing this! Just one comment on keeping InitialBeaconState generic and not pass genesis.json values as an extra argument. We should use initialValidatorDeposits instead.

@@ -44,6 +45,11 @@ func InitialBeaconState(
latestBlockRoots[i] = params.BeaconConfig().ZeroHash[:]
}

latestBalances := make([]uint64, len(genesisValidatorRegistry))
Copy link
Member

Choose a reason for hiding this comment

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

This is not part of the spec under get_initial_beacon_state . See https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#on-startup. I'd rather keep InitialBeaconState generic and not work with genesis.json which will be deprecated super soon, otherwise, it feels kinda hacky. Can we just pass the values of genesis.json (pub key and balances) inside initialValidatorDeposits and use that instead? It will still register via v.ProcessDeposit

Copy link
Member Author

Choose a reason for hiding this comment

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

If we are going to deprecate genesis.json super soon, I rather do it in this PR then and have a clean break. Also i guess initialValidators is not used anywhere so I can start removing that too

Copy link
Member

Choose a reason for hiding this comment

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

Exactly. So outside of InitialBeaconState, we should do something like:

If demo config:
Convert Genesis.json to initialValidatorDeposit

Lmk if that doesn't make sense

Copy link
Member

Choose a reason for hiding this comment

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

Reason being genesisValidatorRegistry will always be empty when we don't use Genesis.json

@terencechain terencechain changed the title Fix Bug On Startup Fix Bug On Startup Using Genesis.json Jan 6, 2019
@prestonvanloon
Copy link
Member

What is the bug exactly?

@codecov
Copy link

codecov bot commented Jan 8, 2019

Codecov Report

Merging #1258 into master will increase coverage by 2.89%.
The diff coverage is 79.59%.

@@            Coverage Diff             @@
##           master    #1258      +/-   ##
==========================================
+ Coverage   70.63%   73.52%   +2.89%     
==========================================
  Files          53       84      +31     
  Lines        4025     5809    +1784     
==========================================
+ Hits         2843     4271    +1428     
- Misses        959     1195     +236     
- Partials      223      343     +120

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.

looks good! thanks for the fix. One last thing, there's still a genesis-json reference under VALIDATOR_REGISTRATION.md, please remove that = )

@nisdas nisdas merged commit bb7b0a3 into prysmaticlabs:master Jan 9, 2019
@nisdas nisdas deleted the bugFix branch January 9, 2019 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants