Further flesh out round trip request/response API #1089

pipermerriam · 2018-07-25T03:41:02Z

What was wrong?

Still too much manual work to be done for round trip requests for block headers.

How was it fixed?

established base class for p2p requests for data in p2p.peer.BaseRequest
setup data structure on ETHPeer which houses a mapping between Command -> Future.
In the handle_sub_process_msg, if there is a waiting future for a given command, it will set the future result.
New API ETHPeer.get_block_headers which does the full request/response round trip.

This has a really nice side effect of not needing the chain syncers to handle the BlockHeaders response, and instead can just await peer.get_block_headers(...), removing a decent amount of complexity from the class. If we continue this pattern for the rest of the block body parts I think the chain syncer is going to look a lot cleaner and easier to grok.

Cute Animal Picture

pipermerriam · 2018-07-25T14:24:14Z

@gsalgado wanted to ping you with this early POC. This is the sort of API that I want to move towards for peer interactions. Curious to hear your thoughts.

gsalgado · 2018-07-25T15:26:56Z

p2p/peer.py

@@ -694,6 +698,13 @@ def handle_sub_proto_msg(self, cmd: protocol.Command, msg: protocol._DecodedMsgT
            if actual_td > self.head_td:
                self.head_hash = actual_head
                self.head_td = actual_td
+        elif isinstance(cmd, eth.BlockHeaders):
+            # try to match with a request
+            for request, waiter in self.pending_requests.items():


I don't think we need to support multiple in-flight BlockHeader requests as we should never send multiple concurrent requests to a single peer, and it'd be better to have an API that enforces that rather than one that supports multiple concurrent requests as I'm pretty sure that will end up being misused.

I like that.

pipermerriam · 2018-07-30T20:39:30Z

@gsalgado this is ready for a second pass.

gsalgado

Looks good! I've a few suggestions but my main concern is the exception with the same name as the one from the eth package

gsalgado · 2018-07-31T06:09:06Z

p2p/chain.py

@@ -313,9 +302,6 @@ class LightChainSyncer(BaseHeaderChainSyncer):
                          msg: protocol._DecodedMsgType) -> None:
        if isinstance(cmd, les.Announce):
            self._sync_requests.put_nowait(peer)
-        elif isinstance(cmd, les.BlockHeaders):
-            msg = cast(Dict[str, Any], msg)
-            self._handle_block_headers(tuple(cast(Tuple[BlockHeader, ...], msg['headers'])))


Maybe it's worth leaving a comment here that BlockHeaders messages are not handled here because they're handled in a Peer method? Otherwise someone reading this code might have a hard time figuring out why those messages are not handled like the others

Actually, we should probably leave the elif block here otherwise it will be handled in the else block, which logs a debug msg and may be confusing to someone reading the logs

gsalgado · 2018-07-31T06:09:40Z

p2p/chain.py

@@ -538,9 +525,7 @@ def request_receipts(self, target_td: int, headers: List[BlockHeader]) -> int:
    async def _handle_msg(self, peer: HeaderRequestingPeer, cmd: protocol.Command,
                          msg: protocol._DecodedMsgType) -> None:
        peer = cast(ETHPeer, peer)
-        if isinstance(cmd, eth.BlockHeaders):
-            self._handle_block_headers(tuple(cast(Tuple[BlockHeader, ...], msg)))
-        elif isinstance(cmd, eth.BlockBodies):


gsalgado · 2018-07-31T06:11:07Z

p2p/eth.py

+        self.skip = skip
+        self.reverse = reverse
+
+    def validate_response(self, response: Any) -> None:


Do we need to use Any here, even though you raise a ValidationError if it's not a tuple?

I believe Any is what we want. It is allowed to pass anything into this function, but it will raise a validation error if it's not the correct type or it isn't well formed.

That said, I can see an argument for dropping both isinstance checks here if we think that mypy is sufficient to catch those cases (Which is the way I'm leaning)

Now that I look at it, I don't think we get much protection from mypy here, so I'm leaning towards keeping this as Any and keeping the type checks within the function body. The way the message handling works in the Peer.handle_sub_proto_msg I don't think mypy is going to be able to enforce the appropriate types for the validate_response message.

gsalgado · 2018-07-31T06:13:00Z

p2p/exceptions.py

@@ -159,3 +159,10 @@ class NoInternalAddressMatchesDevice(BaseP2PError):
    def __init__(self, *args: Any, device_hostname: str=None, **kwargs: Any) -> None:
        super().__init__(*args, **kwargs)
        self.device_hostname = device_hostname
+
+
+class ValidationError(BaseP2PError):


I'm a bit worried about this as it's the same name of the exception from the eth package, and I believe we'll often end up writing code that catches one when in fact is the other that will be raised. Like we just saw with the OperationCancelled exception after the move to the cancel_token lib

cc @carver

how would the two of you feel about a common/shared validation library that exposed a common ValidationError exception that we could use across all of our libraries. It is one of those things that we have implemented almost everywhere.

gsalgado · 2018-07-31T06:19:22Z

p2p/peer.py

+            else:
+                future.set_result(msg)
+                self.pending_requests.pop(cmd_type)
+


We could move the whole block above into BasePeer.handle_sub_proto_msg() and then avoid having to duplicate it in both ETHPeer and LESPeer, no?

gsalgado · 2018-07-31T06:20:59Z

p2p/protocol.py

+
+    @property
+    @abstractmethod
+    def MAX_HEADERS_FETCH(self) -> int:


I find it weird to have an all-uppercase property. Any reason for that?

it's a constant so I mirrored the casing. I'm fine lower casing it.

gsalgado · 2018-07-31T06:22:50Z

p2p/protocol.py

+    @property
+    @abstractmethod
+    def MAX_HEADERS_FETCH(self) -> int:
+        pass


shouldn't this raise NotImplementedError()? We definitely don't want it to return None, right?

The abstractmethod decorator should take care of this for us, preventing us from instantiating the class if this property has not been overridden.

gsalgado · 2018-07-31T06:24:04Z

tests/p2p/test_header_request_object.py

@@ -13,6 +14,23 @@
 BLOCK_HASH = b'\x01' * 32


+class HeaderRequest(BaseHeaderRequest):
+    MAX_HEADERS_FETCH = 192


I thought you couldn't overwrite an @abstractmethod by defining a class attribute in a subclass. Am I missing something?

Brief testing shows this works as expected. The parent class with the abstract property disallows instantiation. The child class with the property set, allows instantiation and proper attribute access.

gsalgado · 2018-07-31T06:30:04Z

p2p/peer.py

+            try:
+                request.validate_response(msg)
+            except ValidationError:
+                pass


We should log this, probably as a warning even, no?

Yes, though I've been questioning the only one request of a single type in-flight per message type restriction. I'm going to keep it in place, but unless the spec says that it's disallowed, I'd like to support it eventually. Sending concurrent requests to high performing peers seems like a way to speed up sync.

Fair enough, but IIRC geth doesn't do that, so it might be worth finding out why before we consider doing it ourselves.

pipermerriam · 2018-07-31T16:26:16Z

@gsalgado PR review pushed. It includes some minor logic changes to ensure we don't end up with dangling requests in Peer.pending_requests. Mind giving another 👀

gsalgado

👍

gsalgado reviewed Jul 25, 2018

View reviewed changes

pipermerriam force-pushed the piper/full-request-to-response-await-API branch from d0879c2 to 9d69f7a Compare July 25, 2018 23:25

pipermerriam force-pushed the piper/foundation-for-request-and-response-linking branch from 2646102 to 04f3dd4 Compare July 27, 2018 18:04

pipermerriam force-pushed the piper/full-request-to-response-await-API branch 2 times, most recently from f02aaa1 to 5f6bb64 Compare July 27, 2018 22:30

pipermerriam added the PR state: WIP label Jul 27, 2018

pipermerriam mentioned this pull request Jul 28, 2018

Implement stronger linkage between requests for headers and their responses #1063

Merged

pipermerriam force-pushed the piper/full-request-to-response-await-API branch from 3996fcd to 04c002a Compare July 30, 2018 15:44

pipermerriam changed the base branch from piper/foundation-for-request-and-response-linking to master July 30, 2018 15:58

pipermerriam force-pushed the piper/full-request-to-response-await-API branch from 307d1cb to d08a4ff Compare July 30, 2018 18:34

Implement round trip awaitable API for retrieving block headers

b6afa5f

pipermerriam force-pushed the piper/full-request-to-response-await-API branch from d08a4ff to b6afa5f Compare July 30, 2018 19:25

pipermerriam changed the title ~~WIP: POC for round trip request/response API~~ Further fless out round trip request/response API Jul 31, 2018

gsalgado requested changes Jul 31, 2018

View reviewed changes

PR feedback

c36d0f1

pipermerriam changed the title ~~Further fless out round trip request/response API~~ Further flesh out round trip request/response API Jul 31, 2018

pipermerriam added comp: trinity type: feature type: refactor prio: P3 important effort: E2 days and removed PR state: WIP labels Jul 31, 2018

gsalgado approved these changes Jul 31, 2018

View reviewed changes

pipermerriam merged commit 2f8f89a into master Jul 31, 2018

This was referenced Jul 31, 2018

Start tracking peer latency #1128

Closed

POC for issue 1035 - better correlation between requests and responses. #1038

Closed

Expand round trip peer request/response API to ETH protocol (Receipts, NodeData, and BlockBodies) #1131

Closed

pipermerriam mentioned this pull request Aug 1, 2018

Expand round trip peer request/response API to requests #1132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further flesh out round trip request/response API #1089

Further flesh out round trip request/response API #1089

pipermerriam commented Jul 25, 2018 •

edited

pipermerriam commented Jul 25, 2018

gsalgado Jul 25, 2018

pipermerriam Jul 25, 2018

pipermerriam commented Jul 30, 2018

gsalgado left a comment

gsalgado Jul 31, 2018

gsalgado Jul 31, 2018

gsalgado Jul 31, 2018

gsalgado Jul 31, 2018

pipermerriam Jul 31, 2018

pipermerriam Jul 31, 2018

gsalgado Jul 31, 2018

pipermerriam Jul 31, 2018

pipermerriam Jul 31, 2018

gsalgado Jul 31, 2018

gsalgado Jul 31, 2018

pipermerriam Jul 31, 2018

gsalgado Jul 31, 2018

pipermerriam Jul 31, 2018

gsalgado Jul 31, 2018

pipermerriam Jul 31, 2018

gsalgado Jul 31, 2018

pipermerriam Jul 31, 2018

gsalgado Jul 31, 2018

pipermerriam commented Jul 31, 2018

gsalgado left a comment

Further flesh out round trip request/response API #1089

Further flesh out round trip request/response API #1089

Conversation

pipermerriam commented Jul 25, 2018 • edited

What was wrong?

How was it fixed?

Cute Animal Picture

pipermerriam commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Jul 30, 2018

gsalgado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Jul 31, 2018

gsalgado left a comment

Choose a reason for hiding this comment

pipermerriam commented Jul 25, 2018 •

edited