Sync ethereum/eth2.0-specs#374 part1: types, constants, deposit_helpers #1700

hwwhww · 2019-01-07T15:32:34Z

What was wrong?

ethereum/consensus-specs#374 updated a loooot logic of validator status.

How was it fixed?

Sync the constants. Also solve partial #1688: remove SHARD_PERSISTENT_COMMITTEE_CHANGE_PERIOD.

Sync related data structures:

DepositData
- Rename field value to amount, amount in Gwei.

ValidatorRecord

Add new fields:

# Slot when validator activated
('activation_slot', uint64),
# Slot when validator exited
('exit_slot', uint64),
# Slot when validator withdrew
('withdrawal_slot', uint64),
# Slot when validator was penalized
('penalized_slot', uint64),
# Status flags
('status_flags', uint64),

activation_slot, exit_slot, penalized_slot, withdrawal_slot are usedto determine if a validator is active.
status_flags is for indicating that the validator has initialized exist or being withdrawable.

Remove old fields:

# Status code
('status', uint64),
# Slot when validator last changed status (or 0)
('latest_status_change_slot', uint64),

ValidatorRegistryDeltaBlock

eth.beacon.helpers
1. Update get_new_shuffing
  1. Rename to get_shuffling
  2. Add slot: SlotNumber parameter: Invariant - if get_shuffling(seed, validators, shard, slot) returns some value x, it should return the same value x for the same seed and shard and possible future modifications of validators forever in phase 0, and until the ~1 year deletion delay in phase 2 and in the future.
2. Update get_active_validator_indices: add slot: SlotNumber parameter.
Refactor
1. BeaconState
  1. Update BeaconState.update_validator(validator_index: ValidatorIndex, validator: ValidatorRecord, balance: Gwei) API.
  2. Check len(validator_registry) == len(validator_balances)
Bugix: denoms.gwei != GWEI_PER_ETH 🙈
- Define GWEI_PER_ETH in eth/beacon/constants.py and replace denoms.gwei with it.
Apply PR#1677 feedback

Cute Animal Picture

djrtwo · 2019-01-07T17:57:38Z

I think denoms.gwei does equal GWEI_PER_ETH..

I just ran this locally:

>>> import eth_utils
>>> eth_utils.denoms
<class 'eth_utils.currency.denoms'>
>>> eth_utils.denoms.gwei
1000000000
>>> eth_utils.denoms.gwei == 10**9
True

hwwhww · 2019-01-07T18:09:20Z

@djrtwo
I did that months ago! But I think that's just a coincidence. eth_utils.denoms.gwei is "wei per gwei" (see https://github.com/ethereum/eth-utils/blob/master/eth_utils/units.py), conceptionally different. 😅

djrtwo · 2019-01-07T18:11:27Z

aaahh. I see
Tricky!

Reviewing PR now

tests/beacon/conftest.py

pipermerriam · 2019-01-07T19:16:11Z

tests/beacon/types/test_states.py

 def test_defaults(sample_beacon_state_params):
    state = BeaconState(**sample_beacon_state_params)
    assert state.validator_registry == sample_beacon_state_params['validator_registry']
    assert state.validator_registry_latest_change_slot == sample_beacon_state_params['validator_registry_latest_change_slot']  # noqa: E501


+def test_validator_registry_and_balances_length(sample_beacon_state_params, far_future_slot):
+    # When len(BeaconState.validator_registry) != len(BeaconState.validtor_balances)
+    with pytest.raises(ValueError):


Is ValidationError a more appropriate error here?

A user should never be able to provide data/input that would cause this to trigger (via signed messages, txs, etc). A developer could. Does that make it a ValidationError?

@djrtwo yes, that's also what I thought! We should ensure len(BeaconState.validator_registry) == len(BeaconState.validtor_balances) during writing eth APIs.

@djrtwo

ValidationError if it is possible for this code path to be encountered under some form of live conditions like somehow ending up with an invalid state where the registry and balances get out of sync.

ValueError for lower level errors, normally indicative of us having some kind of broken code which is passing in the wrong type of value or a malformed value.

Admittedly these two definitions are not fully independent. Maybe rule of thumb is: 1) A user should never see a ValueError, nor should anything like the ethereum/tests end up triggering a ValueError. ValidationError may occasionally show up for users and is likely a regular result for many of the ethereum/tests style tests which test edge cases.

Hmmm. I'm still a bit unclear on this.
There is an invariant that and addition to or removal from one of these arrays should always require an symmetric addition/removal from the other array. All helper functions such as add_pending_validator should enforce this invariant.

The state would only be out of sync if one of these helper functions was broken or if a user decided to manipulate/create a state directly with mismatched arrays. Because of the assumed atomicity of operations affecting the length of these two arrays, the network should never be able to provide data that gets them out of sync. This is different than say, the network providing an invalid block that throws a ValidationError when processing.

I somewhat think ValueError is appropriate here, but I am still trying to grok the difference and hope for some more clarity :)

@pipermerriam thanks for clarifying! FYI the reason why we put validator balances into a separate list in BeaconState (#1647) was clarified here by Justin:

It's an optimisation to minimise the amount of hashing when computing the beacon state root. The validator_registry (which contains most of the state) changes slowly and requires little hashing effort to maintain. The validator_balances is a smaller part of the state with much higher churn, hence why it is segregated.

So I do think this ValueError here fits "for lower level errors, normally indicative of us having some kind of broken code which is passing in the wrong type of value or a malformed value" since it's for avoiding reckless coding.

👍 seems like this could fit either case and that's expected since the two cases do have some conceptual overlap. No strong opinion of what should be done if anything here.

eth/beacon/deposit_helpers.py

eth/beacon/helpers.py

eth/beacon/state_machines/configs.py

tests/beacon/helpers.py

eth/beacon/types/validator_records.py

tests/beacon/test_helpers.py

djrtwo · 2019-01-08T00:38:45Z

tests/beacon/types/test_states.py

 def test_defaults(sample_beacon_state_params):
    state = BeaconState(**sample_beacon_state_params)
    assert state.validator_registry == sample_beacon_state_params['validator_registry']
    assert state.validator_registry_latest_change_slot == sample_beacon_state_params['validator_registry_latest_change_slot']  # noqa: E501


+def test_validator_registry_and_balances_length(sample_beacon_state_params, far_future_slot):
+    # When len(BeaconState.validator_registry) != len(BeaconState.validtor_balances)
+    with pytest.raises(ValueError):


A user should never be able to provide data/input that would cause this to trigger (via signed messages, txs, etc). A developer could. Does that make it a ValidationError?

eth/beacon/types/validator_records.py

pipermerriam · 2019-01-09T17:34:48Z

eth/beacon/helpers.py

+                  slot: SlotNumber,
+                  epoch_length: int,
+                  target_committee_size: int,
+                  shard_count: int) -> Iterable[Iterable[ShardCommittee]]:


minor nitpick

Return type here would be more correct as a Tuple type.

pipermerriam · 2019-01-09T17:39:21Z

eth/beacon/types/validator_records.py

-            latest_status_change_slot=latest_status_change_slot,
+            activation_slot=FAR_FUTURE_SLOT,
+            exit_slot=FAR_FUTURE_SLOT,
+            withdrawal_slot=FAR_FUTURE_SLOT,


I assume that we are using FAR_FUTURE_SLOT to represent the concept of undefined or None since it works out to something like 5 billion centuries in the future?

Yes, it is essentially an "unset" flag on the var. It works nicely because we can use it in status checks.'
example: is_active -- v.activation_slot <= state.slot < v.exit_slot

in the case that neither activation_slot nor exit_slot have been updated, the validator is not active.

in the case that activation_slot is set but is in the future, the validator is non active

in the case that activation_slot is in the past, but exit_slot is not yet set, then the validator is active

in the case that activation_slot is in the pastandexit_slot` has been set but is still in the future, the validator is active

in the case that activation_slot and exit_slot are in the past, then the validator is not active and is instead exited.

Is there any way that a validator could set any of these three fields to this value themselves? If so, it might be useful to explicitely define 2**64 - 1 as a disallowed value so that we have strong assurances that when any of these values is equal to 2**64 -1 we know that it means unset.

Yeah, that seems reasonable.

These values can only change via state transitions that a validator can trigger but not control the exact value of (e.g. requesting and exit which would set their exit_slot as state.slot + ENTRY_EXIT_DELAY, or getting penalized which would set theirpenalization_slot to state.slot). These state transitions all change the value from FAR_FUTURE_SLOT to a value that is a function of the current state.slot

…lpers

Define `GWEI_PER_ETH` in `eth/beacon/constants.py` and replace `denoms.gwei` with it.

1. Move `FAR_FUTURE_SLOT` to `eth.beacon.constants` 2. Rename `get_pending_validator` to `create_pending_validator` 3. Add some docstrings

hwwhww · 2019-01-10T08:09:46Z

@djrtwo I'm going to merge it. Feel free to give post-merge review if any, I'll amend in the part2 PR. :)

hwwhww added PR state: WIP eth2.0 labels Jan 7, 2019

hwwhww requested a review from djrtwo January 7, 2019 17:22

hwwhww removed the PR state: WIP label Jan 7, 2019

pipermerriam reviewed Jan 7, 2019

View reviewed changes

djrtwo reviewed Jan 8, 2019

View reviewed changes

djrtwo reviewed Jan 9, 2019

View reviewed changes

eth/beacon/types/validator_records.py Outdated Show resolved Hide resolved

hwwhww force-pushed the rework_deposit_helpers branch from cba7498 to 08f4472 Compare January 9, 2019 17:27

hwwhww requested review from pipermerriam and djrtwo January 9, 2019 17:30

pipermerriam approved these changes Jan 9, 2019

View reviewed changes

hwwhww added 10 commits January 10, 2019 15:58

Sync ethereum/consensus-specs#374 part1: types, constants, deposit_he…

64925be

…lpers

Move some changes to next PR

07df477

Bugix: denoms.gwei != GWEI_PER_ETH

ae65c40

Define `GWEI_PER_ETH` in `eth/beacon/constants.py` and replace `denoms.gwei` with it.

Apply PR#1677 feedback

5c140b1

Refactor

5850e0b

Refactor and add testcases of test_get_shuffling_is_complete

30e6cea

PR feedback

6fb899b

PR feedback

7412023

1. Move `FAR_FUTURE_SLOT` to `eth.beacon.constants` 2. Rename `get_pending_validator` to `create_pending_validator` 3. Add some docstrings

Remove eth/beacon/exceptions.py

41a9439

PR feedback

d2f76bd

hwwhww force-pushed the rework_deposit_helpers branch from 08f4472 to d2f76bd Compare January 10, 2019 08:04

hwwhww merged commit 583a193 into ethereum:master Jan 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync ethereum/eth2.0-specs#374 part1: types, constants, deposit_helpers #1700

Sync ethereum/eth2.0-specs#374 part1: types, constants, deposit_helpers #1700

hwwhww commented Jan 7, 2019 •

edited

djrtwo commented Jan 7, 2019

hwwhww commented Jan 7, 2019

djrtwo commented Jan 7, 2019

pipermerriam Jan 7, 2019

djrtwo Jan 8, 2019 •

edited

hwwhww Jan 8, 2019

pipermerriam Jan 8, 2019

djrtwo Jan 9, 2019 •

edited

hwwhww Jan 9, 2019

pipermerriam Jan 9, 2019

djrtwo Jan 8, 2019 •

edited

pipermerriam Jan 9, 2019

pipermerriam Jan 9, 2019

djrtwo Jan 9, 2019

pipermerriam Jan 9, 2019

djrtwo Jan 9, 2019

hwwhww commented Jan 10, 2019

Sync ethereum/eth2.0-specs#374 part1: types, constants, deposit_helpers #1700

Sync ethereum/eth2.0-specs#374 part1: types, constants, deposit_helpers #1700

Conversation

hwwhww commented Jan 7, 2019 • edited

What was wrong?

How was it fixed?

Cute Animal Picture

djrtwo commented Jan 7, 2019

hwwhww commented Jan 7, 2019

djrtwo commented Jan 7, 2019

Choose a reason for hiding this comment

djrtwo Jan 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo Jan 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo Jan 8, 2019 • edited

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

hwwhww commented Jan 10, 2019

hwwhww commented Jan 7, 2019 •

edited

djrtwo Jan 8, 2019 •

edited

djrtwo Jan 9, 2019 •

edited

djrtwo Jan 8, 2019 •

edited