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

Discvv5.1 - the latest spec updates #76

Merged
merged 99 commits into from
Oct 13, 2020

Conversation

Nashatyrev
Copy link
Contributor

@Nashatyrev Nashatyrev commented Oct 6, 2020

PR Description

Adopted the latest changes to Discv5.1 spec PR: ethereum/devp2p#157 starting from commit d19fbe7b

  • Packet classes: fields reorg
  • Message encrypt/decrypt now requires maskingIV as a part of AES/GCM AD. That's why we need now to pull RawMessage.maskingIV till message decryption. This is done via additional Envelope field MASKING_IV
  • The whole WhoAreYou message is now mixed to id-signature input. We need to remember the challenge of outbound WhoAreYouPacket in the NodeSession to validate the next inbound HandshakePacket
  • WhoAreYou has no more src-node-id thus we need to find session via original-nonce (as it was in v5.0 with authTag)
  • Adopted the latest test vectors
  • Add a bit more type-safety to Envelope class

@Nashatyrev Nashatyrev changed the title Feature/v5.1 last updates 1 Discvv5.1 - the latest spec updates Oct 6, 2020
@fjl
Copy link

fjl commented Oct 7, 2020

This is passing almost all of my tests except for PingHandshakeInterrupted.

In this test, the node receives PING and should respond with WHOAREYOU.
The test then sends another message packet, ignoring the challenge.
The node should respond with another WHOAREYOU for this second
message packet.

We added this to the spec a couple days ago. Would be nice if you could fix it before merging.

(2997d331417a5899) >> PING/v5
(2997d331417a5899) << WHOAREYOU/v5
(2997d331417a5899) >> PING/v5
(2997d331417a5899) << PONG/v5
-- OK Ping (340.6ms)
(d9ed6382e5798e98) >> PING/v5
(d9ed6382e5798e98) << WHOAREYOU/v5
(d9ed6382e5798e98) >> PING/v5
-- OK PingLargeRequestID (318.4ms)
(4ebd3ecca8e66336) >> PING/v5
(4ebd3ecca8e66336) << WHOAREYOU/v5
(4ebd3ecca8e66336) >> PING/v5
(4ebd3ecca8e66336) << PONG/v5
(4ebd3ecca8e66336) >> PING/v5
(4ebd3ecca8e66336) << WHOAREYOU/v5
got WHOAREYOU for new session as expected
(4ebd3ecca8e66336) >> PING/v5
(4ebd3ecca8e66336) << PONG/v5
(4ebd3ecca8e66336) >> PING/v5
(4ebd3ecca8e66336) << WHOAREYOU/v5
got WHOAREYOU for new session as expected
-- OK PingMultiIP (61.2ms)
(02addb7ed1f805fc) >> PING/v5
(02addb7ed1f805fc) << WHOAREYOU/v5
got WHOAREYOU for PING
(02addb7ed1f805fc) >> PING/v5
expected WHOAREYOU, got read udp 192.168.3.21:57061: i/o timeout
-- FAIL PingHandshakeInterrupted (313.6ms)
(e1507f192949d7e8) >> TALKREQ/v5
(e1507f192949d7e8) << WHOAREYOU/v5
(e1507f192949d7e8) >> TALKREQ/v5
(e1507f192949d7e8) << TALKRESP/v5
(e1507f192949d7e8) >> TALKREQ/v5
(e1507f192949d7e8) << TALKRESP/v5
-- OK TalkRequest (58.6ms)
(17ba7d15e1069974) >> FINDNODE/v5
(17ba7d15e1069974) << WHOAREYOU/v5
(17ba7d15e1069974) >> FINDNODE/v5
(17ba7d15e1069974) << NODES/v5
-- OK FindnodeZeroDistance (64.8ms)
(9f5befb513dcd7c4) >> PING/v5
(94d089665a1f11dc) >> PING/v5
(66418e05cd479efa) >> PING/v5
(719166b5c811eed9) >> PING/v5
(ee2b7948e16f629f) >> PING/v5
(9f5befb513dcd7c4) << WHOAREYOU/v5
(9f5befb513dcd7c4) >> PING/v5
(94d089665a1f11dc) << WHOAREYOU/v5
(94d089665a1f11dc) >> PING/v5
(66418e05cd479efa) << WHOAREYOU/v5
(66418e05cd479efa) >> PING/v5
(719166b5c811eed9) << WHOAREYOU/v5
(719166b5c811eed9) >> PING/v5
(ee2b7948e16f629f) << WHOAREYOU/v5
(ee2b7948e16f629f) >> PING/v5
(9f5befb513dcd7c4) << PONG/v5
(94d089665a1f11dc) << PONG/v5
(66418e05cd479efa) << PONG/v5
(719166b5c811eed9) << PONG/v5
(ee2b7948e16f629f) << PONG/v5
(9f5befb513dcd7c4) << PING/v5
(9f5befb513dcd7c4) >> PONG/v5
bystander node 9f5befb513dcd7c41a5db3ce7d4d3a2bc69d650d2990182f17a4f2dd4959cd9c added to remote table
(94d089665a1f11dc) << PING/v5
(94d089665a1f11dc) >> PONG/v5
bystander node 94d089665a1f11dcc71e159d20ec7e5dd851d0ed7967eb09a5e5345b1c1416b3 added to remote table
(719166b5c811eed9) << PING/v5
(719166b5c811eed9) >> PONG/v5
bystander node 719166b5c811eed9bff72a30ca5e69f2a6be6a03ec123f00805b723fc0c4456a added to remote table
(66418e05cd479efa) << PING/v5
(66418e05cd479efa) >> PONG/v5
bystander node 66418e05cd479efa1371499d2f0fd9fb0ba0e5bd00e25232b854a89bb01ad1ef added to remote table
(ee2b7948e16f629f) << PING/v5
(ee2b7948e16f629f) >> PONG/v5
bystander node ee2b7948e16f629f812297db6d6069a64e0da11b4cc153b134161cd46ba77400 added to remote table
all 5 bystander nodes were added
(1320b97164e34053) >> FINDNODE/v5
(1320b97164e34053) << WHOAREYOU/v5
(1320b97164e34053) >> FINDNODE/v5
(1320b97164e34053) << NODES/v5
(1320b97164e34053) << NODES/v5
remote returned 5 nodes for distance list [254 256 255]
all 5 expected nodes were returned
(ee2b7948e16f629f) shutting down: read udp 192.168.3.21:34121: use of closed network connection
(719166b5c811eed9) shutting down: read udp 192.168.3.21:53478: use of closed network connection
(66418e05cd479efa) shutting down: read udp 192.168.3.21:60398: use of closed network connection
(94d089665a1f11dc) shutting down: read udp 192.168.3.21:33700: use of closed network connection
(9f5befb513dcd7c4) shutting down: read udp 192.168.3.21:48154: use of closed network connection
-- OK FindnodeResults (1.5104s)
6/7 tests passed.

@Nashatyrev
Copy link
Contributor Author

@fjl thanks for testing! Results look surprisingly good 👍
Will try reproducing the failed case locally and fix it

@Nashatyrev
Copy link
Contributor Author

@fjl fixed the issue with latest commits

Copy link

@mbaxter mbaxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Nashatyrev
Copy link
Contributor Author

@fjl turned out that ephemeral public key was in uncompressed form. Curious why your tests don't catch this

@Nashatyrev Nashatyrev merged commit 82b1630 into Consensys:master Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants