Prysm phase 1 feedback

[terencechain]

Quick update regarding phase 1 work with Prysm. I’ve implemented most of what’s in phase 1 spec today except for custody, light client and fraud proof logic. With the proper validator shard duties in place, without networking, we can begin to simulate shard transition and crosslink in a local enviroment. This work is currently getting done here and it will get migrated to the official Prysm repo post beacon chain mainnet. Here’s some preliminary feedback from our end after implemented shard transition logic:

• Add Shard to AttestationData, always having to call get_shard is not elegant. Being able to quickly validate Shard correctness for any incoming attestation is valuable
• Add Shard to ShardBlock, same reason as above. Able to quickly validate Shard correctness for a shard block is valuable
• Refactor shard proposer reward portion to its own method inside process_crosslink_for_shard. Would love to see process_crosslink_for_shard get split to be more granular.
• Rename AttestationData’s head_shard_root to shard_head_root, it resonates better with shard_transition_root. Both prefix shard.
• Rename is_shard_attestation to is_ontime_shard_attestation since attestation needs to be on time as well
• Rename verify_shard_transition_false_positives to verify_empty_shard_transition or something better 😅
• Put unpack_compact_validator inside light client spec since it’s not used within the scope in phase1 beacon chain
• Question: is there significant amount of save on hashing to put online_countdown as it’s on field in BeaconState rather than having each Validator track own online countdown

[hwwhww]

• Add Shard to AttestationData, always having to call get_shard is not elegant. Being able to quickly validate Shard correctness for any incoming attestation is valuable

Agreed with it’s not elegant, but a bit hesitant since we have a lot of Attestation traffic 😬

• Add Shard to ShardBlock, same reason as above. Able to quickly validate Shard correctness for a shard block is valuable
• Refactor shard proposer reward portion to its own method inside process_crosslink_for_shard. Would love to see process_crosslink_for_shard get split to be more granular.
• Rename AttestationData’s head_shard_root to shard_head_root, it resonates better with shard_transition_root. Both prefix shard.
• Rename is_shard_attestation to is_ontime_shard_attestation since attestation needs to be on time as well
• Rename verify_shard_transition_false_positives to verify_empty_shard_transition or something better 😅

All sound good to me too 👍

• Put unpack_compact_validator inside light client spec since it’s not used within the scope in phase1 beacon chain

(I don’t feel strongly about it)
Vitalik suggested that unpack_compact_validator should be next to pack_compact_validator.

• Question: is there significant amount of save on hashing to put online_countdown as it’s on field in BeaconState rather than having each Validator track own online countdown

Correct!

[hwwhww]

Update: the remaining items:

• Add Shard to AttestationData, always having to call get_shard is not elegant. Being able to quickly validate Shard correctness for any incoming attestation is valuable
• Refactor shard proposer reward portion to its own method inside process_crosslink_for_shard. Would love to see process_crosslink_for_shard get split to be more granular.
• Put unpack_compact_validator inside light client spec since it’s not used within the scope in phase1 beacon chain

[terencechain]

Refactor shard proposer reward portion to its own method inside process_crosslink_for_shard. Would love to see process_crosslink_for_shard get split to be more granular.

This is what I have implemented for Prysm: https://github.com/terencechain/prysm-phase1/blob/phase1/beacon-chain/core/helpers/shard.go#L278-L334

They are helpers to group attestations by 1.) on-time and committee_id 2.) shard_transition_root.
Another helper is to calculate whether a given list of attestations with corresponded committee can crosslink. I used these helpers for beacon-chain's process_crosslink_for_shard and validator's get_shard_winning_roots

These helpers are similar to get_matching_attestations for epoch processing. With that said, I understand that specs are optimized for readability. We can skip them if we think the helpers bring lesser value in that regard. I don't feel strongly about enforcing them into the spec : )