Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor discv5 typing and naming changes #905

Merged
merged 6 commits into from Aug 13, 2019

Conversation

@jannikluhn
Copy link
Contributor

commented Aug 12, 2019

What was wrong?

  • in the discv5 code we use bytes where more specific types would be appropriate
  • the parameter initiator_key for preparing or decrypting auth tag is incorrectly named most of the time (it's only true for auth header packets which are sent only by the handshake initiator -- in all other cases it could also be the recipient)
  • sender and receiver in in incoming/outgoing packets/datagrams refer to the peer's endpoint, even though usually we'll want to refer to peer's by their node id

How was it fixed?

  • Add IDNonce, NodeID, and Tag type
  • rename initiator_key to key for all auth tag encryption and decryption functions
  • rename sender/receiver to sender/receiver_endpoint

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

jannikluhn added some commits Aug 12, 2019

Rename sender/receiver to sender/receiver_endpoint
This distinguishes from the peer's node id which is a more appropriate
representation of the peer.
Rename initiator_key argument to just key
initiator_key is only accurate for auth header packets as they are sent
exclusively by the handshake initiator.

@jannikluhn jannikluhn force-pushed the jannikluhn:discv5-typing-revamp branch from f3b3a26 to d5e812e Aug 12, 2019

@@ -33,28 +33,28 @@
# Data structures
#
class Endpoint(NamedTuple):
ip_address: bytes
ip_address: str

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Aug 12, 2019

Member

Curious if you looked into using p2p.kademlia.Address for this and whether we could re-use that instead of using Endpoint?

This comment has been minimized.

Copy link
@jannikluhn

jannikluhn Aug 13, 2019

Author Contributor

I haven't until now, but I'd like to keep Endpoint for two reasons:

  • We don't need the TCP port in Address
  • I like the immutability of Endpoint, so we don't have to think if we reuse it (e.g. when creating the response to an incoming message)
datagram, (ip_address, port) = await socket.recvfrom(DATAGRAM_BUFFER_SIZE)
logger.debug(f"Received {len(datagram)} bytes from {(ip_address, port)}")
incoming_datagram = IncomingDatagram(datagram, Endpoint(ip_address, port))
datagram, endpoint = await socket.recvfrom(DATAGRAM_BUFFER_SIZE)

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Aug 12, 2019

Member

looks like endpoint here will just be a regular tuple instead of Endpoint. Is this intentional?

@jannikluhn jannikluhn merged commit 99c3738 into ethereum:master Aug 13, 2019

38 checks passed

ci/circleci: docker-image-build-test Your tests passed on CircleCI!
Details
ci/circleci: py36-docs Your tests passed on CircleCI!
Details
ci/circleci: py36-eth1-core Your tests passed on CircleCI!
Details
ci/circleci: py36-eth1-plugins Your tests passed on CircleCI!
Details
ci/circleci: py36-eth2-core Your tests passed on CircleCI!
Details
ci/circleci: py36-eth2-fixtures Your tests passed on CircleCI!
Details
ci/circleci: py36-eth2-integration Your tests passed on CircleCI!
Details
ci/circleci: py36-eth2-plugins Your tests passed on CircleCI!
Details
ci/circleci: py36-eth2-utils Your tests passed on CircleCI!
Details
ci/circleci: py36-integration Your tests passed on CircleCI!
Details
ci/circleci: py36-lightchain_integration Your tests passed on CircleCI!
Details
ci/circleci: py36-lint Your tests passed on CircleCI!
Details
ci/circleci: py36-long_run_integration Your tests passed on CircleCI!
Details
ci/circleci: py36-p2p Your tests passed on CircleCI!
Details
ci/circleci: py36-p2p-trio Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-blockchain Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-byzantium Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-constantinople Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-frontier Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-homestead Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-petersburg Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-spurious_dragon Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-tangerine_whistle Your tests passed on CircleCI!
Details
ci/circleci: py36-wheel-cli Your tests passed on CircleCI!
Details
ci/circleci: py37-eth1-core Your tests passed on CircleCI!
Details
ci/circleci: py37-eth1-plugins Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-core Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-fixtures Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-integration Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-plugins Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-utils Your tests passed on CircleCI!
Details
ci/circleci: py37-libp2p Your tests passed on CircleCI!
Details
ci/circleci: py37-lint Your tests passed on CircleCI!
Details
ci/circleci: py37-p2p Your tests passed on CircleCI!
Details
ci/circleci: py37-rpc-state-quadratic Your tests passed on CircleCI!
Details
ci/circleci: py37-rpc-state-sstore Your tests passed on CircleCI!
Details
ci/circleci: py37-rpc-state-zero_knowledge Your tests passed on CircleCI!
Details
ci/circleci: py37-wheel-cli Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.