More feature flag deletions #7533
Conversation
terencechain
added
Ready For Review
| enableBlst, | ||
| enableEth1DataMajorityVote, | ||
| enableAttBroadcastDiscoveryAttempts, |
There was a problem hiding this comment.
My bad. I'm going to leave this in.
validator/client/runner.go
Outdated
| @@ -62,18 +62,14 @@ func run(ctx context.Context, v Validator) { | |||
| log.Fatalf("Slasher is not ready: %v", err) | |||
| } | |||
| } | |||
| if featureconfig.Get().WaitForSynced { | |||
| if err := v.WaitForSynced(ctx); err != nil { | |||
There was a problem hiding this comment.
Are you going to delete this method too?
There was a problem hiding this comment.
This feature looks like it wasn't launched yet, but you are removing it?
There was a problem hiding this comment.
Similar to StateGenSigVerify, this looks to be a feature that we should never release on beta/mainnet. This skips WaitForChainStart and is only utilized during test (e.g. e2e). Could also be dangerous if someone were to try this on mainnet
There was a problem hiding this comment.
Chatted with @0xKiwi this feature is not done. We'll leave it in for now
beacon-chain/sync/metrics.go
Outdated
| if featureconfig.Get().DisableDynamicCommitteeSubnets { | ||
| for i := uint64(0); i < params.BeaconNetworkConfig().AttestationSubnetCount; i++ { | ||
| formattedTopic := fmt.Sprintf(attTopic, digest, i) | ||
| topicPeerCount.WithLabelValues(formattedTopic).Set(float64(len(s.p2p.PubSub().ListPeers(formattedTopic)))) | ||
| } | ||
| } else { | ||
| for _, committeeIdx := range indices { | ||
| formattedTopic := fmt.Sprintf(attTopic, digest, committeeIdx) | ||
| topicPeerCount.WithLabelValues(formattedTopic).Set(float64(len(s.p2p.PubSub().ListPeers(formattedTopic)))) | ||
| } | ||
| for _, committeeIdx := range indices { | ||
| formattedTopic := fmt.Sprintf(attTopic, digest, committeeIdx) | ||
| topicPeerCount.WithLabelValues(formattedTopic).Set(float64(len(s.p2p.PubSub().ListPeers(formattedTopic)))) | ||
| } |
There was a problem hiding this comment.
I don't think this is a good idea. I think it should always be in the path of DisableDynamicCommitteeSubnets.
If was on committee 5 and I have 10 peers on that subnet. Prometheus polls this metric, and the topic is updated with the value 10.
Now, one epoch later, I am no longer on committee 5. When prometheus polls the metrics again, the value for committee subnet 5 is still 10 and it is not updated. This leads to stale and inaccurate results.
Codecov Report
@@ Coverage Diff @@
## master #7533 +/- ##
==========================================
+ Coverage 61.62% 61.66% +0.03%
==========================================
Files 421 421
Lines 29740 29428 -312
==========================================
- Hits 18328 18146 -182
+ Misses 8452 8376 -76
+ Partials 2960 2906 -54 |
Part of #7509
This PR removes the following flags. The rationales are outlined in the issue above.
disable-broadcast-slashingsdisable-dynamic-committee-subnetsdisable-update-head-attestationdisable-noisewait-for-synceddisable-new-beacon-state-locksinit-sync-verbosedisable-finalized-deposits-cacheNote: deep source failed coverage reported lines that's out of this scope