Implement update_validator_registry and seek for a quick review

[hwwhww]

What was wrong?

Part of #223 update_validator_registry

How was it fixed?

I found that I need to finish #306 to upgrade it to the latest version.

But meanwhile, I implemented two versions of update_validator_registry and hope to get some suggestions/advices:

1. update_validator_registry_2: update_validator_registry_2 is similar to the spec: https://github.com/ethereum/eth2.0-specs/blob/dev/specs/core/0_beacon-chain.md#validator-registry-and-shuffling-seed-data:

   # Activate validators within the allowable balance churn
   # Iterate the validator registry, see if it's ready to be activated
   # Check the balance churn would be within the allowance
   # If so, activate the validator
   
   # Exit validators within the allowable balance churn
   # Iterate the validator registry, see if it's ready to be exited
   # Check the balance churn would be within the allowance
   # If so, exit the validator

2. update_validator_registry: there are some logic that could be refactored in update_validator_registry_2, so I made update_validator_registry… but after I finished it, it’s still not ideal since the arguments are not exactly the same.

What do you think about update_validator_registry v.s. update_validator_registry_2?
Any feedback would be appreciated. 🙏

Cute Animal Picture

[ralexstokes]

nitpick (spelling): s/active/activate/

may want to hold off if you think this function will not end up in the final PR

[ralexstokes]

@hwwhww i prefer update_validator_registry. part of this is personal preference but i would rather orchestrate a bunch of small functions, than have to manage one big (monolithic) function.

the primary benefits come via ease of understanding/comprehension and ease of testing (the more names you have for a thing, the more control you have over it; here we go from no names for the subroutines to having at least one name.) the main argument against this implementation is some perceived difficulty in understanding. i would contend that there is a bit more upfront grokking required ("ok, what are these functions, how are they being used in the callee") but once that cost is incurred, all downstream maintenance is much easier.

since you seem concerned about the churn_fn_kwargs not matching (correct me if I'm misreading your comment about "not exactly the same), i'll suggest another possible route to just pass a closure containing the necessary actions to perform. we can achieve uniformity by abstracting out the differences w/ a trusty lambda.

something like:

# in `update_validator_registry`
churn_fn = lambda state, index: activate_validator(state, index, False, config.GENESIS_EPOCH, config.SLOTS_PER_EPOCH, config.ACTIVATION_EXIT_DELAY)
state = churn_validators(
       state=state,
       config=config,
       check_should_churn_fn=is_ready_to_active,
       churn_fn=churn_fn,
       current_epoch=current_epoch,
       max_balance_churn=max_balance_churn,
   )

you could do something similar w/ functools.partial but referring to that implementation it seems like we would need to change the parameters for constants from positional args to keyword args...

we could also make this into a "factory function" but i don't think it's necessary here given how simple this closure is....

( but for completeness, by factory function i just mean something like:

def mk_f(f, args):
    @functools.wraps(f)
    def g(state, index):
        f(state, index, *args)
    return g

)

btw i think what is already here is fine, altho i was asking myself at first why duplicate some of the config constants

edit: another point to consider is that the overarching structure of this function is less likely to change given the status of the phase 0 spec, while the parts that are more likely to change are the exact details (which we have already started to abstract behind a barrier w/ the use of higher-order funcs in churn_validator!)..... i would actually elect more towards update_validator_registry_2 (and not have left such a long comment ;) ) if the spec was less stable

[djrtwo]

I generally agree with @ralexstokes. update_validator_registry might be considered overkill by some, but I found it very easy to read/review.

[djrtwo]

Use positional args here

[hwwhww]

@ralexstokes @djrtwo thanks for the feedback! I like update_validator_registry more after applying @ralexstokes's suggestion! Choosing update_validator_registry now and it's also easy to add update_validator_registry_2 back if there's some radical change from the spec... 😬