Add executable gossip validation functions for phase0

[jtraglia]

This PR starts the process of making the networking specifications executable:

• Make p2p interface specifications executable/testable #4611

This PR only adds specs for phase0. If merged, I will make follow up PRs for new topics & updates.

There are 74 reference tests to ensure clients adhere to the specs.

I tested these with Teku and can confirm that all but the following tests are passing:

• gossip_beacon_aggregate_and_proof__reject_block_failed_validation
• gossip_beacon_aggregate_and_proof__ignore_finalized_not_ancestor
• gossip_beacon_attestation__reject_block_failed_validation
• gossip_beacon_attestation__ignore_already_seen
• gossip_beacon_block__reject_finalized_checkpoint_not_ancestor
• gossip_beacon_block__reject_parent_failed_validation
• gossip_beacon_block__ignore_slot_not_greater_than_finalized

These are ignored because Teku performs these checks outside of their validation functions.

See: Consensys/teku@master...jtraglia:teku:executable-networking-specs

[nflaig]

this is great, will try to integrate the tests into Lodestar, we had many missed gossip checks in the past due to lack of test coverage

[nflaig]

Lodestar is passing 74/74 and was pretty easy to integrate ChainSafe/lodestar#8965, also caught 3 spec mismatches

[jtraglia]

Lodestar is passing 74/74 and was pretty easy to integrate ChainSafe/lodestar#8965, also caught 3 spec mismatches

That's awesome 🔥 I'll push to get this reviewed/merged soon.

[jihoonsong]

This is nice. It delivers clearly in code than natural language. Great work!

[jihoonsong]

I've given some thoughts on abstraction/generalization around MAXIMUM_GOSSIP_CLOCK_DISPARITY as we already know the p2p specs for next forks. It's a bit lengthy comment so please bear with me.

MAXIMUM_GOSSIP_CLOCK_DISPARITY is used in comparision between slots, or current time against the time passed within a slot. With the given slot and current_time_ms, we can compare them by either 1) converting the given slot to milliseconds and +- MAXIMUM_GOSSIP_CLOCK_DISPARITY, or 2) converting current_time_ms +- MAXIMUM_GOSSIP_CLOCK_DISPARITY to slot. Although two are equivalent, I think it is better to convert slot to milliseconds because it is more generalized, i.e., it can handle the sub-slot comparision such as the lightclient case.

Then, we can summarize that MAXIMUM_GOSSIP_CLOCK_DISPARITY is used in three cases:

i) <= : check if the message is not from future
ii) == : check if the message is for current slot
iii) range : check if the message is within a range

We can define a helper function for each of those cases in Phase0 and reuse them in subsequent forks. Example signatures for those functions are:

i) def is_not_from_future_slot(state: BeaconState, slot: Slot, current_time_ms: uint64) -> bool
ii) def is_current_slot(state: BeaconState, slot: Slot, current_time_ms: uint64) -> bool
iii) def is_within_slot_range(state: BeaconState, start_slot: Slot, end_slot: Slot, current_time_ms: uint64) -> bool

For instance, is_valid_attestation_slot_time can call or be replaced with is_within_slot_range(state, attestation_slot, Slot(attestation_slot + ATTESTATION_PROPAGATION_SLOT_RANGE), current_time_ms)

Maybe it's an overgeneralization? Keen to learn how this sounds.

[jihoonsong]

Regarding MAXIMUM_GOSSIP_CLOCK_DISPARITY, do we have to consider it in execution_payload_bid, proposer_preferences and inclusion_list as well?

[jtraglia]

Yes, I like this idea a lot. Generalized functions that we can use later is good. Fixed df2ae7c.

I didn't add the is_current_slot function here just yet. We can introduce that in Altair.

def is_current_slot(
    state: BeaconState,
    slot: Slot,
    current_time_ms: uint64,
) -> bool:
    """
    Check if the given slot is the current slot
    (with MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance).
    """
    return is_within_slot_range(state, slot, slot, current_time_ms)

Regarding MAXIMUM_GOSSIP_CLOCK_DISPARITY, do we have to consider it in execution_payload_bid, proposer_preferences and inclusion_list as well?

Yes, and it looks like we don't say that in the specs 😢 Let's be sure to do this when we make the networking specs for those executable. I don't want to introduce natural language for that right now.

[jihoonsong]

What do you think about specifying which state it should pass? This can be extended to other parameters as well, making it more explicit.

For instance, state in this validation is used for beacon committee calculation so it should be fresh enough to calculate beacon committee at target epoch. Clients adopt different approaches such as fetching state at the first slot of target epoch or one epoch earlier to target epoch, or using cached beacon committee calculation.

The spec could always assume state and store at the head, but maybe it's better to explain what are allowed.

[jtraglia]

Yes, I do prefer to be explicit when possible. I don't want to rename the variables (eg state -> head_state) because that will make the function harder to read. On the other hand, we have assumptions like this all of the place, so not specifying it here is on-par with existing specs. For now, I pushed this commit (9f31fbb) which tells the reader that the state is the head state. This feels a bit lazy, so maybe we can follow up later which explains why for each one.

[jtraglia]

@nflaig @zilm13 here are the latest reference tests: reftests.zip

Please test them out if/when you have time 🙂

[nflaig]

Please test them out if/when you have time 🙂

yes we do pass them, looks like there are a lot more tests now (thanks for the zip)