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

discv5: add wire protocol test vectors #123

Merged
merged 7 commits into from Nov 5, 2019
Merged

Conversation

@AgeManning
Copy link
Contributor

@AgeManning AgeManning commented Oct 29, 2019

Overview

This adds a collection of test vectors to the discv5 wire protocol. Although not entirely comprehensive, it should help new implementations conform to the specification and current implementations by passing these tests.

@AgeManning AgeManning force-pushed the test-vectors branch 2 times, most recently from ae42ec0 to 664b1c8 Oct 30, 2019
Copy link

@zilm13 zilm13 left a comment

Great job!
Started to add these tests to our code, in Harmony, and have some feedback.

discv5/discv5-wire-test-vectors.md Outdated Show resolved Hide resolved
discv5/discv5-wire-test-vectors.md Show resolved Hide resolved

#### Input Parameters

id: 1
Copy link

@zilm13 zilm13 Oct 31, 2019

Choose a reason for hiding this comment

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

I'd clarify here and in spec whether id is a number and if it's number, is it signed. Also we should have some byte bounds for its size. In our (Harmony) implementation and in Geth I see that id is just a byte array, and as some implementations prefer randomness, I'd prefer hex input here.

Copy link
Contributor Author

@AgeManning AgeManning Oct 31, 2019

Choose a reason for hiding this comment

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

Good point. In my impl I have it as a u64, which when RLP encoded gives the byte length required to fit the number. I'd also prefer hex input here and was an oversight by me here.


#### Input Parameters

tag: 0x0101010101010101010101010101010101010101010101010101010101010101
Copy link

@zilm13 zilm13 Oct 31, 2019

Choose a reason for hiding this comment

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

Personally I'd like to see more atomic input here and in next cases, so not tag, but homeNodeId and destNodeId instead, as we could fail on any step and lots of steps are missed here, especially in complex packets like AuthHeaderMessagePacket

Copy link
Contributor Author

@AgeManning AgeManning Oct 31, 2019

Choose a reason for hiding this comment

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

Agree, these are some simple quick things I had access to easily. I think we need to add a lot more tests to be comprehensive.


#### Expected Output

nodes-response-rlp: 0x04f8f80102b8f4f8f2b877f875b8401ce2991c64993d7c84c29a00bdc871917551c7d330fca2dd0d69c706596dc655448f030b98a77d4001fd46ae0112ce26d613c5a6a02a81a6223cd0c4edaa53280182696482763489736563703235366b31a103ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd3138b877f875b840d7f1c39e376297f81d7297758c64cb37dcc5c3beea9f57f7ce9695d7d5a67553417d719539d6ae4b445946de4d99e680eb8063f29485b555d45b7df16a1850130182696482763489736563703235366b31a1030e2cb74241c0c4fc8e8166f1a79a05d5b0dd95813a74b094529f317d5c39d235
Copy link

@zilm13 zilm13 Oct 31, 2019

Choose a reason for hiding this comment

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

I'm not getting the same result. Could we clarify it with Geth? Some nested rlp structures are not straight-forward to encode, so I'm not sure, who have the right result here. 0x04f8f20102f8eef875b8401ce2991c64993d7c84c29a00bdc871917551c7d330fca2dd0d69c706596dc655448f030b98a77d4001fd46ae0112ce26d613c5a6a02a81a6223cd0c4edaa53280182696482763489736563703235366b31a103ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd3138f875b840d7f1c39e376297f81d7297758c64cb37dcc5c3beea9f57f7ce9695d7d5a67553417d719539d6ae4b445946de4d99e680eb8063f29485b555d45b7df16a1850130182696482763489736563703235366b31a1030e2cb74241c0c4fc8e8166f1a79a05d5b0dd95813a74b094529f317d5c39d235 for me

Copy link
Contributor Author

@AgeManning AgeManning Oct 31, 2019

Choose a reason for hiding this comment

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

Now that you bring this up, I realise I've not tested this one with geth.
We should also add the case when no ENR is returned to check that encoding.
The spec is [... , [ENR, .. ] ] . I've built an ENR list, encoded it, then add it as a single field in the outer list.

Copy link
Contributor

@fjl fjl Nov 1, 2019

Choose a reason for hiding this comment

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

@zilm13 your version is correct (tip: use cmd/rlpdump from go-ethereum codebase to check)

04
[
  01,
  02,
  [
    [
      1ce2991c64993d7c84c29a00bdc871917551c7d330fca2dd0d69c706596dc655448f030b98a77d4001fd46ae0112ce26d613c5a6a02a81a6223cd0c4ed\
aa5328,
      01,
      "id",
      "v4",
      "secp256k1",
      03ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd3138,
    ],
    [
      d7f1c39e376297f81d7297758c64cb37dcc5c3beea9f57f7ce9695d7d5a67553417d719539d6ae4b445946de4d99e680eb8063f29485b555d45b7df16a\
185013,
      01,
      "id",
      "v4",
      "secp256k1",
      030e2cb74241c0c4fc8e8166f1a79a05d5b0dd95813a74b094529f317d5c39d235,
    ],
  ],
]

Copy link
Contributor Author

@AgeManning AgeManning Nov 3, 2019

Choose a reason for hiding this comment

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

Yep, have corrected the test vector.

@AgeManning
Copy link
Contributor Author

@AgeManning AgeManning commented Oct 31, 2019

Thanks for the feedback! I've corrected some of the easier ones, and hopefully when I get some more time, I may able to add to these.

I'll let you know when I get the chance to compare the nested ENR encodings with geth and can hopefully resolve our mismatch.

@fjl fjl changed the title Adds discv5 wire test vectors discv5: add wire protocol test vectors Nov 1, 2019
Copy link

@zilm13 zilm13 left a comment

Finished adding all other tests from your vectors, except topic-related (this part of discovery is not done in our client). There are some non-matches for me.

discv5/discv5-wire-test-vectors.md Outdated Show resolved Hide resolved
discv5/discv5-wire-test-vectors.md Show resolved Hide resolved

#### Expected Outputs

id-nonce-sig: 0x3b7b8ce9df3fbd9b6367c365622ccc82a2cb9d94219401e7b08e3194f9f835764a07caad38bf0f5a7a89501a8156bb053c880774502f5cd8a6190fbe374adc89
Copy link

@zilm13 zilm13 Nov 3, 2019

Choose a reason for hiding this comment

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

Result doesn't match for me and geth. I get 0xcf2bf743fc2273709bbc5117fd72775b0661ce1b6e9dffa01f45e2307fb138b90da16364ee7ae1705b938f6648d7725d35fe7e3f200e0ea022c1360b9b2e7385 for me and in Geth

Copy link
Contributor Author

@AgeManning AgeManning Nov 5, 2019

Choose a reason for hiding this comment

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

Sorry. This was a copy-paste error. The result I was showing was for an id-nonce consisting of all one's where the nonce here is the secret-key from the previous test vector.
The secret key is actually 33 bytes and the nonce should be 32. I've updated this test, making the nonce 32 bytes.

Copy link

@zilm13 zilm13 Nov 5, 2019

Choose a reason for hiding this comment

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

Checked updated vector: matches for me


##### Expected Output

auth-pt: 0xf84405b840f753ac31b017536bacd0d0238a1f849e741aef03b7ad5db1d4e64d7aa80689931f21e590edcf80ee32bb2f30707fec88fb62ea8fbcd65b9272e9a0175fea976bc0
Copy link

@zilm13 zilm13 Nov 3, 2019

Choose a reason for hiding this comment

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

As idNonceSig is incorrect, this one is too.
0xf84405b840f753ac31b017536bacd0d0238a1f849e741aef03b7ad5db1d4e64d7aa80689931f21e590edcf80ee32bb2f30707fec88fb62ea8fbcd65b9272e9a0175fea976b80 for me

Copy link
Contributor

@fjl fjl Nov 4, 2019

Choose a reason for hiding this comment

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

Signature value depends on how signing is implemented. The output will match only if your library implements deterministic signatures, but that's not a requirement.

Copy link
Contributor Author

@AgeManning AgeManning Nov 5, 2019

Choose a reason for hiding this comment

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

This is not an error due to nonce-sig mismatch. In fact, we agree on the nonce sig. The difference between our results is the rlp encoding of an empty ENR. Your result is encoding an empty enr as an empty string 0x80, whereas I (and I assume geth) are encoding it as an empty list [] 0xc0. I assume the empty list is the correct encoding.

Copy link

@zilm13 zilm13 Nov 5, 2019

Choose a reason for hiding this comment

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

@AgeManning nice catch, fixed. Everything matches for me since now.

@fjl fjl merged commit fa37304 into ethereum:master Nov 5, 2019
1 check passed
@fjl
Copy link
Contributor

@fjl fjl commented Nov 5, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants