Shard fork choice rule #1773

hwwhww · 2020-04-29T16:01:44Z

messy drafting!
building test case with #1703 fixes & testing tool.

Questions:

1. In on_shard_block, there is a check of "4. Check block is a descendant of the finalized block at the checkpoint finalized slot". It might not handle the shard block between the latest finalized slot and the initial shard block slot (PHASE_1_GENESIS_SLOT). Should we keep it?
2. In get_head: currently using store.block_states[head_beacon_root].shard_states[shard].latest_block_root (the most recent accepted crosslink) as the base shard block. Some sanity confirm would be appreciated.
3. Follow up the discussion in the weekly call, get_filtered_shard_block_tree is much cleaner than the beacon version (get_filtered_block_tree), I think we still need it in case the beacon chain stale?

TODO

add more tests

djrtwo

Great start! Did a quick pass. Added some high level comments below, and answered your immediate questions

It might not handle the shard block between the latest finalized slot and the initial shard block slot (PHASE_1_GENESIS_SLOT). Should we keep it?

I'm not sure I follow here. The first shard block would happen at PHASE_1_GENESIS_SLOT and reference the beacon state at that slot (and thus be in the chain of whatever was finalized prior to that slot)

In get_head: currently using store.block_states[head_beacon_root].shard_states[shard].latest_block_root (the most recent accepted crosslink) as the base shard block. Some sanity confirm would be appreciated.

Sanity checked 👍

I think we still need it in case the beacon chain stale?

I don't think we need this. See my comments below

specs/phase1/shard-fork-choice.md

hwwhww · 2020-05-01T03:33:47Z

@djrtwo Thanks for the review!

I'm not sure I follow here. The first shard block would happen at PHASE_1_GENESIS_SLOT and reference the beacon state at that slot (and thus be in the chain of whatever was finalized prior to that slot)

Ahh this question would be solved by the answer of question 2 😅
Unrelated FYI: in upgrade_to_phase1, we set the initial shard_state.slot to PHASE_1_GENESIS_SLOT. So the first valid shard block would be at slot PHASE_1_GENESIS_SLOT + 1. Which means, after beacon state transition:

if state.slot == PHASE_1_GENESIS_SLOT, then state.shard_states[shard].slot == state.slot
else, state.shard_states[shard].slot < state.slot
We could remove the a slot delay at phase 1 boundary by setting default shard_state.slotto PHASE_1_GENESIS_SLOT - 1 though. Does that make sense to you?

get_filtered_shard_block_tree

ah now i see it, the while loop below it should be able to handle the shard block tree. Thanks!

specs/phase1/shard-fork-choice.md

djrtwo

great! so close

specs/phase1/fork-choice.md

specs/phase1/shard-fork-choice.md

djrtwo

looks good! (quick comment on multi-line operation style)

specs/phase1/shard-fork-choice.md

hwwhww

@dankrad @djrtwo

Changelog

Added a TODO flag for latest message issue. Thanks for the review!
Added get_pendings_shard_blocks per 74204f7#r434780344
Rework test_basic to process multiple beacon blocks and shard blocks
- The function check_pending_shard_blocks tests get_pendings_shard_blocks
Note that for mainnet config, it will run for ~2 epochs

specs/phase1/shard-fork-choice.md

hwwhww · 2020-06-04T13:49:40Z

specs/phase1/shard-transition.md

@@ -50,6 +50,9 @@ def verify_shard_block_message(beacon_state: BeaconState,
                               shard: Shard) -> bool:
    assert block.shard_parent_root == shard_state.latest_block_root
    assert block.slot == slot
+    next_slot = Slot(beacon_state.slot + 1)


~~## ISSUE~~
~~We have two places call verify_shard_block_message~~
~~1. From validator guide get_shard_transition: beacon_state the lookahead state where slot = head beacon state slot + 1~~
~~2. From shark choice rule on_shard_block: beacon_state is the head block.~~

~~However, #1704 will remove (1). So I leave it here.~~

Second thought: #1875

specs/phase1/shard-transition.md

Add check for `block.beacon_parent_root` per Terence's suggestion Update `get_shard_transition` 1. Disable verification: it will be fix in v-guide 2. Use `on_time_slot` to compute offset_slots Rework tests

should be transitioned.

specs/phase1/shard-fork-choice.md

Co-authored-by: terence tsao <[email protected]>

djrtwo

Minor comments. Looking good. Happy to merge soon with #1875

specs/phase1/shard-fork-choice.md

djrtwo · 2020-06-08T15:45:07Z

specs/phase1/shard-fork-choice.md

+    # Check the block is valid and compute the post-state
+    beacon_head_root = get_head(store)
+    beacon_head_state = store.block_states[beacon_head_root]
+    assert verify_shard_block_message(beacon_head_state, pre_shard_state, shard_block, shard_block.slot, shard)


Why do we pass in shard_block.slot as an independent variable? Is there ever a case that we want the slot passed in to differ from shard.slot?

Yes it seems leftover of outdated assumption. 👍
I realized we can also remove shard since we got block.shard, so it's now just verify_shard_block_message(beacon_parent_state, shard_parent_state, block) 🎉

Handle `beacon_parent_root` checks

hwwhww added phase1 scope:fork-choice labels Apr 29, 2020

hwwhww force-pushed the hwwhww/shard_fork_choice branch from 2a9818b to b1b739d Compare April 30, 2020 12:06

djrtwo reviewed Apr 30, 2020

View reviewed changes

specs/phase1/shard-fork-choice.md Outdated Show resolved Hide resolved

specs/phase1/shard-fork-choice.md Outdated Show resolved Hide resolved

specs/phase1/shard-fork-choice.md Outdated Show resolved Hide resolved

specs/phase1/shard-fork-choice.md Outdated Show resolved Hide resolved

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

hwwhww force-pushed the hwwhww/shard_fork_choice branch from 16b3fba to 49a900b Compare May 1, 2020 18:56

hwwhww added 5 commits May 5, 2020 21:41

wip shard fork choice rule

dab5a93

Refactor

cddf9cf

Make ShardStore an independent object

8fafb6a

Remove get_filtered_shard_block_tree

fca1bbc

Add (shard, shard_root) to LatestMessage

79b1b4b

hwwhww force-pushed the hwwhww/shard_fork_choice branch from 49a900b to 79b1b4b Compare May 5, 2020 13:43

hwwhww changed the base branch from hwwhww/signed_pattern to dev May 5, 2020 13:44

terencechain reviewed May 11, 2020

View reviewed changes

specs/phase1/shard-fork-choice.md Show resolved Hide resolved

hwwhww added 2 commits May 30, 2020 03:56

Fix test

870ad8b

Merge branch 'dev' into hwwhww/shard_fork_choice_3

63de59d

djrtwo reviewed May 31, 2020

View reviewed changes

PR review from Danny

142ba17

dankrad reviewed Jun 2, 2020

View reviewed changes

specs/phase1/shard-fork-choice.md Outdated Show resolved Hide resolved

djrtwo reviewed Jun 2, 2020

View reviewed changes

specs/phase1/shard-fork-choice.md Outdated Show resolved Hide resolved

djrtwo approved these changes Jun 2, 2020

View reviewed changes

terencechain reviewed Jun 2, 2020

View reviewed changes

specs/phase1/shard-fork-choice.md Outdated Show resolved Hide resolved

hwwhww added 5 commits June 3, 2020 22:31

Apply PR feedback from Danny and Terence

5c5cedd

Merge branch 'dev' into hwwhww/shard_fork_choice

58e75c2

head_shard_root -> shard_head_root

e1981a7

Bugfix: should set shard for empty proposal

d344521

Use epoch of the shard_block.slot for generating seed

26aae40

hwwhww changed the title ~~[WIP] shard fork choice rule~~ Shard fork choice rule Jun 4, 2020

hwwhww marked this pull request as ready for review June 4, 2020 12:35

hwwhww force-pushed the hwwhww/shard_fork_choice branch from 9100be6 to 0a107d5 Compare June 4, 2020 12:41

hwwhww commented Jun 4, 2020

View reviewed changes

specs/phase1/shard-fork-choice.md Outdated Show resolved Hide resolved

hwwhww commented Jun 4, 2020

View reviewed changes

hwwhww mentioned this pull request Jun 4, 2020

phase 1 validator guide (round 1) #1704

Merged

16 tasks

terencechain reviewed Jun 5, 2020

View reviewed changes

specs/phase1/shard-transition.md Outdated Show resolved Hide resolved

hwwhww mentioned this pull request Jun 5, 2020

Handle beacon_parent_root checks #1875

Merged

hwwhww added 6 commits June 6, 2020 02:39

WIP test case

c9a53b8

Verify shard_block.slot fits the expected offset_slots

727353c

Add get_pendings_shard_blocks

f8597d2

Update shard fork choice rule to be able to handle mainnet config

ab42eee

Add TODO flag of latest message

6f9c290

Fix typo

a154d0c

hwwhww force-pushed the hwwhww/shard_fork_choice branch from 0a107d5 to a154d0c Compare June 5, 2020 18:39

hwwhww added 4 commits June 6, 2020 02:39

Fix verify_shard_block_message

2d4788f

Add check for `block.beacon_parent_root` per Terence's suggestion Update `get_shard_transition` 1. Disable verification: it will be fix in v-guide 2. Use `on_time_slot` to compute offset_slots Rework tests

clean leftover

2afa315

Per #1704 discussion, remove on_time_slot: the given beacon_state

a71c0a5

should be transitioned.

Apply PR feedback from Danny

a4cc189

terencechain reviewed Jun 6, 2020

View reviewed changes

specs/phase1/shard-fork-choice.md Outdated Show resolved Hide resolved

hwwhww and others added 2 commits June 8, 2020 17:12

PR feedback from Terence: fix get_shard_latest_attesting_balance

4355057

Co-authored-by: terence tsao <[email protected]>

Rename build_shard_transitions_till_slot to get_shard_transitions

7e67aae

djrtwo reviewed Jun 8, 2020

View reviewed changes

hwwhww added 4 commits June 8, 2020 23:49

PR feedback from danny: simplify verify_shard_block_message params

e03a970

Merge pull request #1875 from ethereum/hwwhww/shard_fork_choice_part2

9b3f45d

Handle `beacon_parent_root` checks

Merge branch 'dev' into hwwhww/shard_fork_choice

3b749d7

PR feedback from danny

2d895e9

hwwhww merged commit f279e30 into dev Jun 8, 2020

hwwhww deleted the hwwhww/shard_fork_choice branch June 8, 2020 17:39

hwwhww mentioned this pull request Jun 9, 2020

Phase 1 known issues checklist #1768

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shard fork choice rule #1773

Shard fork choice rule #1773

hwwhww commented Apr 29, 2020 •

edited

Loading

djrtwo left a comment

hwwhww commented May 1, 2020

djrtwo left a comment

djrtwo left a comment

hwwhww left a comment

hwwhww Jun 4, 2020 •

edited

Loading

hwwhww Jun 5, 2020

djrtwo left a comment

djrtwo Jun 8, 2020

hwwhww Jun 8, 2020

Shard fork choice rule #1773

Shard fork choice rule #1773

Conversation

hwwhww commented Apr 29, 2020 • edited Loading

djrtwo left a comment

Choose a reason for hiding this comment

hwwhww commented May 1, 2020

djrtwo left a comment

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

hwwhww left a comment

Choose a reason for hiding this comment

hwwhww Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

hwwhww Jun 5, 2020

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

djrtwo Jun 8, 2020

Choose a reason for hiding this comment

hwwhww Jun 8, 2020

Choose a reason for hiding this comment

hwwhww commented Apr 29, 2020 •

edited

Loading

hwwhww Jun 4, 2020 •

edited

Loading