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 eth66 test suite #22363

Merged
merged 9 commits into from
Feb 25, 2021
Merged

Conversation

renaynay
Copy link
Contributor

@renaynay renaynay commented Feb 22, 2021

This PR adds the eth66 test suite to the devp2p rlpx test suite, duplicating the tests found in the eth test suite and also adding a few eth66-specific tests.

@renaynay renaynay marked this pull request as ready for review February 22, 2021 15:54
@renaynay renaynay requested review from fjl and holiman and removed request for fjl February 22, 2021 15:55
@holiman
Copy link
Contributor

holiman commented Feb 22, 2021

How does the eth66-test command work? What is the prerequisite for running it? What happens if I just point it at an eth66 mainnet node?

@renaynay
Copy link
Contributor Author

@holiman I added some docs to the README.md. It is not possible to run this test suite against a mainnet node because the node has to be initialized with the chain and genesis file provided in the testdata/ directory in ethtest.

@holiman
Copy link
Contributor

holiman commented Feb 23, 2021

One little nitpick: it might be nice if the command exits orderly on errors such as not finding files:

panic: open cmd/devp2p/internal/ethtest/testdata/genesis.json: no such file or directory

goroutine 1 [running]:
github.com/ethereum/go-ethereum/cmd/devp2p/internal/ethtest.NewSuite(0xc0004629a0, 0x7ffdb1c732ea, 0x24, 0x7ffdb1c7330f, 0x31, 0x734ecd)
	/home/user/go/src/github.com/ethereum/go-ethereum/cmd/devp2p/internal/ethtest/suite.go:59 +0x25d
main.rlpxEth66Test(0xc000332c60, 0x0, 0xc000332c60)
	/home/user/go/src/github.com/ethereum/go-ethereum/cmd/devp2p/rlpxcmd.go:117 +0xa5
gopkg.in/urfave/cli%2ev1.HandleAction(0xd5f620, 0xeea2a8, 0xc000332c60, 0xc0002ede00, 0x0)
	/home/user/go/pkg/mod/gopkg.in/urfave/[email protected]/app.go:490 +0x82
gopkg.in/urfave/cli%2ev1.Command.Run(0xead8e5, 0xa, 0x0, 0x0, 0x0, 0x0, 0x0, 0xecdd01, 0x28, 0x0, ...)
	/home/user/go/pkg/mod/gopkg.in/urfave/[email protected]/command.go:210 +0x9fb
gopkg.in/urfave/cli%2ev1.(*App).RunAsSubcommand(0xc00032aea0, 0xc0003329a0, 0x0, 0x0)
	/home/user/go/pkg/mod/gopkg.in/urfave/[email protected]/app.go:379 +0x8d4
gopkg.in/urfave/cli%2ev1.Command.startApp(0xea7255, 0x4, 0x0, 0x0, 0x0, 0x0, 0x0, 0xeb0313, 0xd, 0x0, ...)
	/home/user/go/pkg/mod/gopkg.in/urfave/[email protected]/command.go:298 +0x85f
gopkg.in/urfave/cli%2ev1.Command.Run(0xea7255, 0x4, 0x0, 0x0, 0x0, 0x0, 0x0, 0xeb0313, 0xd, 0x0, ...)
	/home/user/go/pkg/mod/gopkg.in/urfave/[email protected]/command.go:98 +0x11e8
gopkg.in/urfave/cli%2ev1.(*App).Run(0x14a7f60, 0xc0000321e0, 0x6, 0x6, 0x0, 0x0)
	/home/user/go/pkg/mod/gopkg.in/urfave/[email protected]/app.go:255 +0x768
main.main()
	/home/user/go/src/github.com/ethereum/go-ethereum/cmd/devp2p/main.go:71 +0x51

Otherwise LGTM

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

One small suggestion

Amount: 2,
},
}
req2 := &eth.GetBlockHeadersPacket66{
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it would be good to make the two requests non-identical? If e.g. we request one header in one of the requests, and 3 headers in the other, and expect one response to have one and one to have 3.
That way we check that the remote part does indeed not care about the request id, and thus does not mix up the two requests.

@fjl
Copy link
Contributor

fjl commented Feb 23, 2021

Why is this a separate command? We should just run these tests as part of eth-test.

}
if tx.Hash() != txHashes[len(txHashes)-1] {
t.Fatalf("wrong announcement received, wanted %v got %v", tx, txHashes)
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

How can this possibly ever succeed? It will loop forever, or, in this case, eventually error out on when the remote peer don't send any more data:

unexpected message in sendSuccessfulTx: (*ethtest.Error)(could not read from connection: read tcp 127.0.0.1:45992->127.0.0.1:30303: i/o timeout)

You never exit the loop otherwise.

Copy link
Contributor

@holiman holiman Feb 24, 2021

Choose a reason for hiding this comment

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

So, what it's actually failing on is:

wrong announcement received, wanted 
0x7d58c33ec2867fd7f9631b31258468d8b1b9cf71f57672e12eacc4e5a63ed84a got 
[0x7d58c33ec2867fd7f9631b31258468d8b1b9cf71f57672e12eacc4e5a63ed84a 0x469d063bf70cc83df14ce191337642698da263d47d8003a7df581cdeadc2e601]

It does announce 0x7d58c33ec2867fd7f9631b31258468d8b1b9cf71f57672e12eacc4e5a63ed84a, just not in the particular location where the test is looking :)

@karalabe karalabe added this to the 1.10.0 milestone Feb 25, 2021
@fjl fjl merged commit de9465f into ethereum:master Feb 25, 2021
@renaynay renaynay deleted the eth66-test-suite branch February 25, 2021 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants