Whisk review implementor notes#1
Whisk review implementor notes#1dapplion wants to merge 1 commit intonalinbhardwaj:whisk-curdleproofsfrom
Conversation
| | `DOMAIN_WHISK_SHUFFLE` | `DomainType('0x07100000')` | | ||
| | `DOMAIN_WHISK_PROPOSER_SELECTION` | `DomainType('0x07200000')` | | ||
| | `DOMAIN_WHISK_SHUFFLE` | `DomainType('0x08000000')` | | ||
| | `DOMAIN_WHISK_PROPOSER_SELECTION` | `DomainType('0x09000000')` | |
There was a problem hiding this comment.
In spec it's more common to use sequential domains
| def get_k_commitment(k: BLSFieldElement) -> BLSG1Point: | ||
| return bls.G1_to_bytes48(bls.multiply(bls.G1, k)) |
There was a problem hiding this comment.
de-dup code and simplify crypto interface
| whisk_tracker: WhiskTracker # Whisk tracker (r * G, k * r * G) [New in Whisk] | ||
| whisk_k_commitment: BLSG1Point # Whisk k commitment k * BLS_G1_GENERATOR [New in Whisk] | ||
| r_g: BLSG1Point # r * G | ||
| k_r_g: BLSG1Point # k * r * G |
There was a problem hiding this comment.
- See comment on main PR for keeping the user facing case strict snake [WIP] Introduce consensus code for Whisk (SSLE) with Curdleproofs ethereum/consensus-specs#3205 (comment)
| whisk_candidate_trackers: Vector[WhiskTracker, WHISK_CANDIDATE_TRACKERS_COUNT] # [New in Whisk] | ||
| whisk_proposer_trackers: Vector[WhiskTracker, WHISK_PROPOSER_TRACKERS_COUNT] # [New in Whisk] | ||
| whisk_trackers: List[WhiskTracker, VALIDATOR_REGISTRY_LIMIT] # Whisk tracker (r * G, k * r * G) [New in Whisk] | ||
| whisk_k_commitments: List[BLSG1Point, VALIDATOR_REGISTRY_LIMIT] # Whisk k commitment k * BLS_G1_GENERATOR [New in Whisk] |
There was a problem hiding this comment.
- Adding this new fields in the state instead of in the validator record simplifies implementation and reduces hashing cost. See [WIP] Introduce consensus code for Whisk (SSLE) with Curdleproofs ethereum/consensus-specs#3205 (comment)
| state.whisk_proposer_trackers[i] = state.whisk_candidate_trackers[index] | ||
|
|
||
|
|
||
| def select_whisk_candidate_trackers(state: BeaconState, epoch: Epoch) -> None: |
There was a problem hiding this comment.
Split select_* functions to re-use in fork transition
| # ... | ||
| body: BeaconBlockBody | ||
| proposer_index: ValidatorIndex | ||
| whisk_opening_proof: ByteList[WHISK_MAX_OPENING_PROOF_SIZE] # [New in Whisk] |
There was a problem hiding this comment.
Best to add new fields in body and not in BeaconBlock obj, such that the BeaconBlockHeader maps to BeaconBlock with body as root
| M: BLSG1Point, | ||
| shuffle_proof: ByteList[WHISK_MAX_SHUFFLE_PROOF_SIZE]) -> bool: | ||
| """ | ||
| Verify `post_shuffle_trackers` is a permutation of `pre_shuffle_trackers`. | ||
| Note: whisk_shuffle_proof_m_commitment is included in whisk_shuffle_proof struct |
There was a problem hiding this comment.
Merge M into shuffle_proof to drop one extra field from the block, and simplify crypto interface
| # ... | ||
| process_whisk(state, block.body) # [New in Whisk] | ||
| whisk_process_shuffled_trackers(state, block.body) # [New in Whisk] | ||
| whisk_process_registration(state, block.body) # [New in Whisk] |
There was a problem hiding this comment.
No strong reason here, just considered that it reads better than process_whisk calling whisk_process_shuffled_trackers internally. It communicates to implementers that there are two things which are not dependent on each other
| Return the beacon proposer index at the current slot. | ||
| """ | ||
| print("get_beacon_proposer_index", state.latest_block_header.slot, state.slot) | ||
| # assert state.latest_block_header.slot == state.slot # sanity check `process_block_header` has been called |
There was a problem hiding this comment.
Removed as I assume this is a debug artifact. I'm unsure if it's necessary to spec this function since it adds no new information. Is it necessary for the executable spec to work properly? @nalinbhardwaj
1 participant
Various non-critical comments surfaced from implementing. Will annotated comments below on each line