Rework helper functions - part 2 #1563

hwwhww · 2018-12-08T03:35:16Z

What was wrong?

#1510

How was it fixed?

Fix get_shard_committees_at_slot
- The reason why I add _get_shard_committees_at_slot is for making the tests easier.
Fix get_block_hash
Add get_beacon_proposer_index
Add get_attestation_participants
Add get_effective_balance
Add get_new_validator_registry_delta_chain_tip

Also, make the tests for these functions not depend on genesis state or block.

Cute Animal Picture

…in_tip`

ChihChengLiang

Nice work!
For other reviewers, note that get_attestation_participants is different from the spec. In spec the function takes attestation_data as argument and extracts slot and shard from it.

eth/beacon/helpers.py

pipermerriam

Only reviews implementation architecture and not correctness WRT specification.

pipermerriam · 2018-12-10T19:25:51Z

eth/beacon/helpers.py

+    proposer_committee_size = len(first_shard_committee.committee)
+
+    if proposer_committee_size <= 0:
+        raise ValueError(


Do you think ValidationError would be more appropriate for these? My loose guideline has been:

ValidationError for things that we might encounter in live network conditions (like someone sending us a transaction with an invalid signature).

ValueError for things that indicate broken code (and that when they occur we address them directly rather than by try/except catching the ValueError)

Agreed.

In extreme conditions in which there are fewer validators than there are slots in an epoch (< 64 validators), some slots would have no associated committee and thus no associated proposer. In this case, an incoming block for such an empty slot should fail validation.

[if we have fewer than 64 validators we probably have bigger problems on our hands, but the code should still run :) ]

pipermerriam · 2018-12-10T19:30:27Z

eth/beacon/helpers.py

+        current_validator_registry_delta_chain_tip +
+        flag.to_bytes(1, 'big') +
+        index.to_bytes(3, 'big') +
+        # TODO: currently, we use 256-bit pubkey which is different form the spec


Not for this PR but something I think we should address.

We have tended to use eth_keys data types for private and public key representations. I suspect that we'll have to make some modifications to eth_keys to support new key types? If this is accurate, do you think you can open issues both here and in eth_keys to track those needs?

pipermerriam · 2018-12-10T19:31:56Z

tests/beacon/conftest.py

    return {
        'slot': 10,
        'randao_reveal': b'\x55' * 32,
        'candidate_pow_receipt_root': b'\x55' * 32,
-        'ancestor_hashes': (),
+        'ancestor_hashes': tuple([ZERO_HASH32 for _ in range(2 * epoch_length)]),


Maybe more idiomatic.

ancestor_hashes = tuple(itertools.repeat(ZERO_HASH32, 2 * epoch_length))

djrtwo · 2018-12-10T23:35:43Z

eth/beacon/helpers.py

-        recent_block_hashes: Sequence[Hash32],
-        current_block_slot_number: int,
+        latest_block_hashes: Sequence[Hash32],
+        current_block_slot: int,


I would just call this current_slot as the state can transition slot by slot without necessarily having a block

Suggested change

current_block_slot: int,

current_slot: int,

djrtwo · 2018-12-10T23:37:08Z

eth/beacon/helpers.py

    """
-    if len(recent_block_hashes) != epoch_length * 2:
+    if len(latest_block_hashes) != epoch_length * 2:


This condition is not valid. lastest_block_hashes are added to one by one (as new slots progress), but are then trimmed down an entire EPOCH_LENGTH at the end of the per-epoch processing.

djrtwo · 2018-12-10T23:37:49Z

eth/beacon/helpers.py

            )
        )

-    slot_relative_position = current_block_slot_number - epoch_length * 2
+    slot_relative_position = current_block_slot - epoch_length * 2


use len

Suggested change

slot_relative_position = current_block_slot - epoch_length * 2

slot_relative_position = current_block_slot - len(latest_block_hashes)`

djrtwo · 2018-12-10T23:42:31Z

eth/beacon/helpers.py

            slot,
            epoch_length,
        )


 @to_tuple
-def get_hashes_to_sign(recent_block_hashes: Sequence[Hash32],
+def get_hashes_to_sign(latest_block_hashes: Sequence[Hash32],


We no longer sign the series of hashes. Instead we sign a bunch of discrete data that can be pulled from state.
I believe this can be removed.
See https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#attestationdata

djrtwo · 2018-12-10T23:43:55Z

eth/beacon/helpers.py

-
-@to_tuple
-def get_new_recent_block_hashes(old_block_hashes: Sequence[Hash32],
+def get_new_latest_block_hashes(old_block_hashes: Sequence[Hash32],


We no longer need this. The latest_block_hashes are now just updated with a one-liner

state.latest_block_hashes = state.latest_block_hashes + [latest_hash]

If we keep the method, we should simplify it to the spec requirements

djrtwo · 2018-12-10T23:47:51Z

eth/beacon/helpers.py

@@ -170,54 +157,45 @@ def get_new_recent_block_hashes(old_block_hashes: Sequence[Hash32],
 # Get shards_committees or indices
 #
 @to_tuple
-def get_shards_committees_for_slot(
-        crystallized_state: 'CrystallizedState',
+def _get_shard_committees_at_slot(


get_shard_committees_at_slot has a simplified logic and no longer uses lastest_state_recalculation_slot (this field is actually entirely removed).

The slot_relative_position is now calculated from the current slot in the state, so we should pass that slot in and use it here instead.

https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#get_shard_committees_at_slot

Opened #1575 to address it.

djrtwo · 2018-12-10T23:53:17Z

eth/beacon/helpers.py

+    proposer_committee_size = len(first_shard_committee.committee)
+
+    if proposer_committee_size <= 0:
+        raise ValueError(


Agreed.

In extreme conditions in which there are fewer validators than there are slots in an epoch (< 64 validators), some slots would have no associated committee and thus no associated proposer. In this case, an incoming block for such an empty slot should fail validation.

[if we have fewer than 64 validators we probably have bigger problems on our hands, but the code should still run :) ]

djrtwo · 2018-12-10T23:59:26Z

eth/beacon/helpers.py

+    try:
+        shard_committee = shard_committees[0]
+    except IndexError:
+        raise ValueError("shard_committees should not be empty.")


Following @pipermerriam's logic, this too should be a ValidationError.

The way a user would trigger it would be to pack a block with an attestation not represented in by a committee at that slot. This should cause block validation to fail.

djrtwo · 2018-12-11T00:00:17Z

eth/beacon/helpers.py

+        raise ValueError("shard_committees should not be empty.")
+
+    if len(participation_bitfield) != get_bitfield_length(len(shard_committee.committee)):
+        raise ValueError(


Similarly, a user could trigger this by adding malformed attestations to a block.
Throw ValidationError

djrtwo · 2018-12-11T00:04:38Z

tests/beacon/test_helpers.py

        from_slot,
        to_slot,
        epoch_length,
    )
    assert len(result) == to_slot - from_slot + 1


-@pytest.mark.xfail(reason="Need to be fixed")
-def test_get_hashes_to_sign(genesis_block, epoch_length):
+def test_get_hashes_to_sign(sample_block, epoch_length):


No longer needed

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

hwwhww added 4 commits December 8, 2018 11:22

Fix get_shard_committees_at_slot

13bdd94

Fix get_block_hash

802acc1

Add get_beacon_proposer_index

e7743f5

Add get_attestation_participants

feab65d

hwwhww added PR state: WIP eth2.0 labels Dec 8, 2018

hwwhww added 3 commits December 8, 2018 16:25

Clean up

700fe16

Add get_effective_balance and `get_new_validator_registry_delta_cha…

24b0749

…in_tip`

Clean up

8487575

hwwhww removed the PR state: WIP label Dec 9, 2018

hwwhww requested review from ChihChengLiang and djrtwo December 9, 2018 14:06

ChihChengLiang approved these changes Dec 9, 2018

View reviewed changes

eth/beacon/helpers.py Outdated Show resolved Hide resolved

eth/beacon/helpers.py Outdated Show resolved Hide resolved

pipermerriam approved these changes Dec 10, 2018

View reviewed changes

djrtwo reviewed Dec 11, 2018

View reviewed changes

hwwhww and others added 3 commits December 13, 2018 04:22

PR feedback

69e8f3f

Fix get_effective_balance (ethereum/consensus-specs#297)

18ad8a4

Apply suggestions from code review

f084ad9

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

hwwhww merged commit 7e3b313 into ethereum:master Dec 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework helper functions - part 2 #1563

Rework helper functions - part 2 #1563

hwwhww commented Dec 8, 2018 •

edited

ChihChengLiang left a comment

pipermerriam left a comment

pipermerriam Dec 10, 2018

djrtwo Dec 10, 2018

pipermerriam Dec 10, 2018

pipermerriam Dec 10, 2018

djrtwo Dec 10, 2018

djrtwo Dec 10, 2018

djrtwo Dec 10, 2018

djrtwo Dec 10, 2018

djrtwo Dec 10, 2018

djrtwo Dec 10, 2018

hwwhww Dec 11, 2018

djrtwo Dec 10, 2018

djrtwo Dec 10, 2018

djrtwo Dec 11, 2018

djrtwo Dec 11, 2018

	slot_relative_position = current_block_slot - epoch_length * 2
	slot_relative_position = current_block_slot - len(latest_block_hashes)`

Rework helper functions - part 2 #1563

Rework helper functions - part 2 #1563

Conversation

hwwhww commented Dec 8, 2018 • edited

What was wrong?

How was it fixed?

Cute Animal Picture

ChihChengLiang left a 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

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

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 Dec 8, 2018 •

edited