Implement eth/64 protocol #1543
Conversation
Yeah, I thought about that too but ended up abandoning the thought because:
|
cburgdorf
force-pushed
the
christoph/feat/eth64
branch
13 times, most recently
from
65bccfc to
dcf264b
Compare
|
Alright, this should be ready for review. |
|
I have reassigned this to you @gsalgado because I think @pipermerriam is very busy right now. |
| return chain | ||
|
|
||
|
|
||
| @pytest.fixture(params=(ETHV63PeerPairFactory, ETHPeerPairFactory)) |
There was a problem hiding this comment.
So, this causes all fixtures that use this one to be parameterized as well, I guess?
There was a problem hiding this comment.
Correct! I tried to keep the tests structure similar to the LESV1 vs LESV2 tests that use the same pattern?
tests/core/p2p-proto/test_eth_api.py
Outdated
| ) | ||
| from trinity.tools.factories import ( | ||
| BlockHashFactory, | ||
| ChainContextFactory, | ||
| ETHPeerPairFactory, | ||
| ) | ||
| from trinity.tools.factories.eth.proto import ETHV63PeerPairFactory |
There was a problem hiding this comment.
Why is this factory not imported from the same place as ETHPeerPairFactory?
There was a problem hiding this comment.
This is me testing out PyCharm Professional and relying on its auto import feature 😅 I'll fix that.
| self.protocol.send(StatusV63(payload)) | ||
|
|
||
|
|
||
| class ETHAPI(BaseETHAPI): |
There was a problem hiding this comment.
Why did you chose not to include the version in the class name here?
There was a problem hiding this comment.
I have only put in the version numbers in classes that aren't on the latest version.
E.g.
ETHV63API
ETHProtocolV63
ETHV63PeerFactory
vs
ETHAPI
ETHProtocol
ETHPeerFactory
| raise WrongForkIDFailure( | ||
| f"{multiplexer.remote} network " | ||
| f"({receipt.handshake_params.fork_id}) is incompatible to ours " | ||
| f"({self.handshake_params.fork_id}), disconnecting" |
There was a problem hiding this comment.
Maybe include the actual exception msg here as well, as it would give extra details about where exactly the incompatibility lies.
trinity/protocol/eth/handshaker.py
Outdated
| ) | ||
| except BaseForkIDValidationError: | ||
| raise WrongForkIDFailure( | ||
| f"{multiplexer.remote} network " |
cburgdorf
force-pushed
the
christoph/feat/eth64
branch
from
dcf264b to
0a5a9f3
Compare
|
Just ran a last live test and had this error blasting out. I had live tested this before with a mix of So I need to hold of merging this until I figured out what's wrong here. |
|
@gsalgado Mind taking a look at the most recent commit that I added? This deals with the |
p2p/multiplexer.py
Outdated
| try: | ||
| cmd = command_type.decode(msg, msg_proto.snappy_support) | ||
| except rlp.exceptions.DeserializationError: | ||
| raise MalformedMessage(f"Failed to decode {msg} for {command_type}") |
There was a problem hiding this comment.
Maybe use raise MalformedMessage(...) from err, to include the original error?
cburgdorf
force-pushed
the
christoph/feat/eth64
branch
from
732c53f to
0f1ecd0
Compare
What was wrong?
Trinity is currently on
eth/63whereas Geth has implementedeth/64andeth/65already. We are pushing for the inclusion of request ids aseth/66.Before we add an implementation for the proposed
eth/66protocol, we should first implementeth/64andeth/65.How was it fixed?
This implements the
eth/64protocol according to EIP 2124. The implementation follows the general principle of how LESV1, LESV2 are implemented side by side.This also adds a tiny cleanup to combine three different places with the same
if/elseconstruct to choose the concrete API version into a single helper function.Real life testing also demonstrates we are having
eth/64peers alongsideeth/63peers in the pool.To-Do
Cute Animal Picture