-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Feature gate all caches #3894
Feature gate all caches #3894
Conversation
beacon-chain/cache/committee.go
Outdated
@@ -67,6 +68,9 @@ func NewCommitteeCache() *CommitteeCache { | |||
// ShuffledIndices fetches the shuffled indices by epoch and shard. Every list of indices | |||
// represent one committee. Returns true if the list exists with epoch and shard. Otherwise returns false, nil. | |||
func (c *CommitteeCache) ShuffledIndices(epoch uint64, shard uint64) ([]uint64, error) { | |||
if !featureconfig.Get().EnableShuffledIndexCache { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the committee cache is already gated by the new-cache flag if I am not wrong.
@terencechain can you confirm ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's correct, we don't need this flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then that flag needs to be moved here. Not putting the flag inside of the cache allows for misuse without proper flag control.
Per discussion of last night, checkpoint state is KV store is required for fork choice as it's currently implemented in the fork choice spec. We should move |
Codecov Report
@@ Coverage Diff @@
## master #3894 +/- ##
=======================================
Coverage 57.54% 57.54%
=======================================
Files 193 193
Lines 12499 12499
=======================================
Hits 7192 7192
Misses 4297 4297
Partials 1010 1010 |
Confirmed locally that the chains advance normally with mixed nodes (cache on vs cache off) and that having the cache off is a minimal impact. I noticed only 1.4% difference in wall time for assignments with 128 validators in the system. |
As part of investigating #3885, we want to disable all caches until we can carefully roll them out with detection of abnormalities in block production.