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

Add receive server for validator service #580

Merged
merged 11 commits into from
May 4, 2019

Conversation

mhchia
Copy link
Contributor

@mhchia mhchia commented May 2, 2019

What was wrong?

Our validator service has no corresponding handler for the command NewBeaconBlock.

How was it fixed?

Based on BCCReceiveServer, the work of @NIC619 from #465.

  • Add BCCReceiveServer to define the handler for NewBeaconBlock.
  • Add method BCCReceiveServer._try_import_or_handle_orphan to check and maintain orphan blocks in BCCReceiveServer.orphan_block_pool. Currently, it is a in-memory set. We can further optimize it for purposes through changing orphan_block_pool._pool.
  • Add method BCCReceiveServer._request_block_by_root used by BCCReceiveServer._try_import_or_handle_orphan to request the parents of the orphans from peers.
  • Add a handler for BeaconBlocks to handle the response of our sent requests.

Cute Animal Picture

put a cute animal picture link inside the parentheses

@mhchia mhchia requested review from ChihChengLiang and NIC619 May 2, 2019 16:12
@mhchia
Copy link
Contributor Author

mhchia commented May 2, 2019

Tests failed in https://circleci.com/gh/mhchia/trinity/1857. It seems to be related to the change from signed_root to signing_root. Will investigate it tomorrow.

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.

The only problem is with the block.root/block.signed_root. Will take another pass on the tests tomorrow.

trinity/protocol/bcc/servers.py Show resolved Hide resolved
trinity/protocol/bcc/servers.py Outdated Show resolved Hide resolved
trinity/protocol/bcc/servers.py Outdated Show resolved Hide resolved
trinity/protocol/bcc/servers.py Outdated Show resolved Hide resolved
trinity/protocol/bcc/servers.py Outdated Show resolved Hide resolved
trinity/protocol/bcc/servers.py Outdated Show resolved Hide resolved
trinity/protocol/bcc/servers.py Show resolved Hide resolved
while len(blocks_to_be_imported) != 0:
block = blocks_to_be_imported.pop()
# try to import the block
if not self._is_block_root_in_db(block.previous_block_root):
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also check if block.previous_block_root in self._is_block_root_in_orphan_block_pool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't quite understand. Could you elaborate more on the advantage to check if block.previous_block_root in self._is_block_root_in_orphan_block_pool over if not self._is_block_root_in_db(block.previous_block_root)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant we can first check if it's parent is in the orphan peer pool then next check if it's parent is in the db. This helps by not having to lookup the db in case where it's parent is already in the pool.
But after a second thought this doesn't really help much(or even worse this adds another lookup into orphan pool) since case like this is probably rare.

trinity/protocol/bcc/servers.py Outdated Show resolved Hide resolved
trinity/protocol/bcc/servers.py Outdated Show resolved Hide resolved
And change `BeaconBlock` to `SerenityBeaconBlock`, to align with the
TestChain.
tests/core/p2p-proto/bcc/test_receive_server.py Outdated Show resolved Hide resolved
tests/core/p2p-proto/bcc/test_receive_server.py Outdated Show resolved Hide resolved
mhchia added 2 commits May 3, 2019 18:43
- To avoid repeated block generating patterns
- Fix typing in the test
- Rename `map_requested_id_block_root` to `map_request_id_block_root`
@mhchia
Copy link
Contributor Author

mhchia commented May 3, 2019

Updated the PR according to the review feedback.

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.

Looks great!

@mhchia mhchia merged commit cafe206 into ethereum:master May 4, 2019
@mhchia mhchia deleted the feature/receive-server branch May 4, 2019 07:07
@NIC619 NIC619 mentioned this pull request May 5, 2019
5 tasks
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