Rework shard block and fraud proof (shard state transition) spec by hwwhww · Pull Request #1703 · ethereum/consensus-specs

hwwhww · 2020-04-02T17:43:58Z

Issue

Make the fraud-proofs.md spec in Python functions.
Add a basic test for shard transition.

How did I fix it

beacon-chain.md
1. Rework shard block
  1. Replace block wrapper with signable pattern
  2. Add proposer_index to shard block since the proposal index helps clients to validate the block in network level.
2. Refactor get_light_client_committee a bit.
3. Fix the wrong get_shard_proposer_index function calls
4. Update apply_shard_transition
5. Fix typo attestation.slot == state.slot -> attestation.data.slot == state.slot in is_winning_attestation
6. Check verify_shard_transition_false_positives after process_operations
7. Fix shard_attestations filter in process_crosslinks: since attestations come from block, should use attestation.data.slot + MIN_ATTESTATION_INCLUSION_DELAY == state.slot
8. [TBD] Allow empty light_client_signature to make the tests pass
phase1-fork.md:
- Add stub PHASE_1_GENESIS_SLOT, set it to SLOTS_PER_EPOCH now.
Rewrite the current fraud-proofs (shard state transition) spec in Python code
Add basic sanity test for shard transition and beacon block processing

djrtwo

Wow! I have a lot of comments and there are things to work through, but great job!

This stuff is... complicated. A lot of subtleties and moving parts.
I tried to hit the high points on this wave of review. Let's iterate and I'll do another deeper review after.

(I haven't reviewed the changes/additions to testing yet)

djrtwo · 2020-04-15T21:55:28Z

+    else:
+        proposal = get_winning_proposal(beacon_state, choices)
+
+    shard_parent_root = hash_tree_root(proposal.message)


This will signal that the parent root was an empty proposal. If the shard slot is skipped, then the parent should be the previous non-skipped block

i.e., the actual shard chain should only be composed of actual proposals and not these empty skip blocks as flags for the skip slot

Right, shard_parent_root could be the indicator for some cases, e.g., offset_slots = [x+1, x+2, x+3] where slot x+1 is non-skipped, x+2 and x+3 slots are skipped.
We can see x+3 is a skipped slot since it shares the same shard_parent_root as x+2, but we can't tell if x+2 is skipped from its shard_parent_root though.

hwwhww · 2020-04-16T16:37:49Z

Thanks @djrtwo!
All comments are applied or replied.

Changelog

Apply PR feedback
Rename & fix typos
phase1-fork.md:
- Add stub PHASE_1_GENESIS_SLOT, set it to SLOTS_PER_EPOCH now.
fraud-proofs.md and beacon-chain.md
1. bb49c08 and 010201c:
  1. Agreed that the skipped slot should affect gasprice.
  2. Should have used beacon_parent_root=get_block_root_at_slot(state, offset_slots[i]), thanks!
    1. We could use the latest non-skipped slot block as the shard parent block, but it seems to not affect shard state at all? I simplified it to just BeaconBlock(slot=offset_slot[i]) in 010201c now since only slot matters in shard transition now.
2. 1aab788: Add verify_shard_block_message. Currently it won’t throw exception from get_shard_transition since get_shard_transition is a helper to select the valid proposals. Note that the honest validator guide may use verify_shard_block_message, so verify_shard_block_message and is_valid_fraud_proof would only be called upon receiving the shard blocks.

djrtwo

awesome!
Another wave of comments. getting close :)

djrtwo · 2020-04-23T18:43:23Z

+```
+
+```python
+def compute_shard_transition_digest(beacon_state: BeaconState,


This isn't currently a function of the beacon state but could see it using it in Phase 2.
It'd be nice if we get this interface right for Phase. Just a note to think about it some more

djrtwo · 2020-04-23T18:45:36Z

+                               slot: Slot,
+                               shard: Shard) -> bool:
+    assert block.shard_parent_root == shard_state.latest_block_root
+    assert block.beacon_parent_root == get_block_root_at_slot(beacon_state, slot)


This assumes that we won't use this function during the slot that the shard block was created (otherwise get_block_root_at_slot(beacon_state, slot) would fail).

Do you think this is a safe assumption?

It seems likely due to "TODO these validations should have been checked upon receiving shard blocks" that we might want to have validators use this before attesting during the slot the shard block was created

We can wait until we get back to the validator guide to modify if you want

Yes, I suppose the shard fork choice rule I'm working on will guarantee the given shard blocks are validated. The validator guide can assume that the validator already has the head shard block. I'll revisit the shard transition section again in the fork choice rule PR or after it.

terencechain

I implemented most of these in Prysm phase1 client. Just a few minor comments :)

terencechain · 2020-04-27T16:37:37Z

+```
+
+```python
+def verify_shard_block_message(beacon_state: BeaconState,


might be best to move verify_shard_block_message and verify_shard_block_signature under helpers since they are only used by helper methods

Yep! To make less burden for the reviewers, I'll rearrange the code sections at the end of this PR review.

terencechain · 2020-04-27T16:42:35Z

+### Shard state transition function
+
+```python
+def shard_state_transition(beacon_state: BeaconState,


The naming of shard_state_transition is pretty similar to get_shard_transition, but I can't think of a better name now...

Yeah "transition" is everywhere 😅
Open to suggestions!

hwwhww · 2020-04-29T16:14:36Z

Substantive changelog

Fix typo attestation.slot == state.slot -> attestation.data.slot == state.slot in is_winning_attestation
Check verify_shard_transition_false_positives after process_operations
Fix shard_attestations filter in process_crosslinks: since attestations come from block, should use attestation.data.slot + MIN_ATTESTATION_INCLUSION_DELAY == state.slot
[TBD] Allow empty light_client_signature to make the tests pass
Tests updates
- Rework and refactor test_process_crosslink
- Add basic phase 1 test_blocks ✨
  - test_process_beacon_block_with_normal_shard_transition: with basic on time shard attestation
  - test_process_beacon_block_with_empty_proposal_transition: The protocol acutally allows on time attestation that votes for "empty shard block proposal", i.e., creating shard transition with shard_blocks=[] for the skipped shard block.
- After (3), some other tests failed. (e.g., test_finality), this PR updates the test helpers for backward compatibility, still kinda messy though.

djrtwo · 2020-04-30T16:01:42Z

Fix shard_attestations filter in process_crosslinks: since attestations come from block, should use attestation.data.slot + MIN_ATTESTATION_INCLUSION_DELAY == state.slot

Note, MIN_ATTESTATION_INCLUSION_DELAY is essentially deprecated wth the new phase 1 design and must be set to 1

djrtwo · 2020-04-30T16:21:47Z

Looks good in general! I'm happy to merge soon and move on with subsequent testing PRs

`get_shard_committee`

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

Fix the wrong `get_shard_proposer_index` parameters order Phase 1 WIP Add shard transition basic test Fix lint error Fix

Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>

PR feedback from Danny and some refactor 1. Add stub `PHASE_1_GENESIS_SLOT` 2. Rename `get_updated_gasprice` to `compute_updated_gasprice` 3. Rename `compute_shard_data_roots` to `compute_shard_body_roots` Apply shard transition for the skipped slots Refactor `shard_state_transition` Get `beacon_parent_root` from offset slot Add more test Add `verify_shard_block_message` Add `> 0` Keep `beacon_parent_block` unchanged in `is_valid_fraud_proof` Remove some lines Fix type Refactor + simplify skipped slot processing

Use `ShardBlock` in `shard_state_transition` PR feedback 1. Rename `ShardState.data` -> `ShardState.transition_digest` 2. Rename `compute_shard_transition_data` to `compute_shard_transition_digest` 3. Add `assert state.slot > PHASE_1_GENESIS_SLOT` just in case, may move it later Add `get_post_shard_state` as a pure function wrapper

Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>

Fix wrong field names Fix `build_attestation_data` and other PR feedback from Danny and Terence 1. Rename `get_previous_slot` to `compute_previous_slot` 2. Break down `build_empty_block` into `get_state_and_beacon_parent_root_at_slot`, use it in `build_shard_block` 3. Set defult `slot` to `shard_state.slot + 1` in `build_shard_block` Update `verify_shard_block_message`: check beacon_parent_root at fork choice rule stage instead of state transition Fix `beacon-chain.md` 1. Fix typo `attestation.slot == state.slot` -> `attestation.data.slot == state.slot` in `is_winning_attestation` 2. Check `verify_shard_transition_false_positives` **after** `process_operations` 3. Fix `shard_attestations` filter in `process_crosslinks`: since attestations come from block, should use `attestation.data.slot + MIN_ATTESTATION_INCLUSION_DELAY == state.slot` 4. [TBD] Allow empty `light_client_signature` to make the tests pass 5. [TBD] Add `is_shard_attestation`, filter out empty `ShardTransition()` Rework `test_process_crosslink` Add basic phase 1 `test_blocks` Add more test cases Revert `is_shard_attestation` and fix test cases backward compatibility. Remove `test_process_beacon_block_no_shard_transition` and consider it as invalid case.

hwwhww · 2020-05-01T18:44:14Z

@djrtwo

Note, MIN_ATTESTATION_INCLUSION_DELAY is essentially deprecated wth the new phase 1 design and must be set to 1

👍 Added a comment to note that.

I'm happy to merge soon and move on with subsequent testing PRs

I refactored the tests and I'm happier with it now 😄
also squashed the reviewed commits by review cycles. I think it's okay to merge it now and then add more tests with other PRs.

djrtwo

great job!
lets mergee

hwwhww added the phase 1 label Apr 2, 2020

djrtwo changed the base branch from phase1-tests to dev April 3, 2020 16:47

terencechain reviewed Apr 4, 2020

View reviewed changes

Comment thread specs/phase1/beacon-chain.md Outdated

hwwhww force-pushed the hwwhww/signed_pattern branch 3 times, most recently from 4c0afb4 to 3fc86f3 Compare April 9, 2020 17:49

hwwhww marked this pull request as ready for review April 10, 2020 17:15

hwwhww mentioned this pull request Apr 14, 2020

Misc update for light-client-sync spec + make it executable #1725

Closed

3 tasks

djrtwo reviewed Apr 15, 2020

View reviewed changes

hwwhww force-pushed the hwwhww/signed_pattern branch 2 times, most recently from 536c073 to 010201c Compare April 16, 2020 16:35

djrtwo reviewed Apr 23, 2020

View reviewed changes

terencechain reviewed Apr 27, 2020

View reviewed changes

hwwhww force-pushed the hwwhww/signed_pattern branch from 1b11dff to 055d5e0 Compare April 28, 2020 04:33

hwwhww mentioned this pull request Apr 29, 2020

Shard fork choice rule #1773

Merged

4 tasks

terencechain reviewed Apr 29, 2020

View reviewed changes

Comment thread specs/phase1/beacon-chain.md Outdated

djrtwo reviewed Apr 30, 2020

View reviewed changes

Comment thread tests/core/pyspec/eth2spec/test/phase_1/sanity/test_blocks.py

Comment thread tests/core/pyspec/eth2spec/test/phase_1/sanity/test_blocks.py

terencechain reviewed May 1, 2020

View reviewed changes

Comment thread specs/phase1/beacon-chain.md Outdated

hwwhww mentioned this pull request May 1, 2020

256-bit custody atoms for better alignment with rest of the spec and greater efficiency #1705

Merged

hwwhww and others added 7 commits May 2, 2020 02:31

beacon-chain.md: Replace block wrapper with signable pattern

feb27a1

Add proposer_index to shard block

e9f1e41

Make shard_state_transition more like beacon state_transition function

5f69afe

Refactor get_light_client_committee to similar to

be50020

`get_shard_committee`

Add verify_fraud_proof function

247a6c8

Apply @terencechain 's review feedback

849d3f8

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

[squashed] shard transition wip

4e8a7ff

Fix the wrong `get_shard_proposer_index` parameters order Phase 1 WIP Add shard transition basic test Fix lint error Fix

hwwhww and others added 13 commits May 2, 2020 02:31

Refactor get_shard_state_transition_result

afa12ca

Rename build_empty_shard_block to build_shard_block

e645d6b

Apply suggestions from code review from @djrtwo

9724cb8

Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>

Apply suggestions from code review

c8a473b

Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>

Check head_shard_root of all transition_attestations

e758fb7

PR feedback from terence

ff85025

Refactor the tests

977cd73

specs/phase1/fraud-proofs.md -> specs/phase1/shard-transition.md

b43e24a

Reorg the file structure

4558c7d

Reorg beacon-chain spec a bit

7a77018

hwwhww force-pushed the hwwhww/signed_pattern branch from 2c4d942 to 7a77018 Compare May 1, 2020 18:35

djrtwo approved these changes May 4, 2020

View reviewed changes

djrtwo merged commit 71dc744 into dev May 4, 2020

djrtwo deleted the hwwhww/signed_pattern branch May 20, 2020 23:27

Conversation

hwwhww commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

How did I fix it

Uh oh!

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hwwhww commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

terencechain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hwwhww commented Apr 29, 2020

Substantive changelog

Uh oh!

Uh oh!

djrtwo commented Apr 30, 2020

Uh oh!

Uh oh!

Uh oh!

djrtwo commented Apr 30, 2020

Uh oh!

Uh oh!

hwwhww commented May 1, 2020

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

hwwhww commented Apr 2, 2020 •

edited

Loading

hwwhww commented Apr 16, 2020 •

edited

Loading