Add Balance Field Trie #9793

nisdas · 2021-10-18T13:13:05Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Adds in a balance field trie for out state. This will allow the beacon state to cache the relevant leaves of the trie and prevent
it from recomputing everything for each block.
Add Tests
Gate behind feature flag.

Which issues(s) does this PR fix?

N.A

Other notes for review

into addBalanceFieldTrie

nisdas · 2021-10-23T03:29:35Z

encoding/ssz/helpers.go

@@ -113,6 +114,51 @@ func Pack(serializedItems [][]byte) ([][]byte, error) {
 	return chunks, nil
 }

+// PackByChunk a given byte array's final chunk with zeroes if needed.
+func PackByChunk(serializedItems [][]byte) ([][bytesPerChunk]byte, error) {


This is a copy of Pack except that we enforce that [32]byte roots are returned at the end instead of a slice. This copy will be the canonical one once #9792 is merged in.

beacon-chain/state/fieldtrie/field_trie.go

rkapka · 2021-10-26T11:32:07Z

beacon-chain/state/types/types.go

@@ -82,6 +87,17 @@ func (f FieldIndex) String(stateVersion int) string {
 	}
 }

+// CompressedLength returns the compressed length of the type(number of


Suggested change

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

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

beacon-chain/state/fieldtrie/field_trie_helpers.go

rkapka

Please add tests to handleBalanceSlice.

rkapka · 2021-10-29T08:24:34Z

beacon-chain/state/fieldtrie/field_trie_helpers.go

@@ -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) {


This function needs tests.

rkapka · 2021-10-29T08:27:39Z

beacon-chain/state/v1/state_trie.go

+				if balLimit == 0 {
+					if len(b.state.Balances) == 0 {
+						balLimit = 1
+					} else {
+						balLimit = uint64(len(b.state.Balances))
+					}
+				}


Do we really need this? balLimit can be zero only if ValidatorRegistryLimit = 0 OR elemSize = 0, both of which are practically impossible.

rkapka · 2021-10-29T08:34:17Z

beacon-chain/state/fieldtrie/field_trie_helpers.go

+				// We only select from a field value, if the
+				// index exists in our element list. If it doesn't
+				// we assume a zero value.


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.

rkapka · 2021-10-29T10:11:31Z

You also need to run gofmt somewhere.

into addBalanceFieldTrie

kasey · 2021-10-26T13:06:17Z

beacon-chain/state/fieldtrie/field_trie.go

@@ -49,15 +51,17 @@ func NewFieldTrie(field types.FieldIndex, dataType types.DataType, elements inte
 			reference:   stateutil.NewRef(1),
 			RWMutex:     new(sync.RWMutex),


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?

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

kasey · 2021-10-29T18:47:05Z

beacon-chain/state/fieldtrie/field_trie_helpers.go

+func handleBalanceSlice(val []uint64, indices []uint64, convertAll bool) ([][32]byte, error) {
+	if convertAll {
+		balancesMarshaling := make([][]byte, 0)
+		for i := 0; i < len(val); i++ {


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.

beacon-chain/state/fieldtrie/field_trie_helpers.go

kasey · 2021-10-29T19:26:01Z

beacon-chain/state/fieldtrie/field_trie_helpers.go

+			return nil, errors.Errorf("Wanted type of %v but got %v",
+				reflect.TypeOf([]uint64{}).Name(), reflect.TypeOf(elements).Name())
+		}
+		return handleBalanceSlice(val, indices, convertAll)


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.

kasey · 2021-10-29T19:36:21Z

encoding/ssz/helpers.go

@@ -113,6 +114,51 @@ func Pack(serializedItems [][]byte) ([][]byte, error) {
 	return chunks, nil
 }

+// PackByChunk a given byte array's final chunk with zeroes if needed.
+func PackByChunk(serializedItems [][]byte) ([][bytesPerChunk]byte, error) {


we need some unit tests on this one

kasey · 2021-10-29T19:41:32Z

encoding/ssz/helpers.go

+		// we set it as numItems itself.
+		if j > numItems {
+			j = numItems
+		}


The right padding here is a little subtle and maybe worthy of comment. It works because ToBytes32 allocates a [32]byte (which has a zero value of 32 empty-bytes) and then copies the slice into that value. This condition results in the length of the slice passed to ToBytes32 to be less than 32 bytes for the final iteration over orderedItems when len(orderedItems) % 32 != 0.

kasey · 2021-10-29T19:45:34Z

encoding/ssz/helpers.go

+	var orderedItems []byte
+	for _, item := range serializedItems {
+		orderedItems = append(orderedItems, item...)
+	}


instead of doing the extra loop at the top of the function to check if areAllEmpty, could you let that condition get to right after this loop and then add this check?

if len(orderedItems) == 0 { return [][bytesPerChunk]byte{emptyChunk}, nil }

example showing that a flattened 2d array consisting of all empty sub-arrays would be len = 0:
https://play.golang.org/p/onJkAnvnqfj

terencechain · 2021-11-02T21:32:44Z

testing/spectest/mainnet/altair/epoch_processing/epoch_processing_test.go

+	"github.com/prysmaticlabs/prysm/config/features"
+)
+
+func TestMain(m *testing.M) {


What is this? I remember @prestonvanloon and I saw something similar back in hf1 branch and decided to delete it

This basically runs all tests in that package with those flags pre-loaded. Since this touches HTR, I wanted to run them through spec tests here.

Any reason it has to call TestMain than an explicit name like TestEpochProcessWithBalanceTrie ?

oh this has an explanation for why it is that way: https://pkg.go.dev/testing#hdr-Main

This basically allows for test setup and cleanup rather than doing it per test for the whole package.

terencechain · 2021-11-02T21:33:05Z

testing/spectest/mainnet/altair/sanity/sanity_test.go

+	"github.com/prysmaticlabs/prysm/config/features"
+)
+
+func TestMain(m *testing.M) {


terencechain · 2021-11-02T21:33:14Z

testing/spectest/mainnet/phase0/sanity/sanity_test.go

+	"github.com/prysmaticlabs/prysm/config/features"
+)
+
+func TestMain(m *testing.M) {


into addBalanceFieldTrie

nisdas added 10 commits October 18, 2021 21:05

save stuff

314954e

Merge branch 'develop' of https://github.com/prysmaticlabs/geth-sharding

b13bfa0

into addBalanceFieldTrie

fix in v1

a8d1e68

clean up more

1d4fbdb

Merge branch 'develop' of https://github.com/prysmaticlabs/geth-sharding

f2b8cd3

into addBalanceFieldTrie

fix bugs

1735505

add comments and clean up

46752eb

Merge branch 'develop' of https://github.com/prysmaticlabs/geth-sharding

4593126

into addBalanceFieldTrie

add flag + test

7b631cb

add tests

4978f49

nisdas added the Ready For Review label Oct 21, 2021

nisdas marked this pull request as ready for review October 21, 2021 14:57

nisdas requested a review from a team as a code owner October 21, 2021 14:57

nisdas requested review from prestonvanloon, terencechain and james-prysm October 21, 2021 14:57

Merge branch 'develop' into addBalanceFieldTrie

5ae2eed

nisdas commented Oct 23, 2021

View reviewed changes

Merge branch 'develop' into addBalanceFieldTrie

6847e95

rkapka reviewed Oct 26, 2021

View reviewed changes

Merge branch 'develop' into addBalanceFieldTrie

e33972f

rkapka reviewed Oct 28, 2021

View reviewed changes

beacon-chain/state/fieldtrie/field_trie_helpers.go Show resolved Hide resolved

rkapka requested changes Oct 29, 2021

View reviewed changes

rkapka reviewed Oct 29, 2021

View reviewed changes

nisdas added 4 commits October 29, 2021 20:44

fmt

9ebe1da

Merge branch 'develop' of https://github.com/prysmaticlabs/geth-sharding

79e9a87

into addBalanceFieldTrie

radek's review

01d1693

gaz

3f3cf35

rkapka previously approved these changes Oct 29, 2021

View reviewed changes

kasey reviewed Oct 29, 2021

View reviewed changes

terencechain reviewed Nov 2, 2021

View reviewed changes

nisdas added 2 commits November 3, 2021 14:04

Merge branch 'develop' of https://github.com/prysmaticlabs/geth-sharding

cd9e950

into addBalanceFieldTrie

kasey's review

bcea951

nisdas dismissed rkapka’s stale review via bcea951 November 3, 2021 06:48

nisdas added 2 commits November 3, 2021 15:11

gaz and new conditional

401ca3e

Merge branch 'develop' into addBalanceFieldTrie

24829a7

rkapka previously approved these changes Nov 5, 2021

View reviewed changes

nisdas added 4 commits November 8, 2021 12:47

Merge branch 'develop' into addBalanceFieldTrie

fcfbc7e

Merge branch 'develop' into addBalanceFieldTrie

14d23af

Merge branch 'develop' into addBalanceFieldTrie

3949195

improve naming

75e6c5e

nisdas dismissed rkapka’s stale review via 75e6c5e November 12, 2021 07:10

nisdas added 7 commits November 13, 2021 07:44

Merge branch 'develop' into addBalanceFieldTrie

81c1d8c

Merge branch 'develop' into addBalanceFieldTrie

bb1f088

Merge branch 'develop' into addBalanceFieldTrie

89dbb21

Merge branch 'develop' into addBalanceFieldTrie

e52be54

Merge branch 'develop' into addBalanceFieldTrie

88189e2

Merge branch 'develop' into addBalanceFieldTrie

852715d

Merge branch 'develop' into addBalanceFieldTrie

7c55600

rkapka approved these changes Nov 19, 2021

View reviewed changes

nisdas merged commit 0ade1f1 into develop Nov 19, 2021

delete-merged-branch bot deleted the addBalanceFieldTrie branch November 19, 2021 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Balance Field Trie #9793

Add Balance Field Trie #9793

nisdas commented Oct 18, 2021 •

edited

Loading

nisdas Oct 23, 2021

rkapka Oct 26, 2021

rkapka left a comment

rkapka Oct 29, 2021

rkapka Oct 29, 2021

rkapka Oct 29, 2021

rkapka commented Oct 29, 2021

kasey Oct 26, 2021

nisdas Nov 3, 2021

kasey Oct 29, 2021

kasey Oct 29, 2021

kasey Oct 29, 2021

kasey Oct 29, 2021

kasey Oct 29, 2021

kasey Oct 29, 2021

terencechain Nov 2, 2021

nisdas Nov 3, 2021

terencechain Nov 3, 2021

nisdas Nov 3, 2021 •

edited

Loading

terencechain Nov 2, 2021

terencechain Nov 2, 2021

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

-				// 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.

		@@ -49,15 +51,17 @@ func NewFieldTrie(field types.FieldIndex, dataType types.DataType, elements inte
		reference: stateutil.NewRef(1),
		RWMutex: new(sync.RWMutex),

Add Balance Field Trie #9793

Add Balance Field Trie #9793

Conversation

nisdas commented Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka commented Oct 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas commented Oct 18, 2021 •

edited

Loading

nisdas Nov 3, 2021 •

edited

Loading