-
Notifications
You must be signed in to change notification settings - Fork 146
Some ETH Protocol changes to prepare the room for eth/66 #1676
Some ETH Protocol changes to prepare the room for eth/66 #1676
Conversation
551123e
to
72c9855
Compare
72c9855
to
1c7f8b2
Compare
1c7f8b2
to
1a66a5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one question
return ( | ||
ETHV63Handshaker(handshake_v63_params), | ||
ETHHandshaker(handshake_params, head.block_number, fork_blocks), | ||
ETHHandshaker(handshake_params, head.block_number, fork_blocks, highest_eth_protocol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we return v63 because that's the oldest version and we want to try only the oldest and newest? Maybe add a comment to that effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ETHV63Handshaker
is only for V63
as it uses a different Status
command. The ETHHandshaker
is used across all versions above V63
. However, it takes in highest_eth_protocol
to be able to tell the other side our highest supported protocol version. But even if we report V65
as the highest version and we are faced with another peer that only supports V64
, we'll end up doing the handshake with ETHHandshaker
in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to add a comment in the source.
What was wrong?
This is a fall out from #1672 which implements the upcoming eth/66 protocol. This PR contains only a few cleanups that should make #1672 cleaner and easier to read.
How was it fixed?
NormalizerAPI
API to makenormalize_result
non-static and hence allow better code reuse with the introduction of aDefaultNormalizer
. To elaborate, I believe the only reason why we even have theNormalizerAPI
is theis_normalization_slow
property. If not for that, we could simply allow a function be used instead of implementingNormalizerAPI
. But considering that we need to implement classes for theNormalizerAPI
, it feels like an extra burden that we effectively dissallow constructor usage by forcingnormalize_result
to be static. This leads down a road of having to implement classes that are mostly copy and paste with just tiny differences.Making
normalize_result
non-static gives us more flexibility and allows us to use one normalizer for common scenarios e.g:Renamed protocol types that were not versioned and which become obsolete in ETH/66 to contain a
V65
version flag.Made sure some of our tests operate on all the different ETH protocols that we support. The biggest change here is that the
ETHPeerFactory
will use thesupported_sub_protocols
property of the peer to set the highest protocol on theETHHandshaker
.Removed some intermediate variables in
eth/commands.py
as I felt they are making the code harder to parse when some of these commands become aV66
counterpart (not part of this PR but can be seen in WIP: Implement eth/66 #1672)To-Do
Cute Animal Picture