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

Add arg to take Unix integer as genesis time #686

Merged
merged 4 commits into from
Jun 4, 2019

Conversation

ChihChengLiang
Copy link
Contributor

What was wrong?

Now Eth2 network generator only takes seconds delay as the genesis time.

How was it fixed?

Add a parser argument to take Unix integer as genesis time

Cute Animal Picture

put a cute animal picture link inside the parentheses

genesis_time_group = testnet_generator_parser.add_mutually_exclusive_group(
required=True,
)
genesis_time_group.add_argument(
"--genesis-delay",
Copy link
Member

Choose a reason for hiding this comment

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

You could probably simplify this by changing this to simply compute what the equivalent would be in --genesis-time and to have it populate that value onto genesis_time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the description "Seconds before genesis time from now" should be changed to "Set seconds delay after the genesis state is created as genesis time".
Since the usage of --genesis-delay option is for debugging, and the genesis state takes a long time to create, the "delay" actually starts after the latter is done. So for example when debugging in eth2/beacon/scripts/run_beacon_nodes.py, the choice of delay can be independent of the time to create the genesis state.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

generally looks good! left some comments

@ChihChengLiang ChihChengLiang merged commit 1a1dd1a into ethereum:master Jun 4, 2019
@ChihChengLiang ChihChengLiang deleted the set-genesis-time branch June 4, 2019 14:30
@@ -156,7 +187,7 @@ def generate_keys(cls,

@classmethod
def generate_genesis_state(cls,
genesis_delay: Second,
get_genesis_time: Callable[[], Timestamp],
Copy link
Member

Choose a reason for hiding this comment

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

Reviewing this a second time I'm a little confused by this API. Can you explain why we don't/can't just pass in a firm timestamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two possibilities for get_genesis_time: the one produced with get_genesis_time_from_constant()and the other by get_genesis_time_from_delay(). The reason to have a get_genesis_time interface is beacause for the latter case, the genesis time is determined after the create_mock_genesis finished.

An alternative could be

def generate_genesis_state(..., genesis_time, genesis_delay=None):
    state, _ = create_mock_genesis(
            ...
            genesis_time=genesis_time if genesis_time is not None else 0,
        )
    if genesis_delay is not None:
        state = state.copy(genesis_time=Timestamp(int(time.time()) + genesis_delay))
    ...

@pipermerriam
Copy link
Member

pipermerriam commented Jun 4, 2019 via email

@ChihChengLiang
Copy link
Contributor Author

Exactly

Gauddel pushed a commit to Gauddel/trinity that referenced this pull request Jun 6, 2019
* add arg to take specific genesis time

* fix log style

* PR feedback

* test_missing_genesis_time_arg
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants