feat: BLS key rotation #249
Conversation
There was a problem hiding this comment.
Just did a quick glance review. Will try to summarize what this is doing to make sure I'm not missing anything:
- operator can either be ejected (force deregister) or deregister by themself
- both of those have cooldowns (ejectionCooldown and deregistrationCooldown) which force the operator to wait before reregistering
- We keep the entire history of registered bn254 G1 pubkey hash onchain, such that the AVS can slash an operator for misbehavior using a previous key
Have a Q related to why we need deregistrationCooldown below.
| // Check that the operator can reregister | ||
| require( | ||
| lastEjectionTimestamp[operator] + ejectionCooldown < block.timestamp && | ||
| lastDeregistrationTimestamp[operator] + deregistrationCooldown < block.timestamp, |
There was a problem hiding this comment.
why do we need a deregistrationCooldown if we keep the entire history of keys onchain?
There was a problem hiding this comment.
Or is 0 a reasonable value for deregistrationCooldown?
There was a problem hiding this comment.
@gpsanant has suggested this should be a few days. I think something like an hour is more reasonable
There was a problem hiding this comment.
this...limits the rate at which operators can edit their keys. I don't think it's serving a serious purpose beyond maybe addressing some theoretical worry where an operator keeps changing their key? I guess if you need to search past keys it's potentially problematic if the growth rate of the historical storage isn't limited?
| function setDeregistrationCooldown(uint256 _deregistrationCooldown) external onlyOwner { | ||
| deregistrationCooldown = _deregistrationCooldown; | ||
| } |
There was a problem hiding this comment.
needs an event.
might want this added to the initializer as well.
I'm considering if any input sanitization makes sense here too -- should this have a min or max? is 0 an OK input? I feel like it's probably OK to not have this, given the owner-only nature of the function... I don't see any super "unsafe" values
There was a problem hiding this comment.
Didn't add to init because of bytecode size but I will see if it can be squeezed in. I could see really long values being not great for the delay but will probably not add sanitization for the same bytecode size issue
There was a problem hiding this comment.
discussed in-person, no sanitization seems OK, not in initializer is also acceptable to me (although not my ideal), no event is something I can live with but probably the most desirable of these
There was a problem hiding this comment.
The new view functions LGTM. They differ a bit from the pattern elsewhere due to the historical storage model being different here, but as far as I can tell it's all written correctly. Adding some unit tests that verify the behavior of these view functions would be 🔥 but I'm not going to require that for my approval on this PR.
The nits that I would have considered blocking on have been addressed.
Nice work 👍
No description provided.