remove withdrawn_epoch

[djrtwo]

Handle 4th point in #2758 due to the desire to not change the shape of the validators datastructure and merkle tree.

• Remove fully_withdrawn_epoch
• Add additional full withdrawal tests for various 0 balance iterations
• Add randomized full withdrawals tests
• Add top-up to fully withdrawn validator test
• [Bonus] add some phase0+ tests for top-ups, showing no effective balance change

The point of fully_withdrawn_epoch is to be able to potentially re-use indices after some extended period of non-use (e.g. 3x max best-WS period -- (3x4) 12 months). A zero balance and withdrawable_epoch are sufficient to capture this logic for safe reuse. Thus remove fully_withdrawn_epoch

In the event that a top-up is made to such a validator, a new full withdrawal is initiated. This requires 1ETH to perform and is rate limited by the Deposit queue.

[hwwhww]

The PR logic looks good to me. Great work 👍

We need to double-check if we do want the "re-use indices" feature.

• Pros:
  • Code simplification
  • Save data storage and validator set iteration time
• Cons:
  • The fully_withdrawn_epoch was useful to look up the validator withdrawal time.
  • The validator index is no more the reliable identifier. It has to come with their pubkey.

Generally, I think pros > cons, so the PR looks good to me. 👍 But would love to hear more feedback too.

[potuz]

I am not fully convinced that reusing the validator index is really that much impactful. It sounds to me as an implementation detail more than a specification item I am concerned by validator registry size. But applications could simply delete the validator entry from whatever storage option they choose and add an index field to the validator object that is guaranteed to be increasing. Loops run over the storage object, not over the indexing. Adding a field discards the benefit of this PR about keeping the number of fields a power of 2, which can be used to speed up Merkleization, but that in itself is a hack.

[dapplion]

I would like to capitalize on this optimization even if re-usable indexes are not adopted. The validator tree is the most expensive part of the state, not increasing depth by one unless really necessary has a non-negligible impact

[potuz]

I would like to capitalize on this optimization even if re-usable indexes are not adopted. The validator tree is the most expensive part of the state, not increasing depth by one unless really necessary has a non-negligible impact

Depth does not change in the validator registry Merkle tree, the hack is that if you lay out all field roots contiguously you can hash that tree as if it were of depth log(VALIDATOR_REGISTRY_LIMIT)+3 and get the right root. However, this can only be useful when hashing the full validator set which is seldomly done. Adding an entry to the validator object screws this hack more because of the zero allocations needed (if the same technique is to be used) than the depth increase.

The point I want to make is that we can easily keep the validator registry dense, not sparse, and not reuse validator indices at the same time without much complexity, and the cost of having to check public key is likely worse than simply adding an scalar index field.

[djrtwo]

I am not fully convinced that reusing the validator index is really that much impactful.

I just mainly want to point out that use of withdrawable_epoch and zero-balance give us the same reuse safety In the event that we want to reuse. So might as well not add the withdrawn epoch, regardless

[ralexstokes]

looks great!

I like keeping the option to implement reusable validator indices (even though it is in aggregate probably not our best idea) and this PR would keep the validator subtree aligned along nice power of two

I left two suggestions for what I think are missing asserts and otherwise just some notes in case anyone is ever reviewing this later

[ralexstokes]

any reason to not use the run_epoch_processing_with helper?

[djrtwo]

yes this pattern is useful when you need to capture information right before the function is called.

In this case, balance updates can/will occur during those interim epoch processings. So in some cases this changes the to_be_withdrawn indices in some test cases

[ralexstokes]

note: this violates some spec invariants but is not material for the thing this test intends to verify so we can let it be

[djrtwo]

This does not actually do so

You can have an epoch in which your balance was initially spec.MAX_EFFECTIVE_BALANCE - spec.EFFECTIVE_BALANCE_INCREMENT - 1000 and your initial effective balance is as above. Then you could receiv a 1 ETH deposit, pushing your balance (and effective) to the balance above as your initial conditional