[WIP] Extend round trip API to GetBlockBodies LES protocol command #1372

hoi · 2018-10-07T23:36:47Z

What was wrong?

The following LES command pair does not have a round trip API available to it.

GetBlockBodies -> BlockBodies

How was it fixed?

Use and/or extend the existing Handler/Manager pattern to implement round trip APIs for these command pairs. At minimum each of these should be tested at the Request level and the Peer level. Feel free to borrow patterns and test cases from the existing tests.

Cute Animal Picture

pipermerriam · 2018-10-08T16:41:40Z

trinity/protocol/les/exchanges.py

@@ -11,20 +11,27 @@
 from trinity.protocol.common.exchanges import (
    BaseExchange,
 )
+# Q: What is the order to imports?  and why sometimes new line between imports and sometimes not?


ordering is:

standard library

3rd party

local library

local modules (these are all from . import style imports

Within each section we alphabetize and normally do whitespace breaks between libraries.

Within each of those sections

ah, great! I wonder if my ide can set this order. I've done it in the past for my IntelliJ.

pipermerriam · 2018-10-08T16:44:32Z

trinity/protocol/les/normalizers.py


 TResult = TypeVar('TResult')
 LESNormalizer = BaseNormalizer[Dict[str, Any], TResult]


+# Q: Shouldn't this be named GetBlockHeadersNormalizer?


No, because it normalizes the response which is a Blockheaders message (responding to a GetBlockHeaders request).

ah! makes sense

pipermerriam · 2018-10-08T16:45:25Z

trinity/protocol/les/trackers.py

+]
+
+
+# Q: Where can I find the signature for this class?


I don't know what you mean by signature

I'm not sure either, feel free to hop on chat, since it may require some back-and-forth to resolve.

ah, i just meant the input arguments

carver

There's probably room to extract some repeated logic into new methods in trinity/protocol/common, but I wouldn't worry about that refactor until all the tests pass.

carver · 2018-10-08T17:58:28Z

trinity/protocol/les/handlers.py



-class LESExchangeHandler(BaseChainExchangeHandler):
+class ETHExchangeHandler(BaseChainExchangeHandler):


This should stay LES, to indicate that it's for the light protocol.

ah, you're right. didn't mean to change this, actually

carver · 2018-10-08T18:10:07Z

trinity/protocol/les/normalizers.py

 class BlockHeadersNormalizer(LESNormalizer[Tuple[BlockHeader, ...]]):
    @staticmethod
    def normalize_result(message: Dict[str, Any]) -> Tuple[BlockHeader, ...]:
        result = message['headers']
        return result
+
+
+class GetBlockBodiesNormalizer(LESNormalizer[Tuple[BlockBody, ...]]):


Yeah, so for the same reason, the Get should be dropped here. This normalizer only acts on the response value, which is sent with the BlockBodies command.

carver · 2018-10-08T18:16:43Z

trinity/protocol/les/normalizers.py

+    @staticmethod
+    def normalize_result(message: Dict[str, Any]) -> Tuple[BlockBody, ...]:
+        result = message['bodies']
+        return result


I'm fairly sure that we'll want to return block body bundles here, similar to how the eth version does:

py-evm/trinity/protocol/eth/normalizers.py

Lines 48 to 57 in 9ba2c1f

uncles_hashes = tuple(map(

compose(keccak, rlp.encode),

tuple(body.uncles for body in msg)

))

transaction_roots_and_trie_data = tuple(map(

make_trie_root_and_nodes,

tuple(body.transactions for body in msg)

))

body_bundles = tuple(zip(msg, transaction_roots_and_trie_data, uncles_hashes))

but everywhere msg is in this snippet, using result from the PR.

This is going to change the type signature of the the exchange, and so will affect the validator, etc.

carver · 2018-10-08T18:20:50Z

trinity/protocol/les/trackers.py

+]
+
+
+# Q: Where can I find the signature for this class?


I'm not sure either, feel free to hop on chat, since it may require some back-and-forth to resolve.

carver · 2018-10-08T18:24:56Z

Also, it's probably best to reduce the scope on each PR to just one exchange, in this case: GetBlockBodiesExchange. (I only mention it because the PR summary includes the list of all the different exchanges)

carver · 2018-10-08T18:26:01Z

... and excellent cute animal picture 💯

hoi · 2018-10-09T22:30:40Z

Also, it's probably best to reduce the scope on each PR to just one exchange, in this case: GetBlockBodiesExchange. (I only mention it because the PR summary includes the list of all the different exchanges)

I agree. That was my intention too. Thanks!

carver · 2018-11-12T20:15:20Z

Here's a suggestion for how to run a local smoke test to make sure everything is working as expected. Use the new API you created to replace this code:

py-evm/trinity/sync/light/service.py

Lines 170 to 172 in 7d0f188

    
           request_id = gen_request_id() 
        
           peer.sub_proto.send_get_block_bodies([block_hash], request_id) 
        
           reply = await self._wait_for_reply(request_id)

With (roughly):

 block_body_bundles = peer.requests.get_block_bodies((block_hash, ))

Then you'll extract the block and return it.

After making that change, you should be able to run a light trinity node and use web3.py to call w3.eth.getBlock('latest') and get an expected block. (probably on ropsten because mainnet light servers are scarce) If that still works as expected, then the smoke test works well.

Note that the above change should actually be made permanently in trinity. It doesn't have to be made in this PR, but it would be okay to if you want.

carver · 2018-12-17T21:18:34Z

Go ahead and rebase this, fix lint, and force-push 👍

pipermerriam · 2019-01-04T18:39:53Z

A kind reminder that this needs to be migrated to the new https://github.com/ethereum/trinity repository. See #1667 for detailed instructions.

hoi changed the title ~~Extend round trip API to remaining LES protocol commands~~ [WIP] Extend round trip API to remaining LES protocol commands Oct 7, 2018

pipermerriam reviewed Oct 8, 2018

View reviewed changes

carver reviewed Oct 8, 2018

View reviewed changes

hoi force-pushed the hil_api branch from 353357c to 07c2eb1 Compare October 31, 2018 05:57

hoi changed the title ~~[WIP] Extend round trip API to remaining LES protocol commands~~ [WIP] Extend round trip API to GetBlockBodies LES protocol command Oct 31, 2018

hoi force-pushed the hil_api branch 2 times, most recently from 133b687 to 12fc090 Compare October 31, 2018 06:06

pipermerriam assigned carver Nov 12, 2018

hoi and others added 4 commits November 29, 2018 19:56

Extend round trip API to remaining LES protocol commands

b0cc9b3

First version of code changes

108e158

Fixing lint test

4c5e04e

Utilized the API

bc5c220

hoi force-pushed the hil_api branch from 43bf917 to bc5c220 Compare November 29, 2018 20:33

pipermerriam added the TEMP: Migrate to Trinity Repo label Dec 20, 2018

hoi closed this Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Extend round trip API to GetBlockBodies LES protocol command #1372

[WIP] Extend round trip API to GetBlockBodies LES protocol command #1372

hoi commented Oct 7, 2018 •

edited

pipermerriam Oct 8, 2018

hoi Oct 9, 2018

pipermerriam Oct 8, 2018

hoi Oct 9, 2018

pipermerriam Oct 8, 2018

carver Oct 8, 2018

hoi Oct 9, 2018 •

edited

carver left a comment

carver Oct 8, 2018

hoi Oct 9, 2018

carver Oct 8, 2018

hoi Oct 9, 2018

carver Oct 8, 2018

carver Oct 8, 2018

carver commented Oct 8, 2018 •

edited

carver commented Oct 8, 2018

hoi commented Oct 9, 2018

carver commented Nov 12, 2018

carver commented Dec 17, 2018

pipermerriam commented Jan 4, 2019



		class LESExchangeHandler(BaseChainExchangeHandler):
		class ETHExchangeHandler(BaseChainExchangeHandler):

	uncles_hashes = tuple(map(
	compose(keccak, rlp.encode),
	tuple(body.uncles for body in msg)
	))
	transaction_roots_and_trie_data = tuple(map(
	make_trie_root_and_nodes,
	tuple(body.transactions for body in msg)
	))

	body_bundles = tuple(zip(msg, transaction_roots_and_trie_data, uncles_hashes))

[WIP] Extend round trip API to GetBlockBodies LES protocol command #1372

[WIP] Extend round trip API to GetBlockBodies LES protocol command #1372

Conversation

hoi commented Oct 7, 2018 • edited

What was wrong?

How was it fixed?

Cute Animal Picture

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

hoi Oct 9, 2018 • edited

Choose a reason for hiding this comment

carver 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

carver commented Oct 8, 2018 • edited

carver commented Oct 8, 2018

hoi commented Oct 9, 2018

carver commented Nov 12, 2018

carver commented Dec 17, 2018

pipermerriam commented Jan 4, 2019

hoi commented Oct 7, 2018 •

edited

hoi Oct 9, 2018 •

edited

carver commented Oct 8, 2018 •

edited