FastChainSyncer now handles data requests from peers #993
Conversation
p2p/chain.py
Outdated
| elif isinstance(cmd, eth.NodeData): | ||
| # When doing a chain sync we never send GetNodeData requests, so peers should not send | ||
| # us NodeData msgs. | ||
| self.logger.warn("Unexpected NodeData msg from %s", peer) |
There was a problem hiding this comment.
Should we also disconnect from the peer? Otherwise a malicious peer could flood the terminal with these messages.
p2p/chain.py
Outdated
| async def _handle_block_receipts( | ||
| self, peer: ETHPeer, receipts_by_block: List[List[eth.Receipt]]) -> None: | ||
| # When doing a regular sync we never request receipts. | ||
| self.logger.warn("Unexpected BlockReceipts msg from %s", peer) |
There was a problem hiding this comment.
Any reason this was not inlined in the if/elif block on lines 485-500 the same way that the NodeData warning is inlined. Similarly, should we disconnect in this case?
There was a problem hiding this comment.
lines 485-500 are in FastChainSyncer, and during a fast sync we send GetReceipts requests and expect Receipts responses. This is RegularChainSyncer, and during a regular (i.e. block-processing) sync we don't send GetReceipts. I guess it'd make sense to disconnect as well, yeah
There was a problem hiding this comment.
NodeData, on the other hand, is unexpected in both syncers -- it is only the StateDownloader that requests those
There was a problem hiding this comment.
I mis-read the diff and didn't see that those were two different classes.
gsalgado
force-pushed
the
fast-sync-handle-data-requests
branch
3 times, most recently
from
b2e5f79 to
6a94a8a
Compare
| @@ -485,6 +488,8 @@ def disconnect(self, reason: DisconnectReason) -> None: | |||
| self.logger.debug("Disconnecting from remote peer; reason: %s", reason.name) | |||
| self.base_protocol.send_disconnect(reason.value) | |||
| self.close() | |||
| if self.is_running: | |||
| await self.cancel() | |||
There was a problem hiding this comment.
cc @carver as you added a wait_disconnect method in your other PR and I think this is a better pattern.
gsalgado
force-pushed
the
fast-sync-handle-data-requests
branch
4 times, most recently
from
64b24e9 to
a01347e
Compare
It used to handle only GetBlockHeader requests, and now handles the rest as well
Also disconnect from remotes if we get unexpected NodeData or Receipts msgs during a sync
gsalgado
force-pushed
the
fast-sync-handle-data-requests
branch
from
a01347e to
9183298
Compare
It used to handle only GetBlockHeader requests, and now handles the rest as well
Includes commits from other PRs, so please review just f80d8ba