Unregister validator - fix behind feature flag #12316
Conversation
james-prysm
added
Builder
beacon-chain/cache/registration.go
Outdated
| if timeStampExpired(v.Timestamp) { | ||
| delete(regCache.indexToRegistration, id) | ||
| return nil, errors.Wrapf(ErrNotFoundRegistration, "validator id %d", id) | ||
| } |
There was a problem hiding this comment.
This is a mutating operation. You will need to use a write lock if you want to modify the map in this method
beacon-chain/db/kv/blocks.go
Outdated
| func timeStampExpired(ts uint64) bool { | ||
| expiryDuration := time.Duration(params.BeaconConfig().SecondsPerSlot*uint64(params.BeaconConfig().SlotsPerEpoch)*3) * time.Second | ||
| // safely convert unint64 to int64 | ||
| t, err := strconv.ParseInt(fmt.Sprint(ts), 10, 64) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return time.Unix(t, 0).Add(expiryDuration).Unix() < time.Now().Unix() | ||
| } |
There was a problem hiding this comment.
Is this different from the other timeStampExpired in this PR?
There was a problem hiding this comment.
yes this is the same, I didn't find a great place to place this but will try to put it in a place where it'd make sense. was thinking maybe we can keep it in both places until full removal but will try to find a better spot without circular dependencies.
beacon-chain/cache/registration.go
Outdated
| // safely convert unint64 to int64 | ||
| t, err := strconv.ParseInt(fmt.Sprint(ts), 10, 64) |
There was a problem hiding this comment.
This seems like an expensive conversion to string and then to int64. Don't we have some more performance based conversions somewhere?
There was a problem hiding this comment.
will use a better solution based on slack discussions
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
beacon-chain/cache/registration.go
Outdated
| } | ||
|
|
||
| func RegistrationTimeStampExpired(ts uint64) bool { | ||
| expiryDuration := time.Duration(params.BeaconConfig().SecondsPerSlot*uint64(params.BeaconConfig().SlotsPerEpoch)*3) * time.Second |
There was a problem hiding this comment.
is this a spec derived value ? Even if its only a prysm specific value, it shouldn't be a magic number in this function. The number of epochs for the expiry duration should be a constant set outside this function.
There was a problem hiding this comment.
it's not in the specs, but was a number that most other clients also chose from the original proposal
beacon-chain/cache/registration.go
Outdated
| func RegistrationTimeStampExpired(ts uint64) bool { | ||
| expiryDuration := time.Duration(params.BeaconConfig().SecondsPerSlot*uint64(params.BeaconConfig().SlotsPerEpoch)*3) * time.Second | ||
| // safely convert unint64 to int64 | ||
| t := new(big.Int).SetUint64(ts).Int64() |
beacon-chain/db/kv/blocks.go
Outdated
| if err := decode(ctx, enc, reg); err != nil { | ||
| return err | ||
| } | ||
| if cache.RegistrationTimeStampExpired(reg.Timestamp) { |
There was a problem hiding this comment.
why are we checking the cache in the db like this ? It seems like a bad separation of concerns
There was a problem hiding this comment.
This is a utility function I just didn't find a great place to put this time function, i tried to add it in the builder at first but it creates circular dependencies.
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
| if err != nil { | ||
| return false, err | ||
| } | ||
| expiryDuration := params.BeaconConfig().RegistrationDuration |
There was a problem hiding this comment.
I hope this is a good way of doing it...
| func NewRegistrationCache() *RegistrationCache { | ||
| return &RegistrationCache{ | ||
| indexToRegistration: make(map[primitives.ValidatorIndex]*ethpb.ValidatorRegistrationV1), | ||
| lock: sync.RWMutex{}, |
There was a problem hiding this comment.
deepsource complained the lock was open on the struct so now i added it internally here, can revert to align with other caches.
beacon-chain/cache/registration.go
Outdated
| // GetRegistrationByIndex returns the registration by index in the cache and also removes items in the cache if expired. | ||
| func (regCache *RegistrationCache) GetRegistrationByIndex(id primitives.ValidatorIndex) (*ethpb.ValidatorRegistrationV1, error) { |
There was a problem hiding this comment.
Optional: effective go guidelines suggest that you don't use the prefix "Get" for a getter and simply call it the thing like regCache.RegistrationByIndex(...
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
This cache implementation is hidden behind a feature flag and is only applicable while using mev relay features.
--enable-registration-cacheand will not completely replace the db.By enabling the registration cache we are migrating away from persistent registration storage on the beacon node side. We will also be adding expiration to validator registrations to be more aligned with other client teams.
note: This flag and feature will not be recommended for those who are still using the keymanager apis to configure the fee recipients.
The following PR #12354 will add the features required for persistent keymanager API changes with --enable-registration-cache
jaeger with db (old)
jaeger with cache (new)
Learn more about builders here.
https://ethereum.github.io/builder-specs/#/Builder
Which issues(s) does this PR fix?
Fixes #12274