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

Validator service based on beacon plugin #525

Merged
merged 4 commits into from
Apr 30, 2019

Conversation

mhchia
Copy link
Contributor

@mhchia mhchia commented Apr 23, 2019

What was wrong?

Add validator service for #488

How was it fixed?

TODO

  • Confirm which slot should be used when proposing and skipping. (Thanks for @NIC619 's help)
  • Add receive server from Not a plugin Validator #465 (In favor of opening other PRs for "receive server" and "plugin config")
  • Refactor tests: add clean-ups and fixtures

Cute Animal Picture

put a cute animal picture link inside the parentheses

@mhchia mhchia mentioned this pull request Apr 23, 2019
4 tasks
@mhchia mhchia force-pushed the feature/beacon-validator-service branch from f10dbad to 49b719b Compare April 23, 2019 10:22
@mhchia
Copy link
Contributor Author

mhchia commented Apr 24, 2019

Considering moving testing_config.py and trinity/config.py from #465 to solve both CI and mypy errors.

Pass handshake

Add `GetNewBlock`

Basic tests are done

Still need to confirm we are using the right `slot` when proposing

Remove repeated call to calculate pubkey

Update BeaconChainConfig

Update validator propose block test

flake8

flake8 again

Makes mypy happy, except for config.py:632

Extend `BeaconGenesisData`. Use `initialize_chain`

Put `genesis_state` and `genesis_block` to global

`initialize_chain` calls `create_mock_genesis`, which consumes a lot of
time

Skip the test for beacon plugin for now

To make plugin run, `genesis_data` is required in `BeaconChainConfig`.
We need to parse the genesis json to get the data.

Add `test_validator_propose_block_fails`
@mhchia mhchia force-pushed the feature/beacon-validator-service branch from 2656109 to a66ba12 Compare April 25, 2019 09:59
@mhchia mhchia changed the title [WIP] Validator service based on beacon plugin Validator service based on beacon plugin Apr 25, 2019
@mhchia
Copy link
Contributor Author

mhchia commented Apr 25, 2019

This PR should be ready for review.

Items which will be included other PRs.

  • ReceiveServer and its tests.
  • The fix for the fact that tests/plugins/eth2/beacon/test_beacon_plugin.py::test_plugin_boot is skipped for now, due to the lack of genesis.json required by beacon node plugin.

Normal import fails to import modules with paths containing hyphens.
Avoid this with `importlib.import_module`.
Copy link
Contributor

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

Looks good! I leave some questions

trinity/plugins/eth2/beacon/validator.py Outdated Show resolved Hide resolved

NUM_VALIDATORS = 8

privkeys = tuple(int.from_bytes(
Copy link
Contributor

Choose a reason for hiding this comment

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

Have a crazy idea that we should move the plugin test from tests/plugins/eth2/ to tests/eth2/beacon/plugins/, so that we can reuse all the fixtures defined in the conftest.py there. Not sure what do you think. cc @pipermerriam @hwwhww @mhchia

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1. IMO it will help for writing tests.

Copy link
Contributor

@NIC619 NIC619 left a comment

Choose a reason for hiding this comment

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

Most of it looks good!



def get_chain(db):
return chain_class.from_genesis(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we assume that we always get_chain from genesis state/block?

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 I think it might be better to rename it to get_chain_from_genesis.

with pytest.raises(BlockNotFound):
alice.chain.get_canonical_block_by_slot(slot)
# test: the state root should change after skipping the block
assert state.root != root_post_state
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert state.root != root_post_state
assert state.root != root_post_state.root

Copy link
Contributor Author

Choose a reason for hiding this comment

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

root_post_state is already the root of the post state.

tests/plugins/eth2/beacon/test_validator.py Outdated Show resolved Hide resolved
tests/plugins/eth2/beacon/test_validator.py Outdated Show resolved Hide resolved
event_new_slot_called.wait(),
timeout=2,
loop=event_loop,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we determine if this test failed? Does it throw exception like TimeoutError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, after 2 seconds TimeoutError is raised and makes the test fail.

trinity/config.py Show resolved Hide resolved
trinity/plugins/eth2/beacon/validator.py Outdated Show resolved Hide resolved
trinity/plugins/eth2/beacon/validator.py Outdated Show resolved Hide resolved
@mhchia mhchia merged commit 58ea2dd into ethereum:master Apr 30, 2019
@mhchia mhchia deleted the feature/beacon-validator-service branch April 30, 2019 13:04
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.

3 participants