Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

RANDAO processing #252

Merged
merged 13 commits into from
Feb 17, 2019
Merged

RANDAO processing #252

merged 13 commits into from
Feb 17, 2019

Conversation

jannikluhn
Copy link
Contributor

What was wrong?

RANDAO processing was missing and the original PR for this was outdated.

How was it fixed?

Implemented the function process_randao plus the helpers validate_randao, slot_to_epoch, and get_current_epoch according to the spec.

However, some signature checking tests are failing and I don't understand why: Even though a different message is signed, bls.verify still returns True. Any idea what the issue is? (this is the reason why I labeled it as WIP.)

There are two FIXME comments that I think should be addressed in #235 (or here, depending what's merged earlier, cc @NIC619 ).

Cute Animal Picture

put a cute animal picture link inside the parentheses

@hwwhww
Copy link
Contributor

hwwhww commented Feb 2, 2019

@jannikluhn Oh I found the answer, privkey shouldn't be 0. I tried these test cases and it passed:

@pytest.mark.parametrize(
    ["is_valid", "epoch", "expected_epoch", "proposer_privkey", "expected_proposer_privkey"],
    (
        (True, 0, 0, 1, 1),
        (True, 1, 1, 2, 2),
        (False, 0, 1, 1, 1),
        (False, 0, 0, 1, 2),
    )
)
def test_randao_reveal_validation(...

and also tests/eth2/beacon/state_machines/forks/test_serenity_block_processing.py

def test_randao_processing_validates_randao_reveal(sample_beacon_block_params,
                                                   sample_beacon_state_params,
                                                   sample_fork_params,
                                                   config):
    proposer_privkey = 1
    ...

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Just some nitpicks.

And @jannikluhn @NIC619 , about get_current_epoch, get_previous_epoch, and get_next_epoch.... what do you think about adding them as the instance functions of BeaconState?

@NIC619
Copy link
Contributor

NIC619 commented Feb 4, 2019

👍 on making them instance function of BeaconState though IIUC they are already implemented?
https://github.com/ethereum/trinity/blob/master/eth2/beacon/types/states.py#L329-L337

@jannikluhn
Copy link
Contributor Author

+1 on making them instance function of BeaconState though IIUC they are already implemented?
https://github.com/ethereum/trinity/blob/master/eth2/beacon/types/states.py#L329-L337

Didn't know they existed, thanks 👍, will use those.

It would be nice if we could pass the whole config to BeaconState at initialization and use properties instead of getters, but I guess that's not possible without some changes in py-ssz...

@hwwhww
Copy link
Contributor

hwwhww commented Feb 4, 2019

It would be nice if we could pass the whole config to BeaconState at initialization and use properties instead of getters, but I guess that's not possible without some changes in py-ssz...

Yep...
FYI we have an outline of a "forkable" BeaconBlock (i.e., SerenityBeaconBlock), but not a forkable BeaconState (i.e., SerenityBeaconState) yet and I think we can delay the process of making them forkable after we have a stable phase 0 first release. (Now, 20 SSZ objects!!!!)

@hwwhww
Copy link
Contributor

hwwhww commented Feb 13, 2019

I just realized that, one more thing, in this PR, process_randao is not called in eth2.beacon.state_machines.forks.serenity.state_transitions.SerenityStateTransition.per_block_transition:

# TODO: state = process_randao(state, block, self.config)

I imagine you will have to modify eth2/beacon/tools/builder/validator.py to set randao reveal for test_demo.py. Do you think it makes sense to add in this PR? Or the next one?

@hwwhww hwwhww merged commit b6667b6 into ethereum:master Feb 17, 2019
@hwwhww hwwhww mentioned this pull request Feb 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants