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

Put validator balances into a separate list in the state #317

Merged
merged 15 commits into from
Dec 18, 2018

Conversation

vbuterin
Copy link
Contributor

No description provided.

@vbuterin
Copy link
Contributor Author

One aesthetic change I think we should consider is switching the get_effective_balance function to just take the state as an argument instead of (as in this PR right now) the validator balance entry. I could see future changes that use parts of the validator registry entry as well as the balance entry to calculate the effective balance, so we may as well just make grab all the data it needs from the state.

@vbuterin vbuterin changed the title Separate validator balances Separate validator balances into a separate list in the state Dec 14, 2018
@vbuterin vbuterin changed the title Separate validator balances into a separate list in the state Put validator balances into a separate list in the state Dec 14, 2018
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

It might be worth introducing two functions increase_balance(state, validator_index, amount) and decrease_balance(state, validator_index, amount) to mask the changes in validator balance so much of the code doesn't have to think about dealing with validator_balances separate from `validator_registry.

specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Show resolved Hide resolved
@djrtwo
Copy link
Contributor

djrtwo commented Dec 14, 2018

@vbuterin I'm going to clean up per my recommendations and see if we like it.

vbuterin and others added 6 commits December 17, 2018 04:40
Faster editing that way; otherwise every block will require completely reconstructing a 8192-sized Merkle tree.
Edit latest_block_roots in place instead of as a queue
@vbuterin
Copy link
Contributor Author

I updated get_effective_balance to take state and index and fixed a couple other issues.

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

A few small bugs that you can fix with a click.

Other than that, approved!

specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
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.

4 participants