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

Create New Beacon State Data Structure #4602

Merged
merged 32 commits into from
Jan 22, 2020
Merged

Create New Beacon State Data Structure #4602

merged 32 commits into from
Jan 22, 2020

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Jan 21, 2020

This PR resolves #2011


Description

Write why you are making the changes in this pull request

We need a way to avoid cloning the beacon state 56 times per slot as we get closer to a mainnet release. This PR adds a new beacon state data structure that recomputes its hashtreeroot on writes to fields and provides getters which copy fields on reads to maintain immutability. This PR will be backwards compatible with our current beacon state, and will help runtime a lot.

Write a summary of the changes you are making

Adds a new beacon state struct which looks like this:

type BeaconState struct {
    merkleTrieLayers [][][]byte
    state *pbp2p.BeaconState
}

func (b *BeaconState) Balances() []uint64 {
    res := make([]uint64, len(b.state.Balances))
    copy(res, b.state.Balances)
    return res
}

func (b *BeaconState) SetBalances(vals []uint64) error {
    // Recompute the branch up the Merkle trie for the
    // state.Balances field and update the cached value of state.Balances...
    ...
}

Link anything that would be helpful or relevant to the reviewers

We should expect resource consumption and bottlenecks to reduce significantly. This PR only implements the new data structure with tests but does not yet integrate it into our runtime.

Next Steps

Next, we need to begin integrating this new type throughout the repo. We would first need to amend the db.SaveState and db.HeadState functions to take in and return this type. Then, we would to modify the head fetcher (from the chain service) to use this type instead. Afterwards, we can just go package by package, checking for any instances of .HeadState() and replacing with appropriate getters instead. The HeadState() function should no longer clone, but instead just return the new beacon state type because all its fields are protected behind getters that copy.

@rauljordan rauljordan added the Priority: Critical Highest, immediate priority item label Jan 21, 2020
@rauljordan rauljordan self-assigned this Jan 21, 2020
@codecov
Copy link

codecov bot commented Jan 21, 2020

Codecov Report

Merging #4602 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4602   +/-   ##
=======================================
  Coverage   22.69%   22.69%           
=======================================
  Files          54       54           
  Lines        4257     4257           
=======================================
  Hits          966      966           
  Misses       3150     3150           
  Partials      141      141

@rauljordan rauljordan marked this pull request as ready for review January 21, 2020 05:10
Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Awesome patch!

Two concerns:

  • Please confirm that you are using safe copy for byte slices in getters.
  • Please complete godoc comments.

beacon-chain/state/getters.go Outdated Show resolved Hide resolved
beacon-chain/state/setters.go Show resolved Hide resolved
beacon-chain/state/setters.go Show resolved Hide resolved
beacon-chain/state/setters.go Outdated Show resolved Hide resolved
beacon-chain/state/types.go Outdated Show resolved Hide resolved
@rauljordan rauljordan added the Ready For Review A pull request ready for code review label Jan 21, 2020
prestonvanloon
prestonvanloon previously approved these changes Jan 21, 2020
terencechain
terencechain previously approved these changes Jan 21, 2020
@rauljordan rauljordan merged commit abe679e into master Jan 22, 2020
@rauljordan rauljordan deleted the state-service branch January 22, 2020 04:36
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 24, 2020
* begin state service

* begin on the state trie idea

* created beacon state structure

* add in the full clone getter

* return by value instead

* add all setters

* new state setters are being completed

* arrays roots exposed

*  close to finishing all these headerssss

* functionality complete

* added in proto benchmark test

* test for compatibility

* add test for compat

* comments fixed

* add clone

* add clone

* remove underlying copies

* make it immutable

* integrate it into chainservice

* revert

* wrap up comments for package

* address all comments and godocs

* address all comments

* clone the pending attestation properly

* properly clone remaining items

* tests pass fixed bug

* prevent nil pointer exceptions

* fixed up some bugs in the clone comparisons

Co-authored-by: Nishant Das <[email protected]>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 28, 2020
* begin state service

* begin on the state trie idea

* created beacon state structure

* add in the full clone getter

* return by value instead

* add all setters

* new state setters are being completed

* arrays roots exposed

*  close to finishing all these headerssss

* functionality complete

* added in proto benchmark test

* test for compatibility

* add test for compat

* comments fixed

* add clone

* add clone

* remove underlying copies

* make it immutable

* integrate it into chainservice

* revert

* wrap up comments for package

* address all comments and godocs

* address all comments

* clone the pending attestation properly

* properly clone remaining items

* tests pass fixed bug

* prevent nil pointer exceptions

* fixed up some bugs in the clone comparisons

Co-authored-by: Nishant Das <[email protected]>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Critical Highest, immediate priority item Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Perform State Caching With Merkle Structure
5 participants