Avoid unnecessary clamp on `previous_epoch` by `genesis_epoch` #398

ralexstokes · 2019-03-12T15:39:46Z

What was wrong?

Fixes #382.

How was it fixed?

Updated the calculation hanging off the BeaconState.

Cute Animal Picture

ralexstokes · 2019-03-12T20:36:18Z

eth2/beacon/epoch_processing_helpers.py

@@ -142,16 +140,25 @@ def get_winning_root(
 def get_epoch_boundary_attester_indices(


looks like @hwwhww and @ChihChengLiang were involved here -- would love your input on this...

don't exactly like getting the block root inside this function (mainly bc we then have to monkeypatch it later to test it) but the way the spec is written we don't want to call this function unless there are some attestations in attestations.

open to changing this bit if we can think of a nicer/cleaner way.... i'll update if i think of something i like better

We don't want to call this function again and again for every attestations, either.
Maybe something like:

if len(attestations)>0: root = helpers.get_block_root(...)

@ChihChengLiang i don't follow -- given that we are iterating over attestations then we only call helpers.get_block_root if len(attestations)>0 so that condition would always be true....

i think what will be nicer is if I trace up the call stack here and just avoid calling this entirely -- it seems like there is only one place it is being called in the function that determines if the various epochs are even justifiable.

ralexstokes · 2019-03-12T20:38:47Z

@hwwhww @ChihChengLiang this became more involved than the spec change would suggest (surprise!) but is ready for review -- i called attention to the part i'm not super excited about and would love to hear any thoughts!

ralexstokes · 2019-03-14T00:47:48Z

Ok, I took advantage of some fresh helpers from spec version v0.5.0 which handle things like "previous epoch to genesis" gracefully...

I deleted the prior test for the function that no longer explicitly exists, with the understanding that it is only used by the function that gets the balances for the given boundary attestations which is already tested (if there is a bug in this covered function then it will fail the wrapping function's tests)

hwwhww

Some suggestions of the type hintings, the rest of the refactorings/spec sync look good to me. 👍

hwwhww · 2019-03-14T06:37:25Z

eth2/beacon/epoch_processing_helpers.py

+            slot,
+            config.LATEST_BLOCK_ROOTS_LENGTH
+        )
+    ]


I think we would try to avoid returning mutable List.

hwwhww · 2019-03-14T06:52:45Z

eth2/beacon/epoch_processing_helpers.py

        state: 'BeaconState',
-        config: 'BeaconConfig') -> Tuple[Gwei, Gwei]:
-
+        config: 'BeaconConfig') -> Sequence[PendingAttestationRecord]:


As https://github.com/ethereum/snake-charmers-tactical-manual/blob/master/style-guide.md#function-variance mentioned, we should narrow down the scope of return type:

Normally, use Sequence for the input parameters.

If it returns known tuple, use Tuple for the return type.

If there's @to_tuple + yield for output, use Iterable for the return type.

good call -- as long as Tuple is a subtype of Sequence here then I'm happy w/ it :)

i don't know mypy well enough to answer this question but I suspect the subtyping relation holds...

hwwhww · 2019-03-14T06:53:54Z

eth2/beacon/epoch_processing_helpers.py

-        config.MAX_DEPOSIT_AMOUNT,
+def _get_previous_epoch_boundary_attestations(
+        state: 'BeaconState',
+        config: 'BeaconConfig') -> Sequence[PendingAttestationRecord]:


NIC619

LGTM!

NIC619 · 2019-03-14T08:22:36Z

eth2/beacon/epoch_processing_helpers.py

+    slot = get_epoch_start_slot(epoch, config.SLOTS_PER_EPOCH)
+    return [
+        a for a in attestations
+        if a.data.epoch_boundary_root == get_block_root(


We can compute the epoch boundary root beforehand.

if you are referring to the call to get_block_root i started having the same thought but then i remembered why i was doing this in the first place.... and it was because we end up w/ a call to this function for essentially slots before the genesis epoch (where get_block_root will explode)

we purposefully put the call inside the conditional so that it is lazily evaluated.... if this becomes a hotspot perf-wise then we can do something like

slot = # .... if len(attestations) != 0: block_root = get_block_root(...) return tuple( a for a in ... # rest of expression )

or even just add some memoization to this function.... tbh i don't expect we will need to do this in this case :)

eth2/beacon/epoch_processing_helpers.py

ralexstokes · 2019-03-15T00:37:58Z

@hwwhww would love your thoughts on what I did here 03d3ddf to satisfy mypy... i feel like the @to_tuple + generator expression is less efficient than what I had before but mypy was saying I would have to otherwise cast to a Tuple[T] (as the inferred type w/ the prior implementation was Tuple[T,...]

hwwhww · 2019-03-17T02:01:41Z

@ralexstokes (sorry for the late reply!)
Yes, it seems right about how to make mypy happy - either @to_tuple + yield + Iterable or tuple comprehension + Tuple. IMO they are both fine as we can avoid List in both approaches.

/cc @pipermerriam any other trade-off here?

ralexstokes · 2019-03-28T16:40:35Z

this got out of date -- i'll rebase and then merge this in

Prior to this commit, block roots would be fetched for slots prior to the genesis and would fail

Uses some helper functions from the latest version of the spec The main thing we get is a Pythonic way to lazily compute block roots as eager computation was blowing up when finding the block roots before genesis in an earlier commit.

ralexstokes · 2019-03-29T23:44:18Z

Closing in favor of #469.

hwwhww mentioned this pull request Mar 12, 2019

Spec sync - Meteorite 711 #379

Closed

ralexstokes force-pushed the fix-previous-epoch-calculation branch from 84b6913 to d8a230e Compare March 12, 2019 20:27

ralexstokes commented Mar 12, 2019

View reviewed changes

ralexstokes requested review from hwwhww and ChihChengLiang March 12, 2019 20:37

hwwhww approved these changes Mar 14, 2019

View reviewed changes

hwwhww requested a review from NIC619 March 14, 2019 06:57

NIC619 approved these changes Mar 14, 2019

View reviewed changes

ChihChengLiang reviewed Mar 14, 2019

View reviewed changes

eth2/beacon/epoch_processing_helpers.py Outdated Show resolved Hide resolved

ralexstokes force-pushed the fix-previous-epoch-calculation branch from 03d3ddf to 56c2f84 Compare March 15, 2019 00:56

ralexstokes added 14 commits March 28, 2019 16:15

Avoid unnecessary clamp on previous_epoch by genesis_epoch

7ec0bbb

Remove unnecessary genesis_epoch param

6dd0f59

Prefer getter method over ad-hoc calculation

477ef10

Skip processing of rewards and penalties if there are no validators

43bc1aa

Offset starting genesis slot

d90f99b

Make block_root calculation dependent on existence of attestations

d95ee74

Prior to this commit, block roots would be fetched for slots prior to the genesis and would fail

Fix bug by providing correct data type here

753cc3d

patch test to match new function implementation

cc0d396

linter fix

1b23370

add type to satisfy mypy

f07d784

Adjust type hints to be as precise as we desire

c25e20e

Remove an unnecessary call to the list constructor

d097d43

Fixes for mypy

3931060

ralexstokes force-pushed the fix-previous-epoch-calculation branch from 56c2f84 to 3931060 Compare March 28, 2019 23:41

mypy fix

7fa06dc

ralexstokes mentioned this pull request Mar 29, 2019

Update previous epoch calculation #469

Merged

ralexstokes closed this Mar 29, 2019

ralexstokes deleted the fix-previous-epoch-calculation branch March 29, 2019 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid unnecessary clamp on `previous_epoch` by `genesis_epoch` #398

Avoid unnecessary clamp on `previous_epoch` by `genesis_epoch` #398

ralexstokes commented Mar 12, 2019

ralexstokes Mar 12, 2019

ChihChengLiang Mar 13, 2019

ralexstokes Mar 13, 2019

ralexstokes commented Mar 12, 2019

ralexstokes commented Mar 14, 2019

hwwhww left a comment •

edited

Loading

hwwhww Mar 14, 2019

hwwhww Mar 14, 2019

ralexstokes Mar 15, 2019

hwwhww Mar 14, 2019

NIC619 left a comment

NIC619 Mar 14, 2019

ralexstokes Mar 15, 2019

ralexstokes commented Mar 15, 2019

hwwhww commented Mar 17, 2019

ralexstokes commented Mar 28, 2019

ralexstokes commented Mar 29, 2019

		@@ -142,16 +140,25 @@ def get_winning_root(
		def get_epoch_boundary_attester_indices(

Avoid unnecessary clamp on previous_epoch by genesis_epoch #398

Avoid unnecessary clamp on previous_epoch by genesis_epoch #398

Conversation

ralexstokes commented Mar 12, 2019

What was wrong?

How was it fixed?

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralexstokes commented Mar 12, 2019

ralexstokes commented Mar 14, 2019

hwwhww left a comment • edited Loading

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

NIC619 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralexstokes commented Mar 15, 2019

hwwhww commented Mar 17, 2019

ralexstokes commented Mar 28, 2019

ralexstokes commented Mar 29, 2019

Avoid unnecessary clamp on `previous_epoch` by `genesis_epoch` #398

Avoid unnecessary clamp on `previous_epoch` by `genesis_epoch` #398

hwwhww left a comment •

edited

Loading