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: ethtest suite runnable as unit test #22698

Merged
merged 19 commits into from
Apr 23, 2021

Conversation

renaynay
Copy link
Contributor

@renaynay renaynay commented Apr 19, 2021

This PR adds a test file to allow the eth test suite to be runnable as independent unit tests for easier testing.

TODO:

  • allow logging from test suite
  • when running all tests, reuse suite to avoid multiple geth setups

@renaynay renaynay force-pushed the ethtest-unit-tests branch 2 times, most recently from cc004df to 1991d65 Compare April 19, 2021 19:00
@renaynay renaynay marked this pull request as ready for review April 19, 2021 19:09
@renaynay renaynay changed the title cmd/devp2p: ethtest suite runnable as unit test [WIP] cmd/devp2p: ethtest suite runnable as unit test Apr 20, 2021
@renaynay renaynay changed the title [WIP] cmd/devp2p: ethtest suite runnable as unit test cmd/devp2p: ethtest suite runnable as unit test Apr 20, 2021
@renaynay renaynay requested a review from fjl April 21, 2021 12:57
cmd/devp2p/internal/ethtest/suite_test.go Outdated Show resolved Hide resolved
cmd/devp2p/internal/ethtest/suite_test.go Outdated Show resolved Hide resolved
@renaynay renaynay requested a review from fjl April 21, 2021 17:18
@fjl
Copy link
Contributor

fjl commented Apr 21, 2021

ok  	github.com/ethereum/go-ethereum/cmd/devp2p/internal/ethtest	164.936s

So it passes, but also takes a long time. We need to make it run faster.

@fjl
Copy link
Contributor

fjl commented Apr 21, 2021

--- PASS: TestEthSuite (169.65s)
    --- PASS: TestEthSuite/Status (0.00s)
    --- PASS: TestEthSuite/Status_66 (0.00s)
    --- PASS: TestEthSuite/GetBlockHeaders (0.00s)
    --- PASS: TestEthSuite/GetBlockHeaders_66 (0.00s)
    --- PASS: TestEthSuite/SimultaneousRequests_66 (0.00s)
    --- PASS: TestEthSuite/SameRequestID_66 (0.00s)
    --- PASS: TestEthSuite/ZeroRequestID_66 (0.00s)
    --- PASS: TestEthSuite/GetBlockBodies (0.00s)
    --- PASS: TestEthSuite/GetBlockBodies_66 (0.00s)
    --- PASS: TestEthSuite/Broadcast (0.11s)
    --- PASS: TestEthSuite/Broadcast_66 (0.11s)
    --- PASS: TestEthSuite/LargeAnnounce (20.23s)
    --- PASS: TestEthSuite/LargeAnnounce_66 (20.26s)
    --- PASS: TestEthSuite/OldAnnounce (10.01s)
    --- PASS: TestEthSuite/OldAnnounce_66 (10.01s)
    --- PASS: TestEthSuite/MaliciousHandshake (0.12s)
    --- PASS: TestEthSuite/MaliciousStatus (0.02s)
    --- PASS: TestEthSuite/MaliciousHandshake_66 (0.09s)
    --- PASS: TestEthSuite/MaliciousStatus_66 (0.02s)
    --- PASS: TestEthSuite/Transactions (0.21s)
    --- PASS: TestEthSuite/Transactions_66 (0.22s)
    --- PASS: TestEthSuite/MaliciousTransactions (50.16s)
    --- PASS: TestEthSuite/MaliciousTransactions_66 (50.18s)

The long tests are

    --- PASS: TestEthSuite/MaliciousTransactions (50.16s)
    --- PASS: TestEthSuite/MaliciousTransactions_66 (50.18s)
    --- PASS: TestEthSuite/LargeAnnounce (20.23s)
    --- PASS: TestEthSuite/LargeAnnounce_66 (20.26s)
    --- PASS: TestEthSuite/OldAnnounce (10.01s)
    --- PASS: TestEthSuite/OldAnnounce_66 (10.01s)

We need to see why they take so much time. Is it because of large data transfer, or because the test has a large timeout?

@fjl
Copy link
Contributor

fjl commented Apr 21, 2021

I tried setting global timeout to 10s, but no difference.

@fjl
Copy link
Contributor

fjl commented Apr 22, 2021

In MaliciousTransactions_66, the issue seems to be that it hits the timeout multiple times:

# Testing malicious tx propagation: 0
# unexpected message, logging: (*ethtest.Error)(could not read from connection: read tcp 127.0.0.1:55753->127.0.0.1:55751: i/o timeout)
# 
# Testing malicious tx propagation: 1
# unexpected message, logging: (*ethtest.Error)(could not read from connection: read tcp 127.0.0.1:55763->127.0.0.1:55751: i/o timeout)
# 
# Testing malicious tx propagation: 2
# unexpected message, logging: (*ethtest.Error)(could not read from connection: read tcp 127.0.0.1:55765->127.0.0.1:55751: i/o timeout)
# 
# Testing malicious tx propagation: 3
# unexpected message, logging: (*ethtest.Error)(could not read from connection: read tcp 127.0.0.1:55817->127.0.0.1:55751: i/o timeout)
# 
# Testing malicious tx propagation: 4
# unexpected message, logging: (*ethtest.Error)(could not read from connection: read tcp 127.0.0.1:55819->127.0.0.1:55751: i/o timeout)
# 

// Send the transaction
if err := sendConn.Write(&Transactions{tx}); err != nil {
txMsg := Transactions(txs)
if err := sendConn.Write(&txMsg); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should announce the txs individually here. Some of them can be large, and the node might reject the entire announcement when the first item is too big.

Copy link
Contributor

@fjl fjl Apr 22, 2021

Choose a reason for hiding this comment

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

What I meant is, you can just send them in a loop like

for _, tx := range txs {
    if err := sendConn.Write(&Transactions{tx}); err != nil { ... }
}

@fjl fjl merged commit ea54c58 into ethereum:master Apr 23, 2021
@fjl fjl added this to the 1.10.3 milestone Apr 23, 2021
@renaynay renaynay deleted the ethtest-unit-tests branch April 23, 2021 09:58
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
…#22698)

This change adds a Go unit test that runs the protocol test suite
against the go-ethereum implementation of the eth protocol.
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.

2 participants