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

Add Balance Field Trie #9793

Merged
merged 32 commits into from
Nov 19, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
314954e
save stuff
nisdas Oct 18, 2021
b13bfa0
Merge branch 'develop' of https://github.com/prysmaticlabs/geth-shard…
nisdas Oct 19, 2021
a8d1e68
fix in v1
nisdas Oct 19, 2021
1d4fbdb
clean up more
nisdas Oct 19, 2021
f2b8cd3
Merge branch 'develop' of https://github.com/prysmaticlabs/geth-shard…
nisdas Oct 20, 2021
1735505
fix bugs
nisdas Oct 21, 2021
46752eb
add comments and clean up
nisdas Oct 21, 2021
4593126
Merge branch 'develop' of https://github.com/prysmaticlabs/geth-shard…
nisdas Oct 21, 2021
7b631cb
add flag + test
nisdas Oct 21, 2021
4978f49
add tests
nisdas Oct 21, 2021
5ae2eed
Merge branch 'develop' into addBalanceFieldTrie
nisdas Oct 22, 2021
6847e95
Merge branch 'develop' into addBalanceFieldTrie
nisdas Oct 26, 2021
e33972f
Merge branch 'develop' into addBalanceFieldTrie
nisdas Oct 26, 2021
9ebe1da
fmt
nisdas Oct 29, 2021
79e9a87
Merge branch 'develop' of https://github.com/prysmaticlabs/geth-shard…
nisdas Oct 29, 2021
01d1693
radek's review
nisdas Oct 29, 2021
3f3cf35
gaz
nisdas Oct 29, 2021
cd9e950
Merge branch 'develop' of https://github.com/prysmaticlabs/geth-shard…
nisdas Nov 3, 2021
bcea951
kasey's review
nisdas Nov 3, 2021
401ca3e
gaz and new conditional
nisdas Nov 3, 2021
24829a7
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 5, 2021
fcfbc7e
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 8, 2021
14d23af
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 9, 2021
3949195
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 12, 2021
75e6c5e
improve naming
nisdas Nov 12, 2021
81c1d8c
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 12, 2021
bb1f088
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 13, 2021
89dbb21
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 15, 2021
e52be54
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 16, 2021
88189e2
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 18, 2021
852715d
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 19, 2021
7c55600
Merge branch 'develop' into addBalanceFieldTrie
nisdas Nov 19, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions beacon-chain/state/fieldtrie/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ go_library(
"//beacon-chain/state/stateutil:go_default_library",
"//beacon-chain/state/types:go_default_library",
"//crypto/hash:go_default_library",
"//encoding/ssz:go_default_library",
"//proto/prysm/v1alpha1:go_default_library",
"//runtime/version:go_default_library",
"@com_github_pkg_errors//:go_default_library",
Expand Down
58 changes: 47 additions & 11 deletions beacon-chain/state/fieldtrie/field_trie.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type FieldTrie struct {
field types.FieldIndex
dataType types.DataType
length uint64
numOfElems int
}

// NewFieldTrie is the constructor for the field trie data structure. It creates the corresponding
Expand All @@ -26,11 +27,12 @@ type FieldTrie struct {
func NewFieldTrie(field types.FieldIndex, dataType types.DataType, elements interface{}, length uint64) (*FieldTrie, error) {
if elements == nil {
return &FieldTrie{
field: field,
dataType: dataType,
reference: stateutil.NewRef(1),
RWMutex: new(sync.RWMutex),
length: length,
field: field,
dataType: dataType,
reference: stateutil.NewRef(1),
RWMutex: new(sync.RWMutex),
length: length,
numOfElems: 0,
}, nil
}
fieldRoots, err := fieldConverters(field, []uint64{}, elements, true)
Expand All @@ -49,15 +51,17 @@ func NewFieldTrie(field types.FieldIndex, dataType types.DataType, elements inte
reference: stateutil.NewRef(1),
RWMutex: new(sync.RWMutex),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not part of this change set, but why do embed a *sync.RWMutex vs a sync.RWMutex? A RWMutex is usable as its zero value, so does not require initialization, whereas using a pointer type requires explicit initialization and risks nil pointers. So I assume the intent is for copies of the state trie share the same lock?

Copy link
Member Author

Choose a reason for hiding this comment

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

that is correct , the tries are shared between different states so the lock must be shared correctly too.

length: length,
numOfElems: reflect.ValueOf(elements).Len(),
}, nil
case types.CompositeArray:
case types.CompositeArray, types.CompressedArray:
return &FieldTrie{
fieldLayers: stateutil.ReturnTrieLayerVariable(fieldRoots, length),
field: field,
dataType: dataType,
reference: stateutil.NewRef(1),
RWMutex: new(sync.RWMutex),
length: length,
numOfElems: reflect.ValueOf(elements).Len(),
}, nil
default:
return nil, errors.Errorf("unrecognized data type in field map: %v", reflect.TypeOf(dataType).Name())
Expand Down Expand Up @@ -88,13 +92,40 @@ func (f *FieldTrie) RecomputeTrie(indices []uint64, elements interface{}) ([32]b
if err != nil {
return [32]byte{}, err
}
f.numOfElems = reflect.ValueOf(elements).Len()
return fieldRoot, nil
case types.CompositeArray:
fieldRoot, f.fieldLayers, err = stateutil.RecomputeFromLayerVariable(fieldRoots, indices, f.fieldLayers)
if err != nil {
return [32]byte{}, err
}
f.numOfElems = reflect.ValueOf(elements).Len()
return stateutil.AddInMixin(fieldRoot, uint64(len(f.fieldLayers[0])))
case types.CompressedArray:
numOfElems, err := f.field.CompressedLength()
if err != nil {
return [32]byte{}, err
}
// We remove the duplicates here in order to prevent
// duplicated insertions into the trie.
newIndices := []uint64{}
indexExists := make(map[uint64]bool)
newRoots := make([][32]byte, 0, len(fieldRoots)/int(numOfElems))
for i, idx := range indices {
startIdx := idx / numOfElems
if indexExists[startIdx] {
continue
}
newIndices = append(newIndices, startIdx)
indexExists[startIdx] = true
newRoots = append(newRoots, fieldRoots[i])
}
rkapka marked this conversation as resolved.
Show resolved Hide resolved
fieldRoot, f.fieldLayers, err = stateutil.RecomputeFromLayerVariable(newRoots, newIndices, f.fieldLayers)
if err != nil {
return [32]byte{}, err
}
f.numOfElems = reflect.ValueOf(elements).Len()
return stateutil.AddInMixin(fieldRoot, uint64(f.numOfElems))
default:
return [32]byte{}, errors.Errorf("unrecognized data type in field map: %v", reflect.TypeOf(f.dataType).Name())
}
Expand All @@ -105,11 +136,12 @@ func (f *FieldTrie) RecomputeTrie(indices []uint64, elements interface{}) ([32]b
func (f *FieldTrie) CopyTrie() *FieldTrie {
if f.fieldLayers == nil {
return &FieldTrie{
field: f.field,
dataType: f.dataType,
reference: stateutil.NewRef(1),
RWMutex: new(sync.RWMutex),
length: f.length,
field: f.field,
dataType: f.dataType,
reference: stateutil.NewRef(1),
RWMutex: new(sync.RWMutex),
length: f.length,
numOfElems: f.numOfElems,
}
}
dstFieldTrie := make([][]*[32]byte, len(f.fieldLayers))
Expand All @@ -124,6 +156,7 @@ func (f *FieldTrie) CopyTrie() *FieldTrie {
reference: stateutil.NewRef(1),
RWMutex: new(sync.RWMutex),
length: f.length,
numOfElems: f.numOfElems,
}
}

Expand All @@ -135,6 +168,9 @@ func (f *FieldTrie) TrieRoot() ([32]byte, error) {
case types.CompositeArray:
trieRoot := *f.fieldLayers[len(f.fieldLayers)-1][0]
return stateutil.AddInMixin(trieRoot, uint64(len(f.fieldLayers[0])))
case types.CompressedArray:
trieRoot := *f.fieldLayers[len(f.fieldLayers)-1][0]
return stateutil.AddInMixin(trieRoot, uint64(f.numOfElems))
default:
return [32]byte{}, errors.Errorf("unrecognized data type in field map: %v", reflect.TypeOf(f.dataType).Name())
}
Expand Down
53 changes: 53 additions & 0 deletions beacon-chain/state/fieldtrie/field_trie_helpers.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package fieldtrie

import (
"encoding/binary"
"fmt"
"reflect"

"github.com/pkg/errors"
"github.com/prysmaticlabs/prysm/beacon-chain/state/stateutil"
"github.com/prysmaticlabs/prysm/beacon-chain/state/types"
"github.com/prysmaticlabs/prysm/crypto/hash"
"github.com/prysmaticlabs/prysm/encoding/ssz"
ethpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"
"github.com/prysmaticlabs/prysm/runtime/version"
)
Expand Down Expand Up @@ -60,6 +62,13 @@ func fieldConverters(field types.FieldIndex, indices []uint64, elements interfac
reflect.TypeOf([]*ethpb.PendingAttestation{}).Name(), reflect.TypeOf(elements).Name())
}
return handlePendingAttestation(val, indices, convertAll)
case types.Balances:
val, ok := elements.([]uint64)
if !ok {
return nil, errors.Errorf("Wanted type of %v but got %v",
reflect.TypeOf([]uint64{}).Name(), reflect.TypeOf(elements).Name())
}
return handleBalanceSlice(val, indices, convertAll)
Copy link
Contributor

Choose a reason for hiding this comment

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

outside the scope of this PR, I'm noticing that the scope of these helpers is really inconsistent. HandleEth1DataSlice is exported for no apparent reason, HandleByteArrays and HandleValidatorSlice are defined in a different package (stateutil) with no usages beyond the fieldtrie package.

default:
return [][32]byte{}, errors.Errorf("got unsupported type of %v", reflect.TypeOf(elements).Name())
}
Expand Down Expand Up @@ -141,3 +150,47 @@ func handlePendingAttestation(val []*ethpb.PendingAttestation, indices []uint64,
}
return roots, nil
}

func handleBalanceSlice(val []uint64, indices []uint64, convertAll bool) ([][32]byte, error) {
rkapka marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This function needs tests.

if convertAll {
balancesMarshaling := make([][]byte, 0)
for i := 0; i < len(val); i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: i feel like we should prefer:

for i := range(val) {
 ...
}

or in this case, since you aren't using the actual index:

for _, value := range(val) {
...
}

just because it "scans" faster when reading code.

balanceBuf := make([]byte, 8)
binary.LittleEndian.PutUint64(balanceBuf, val[i])
balancesMarshaling = append(balancesMarshaling, balanceBuf)
}
balancesChunks, err := ssz.PackByChunk(balancesMarshaling)
if err != nil {
return [][32]byte{}, errors.Wrap(err, "could not pack balances into chunks")
}
return balancesChunks, nil
}
if len(val) > 0 {
numOfElems, err := types.Balances.CompressedLength()
if err != nil {
return nil, err
}
roots := [][32]byte{}
for _, idx := range indices {
// We split the indexes into their relevant groups. Balances
// are compressed according to 4 values -> 1 chunk.
startIdx := idx / numOfElems
startGroup := startIdx * numOfElems
chunk := [32]byte{}
sizeOfElem := len(chunk) / int(numOfElems)
rkapka marked this conversation as resolved.
Show resolved Hide resolved
for i, j := 0, startGroup; j < startGroup+numOfElems; i, j = i+sizeOfElem, j+1 {
wantedVal := uint64(0)
// We only select from a field value, if the
// index exists in our element list. If it doesn't
// we assume a zero value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// We only select from a field value, if the
// index exists in our element list. If it doesn't
// we assume a zero value.
// We are adding chunks in sets of 4, if the set is at the edge of the array
// then you will need to zero out the rest of the chunk.
// Ex : 41 indexes, so 41 % 4 = 1 .
// Therefore there are 3 indexes, which aren't there yet but we have to add in as a root.
// These are zeroed out.

if int(j) < len(val) {
rkapka marked this conversation as resolved.
Show resolved Hide resolved
wantedVal = val[j]
}
binary.LittleEndian.PutUint64(chunk[i:i+sizeOfElem], wantedVal)
}
roots = append(roots, chunk)
}
return roots, nil
}
return [][32]byte{}, nil
}
5 changes: 4 additions & 1 deletion beacon-chain/state/types/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ go_library(
srcs = ["types.go"],
importpath = "github.com/prysmaticlabs/prysm/beacon-chain/state/types",
visibility = ["//beacon-chain:__subpackages__"],
deps = ["//runtime/version:go_default_library"],
deps = [
"//runtime/version:go_default_library",
"@com_github_pkg_errors//:go_default_library",
],
)

go_test(
Expand Down
16 changes: 16 additions & 0 deletions beacon-chain/state/types/types.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package types

import (
"github.com/pkg/errors"
"github.com/prysmaticlabs/prysm/runtime/version"
)

Expand All @@ -18,6 +19,10 @@ const (
// CompositeArray represents a variable length array with
// a non primitive type.
CompositeArray
// CompressedArray represents a variable length array which
// can pack multiple elements into a leaf of the underlying
// trie.
CompressedArray
)

// String returns the name of the field index.
Expand Down Expand Up @@ -82,6 +87,17 @@ func (f FieldIndex) String(stateVersion int) string {
}
}

// CompressedLength returns the compressed length of the type(number of
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// CompressedLength returns the compressed length of the type(number of
// CompressedLength returns the compressed length of the type (number of

// elements that are able to be packed).
func (f FieldIndex) CompressedLength() (uint64, error) {
switch f {
case Balances:
return 4, nil
default:
return 0, errors.Errorf("field %d doesn't support element compression", f)
}
}

// Below we define a set of useful enum values for the field
// indices of the beacon state. For example, genesisTime is the
// 0th field of the beacon state. This is helpful when we are
Expand Down
1 change: 1 addition & 0 deletions beacon-chain/state/v1/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ go_test(
"//beacon-chain/state:go_default_library",
"//beacon-chain/state/stateutil:go_default_library",
"//beacon-chain/state/types:go_default_library",
"//config/features:go_default_library",
"//config/params:go_default_library",
"//encoding/bytesutil:go_default_library",
"//proto/prysm/v1alpha1:go_default_library",
Expand Down
5 changes: 5 additions & 0 deletions beacon-chain/state/v1/setters_misc.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
types "github.com/prysmaticlabs/eth2-types"
"github.com/prysmaticlabs/prysm/beacon-chain/state/stateutil"
stateTypes "github.com/prysmaticlabs/prysm/beacon-chain/state/types"
"github.com/prysmaticlabs/prysm/config/features"
"github.com/prysmaticlabs/prysm/crypto/hash"
ethpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"
"google.golang.org/protobuf/proto"
Expand Down Expand Up @@ -172,6 +173,10 @@ func (b *BeaconState) addDirtyIndices(index stateTypes.FieldIndex, indices []uin
if b.rebuildTrie[index] {
return
}
// Exit early if balance trie computation isn't enabled.
if !features.Get().EnableBalanceTrieComputation && index == balances {
return
}
totalIndicesLen := len(b.dirtyIndices[index]) + len(indices)
if totalIndicesLen > indicesLimit {
b.rebuildTrie[index] = true
Expand Down
4 changes: 4 additions & 0 deletions beacon-chain/state/v1/setters_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ func (b *BeaconState) SetBalances(val []uint64) error {

b.state.Balances = val
b.markFieldAsDirty(balances)
b.rebuildTrie[balances] = true
return nil
}

Expand All @@ -128,6 +129,7 @@ func (b *BeaconState) UpdateBalancesAtIndex(idx types.ValidatorIndex, val uint64
bals[idx] = val
b.state.Balances = bals
b.markFieldAsDirty(balances)
b.addDirtyIndices(balances, []uint64{uint64(idx)})
return nil
}

Expand Down Expand Up @@ -219,6 +221,8 @@ func (b *BeaconState) AppendBalance(bal uint64) error {
}

b.state.Balances = append(bals, bal)
balIdx := len(b.state.Balances) - 1
b.markFieldAsDirty(balances)
b.addDirtyIndices(balances, []uint64{uint64(balIdx)})
return nil
}
Loading