Remove disable ssz cache feature flag #7513
Conversation
terencechain
added
Ready For Review
beacon-chain/state/types_test.go
Outdated
| @@ -34,8 +34,7 @@ func TestBeaconState_ProtoBeaconStateCompatibility(t *testing.T) { | |||
|
|
|||
| r1, err := customState.HashTreeRoot(ctx) | |||
| require.NoError(t, err) | |||
| beaconState, err := stateTrie.InitializeFromProto(genesis) | |||
| require.NoError(t, err) | |||
| beaconState := customState.Copy() | |||
There was a problem hiding this comment.
Using copy here to ensure that caches don't get shared
Codecov Report
@@ Coverage Diff @@
## master #7513 +/- ##
==========================================
- Coverage 60.98% 60.79% -0.20%
==========================================
Files 428 427 -1
Lines 30714 30402 -312
==========================================
- Hits 18732 18482 -250
+ Misses 8934 8908 -26
+ Partials 3048 3012 -36 |
| @@ -180,11 +180,6 @@ func ConfigureBeaconChain(ctx *cli.Context) { | |||
| log.Warn("Disabled dynamic attestation committee subnets") | |||
| cfg.DisableDynamicCommitteeSubnets = true | |||
| } | |||
| cfg.EnableSSZCache = true | |||
There was a problem hiding this comment.
Don't you have to remove the use of this value? Otherwise this PR is just disabling ssz cache.
There was a problem hiding this comment.
We could. But a lot of the tests will fail because they don't run well with state ssz cache. We'll need to investigate individual tests (~10 of them) and update state pkg to provide setters to clear the ssz cache for these tests to pass.
I was thinking to do that in subsequent PR. Do you prefer to do it in this PR?
There was a problem hiding this comment.
I would not accept this PR as is. This PR disables SSZ cache with no way to turn it on.
We either need to remove usage of this field, or leave line 183 untouched so that the default of ssz cache enabled is still true.
There was a problem hiding this comment.
Big face palm moment 🤦
Line 183 should stay untouched. Thank you
Part of #7509
SSZ cache has been running by default since Topaz. This PR removes the usage for disable ssz cache flag. I didn't completely remove the ssz cache usages but it's heavily used by tests + fuzz. That will be done in subsequent PR