Implement process_crosslink

[NIC619]

What was wrong?

Fix #147

How was it fixed?

NOTE: Epoch transition rule is still subject to change in ethereum/consensus-specs#492.

• Implement helper functions
  • get_current_epoch_attestations/get_previous_epoch_attestations
  • get_attesting_validator_indices
  • get_winning_root
  • tests
• Implement process_crosslinks
  • process_crosslinks
  • test
• Fix
  • bitfield length check in get_attestation_participants

Cute Animal Picture

[hwwhww]

I haven't gone through the tests yet.

[hwwhww]

@djrtwo @NIC619
does it make sense to break if len(to_this_shard_attestations) == 0 (skipped slot)?

[NIC619]

@hwwhww Rebased and updated with PR feedbacks.

[hwwhww]

total_attesting_balance was calculated in get_winning_root?

[NIC619]

Yes it was.
Now optimized by returning winning_balance along with winning_root in get_winning_root so we don't have to calculate the attesting balance of the winning_root again.

[hwwhww]

attesting_validators_indices was calculated in get_winning_root?

[pipermerriam]

Instead of casting to set inline in this conditional which will make a full cop of the object see my other comment about just using a set for this data structure.

[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.

cc @jannikluhn @hwwhww @ralexstokes (and anyone else I'm forgetting).

[jannikluhn]

👍 Something like this would be really cool. However, I'm not sure if type safety can be kept, because we have to dynamically extract arguments from the config tuple (and, according to a quick google search, mypy doesn't seem to understand decorators if they change the function signature.

[pipermerriam]

Consider using a new variable name like updated_latest_crosslinks (and you can just do updated_latest_crosslinks = latest_crosslinks in an else clause) as to not overwrite the local variable.

[NIC619]

We only update the state.latest_crosslinks all at once at the end of the function:

    state = state.copy(
        latest_crosslinks= updated_latest_crosslinks,
    )

so this updated_latest_crosslinks must have all the updated crosslinks and it would be equivalent to just using latest_crosslinks.

[NIC619]

@hwwhww Rebased and apply the PR feedbacks. Now working on the slot_to_epoch part.

[NIC619]

@djrtwo I'm going ahead to merge this. Please let me know if there's anything still needs to be corrected and I will fix them in another PR.

[djrtwo]

looks good!