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

cmd/devp2p: add eth/66 protocol test suite #22286

Closed
fjl opened this issue Feb 8, 2021 · 11 comments
Closed

cmd/devp2p: add eth/66 protocol test suite #22286

fjl opened this issue Feb 8, 2021 · 11 comments
Assignees

Comments

@fjl
Copy link
Contributor

fjl commented Feb 8, 2021

After #22241 is merged, we should extend the protocol test suite to cover the new
message types in eth/66. What needs to be done:

  • Rename existing tests to add 65 in name
  • Duplicate all existing tests with 66 name and change messages to use eth/66 request ID
  • Add checks that the request ID of responses match the requests.
  • Add new test that sends two concurrent requests.

Since we now have eth message definitions in package eth/protocols/eth, we might be
able to remove the message type definitions in package ethtest and use the 'real' message
types instead.

@fjl fjl assigned fjl and renaynay and unassigned fjl Feb 8, 2021
@renaynay
Copy link
Contributor

@fjl re:

Add new test that sends two concurrent requests.

can I just send two simultaneous GetBlockHeaders requests and make sure both the request ID and the headers are valid?

@fjl
Copy link
Contributor Author

fjl commented Feb 19, 2021

Yeah, that's kind of the idea. Basically, the peer needs to be able to deal with concurrent requests in some way.

@fjl
Copy link
Contributor Author

fjl commented Mar 4, 2021

Some follow-up ideas:

  • add test that checks announcement of block hash
  • add test that checks announcement of tx hash
  • add test that announces existing block in chain (should not propagate)
  • add test that announces existing tx in chain (should not propagate)

@fjl
Copy link
Contributor Author

fjl commented Mar 4, 2021

I checked the results on hive and it looks like the 66 tests continue even if the remote node does not support 66.

@fjl
Copy link
Contributor Author

fjl commented Mar 4, 2021

one more idea: test the behavior of eth/65 where a peer requests 2000 tx, but only 1000 are sent (because of response limit). if some txs are missing on the remote node, then the list should be truncated in a certain way.

@renaynay
Copy link
Contributor

renaynay commented Mar 5, 2021

I checked the results on hive and it looks like the 66 tests continue even if the remote node does not support 66.

Okay, will add a check for highest protocol supported by node to proceed with 66 tests.

@renaynay
Copy link
Contributor

renaynay commented Mar 9, 2021

Some follow-up ideas:

  • add test that checks announcement of block hash
  • add test that checks announcement of tx hash

for block hash -- doesn't TestBroadcast cover this?
same for tx hash - there's already a test for this, unless I'm not understanding correctly.

@fjl
Copy link
Contributor Author

fjl commented Mar 11, 2021

doesn't TestBroadcast cover this?

There are two ways to announce a block: with NewBlock, sending the complete block; and with NewBlockHashes, sending only the block hash. In the latter case, the node should request the full block a couple seconds later.

@renaynay
Copy link
Contributor

doesn't TestBroadcast cover this?

There are two ways to announce a block: with NewBlock, sending the complete block; and with NewBlockHashes, sending only the block hash. In the latter case, the node should request the full block a couple seconds later.

Hmm, looks like after receiving the NewBlockHashes announcement, the node requests block headers rather than the full block.

@holiman
Copy link
Contributor

holiman commented Mar 25, 2021

Can this be closed now?

@renaynay
Copy link
Contributor

It's kind of an ongoing effort, but I'd say the base is in place so likely yes.

@fjl fjl closed this as completed May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants