[WIP] Execution-ified fork choice, draft 1 #1068

vbuterin · 2019-05-09T12:37:34Z

Not tested at all. Also missing one optimization that's probably crucial to have even enough efficiency for testing, which is a children cache.

Also missing weak-subjectivity-aware safety measurement and time-aware justified head switching.

JustinDrake · 2019-05-13T08:57:49Z

Looking at this today. (Assigned PR to myself to avoid conflicts with other reviewers.)

Edit: This PR addresses points 1, 3, 4, 5, 6, 9, 10 of issue #1053.

…alik94

djrtwo · 2019-05-15T22:08:25Z

reviewing now! please don't merge even though "approved"

JustinDrake · 2019-05-16T11:05:29Z

We probably want a couple tests before merging.

1. signed_root -> signing_root 2. `get_attesting_balance` is used in state transition spec. Rename `get_attesting_balance` to `get_attesting_balance_from_store`

hwwhww · 2019-05-16T17:34:54Z

Changelog:

Build both 0_beacon-chain.md and 0_fork-choice.md.
- change the order of build_spec argvs from <source phase0> <output phase0 pyspec> to <output phase0 pyspec> <source 0_beacon-chain> <source 0_fork-choice>
Fork choice rule spec
1. signed_root -> signing_root
2. The name get_attesting_balance is used in state transition spec. Rename fork choice rule get_attesting_balance to get_attesting_balance_from_store.

It's blocking by the new SSZ Map type(???).
Is making Store SSZ serializable a requirement? Otherwise, we can transform Store into a simple Python class (not SSZ) in function_puller.py.

vbuterin · 2019-05-16T17:50:38Z

I don't think we need Store to be an SSZ object. It's not a consensus object.

hwwhww · 2019-05-16T17:51:38Z

specs/core/0_fork-choice.md

+    if state.current_justified_epoch > slot_to_epoch(store.blocks[store.justified_root].slot):
+        store.justified_root = state.current_justified_root
+    if state.previous_justified_epoch > slot_to_epoch(store.blocks[store.justified_root].slot):
+        store.justified_root = state.previous_justified_root


I think it's impossible of that both state.current_justified_epoch > slot_to_epoch(store.blocks[store.justified_root].slot) and state.previous_justified_epoch > slot_to_epoch(store.blocks[store.justified_root].slot) happen?

IMHO it's better to avoid using the possible updated store.justified_root in the second condition check (if state.previous_justified_epoch > ...), it looks confusing.

Suggestion:

if state.current_justified_epoch > slot_to_epoch(store.blocks[store.justified_root].slot): store.justified_root = state.current_justified_root elif state.previous_justified_epoch > slot_to_epoch(store.blocks[store.justified_root].slot): store.justified_root = state.previous_justified_root

hwwhww · 2019-05-16T19:17:59Z

I don't think we need Store to be an SSZ object. It's not a consensus object.

👍

An option: if we upgrade the CI to Python 3.7 env (it's currently Python 3.6), we can use @dataclass to define Store concisely.

@dataclass
class Store:
    blocks: Dict[Bytes32, BeaconBlock] = {}
    states: Dict[Bytes32, BeaconState] = {}
    time: int = 0
    latest_targets: Dict[int, Target] = {}
    justified_root: Bytes32 = ZERO_HASH
    finalized_root: Bytes32 = ZERO_HASH

hwwhww · 2019-05-19T15:52:27Z

specs/core/0_fork-choice.md

+```python
+def on_block(store: Store, block: BeaconBlock) -> None:
+    # Check block is a descendant of the finalized block
+    assert get_ancestor(store, signing_root(block), store.blocks[store.finalized_root].slot) == store.finalized_root


The given block hasn't been stored yet here, so inside get_ancestor, it can't access the given block from store.

As a beacon node, I support it needs to store the block before these verifications?

hwwhww · 2019-05-20T10:28:20Z

specs/core/0_fork-choice.md

+    pre_state = store.states[block.parent_root]
+    assert store.time >= pre_state.genesis_time + block.slot * SECONDS_PER_SLOT
+    # Check the block is valid and compute the post-state
+    state = state_transition(pre_state, block)


Ahhhh a mutability issue here! 💣

state_transition returns a state, but note that there is side effect - the given state input would be changed anyway. (Avoid side effect in state transition with mutable object #1059)

Since store.states is a dict, the updated pre_state would mess up store.states.

A quick fix for this PR might still be adding the scary deepcopy inside state_transition that I know people really don't want to have in the spec. :'(

But open to continue discussing in #1059.

1. Make `Target` as a dataclass 2. Fix `on_attestation` typos 3. Add basic tests TBD: 1. deepcopy? 2. Add new block to the store before validation

hwwhww · 2019-05-21T04:05:04Z

Changelog:

Add SSZType.copy() helper to deal with copy.deepcopy
Add some basic tests and make the tests pass:
1. Make Target as a dataclass
2. Fix on_attestation typos
3. Add basic tests
4. Move forward to deal with [WIP] Execution-ified fork choice, draft 1 #1068 (comment) and [WIP] Execution-ified fork choice, draft 1 #1068 (comment)

TBD: to confirm if pre_state = store.states[block.parent_root].copy() and #1068 (comment) are okay.

protolambda · 2019-06-08T12:17:46Z

Is anyone still working on this? Shall I take a look? (Other fork-choice implementation work here: https://github.com/protolambda/lmd-ghost/).

JustinDrake · 2019-06-08T16:10:01Z

Shall I take a look?

Please do :) No one seems to have taken "merge responsibility" for this one.

CarlBeek · 2019-06-16T20:56:04Z

This PR has been deprecated by #1185. This was forked from such an old version of dev, it was easier to start afresh. In addition, all of @hwwhww's open comments have been handled in #1185.

Execution-ified fork choice, draft 1

0642af1

JustinDrake self-assigned this May 13, 2019

JustinDrake added 3 commits May 13, 2019 13:43

Update 0_fork-choice.md

68d555f

Update 0_fork-choice.md

333a08e

Add more notes

847c554

JustinDrake approved these changes May 13, 2019

View reviewed changes

JustinDrake removed their assignment May 13, 2019

JustinDrake added 2 commits May 14, 2019 07:45

Bug fix

7c8565f

Merge branch 'vitalik94' of github.com:ethereum/eth2.0-specs into vit…

b136ced

…alik94

JustinDrake mentioned this pull request May 15, 2019

fix up validator guide crosslink committee instructions #1085

Merged

Merge branch 'dev' into vitalik94

c24218f

hwwhww changed the title ~~Execution-ified fork choice, draft 1~~ [WIP] Execution-ified fork choice, draft 1 May 16, 2019

hwwhww added 3 commits May 17, 2019 00:45

Build 0_fork_choice_rule

b78bbd1

Fix

275c90c

1. signed_root -> signing_root 2. `get_attesting_balance` is used in state transition spec. Rename `get_attesting_balance` to `get_attesting_balance_from_store`

lint

27e3651

hwwhww force-pushed the vitalik94 branch from 0aef32e to 27e3651 Compare May 16, 2019 17:26

hwwhww reviewed May 16, 2019

View reviewed changes

JustinDrake and others added 2 commits May 17, 2019 07:26

Update 0_fork-choice.md

cb3522b

Using dataclass in Python 3.6

27eaa66

hwwhww reviewed May 19, 2019

View reviewed changes

hwwhww mentioned this pull request May 20, 2019

Set genesis_state.latest_block_header with the root of empty BeaconBlockBody #1098

Merged

Merge branch 'dev' into vitalik94

9e9fbf2

hwwhww reviewed May 20, 2019

View reviewed changes

hwwhww added the scope:fork-choice label May 20, 2019

hwwhww added 2 commits May 21, 2019 11:55

Add SSZType.copy() helper

349f80f

Add tests and make the tests pass

1299992

1. Make `Target` as a dataclass 2. Fix `on_attestation` typos 3. Add basic tests TBD: 1. deepcopy? 2. Add new block to the store before validation

JustinDrake mentioned this pull request Jun 9, 2019

Custom genesis trigger, and deposit contract cleanups #1152

Merged

JustinDrake added the milestone:June 30 freeze 🥶 Phase 0 spec freeze for long-lived cross-client testnet label Jun 15, 2019

CarlBeek self-assigned this Jun 16, 2019

CarlBeek mentioned this pull request Jun 16, 2019

Executable fork choice #1185

Merged

CarlBeek closed this Jun 16, 2019

hwwhww mentioned this pull request Jul 11, 2019

Blocks added to the store before some verifications #1288

Closed

djrtwo deleted the vitalik94 branch May 20, 2020 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Execution-ified fork choice, draft 1 #1068

[WIP] Execution-ified fork choice, draft 1 #1068

vbuterin commented May 9, 2019

JustinDrake commented May 13, 2019 •

edited

Loading

djrtwo commented May 15, 2019

JustinDrake commented May 16, 2019

hwwhww commented May 16, 2019 •

edited

Loading

vbuterin commented May 16, 2019

hwwhww May 16, 2019

hwwhww commented May 16, 2019

hwwhww May 19, 2019

hwwhww May 20, 2019 •

edited

Loading

hwwhww commented May 21, 2019 •

edited

Loading

protolambda commented Jun 8, 2019

JustinDrake commented Jun 8, 2019

CarlBeek commented Jun 16, 2019

[WIP] Execution-ified fork choice, draft 1 #1068

[WIP] Execution-ified fork choice, draft 1 #1068

Conversation

vbuterin commented May 9, 2019

JustinDrake commented May 13, 2019 • edited Loading

djrtwo commented May 15, 2019

JustinDrake commented May 16, 2019

hwwhww commented May 16, 2019 • edited Loading

vbuterin commented May 16, 2019

hwwhww May 16, 2019

Choose a reason for hiding this comment

hwwhww commented May 16, 2019

hwwhww May 19, 2019

Choose a reason for hiding this comment

hwwhww May 20, 2019 • edited Loading

Choose a reason for hiding this comment

hwwhww commented May 21, 2019 • edited Loading

protolambda commented Jun 8, 2019

JustinDrake commented Jun 8, 2019

CarlBeek commented Jun 16, 2019

JustinDrake commented May 13, 2019 •

edited

Loading

hwwhww commented May 16, 2019 •

edited

Loading

hwwhww May 20, 2019 •

edited

Loading

hwwhww commented May 21, 2019 •

edited

Loading