Constants arguments clean-up and shuffling lookahead #271

hwwhww · 2019-02-12T12:07:09Z

What was wrong?

Refactor: clean up constants arguments by adding CommitteeConfig
Fix Update helpers for shuffling lookahead and boundary fixes #258

How was it fixed?

1. Refactor: clean up constants arguments by adding `CommitteeConfig`

Sorry for that this PR became more about refactoring than just add a new feature.
When I tried to implement #258, I have to revamp get_crosslink_committees_at_slot helper; and with this new feature, I have to pass four more constants into get_crosslink_committees_at_slot and all the function that calls it.
That's a stronger signal of that some 💩 code here that we need to fix...
Per #146 (comment), I mentioned passing the shuffling-required constants into ShufflingConfig, but after some recent changes, it looks better to have a subset of BeaconConfig - CommitteeConfig to mitigate it.

class CommitteeConfig:
    def __init__(self, config: BeaconConfig):
        # Basic
        self.genesis_epoch = config.GENESIS_EPOCH
        self.shard_count = config.SHARD_COUNT
        self.epoch_length = config.EPOCH_LENGTH
        self.target_committee_size = config.TARGET_COMMITTEE_SIZE

        # For seed
        self.seed_lookahead = config.SEED_LOOKAHEAD
        self.entry_exit_delay = config.ENTRY_EXIT_DELAY
        self.latest_index_roots_length = config.LATEST_INDEX_ROOTS_LENGTH
        self.latest_randao_mixes_length = config.LATEST_INDEX_ROOTS_LENGTH

It's initialized with BeaconConfig in StateMachine layer.
Also, BeaconConfig has been moved from eth2/beacon/state_machines/configs.py → eth2/beacon/configs.py.
Pros: less ugly arguments passing.
Cons: lose some awareness of what are the configuration parameters we are testing.

While this PR is WIP, @pipermerriam @jannikluhn @ralexstokes @djrtwo , any thoughts on this refactoring?

2. Shuffling lookahead

Update get_crosslink_committees_at_slot for shuffling lookahead function get_next_epoch_committee_assignment for the validator to check if they are assigned in the incoming epoch and the context they need.
Use case: https://github.com/ethereum/eth2.0-specs/blob/dev/specs/validator/0_beacon-chain-validator.md#responsibility-lookahead
get_next_epoch_committee_assignment returns NoCommitteeAssignment when no assignment for the given validator_index.

Cute Animal Picture

eth2/beacon/configs.py

eth2/beacon/state_machines/forks/serenity/epoch_processing.py

eth2/beacon/committee_helpers.py

djrtwo

CommitteeConfig seems like a reasonable compromise. That said I'm not fully satisfied :/

djrtwo · 2019-02-13T16:34:45Z

eth2/beacon/committee_helpers.py


    epoch = slot_to_epoch(slot, epoch_length)
    current_epoch = state.current_epoch(epoch_length)
+    previous_epoch = state.previous_epoch(epoch_length, genesis_epoch)


BeaconState knows the genesis_time and thus the genesis_slot/epoch. I would argue that because this is an method on the state, that we can forgo the genesis_epoch param here for cleanliness.

Isn't genesis_time the unix timestamp when chain starts, while genesis_slot and (genesis_epoch) is the offset pivot to slot? Or do you mean, we can find a way to set genesis_slot as a configuration inside BeaconState, not as SSZ field?
On the other hand, it seems it can be solved by ethereum/consensus-specs#504.

ahh. I see. Leave as is for now. This might change when we finish the great integer debate ethereum/consensus-specs#626

djrtwo · 2019-02-13T16:36:05Z

eth2/beacon/committee_helpers.py


    epoch = slot_to_epoch(slot, epoch_length)
    current_epoch = state.current_epoch(epoch_length)
+    previous_epoch = state.previous_epoch(epoch_length, genesis_epoch)
+    next_epoch = EpochNumber(current_epoch + 1)


suggest adding BeaconState.next_epoch for symmetry

djrtwo · 2019-02-13T19:48:58Z

eth2/beacon/tools/builder/validator.py

@@ -258,3 +265,46 @@ def create_mock_signed_attestations_at_slot(
            keymap,
            config.EPOCH_LENGTH,
        )
+
+
+def get_next_epoch_committee_assignment(


I simplified the logic of get_next_epoch_committee_assignment in the latest spec release by passing in registry_change rather than returning both values. Looks like this function is still using the outer [False, True] loop but just returning one assignment.

https://github.com/ethereum/eth2.0-specs/blob/master/specs/validator/0_beacon-chain-validator.md#responsibility-lookahead

djrtwo · 2019-02-13T19:58:01Z

eth2/beacon/validator_status_helpers.py

@@ -111,9 +111,11 @@ def _settle_penality_to_validator_and_whistleblower(
    state.validator_balances[index] -= whistleblower_reward
    validator.penalized_epoch = slot_to_epoch(state.slot)
    """
+    EPOCH_LENGTH = committee_config.EPOCH_LENGTH


capital letters here is not consistent with config assignments in other functions (example: get_crosslink_committees_at_slot)

Yeah, I prefer to use the lower letter for consistency too.

I remember that @ChihChengLiang introduced capital letters in some other functions, and now there are two different style in the codebase. Can we unify it?

Will fix them to lower cases in my existing PRs

djrtwo · 2019-02-13T19:58:36Z

eth2/beacon/validator_status_helpers.py

    """
    Penalize the validator with the given ``index``.

    Exit the validator, penalize the validator, and reward the whistleblower.
    """
-    state = exit_validator(state, index, epoch_length, entry_exit_delay)
+    EPOCH_LENGTH = committee_config.EPOCH_LENGTH


Same as above. Either way is fine imo, but we should be consistent

pipermerriam

Mostly just browsed code. Will leave it to others to verify correctness.

pipermerriam · 2019-02-13T21:41:08Z

eth2/beacon/on_startup.py

-        state.latest_index_roots,
-        genesis_epoch % latest_index_roots_length,
-        index_root,
+    latest_index_roots = tuple(


equivalent to either of the following if you like them more

# using itertools.repeat tuple(itertools.repeat(genesis_active_index_root, latest_index_roots_length)) # using tuple multiplication (genesis_active_index_root,) * latest_index_roots_length

Co-Authored-By: hwwhww <hwwang156@gmail.com>

…och_committee_assignment`

hwwhww added eth2.0 PR state: WIP labels Feb 12, 2019

hwwhww mentioned this pull request Feb 12, 2019

Implement swap-or-not shuffle #272

Closed

NIC619 reviewed Feb 13, 2019

View reviewed changes

hwwhww force-pushed the shuffling_lookahead branch from 6d81274 to 7c1f552 Compare February 13, 2019 15:02

hwwhww removed the PR state: WIP label Feb 13, 2019

hwwhww requested review from djrtwo and pipermerriam February 13, 2019 15:13

hwwhww added the PR state: needs review label Feb 13, 2019

djrtwo reviewed Feb 13, 2019

View reviewed changes

pipermerriam reviewed Feb 13, 2019

View reviewed changes

hwwhww and others added 12 commits February 14, 2019 14:34

wip

6300cf7

wip

cf0e88c

Move BeaconConfig to eth.beacon.configs and fix the tests

4b25831

Apply suggestions from code review

e1b958c

Co-Authored-By: hwwhww <hwwang156@gmail.com>

PR feedback

d781004

Add test for get_next_epoch_committee_count

eeb4f1f

Add more tests for get_crosslink_committees_at_slot new cases

c9f2686

Add tests for get_next_epoch_committee_assignment

83e1341

Capitalize CommitteeConfig

c3f3d7f

Add CommitteeAssignment NamedTuple for the result from `get_next_ep…

d7bd118

…och_committee_assignment`

Update get_next_epoch_committee_assignment

4a4f0b5

PR feedback

e9561b1

hwwhww force-pushed the shuffling_lookahead branch from e5421c5 to e9561b1 Compare February 14, 2019 07:51

BeaconState.next_epoch

42a6d23

hwwhww requested a review from djrtwo February 15, 2019 17:06

djrtwo approved these changes Feb 15, 2019

View reviewed changes

hwwhww merged commit 06c4d1b into ethereum:master Feb 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constants arguments clean-up and shuffling lookahead #271

Constants arguments clean-up and shuffling lookahead #271

hwwhww commented Feb 12, 2019 •

edited

Loading

djrtwo left a comment

djrtwo Feb 13, 2019

hwwhww Feb 14, 2019

djrtwo Feb 15, 2019

djrtwo Feb 13, 2019

djrtwo Feb 13, 2019

djrtwo Feb 13, 2019

hwwhww Feb 14, 2019

ChihChengLiang Feb 14, 2019

djrtwo Feb 13, 2019

pipermerriam left a comment

pipermerriam Feb 13, 2019

Constants arguments clean-up and shuffling lookahead #271

Constants arguments clean-up and shuffling lookahead #271

Conversation

hwwhww commented Feb 12, 2019 • edited Loading

What was wrong?

How was it fixed?

1. Refactor: clean up constants arguments by adding CommitteeConfig

2. Shuffling lookahead

Cute Animal Picture

djrtwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww commented Feb 12, 2019 •

edited

Loading

1. Refactor: clean up constants arguments by adding `CommitteeConfig`