dynamic validator set #256
Conversation
tcoratger
left a comment
tcoratger
left a comment
There was a problem hiding this comment.
This looks nice, I've left couple of comments and I think that for some reasons the test vectors are failing in the CI, probably due to some changes inside the PR.
|
|
||
| # --- Validator Lifecycle Parameters --- | ||
|
|
||
| MIN_ACTIVATION_DELAY: Final = Uint64(8) |
There was a problem hiding this comment.
| MIN_ACTIVATION_DELAY: Final = Uint64(8) | |
| MIN_ACTIVATION_DELAY: Slot = Slot(8) |
| 3. Time for validation and consensus on the new validator | ||
| """ | ||
|
|
||
| MIN_EXIT_DELAY: Final = Uint64(8) |
There was a problem hiding this comment.
| MIN_EXIT_DELAY: Final = Uint64(8) | |
| MIN_EXIT_DELAY: Slot = Slot(8) |
| min_activation_delay: Uint64 | ||
| min_exit_delay: Uint64 |
There was a problem hiding this comment.
| min_activation_delay: Uint64 | |
| min_exit_delay: Uint64 | |
| min_activation_delay: Slot | |
| min_exit_delay: Slot |
| class ValidatorDeposits(SSZList[ValidatorDeposit]): | ||
| """List of validator deposits included in a block.""" | ||
|
|
||
| ELEMENT_TYPE = ValidatorDeposit |
There was a problem hiding this comment.
With #259
| ELEMENT_TYPE = ValidatorDeposit |
| class ValidatorExits(SSZList[ValidatorExit]): | ||
| """List of validator exit requests included in a block.""" | ||
|
|
||
| ELEMENT_TYPE = ValidatorExit |
There was a problem hiding this comment.
With #259
| ELEMENT_TYPE = ValidatorExit |
| from ..deposit import ValidatorDeposits | ||
| from ..exit import ValidatorExits |
There was a problem hiding this comment.
Can we move this to the top of the file?
| class PendingDeposits(SSZList[PendingDeposit]): | ||
| """Queue of validators awaiting activation.""" | ||
|
|
||
| ELEMENT_TYPE = PendingDeposit |
There was a problem hiding this comment.
With #259
| ELEMENT_TYPE = PendingDeposit |
| class ExitQueue(SSZList[ExitRequest]): | ||
| """Queue of validators awaiting removal.""" | ||
|
|
||
| ELEMENT_TYPE = ExitRequest |
There was a problem hiding this comment.
With #259
| ELEMENT_TYPE = ExitRequest |
| min_activation_delay: Uint64 = Uint64(8) | ||
| """Minimum slots before a validator deposit activates.""" | ||
|
|
||
| min_exit_delay: Uint64 = Uint64(8) | ||
| """Minimum slots before a validator exit is removed.""" |
There was a problem hiding this comment.
| min_activation_delay: Uint64 = Uint64(8) | |
| """Minimum slots before a validator deposit activates.""" | |
| min_exit_delay: Uint64 = Uint64(8) | |
| """Minimum slots before a validator exit is removed.""" | |
| min_activation_delay: Slot = Slot(8) | |
| """Minimum slots before a validator deposit activates.""" | |
| min_exit_delay: Slot = Slot(8) | |
| """Minimum slots before a validator exit is removed.""" |
| ) | ||
|
|
||
| encode = state.encode_bytes() | ||
| expected_value = ( |
There was a problem hiding this comment.
I guess that you removed the expected value because now it changed with the new fields added?
|
Just my 2 cents. I think it might be too early to take dynamic validators now, i.e.
So to me if these aren't part of any upcoming lean experimentation or some unknowns we want to explore, we might not want to include it yet? Otherwise it's quite a lot of context to maintain. Or unless we start shipping a modular leanSpec where components can be mixed & matched for different experimentations. In that case then maybe this could go as a separate |
|
In pqinterop on 14 Jan, we concluded that for the finality issue on devnet-1, we can workaround by switching from one client implementation to another, especially given that sync will be available in devnet-2 and checkpoint sync is also becoming available by some of the clients. This PR is a really good initiation though! We can reconsider dynamic validator again once we have the target consensus mechanism in place. I'll close this PR for now but feel free to reopen if I misunderstood or things have changed. |
3 participants
🗒️ Description
This PR introduces dynamic validator set. The current assumption is the validator entry/exit requests would received over wire but it definitely up for discussion like all the other parts of the PR.
toxchecks to avoid unnecessary CI fails:uvx tox