Merklization friendly pending attestations and update per-epoch processing: crosslink, rewards and penalties

[NIC619]

What was wrong?

Fix #381 #383 #384

How was it fixed?

• Split state.latest_attestations into state.previous_epoch_attestations and state.current_epoch_attestations
• Change tie breaks condition for winning_root
• Update condition for punishing penalized active validators
• Handle get_base_reward when no previous balance
• Remove SignedGwei and RewardSettlementContext

Cute Animal Picture

[hwwhww]

Thank you @NIC619!
I haven't finished all the review - first partial review here:

1. In the spec, _process_rewards_and_penalties_for_attestation_inclusion stuff has been moved to compute_normal_justification_and_finalization_deltas - proposer bonus.
2. In _process_rewards_and_penalties_for_finality, IMO the spec version (compute_normal_justification_and_finalization_deltas, only one for-loop) is more succinct that previous_epoch_active_validator_indices+ excluded_active_validators_indices solution here.
3. Generally, in this stage, IMO we do sync the interface as the spec if we can; if we find a way that is much more succinct or efficient than the spec, propose it in the spec repo.

[NIC619]

@hwwhww Thanks for the review! Updated accordingly.

[ralexstokes]

awesome job! looks like you tackled a lot of the 711 stuff in this PR. i've reviewed all of the library changes and will make a pass at the tests tomorrow. i've left some comments on areas i thought could be improved or clarified.

a general suggestion i would add is to prefer smaller PRs in the future :) it makes the reviewer's job easier if we keep them small and focused. this PR could easily be three (one for each issue addressed) and arguably could be broken down further.

either way, this moves us along quite nicely through the 711 PR and i'm looking forward to getting it merged :)

edit: tests seem to check out :)

[ralexstokes]

general comment on readability here: we are already pretty deep into a function and executing a loop inside of a loop. in this inner loop body we have an if/else conditional inside another if/else conditional. the appropriate jargon here is cyclomatic complexity and the idea is that it is a lot for the reader to keep track of!

[ralexstokes]

i'll leave some suggestions -- feel free to take or leave what you find helpful...

1. let's do "one thing at a time" -- the first thing we do is, for every slot in the range, determine if we need to update the shards touched in that slot by the relevant crosslink committees.

so what we could do is to pull out all of the code in this top-level loop into its own function that yields (Shard, CrosslinkRecord) pairs. after we consume the loop iterator (i.e. collected every such pair from all relevant slots) then we want to overwrite the corresponding storage slot w/ the new CrosslinkRecord we got. this looks like a use of update_tuple_item.

2. now we have this new function that yields any such new CrosslinkRecords (along w/ the corresponding Shard so we know which one it belongs to). this corresponds to the inner body of the top-level loop. my next suggestion is arguably stylistic but i'll mention it and you can decide which one you like better -- i think the following gets the high-level idea across better than the nested code we have now:

• given a sequence of (crosslink_committee, shard), map the get_winning_root_and_participants over it.
• map the pair of get_total_balance functions over this to get the inputs to our conditional
• filter where the conditional is true, i.e. if 3 * total_attesting_balance >= 2 * total_balance
• map the remaining streams of computation to our desired (shard, CrosslinkRecord) pairs.

if this isn't clear lmk and I can toss together a gist that sketches this out more clearly :)

[NIC619]

@ralexstokes I took my first stab at breaking down the loops and if/else condition checks. One thing that disturbs me in my approach is that the objects that different functions/filters mapped onto grows to as large as five elements per object - the elements are slot, crosslink_committee, shard, winning_root and attesting_indices respectively.

• slot and winning_root are needed for the final CrosslinkRecord update.
• crosslink_committee and attesting_indices are needed for computing the total_balance and total_attesting_balance.

#
# Crosslinks
#
@to_tuple
def _get_committees_across_slots(
        state: BeaconState,
        config: BeaconConfig,
        start_slot: Slot,
        end_slot: Slot) -> Iterable[Tuple[Slot, Sequence[ValidatorIndex], Shard]]:
    """
    Get the list of crosslink committees at each slot.
    Return (slot, crosslink_committees, shard) pair.
    """
    for slot in range(start_slot, end_slot):
        crosslink_committees_at_slot = get_crosslink_committees_at_slot(
            state,
            slot,
            CommitteeConfig(config),
        )
        crosslink_committee: Sequence[ValidatorIndex]
        shard: Shard
        for crosslink_committee, shard in crosslink_committees_at_slot:
            yield (Slot(slot), crosslink_committee, shard)


def _get_root_and_indices(
        committees_across_slots: Tuple[Slot, Sequence[ValidatorIndex], Shard],
        get_winning_root_and_participants_from_shard: Callable[[Shard], Tuple[Hash32, Sequence[ValidatorIndex]]]) -> Tuple[Slot, Sequence[ValidatorIndex], Shard, Hash32, Sequence[ValidatorIndex]]:
    """
    Apply ``get_winning_root_and_participants`` onto the (slot, crosslink_committees, shard) pair
    and return (slot, crosslink_committees, shard, winning_root, attesting_validator_indices) pair
    """
    slot, crosslink_committee, shard = committees_across_slots
    winning_root, attesting_validator_indices = get_winning_root_and_participants_from_shard(
        shard=shard,
    )
    return (slot, crosslink_committee, shard, winning_root, attesting_validator_indices)


def _filter_attesting_committee(
        committees_and_root_and_indices: Tuple[Slot, Sequence[ValidatorIndex], Shard, Hash32, Sequence[ValidatorIndex]]) -> bool:
    """
    Filter out (slot, crosslink_committees, shard, winning_root, attesting_validator_indices) pairs whose
    ``attesting_validator_indices`` is empty.
    """
    attesting_validator_indices = committees_and_root_and_indices[4]
    return len(attesting_validator_indices) > 0


def _filter_enough_attesting_committee(
        attesting_committees_and_root_and_indices: Tuple[Slot, Sequence[ValidatorIndex], Shard, Hash32, Sequence[ValidatorIndex]],
        get_total_balance_for_indices: Callable[[Sequence[ValidatorIndex]], Gwei]) -> bool:
    """
    Apply ``get_total_balance`` on ``crosslink_committees`` and ``attesting_validator_indices`` 
    from (slot, crosslink_committees, shard, winning_root, attesting_validator_indices) pair
    and filter out pairs which does not have enough attesting balance.
    """
    _, crosslink_committee, _, _, attesting_validator_indices = attesting_committees_and_root_and_indices
    total_attesting_balance = get_total_balance_for_indices(
        validator_indices=attesting_validator_indices,
    )
    total_balance = get_total_balance_for_indices(
        validator_indices=crosslink_committee,
    )
    return (3 * total_attesting_balance >= 2 * total_balance)


def process_crosslinks(state: BeaconState, config: BeaconConfig) -> BeaconState:
    """
    Implement 'per-epoch-processing.crosslinks' portion of Phase 0 spec:
    https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#crosslinks

    For each shard from the past two epochs, find the shard block
    root that has been attested to by the most stake.
    If enough(>= 2/3 total stake) attesting stake, update the crosslink record of that shard.
    Return resulting ``state``
    """
    latest_crosslinks = state.latest_crosslinks
    effective_balances = {
        ValidatorIndex(index): get_effective_balance(
            state.validator_balances,
            ValidatorIndex(index),
            config.MAX_DEPOSIT_AMOUNT,
        )
        for index in range(len(state.validator_registry))
    }
    previous_epoch_start_slot = get_epoch_start_slot(
        state.previous_epoch(config.SLOTS_PER_EPOCH, config.GENESIS_EPOCH),
        config.SLOTS_PER_EPOCH,
    )
    next_epoch_start_slot = get_epoch_start_slot(
        state.next_epoch(config.SLOTS_PER_EPOCH),
        config.SLOTS_PER_EPOCH,
    )

    # Get crosslink committees at each slot
    committees_across_slots = _get_committees_across_slots(state, config, previous_epoch_start_slot, next_epoch_start_slot)

    # Map `get_winning_root_and_participants` onto each crosslink committees
    get_winning_root_and_participants_from_shard = functools.partial(
        get_winning_root_and_participants,
        state=state,
        effective_balances=effective_balances,
        committee_config=CommitteeConfig(config),
    )
    _get_root_and_indices_from_committees = functools.partial(
        _get_root_and_indices,
        get_winning_root_and_participants_from_shard=get_winning_root_and_participants_from_shard,
    )
    committees_and_root_and_indices = map(
        _get_root_and_indices_from_committees,
        committees_across_slots,
    )

    # Filter out crosslink committees which did not attest
    attesting_committees_and_root_and_indices = filter(
        _filter_attesting_committee,
        committees_and_root_and_indices,
    )

    # For each crosslink committee, map `get_total_balance` onto attesting member
    # and total memeber of crosslink committee.
    # And filter out crosslink committees without enough attesting balance.
    get_total_balance_for_indices = functools.partial(
        get_total_balance,
        validator_balances=state.validator_balances,
        max_deposit_amount=config.MAX_DEPOSIT_AMOUNT,
    )
    filter_enough_attesting_committee = functools.partial(
        _filter_enough_attesting_committee,
        get_total_balance_for_indices=get_total_balance_for_indices,
    )
    enough_attesting_committees_and_root_and_indices = filter(
        filter_enough_attesting_committee,
        attesting_committees_and_root_and_indices,
    )

    # Update CrosslinkRecord for shard with enough attesting balance
    for committees_and_root_and_indices in enough_attesting_committees_and_root_and_indices: # type: ignore
        slot, _, shard, winning_root, _ = committees_and_root_and_indices
        latest_crosslinks = update_tuple_item(
            latest_crosslinks,
            shard,
            CrosslinkRecord(
                epoch=slot_to_epoch(Slot(slot), config.SLOTS_PER_EPOCH),
                crosslink_data_root=winning_root,
            ),
        )
    state = state.copy(
        latest_crosslinks=latest_crosslinks,
    )
    return state

[ralexstokes]

nice! yeah this is what I had in mind... i do see what you mean -- some of those type signatures start to get a little unwieldly! if we were fixed on these specific signatures then we could make that better by defining a type alias and using that in its place.

although here we can probably play w/ how we divide up the functions and what they are returning... for example, you have slot, _, shard, winning_root, _ = ... -- why are we returning the values if we are just going to discard them w/ _? I may just be missing their use somewhere else so lmk.

and ultimately some of this is stylistic -- i would support whatever seems the most readable/maintainable to you to go into this PR :) if you think the other implementation is nicer, then i would just suggest we remove some of those branches where we just pass (e.g. by flipping the sense of conditionals, etc.)

[NIC619]

Push my updates which includes:

• type alias(CrosslinkRecordData and CrosslinkParticipantIndices)
  • CrosslinkRecordData = Tuple[Slot, Shard, Hash32]
    • this is for (slot, shard, winning_root) pair
  • CrosslinkParticipantIndices = Tuple[Sequence[ValidatorIndex], Sequence[ValidatorIndex]]
    • this is for (committee, attesting_indices) pair
• and break the gigantic (slot, committee, shard, winning_root, attesting_indices) pair into CrosslinkRecordData and CrosslinkParticipantIndices pairs
  • CrosslinkParticipantIndices is only used to filter out CrosslinkRecordData w/o enough attesting balance
  • in the final loop for CrosslinkRecord update, only CrosslinkRecordData is used

[ralexstokes]

similar comment here about readability, although i think this is fine for now as it closely mirrors the spec and feels more manageable to me than the other spot above...

but you could imagine each chunk of logic that follows an explanatory comment is its own function

[NIC619]

Agree this part is rather more manageable than process_crosslinks. Will leave this as is.

[NIC619]

Thanks for the review @ralexstokes ! It's really helpful!
Sorry I didn't realize I was also fixing #381 when I started working on #383 . I've been pushing some really large PRs lately… apology to anyone who had reviewed them. 🤦‍♂️ Will improve on my tasks segregation skill!

[ralexstokes]

@NIC619 sure, no worries! it is easy for them to grow out of control ^_^

i'm a little concerned about how much is going on in #397!

[hwwhww]

happy with the new rewarding now 👍

But somehow found it's hard to review the crosslink section (comparing to the pretty short code snippet process_crosslinks in the spec). 👎

[NIC619]

I remove my failed experimental functional approach on process_crosslinks. I thought I just need some time to get used to the functional method and it's style but clearly it's that I need more practice on this. 😅

[NIC619]

@ralexstokes Are there many overlap between this PR and yours? Should I squash and merge it?

[ralexstokes]

@NIC619 sorry for the delay on this... I think we are ready to merge this in! The other PR should not overlap with this one.