Fix Up SSZ Cache Branch Recomputation #4558

rauljordan · 2020-01-15T21:35:30Z

Part of #4445

Description

Write why you are making the changes in this pull request

Our old SSZ array roots cache was not recomputing branches if indices changed, instead, it would recompute the entire trie.

Write a summary of the changes you are making

This PR fixes up ssz cache, enables it for e2e tests, and adds a feature flag to enable the flag --enable-ssz-cache

Works at runtime and properly recomputes branches if they change:

[2020-01-16 14:24:22]  INFO initial-sync: Processing block 7071/49321 - estimated time remaining 4h2m48s blocksPerSecond=2.9 peers=2/6
Changed 1 indices, BlockRoots
Changed 1 indices, StateRoots
Changed 1 indices, RandaoMixes
[2020-01-16 14:24:22]  INFO initial-sync: Processing block 7072/49321 - estimated time remaining 3h58m41s blocksPerSecond=3.0 peers=2/6
Changed 1 indices, BlockRoots
Changed 1 indices, StateRoots
Changed 1 indices, RandaoMixes

nisdas · 2020-01-16T01:35:16Z

@rauljordan tests failing


state_root_cache_fuzz_test.go:67: Cached HTR panicked on iteration 6
--
  | panic: runtime error: index out of range [1] with length 1 [recovered]
  | panic: runtime error: index out of range [1] with length 1 [recovered]
  | panic: runtime error: index out of range [1] with length 1

rauljordan · 2020-01-16T02:27:23Z

shared/stateutil/arrays.go

 		return rt, nil
 	}

-	hashKey := hashutil.FastSum256(hashKeyElements)


This cache will never get hit. Arrays always typically get one element added each slot.

rauljordan · 2020-01-16T02:27:39Z

shared/stateutil/attestations.go

-	}
-
-	hashKey := hashutil.FastSum256(hashKeyElements)
-	if hashKey != emptyKey && h.rootsCache != nil {


rauljordan · 2020-01-16T02:28:01Z

shared/stateutil/arrays.go

 		for i := 0; i < len(changedIndices); i++ {
 			rt, err = recomputeRoot(changedIndices[i], chunks, fieldName)
 			if err != nil {
 				return [32]byte{}, err
 			}
 		}
+		lock.Lock()
+		leavesCache[fieldName] = chunks


This was part of the problem, we had to update some cached values after recomputing the root

…2e-ssz-cache

rauljordan · 2020-01-16T20:26:41Z

shared/stateutil/state_root_test.go

@@ -29,21 +37,6 @@ func TestState_FieldCount(t *testing.T) {
 	}
 }

-func TestHashTreeRootEquality(t *testing.T) {


This test will never pass due to #4526 (comment). We will need --define ssz=minimal on this test target for it to pass.

0xKiwi · 2020-01-16T20:31:57Z

shared/featureconfig/flags.go

@@ -18,6 +18,11 @@ var (
 		Name:  "enable-attestation-cache",
 		Usage: "Enable unsafe cache mechanism. See https://github.com/prysmaticlabs/prysm/issues/3106",
 	}
+	// EnableSSZCache --
+	EnableSSZCache = cli.BoolFlag{


Can you un-export this and change the name to enableSSZCacheFlag? No need to export it, that's what the config is for.

0xKiwi · 2020-01-16T20:32:39Z

shared/stateutil/state_root.go

@@ -8,14 +8,17 @@ import (
 	"github.com/pkg/errors"
 	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
 	pb "github.com/prysmaticlabs/prysm/proto/beacon/p2p/v1"
+


Suggested change

0xKiwi · 2020-01-16T20:33:29Z

shared/stateutil/state_root_cache_fuzz_test.go

-	fuzzStateRootCache(t, 3, 100000)
-}
-
-func TestStateRootCacheFuzz_1000000(t *testing.T) {


Why were the larger cases removed?

They take way too long to run now. Now, when doing HTR(state.BlockRoots), we pad the slice to params.BeaconConfig().SlotsPerHistoricalRoot, and the fuzz tests don't really make much use of the cache so much because the inputs change on every iteration, so they take almost 10s to run the 10k fuzz test, let alone the 100k one.

0xKiwi · 2020-01-16T20:34:46Z

shared/stateutil/state_root_test.go

+func init() {
+	config := &featureconfig.Flags{
+		EnableSSZCache: true,
+	}
+	featureconfig.Init(config)
+}
+


Is this being used? Hard to tell.

Yep! It is because it is picked up by the internal stateutil.HashTreeRoot function as being enabled

0xKiwi · 2020-01-16T20:42:16Z

shared/stateutil/state_root_test.go

@@ -82,7 +75,7 @@ func BenchmarkHashTreeRootState_Generic_512(b *testing.B) {
 	genesisState := setupGenesisState(b, 512)
 	b.StartTimer()
 	for i := 0; i < b.N; i++ {
-		if _, err := stateutil.HashTreeRootState(genesisState); err != nil {
+		if _, err := ssz.HashTreeRoot(genesisState); err != nil {


These 3 tests here say BenchmarkHashTreeRootState when it's HashTreeRoot being tested instead, can you update the test names?

codecov · 2020-01-16T21:05:32Z

Codecov Report

❗ No coverage uploaded for pull request base (master@298b081). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4558   +/-   ##
=========================================
  Coverage          ?   43.58%           
=========================================
  Files             ?      193           
  Lines             ?    13708           
  Branches          ?        0           
=========================================
  Hits              ?     5975           
  Misses            ?     6729           
  Partials          ?     1004

* e2e ssz cache busting * use ssz cache for e2e * caching ensure * fix up the cache even more * gazelle * formatting * formatting * add back cache for val registry * sync * fix up commented item * add attestations * Merge branch 'master' into e2e-ssz-cache * Merge branch 'master' into e2e-ssz-cache * Merge branch 'e2e-ssz-cache' of github.com:prysmaticlabs/prysm into e2e-ssz-cache * formatting * gaz * Merge branch 'master' into e2e-ssz-cache * resolve comments * Merge branch 'master' into e2e-ssz-cache * naming of test * Merge refs/heads/master into e2e-ssz-cache * Merge refs/heads/master into e2e-ssz-cache

rauljordan added 8 commits January 15, 2020 10:51

e2e ssz cache busting

c9d5dc2

use ssz cache for e2e

74cae1d

caching ensure

74e20f0

fix up the cache even more

6e1abc3

gazelle

79de17e

formatting

531c298

formatting

dc70007

add back cache for val registry

2eb1f79

rauljordan added 3 commits January 15, 2020 19:49

sync

4278a28

fix up commented item

fe3cf9b

add attestations

a3409e2

rauljordan commented Jan 16, 2020

View reviewed changes

rauljordan added 6 commits January 16, 2020 10:57

Merge branch 'master' into e2e-ssz-cache

30d0f2b

Merge branch 'master' into e2e-ssz-cache

7a641da

Merge branch 'e2e-ssz-cache' of github.com:prysmaticlabs/prysm into e…

242ae88

…2e-ssz-cache

formatting

d20721f

gaz

cfdd319

Merge branch 'master' into e2e-ssz-cache

4820d0c

rauljordan commented Jan 16, 2020

View reviewed changes

0xKiwi reviewed Jan 16, 2020

View reviewed changes

rauljordan added 2 commits January 16, 2020 14:38

resolve comments

3104f88

Merge branch 'master' into e2e-ssz-cache

e4da782

rauljordan self-assigned this Jan 16, 2020

rauljordan added the Ready For Review A pull request ready for code review label Jan 16, 2020

0xKiwi reviewed Jan 16, 2020

View reviewed changes

naming of test

7919faa

0xKiwi approved these changes Jan 16, 2020

View reviewed changes

prylabs-bulldozer bot added 2 commits January 16, 2020 21:07

Merge refs/heads/master into e2e-ssz-cache

298b081

Merge refs/heads/master into e2e-ssz-cache

127bf43

terencechain approved these changes Jan 16, 2020

View reviewed changes

rauljordan added the OK to merge label Jan 16, 2020

prylabs-bulldozer bot merged commit ed52996 into master Jan 16, 2020

delete-merged-branch bot deleted the e2e-ssz-cache branch January 16, 2020 21:40

rauljordan mentioned this pull request Jan 16, 2020

Fix Custom State HashTreeRoot Caching for Arrays of Roots #4445

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Up SSZ Cache Branch Recomputation #4558

Fix Up SSZ Cache Branch Recomputation #4558

rauljordan commented Jan 15, 2020 •

edited

Loading

nisdas commented Jan 16, 2020

rauljordan Jan 16, 2020

rauljordan Jan 16, 2020

rauljordan Jan 16, 2020

rauljordan Jan 16, 2020

0xKiwi Jan 16, 2020

rauljordan Jan 16, 2020

0xKiwi Jan 16, 2020

0xKiwi Jan 16, 2020

rauljordan Jan 16, 2020

0xKiwi Jan 16, 2020

rauljordan Jan 16, 2020

0xKiwi Jan 16, 2020

rauljordan Jan 16, 2020

codecov bot commented Jan 16, 2020 •

edited

Loading

Fix Up SSZ Cache Branch Recomputation #4558

Fix Up SSZ Cache Branch Recomputation #4558

Conversation

rauljordan commented Jan 15, 2020 • edited Loading

Description

nisdas commented Jan 16, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 16, 2020 • edited Loading

Codecov Report

rauljordan commented Jan 15, 2020 •

edited

Loading

codecov bot commented Jan 16, 2020 •

edited

Loading