From 9bbac0d2ccbde7e82afcee760b9fb0a47e16244b Mon Sep 17 00:00:00 2001 From: Aditya Asgaonkar Date: Tue, 21 Apr 2020 18:50:02 -0700 Subject: [PATCH 1/3] Added consistency check for FFG & LMD vote in validate_on_atttestation(), fixes #1636, fixes #1456, fixes #1408 --- specs/phase0/fork-choice.md | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index c42609be0..f844f735b 100644 --- a/specs/phase0/fork-choice.md +++ b/specs/phase0/fork-choice.md @@ -22,6 +22,7 @@ - [`get_latest_attesting_balance`](#get_latest_attesting_balance) - [`filter_block_tree`](#filter_block_tree) - [`get_filtered_block_tree`](#get_filtered_block_tree) + - [`is_descendant_block`](#is_descendant_block) - [`get_head`](#get_head) - [`should_update_justified_checkpoint`](#should_update_justified_checkpoint) - [`on_attestation` helpers](#on_attestation-helpers) @@ -162,7 +163,7 @@ def get_latest_attesting_balance(store: Store, root: Root) -> Gwei: active_indices = get_active_validator_indices(state, get_current_epoch(state)) return Gwei(sum( state.validators[i].effective_balance for i in active_indices - if (i in store.latest_messages + if (i in store.latest_messages and get_ancestor(store, store.latest_messages[i].root, store.blocks[root].slot) == root) )) ``` @@ -220,6 +221,28 @@ def get_filtered_block_tree(store: Store) -> Dict[Root, BeaconBlock]: return blocks ``` +#### `is_descendant_block` + +```python +def is_descendant_block(store: Store, base_root: Root, descendant_root: Root) -> bool: + """ + Checks if the block with root ``descendant_root`` is a descendant of the block with root ``base_root`` + """ + descendants = [base_root] + + # Traverse the descendants block tree and check if ``descendant_root`` is encountered + while(descendants): + if descendants[0] == descendant_root: + return True + descendants.extend([ + root for root in store.blocks.keys() + if store.blocks[root].parent_root == descendants[0] + ]) + descendants = descendants[1:] + + return False +``` + #### `get_head` ```python @@ -286,6 +309,9 @@ def validate_on_attestation(store: Store, attestation: Attestation) -> None: # Attestations must not be for blocks in the future. If not, the attestation should not be considered assert store.blocks[attestation.data.beacon_block_root].slot <= attestation.data.slot + # FFG and LMD vote must be consistent with each other + assert is_descendant_block(store, target.root, attestation.data.beacon_block_root) + # Attestations can only affect the fork choice of subsequent slots. # Delay consideration in the fork choice until their slot is in the past. assert get_current_slot(store) >= attestation.data.slot + 1 From 9acea519382c0fa8752328b106423890f8d1f01c Mon Sep 17 00:00:00 2001 From: Aditya Asgaonkar Date: Sat, 25 Apr 2020 14:17:28 -0700 Subject: [PATCH 2/3] Simplified by re-using get_ancestor() --- specs/phase0/fork-choice.md | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/specs/phase0/fork-choice.md b/specs/phase0/fork-choice.md index f844f735b..42d0cea11 100644 --- a/specs/phase0/fork-choice.md +++ b/specs/phase0/fork-choice.md @@ -22,7 +22,6 @@ - [`get_latest_attesting_balance`](#get_latest_attesting_balance) - [`filter_block_tree`](#filter_block_tree) - [`get_filtered_block_tree`](#get_filtered_block_tree) - - [`is_descendant_block`](#is_descendant_block) - [`get_head`](#get_head) - [`should_update_justified_checkpoint`](#should_update_justified_checkpoint) - [`on_attestation` helpers](#on_attestation-helpers) @@ -221,28 +220,6 @@ def get_filtered_block_tree(store: Store) -> Dict[Root, BeaconBlock]: return blocks ``` -#### `is_descendant_block` - -```python -def is_descendant_block(store: Store, base_root: Root, descendant_root: Root) -> bool: - """ - Checks if the block with root ``descendant_root`` is a descendant of the block with root ``base_root`` - """ - descendants = [base_root] - - # Traverse the descendants block tree and check if ``descendant_root`` is encountered - while(descendants): - if descendants[0] == descendant_root: - return True - descendants.extend([ - root for root in store.blocks.keys() - if store.blocks[root].parent_root == descendants[0] - ]) - descendants = descendants[1:] - - return False -``` - #### `get_head` ```python @@ -310,7 +287,8 @@ def validate_on_attestation(store: Store, attestation: Attestation) -> None: assert store.blocks[attestation.data.beacon_block_root].slot <= attestation.data.slot # FFG and LMD vote must be consistent with each other - assert is_descendant_block(store, target.root, attestation.data.beacon_block_root) + target_slot = compute_start_slot_at_epoch(target.epoch) + assert target.root == get_ancestor(store, attestation.data.beacon_block_root, target_slot) # Attestations can only affect the fork choice of subsequent slots. # Delay consideration in the fork choice until their slot is in the past. From b109e7da5a5c179e1081413c7bc3f8f8db95d95c Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Tue, 5 May 2020 14:46:02 -0600 Subject: [PATCH 3/3] add test for inconsistent head and target in attestation fork choice --- .../test/fork_choice/test_on_attestation.py | 40 ++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/tests/core/pyspec/eth2spec/test/fork_choice/test_on_attestation.py b/tests/core/pyspec/eth2spec/test/fork_choice/test_on_attestation.py index 360c18ccd..b2d33d0aa 100644 --- a/tests/core/pyspec/eth2spec/test/fork_choice/test_on_attestation.py +++ b/tests/core/pyspec/eth2spec/test/fork_choice/test_on_attestation.py @@ -1,7 +1,7 @@ from eth2spec.test.context import PHASE0, with_all_phases, spec_state_test from eth2spec.test.helpers.block import build_empty_block_for_next_slot from eth2spec.test.helpers.attestations import get_valid_attestation, sign_attestation -from eth2spec.test.helpers.state import transition_to, state_transition_and_sign_block, next_epoch +from eth2spec.test.helpers.state import transition_to, state_transition_and_sign_block, next_epoch, next_slot def run_on_attestation(spec, state, store, attestation, valid=True): @@ -116,6 +116,44 @@ def test_on_attestation_mismatched_target_and_slot(spec, state): run_on_attestation(spec, state, store, attestation, False) +@with_all_phases +@spec_state_test +def test_on_attestation_inconsistent_target_and_head(spec, state): + store = spec.get_forkchoice_store(state) + spec.on_tick(store, store.time + 2 * spec.SECONDS_PER_SLOT * spec.SLOTS_PER_EPOCH) + + # Create chain 1 as empty chain between genesis and start of 1st epoch + target_state_1 = state.copy() + next_epoch(spec, target_state_1) + + # Create chain 2 with different block in chain from chain 1 from chain 1 from chain 1 from chain 1 + target_state_2 = state.copy() + diff_block = build_empty_block_for_next_slot(spec, target_state_2) + signed_diff_block = state_transition_and_sign_block(spec, target_state_2, diff_block) + spec.on_block(store, signed_diff_block) + next_epoch(spec, target_state_2) + next_slot(spec, target_state_2) + + # Create and store block new head block on target state 1 + head_block = build_empty_block_for_next_slot(spec, target_state_1) + signed_head_block = state_transition_and_sign_block(spec, target_state_1, head_block) + spec.on_block(store, signed_head_block) + + # Attest to head of chain 1 + attestation = get_valid_attestation(spec, target_state_1, slot=head_block.slot, signed=False) + epoch = spec.compute_epoch_at_slot(attestation.data.slot) + + # Set attestation target to be from chain 2 + attestation.data.target = spec.Checkpoint(epoch=epoch, root=spec.get_block_root(target_state_2, epoch)) + sign_attestation(spec, state, attestation) + + assert attestation.data.target.epoch == spec.GENESIS_EPOCH + 1 + assert spec.compute_epoch_at_slot(attestation.data.slot) == spec.GENESIS_EPOCH + 1 + assert spec.get_block_root(target_state_1, epoch) != attestation.data.target.root + + run_on_attestation(spec, state, store, attestation, False) + + @with_all_phases @spec_state_test def test_on_attestation_target_not_in_store(spec, state):