Reorg helper functions

[hwwhww]

What is wrong?

There are many helper functions in eth/beacon/helpers.py. IMO Several reasons make it worth to be sorted out:

1. Long tests/beacon/test_helpers.py for the tests and the helpers of the tests. It’s not easy to divide the code sections.
2. More recent cleanup ideas on the spec side: The great function cleanup/rework consensus-specs#380
3. Per Reorg validate_proposer_signature py-evm#1684 (comment), @pipermerriam: I'm always a little nervous with indexing into iterables naively like this. I understand/recognize that the previous get_beacon_proposer_index provides some level of guarantee that the beacon_proposer_index is indeed present and accurate in the validator_registry iterable. What if this was abstracted away on the BeaconState class as something like state.get_beacon_proposer(epoch_length):

   proposer = state.get_beacon_proposer(epoch_length)
   proposer_pubkey = proposer.pubkey

   • I hesitate due to the pros and cons if we want to make all of them to instance methods:
     • Pros:
       • Intuitively, state.get_beacon_proposer(epoch_length) makes a lot sense for the user.
     • Cons:
       • As @djrtwo mentioned (The great function cleanup/rework consensus-specs#380 (comment)), pure functions are easier to tests. Also IMO it has clear boundaries of the essential inputs.
       • Not sure if we will apply it on the spec side; if not, our codebase will be quite different from the spec and makes it harder to sync with the spec.

How can it be fixed

Some ideas, TBD:

1. BeaconState
   1. get_block_root(latest_block_roots: Sequence[Hash32], current_slot: SlotNumber, slot: SlotNumber) -> Hash32 -> BeaconState.get_block_root(slot: SlotNumber) -> Hash32.
   2. get_shard_committees_at_slot(state: 'BeaconState', slot: SlotNumber, epoch_length: int) -> Tuple[ShardCommittee] -> BeaconState.get_shard_committees_at_slot(slot: SlotNumber, epoch_length: int) -> Tuple[ShardCommittee]
   3. get_shuffling(*, seed: Hash32, validators: Sequence['ValidatorRecord'], crosslinking_start_shard: ShardNumber, slot: SlotNumber, epoch_length: int, target_committee_size: int, shard_count: int) -> Iterable[Iterable[ShardCommittee]] -> BeaconState.get_shuffling(*, seed: Hash32, crosslinking_start_shard: ShardNumber, slot: SlotNumber, epoch_length: int, target_committee_size: int, shard_count: int) -> Iterable[Iterable[ShardCommittee]]
   4. get_beacon_proposer_index(state: 'BeaconState', slot: SlotNumber,epoch_length: int) -> ValidatorIndex -> BeaconState.get_beacon_proposer_index(slot: SlotNumber,epoch_length: int) -> ValidatorIndex
   5. get_attestation_participants(state: 'BeaconState', slot: SlotNumber, shard: ShardNumber, participation_bitfield: Bitfield, epoch_length: int) -> BeaconState.get_attestation_participants(slot: SlotNumber, shard: ShardNumber, participation_bitfield: Bitfield, epoch_length: int).
   6. get_effective_balance(validator_balances: Sequence[Gwei], index: ValidatorIndex, max_deposit: Ether) -> Beacon.State.get_effective_balance(index: ValidatorIndex, max_deposit: Ether)
2. get_fork_version, get_domain -> Move to eth/beacon/types/fork_data.py
3. verify_slashable_vote_data -> Move to eth/beacon/state_machines/forks/serenity/validation.py

[hwwhww]

Link and copy-paste #228 (comment) by @pipermerriam:

not for this PR but an idea I've had a few times.

I think it is good that each of these nice functional utilities takes only the set of config values that it needs. This helps make it easy to see what config values each function depends on and keeps the implementations decoupled from the config object.

however, it's also really unfortunate how verbose some of this code ends up because you end up having to explicitly provide each parameter...

So I was thinking about an API that allowed you to take a config and auto-magically partial out all of these helper functions. Here's some pseudo code to illustrate.

class BeaconHelperAPI:
    def __init__(self, config):
        # option 1: do it dynamically in constructor (con: hard to maintain type safety)
        self.get_attesting_validator_indices = functools.partial(
            get_attesting_validator_indices,
            epoch_length=config.EPOCH_LENGTH ,
            ...
        )

    def get_attesting_validator_indices(state, attestations, shard, shard_block_root):
        # option 2: mirror the API (con: heavy on boilerplate, duplicates all function signatures)
        return get_attesting_validator_indices(..., epoch_length=self.config.EPOCH_LENGTH, ...)

I'm sure there are some other options for how it could be implemented as well. Here's an overly clever one that could be really good if done well but also might be too clever and end up bad.

class Wrapper:
    def __init__(self, fn):
        self.fn = fn
    def __call__(self, *args, **kwargs):
         return fn(*args, **kwargs)
    def partial(self, config):
         kwargs = {kw: getattr(config, kw.upper()) for kw in keywords_to_partial}
         return functools.partial(self.fn, **kwargs)

def configurable(*keywords_to_partial):
    def outer(fn):
        return Wrapper(fn)
    return outer

@configurable('epoch_length', 'target_committee_size', 'shard_count')
def get_attesting_validator_indices(stat, attestations, shard, shard_block_root, epoch_length, target_committee_size, shard_count):
    ...

# using it in code
get_attestating_validator_indices.partial(config)(state, attestations, shard, shard_block_root)

Anyways, some ideas on how you might make calling these helpers a little less cumbersome, but I'd advocate for ensuring that whatever solution you end up using, that you ensure that you maintain type safety.

[pipermerriam]

BTW, that last code example using the Wrapper should really probably be inverted. The goal would simply be the final API of being able to do helper_fn.partial(config)(<only-non-config-args>) style calling. The exact implementation could be very different. In hindsight I realize that the Wrapper class can easily be done away from by just assigning a partial function to the fn object.

[hwwhww]

I amend the Wrapper pseudo code to:

class Wrapper:
    def __init__(self, fn, keywords_to_partial):
        self.fn = fn
        self.keywords_to_partial = keywords_to_partial

    def __call__(self, *args, **kwargs):
        return self.fn(*args, **kwargs)

    def partial(self, config):
        kwargs = {kw: getattr(config, kw.upper()) for kw in self.keywords_to_partial}
        return functools.partial(self.fn, **kwargs)


def configurable(*keywords_to_partial):
    def outer(fn):
        return Wrapper(fn, keywords_to_partial)
    return outer

@pipermerriam could you elaborate what do you mean by "by just assigning a partial function to the fn object"?

---

Another way to mitigate the verbosity is we can define some reasonable NameTuple subset. e.g., I see we pass (shard_count, epoch_length, target_committee_size) together frequently for shuffling, so maybe we could make a ShufflingConfig.

[ralexstokes]

closing this issue due to staleness. we can revisit if it is still relevant