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

hive: reth does not respond to eth/TestMaliciousHandshake properly #1517

Closed
1 task done
Rjected opened this issue Feb 22, 2023 · 2 comments · Fixed by #1549
Closed
1 task done

hive: reth does not respond to eth/TestMaliciousHandshake properly #1517

Rjected opened this issue Feb 22, 2023 · 2 comments · Fixed by #1549
Labels
A-networking Related to networking in general C-bug An unexpected or incorrect behavior

Comments

@Rjected
Copy link
Member

Rjected commented Feb 22, 2023

Describe the bug

The hive simulator for the eth/TestMaliciousHandshake test does cause reth to fail the handshake, but not in the right way. The simulator outputs the following logs:

skipping suite "discv4" because it doesn't match test pattern eth/TestMaliciousHandshake
1..1
not ok 1 TestMaliciousHandshake
# Testing malicious handshake 0
# dial failed: read tcp 172.17.0.3:35620->172.17.0.4:30303: read: connection reset by peer
exit status 1
skipping suite "snap" because it doesn't match test pattern eth/TestMaliciousHandshake

This is how reth terminates the connection:

2023-02-22T20:32:08.611565Z TRACE Sending disconnect message during the handshake reason=Useless peer
2023-02-22T20:32:08.614217Z TRACE disconnected pending session session_id=SessionId(0) remote_addr=172.17.0.3:35606 error=Some(P2PStreamError(HandshakeError(NoSharedCapabilities)))
2023-02-22T20:32:08.614258Z  WARN Incoming pending session failed remote_addr=172.17.0.3:35606 error=Some(Eth(P2PStreamError(HandshakeError(NoSharedCapabilities))))
2023-02-22T20:32:08.614297Z TRACE The incoming ip address is in the ban list remote_addr=172.17.0.3:35620

This places the IP in the ban list, which causes problems for all succeeding hive tests, because the IP is the same for the rest of the tests.

The other issue is that because of this, the test cannot establish a connection for its other malicious handshakes, since TestMaliciousHandshake actually tests sending multiple malicious handshakes that should not succeed.

Steps to reproduce

Follow instructions in #851 for running hive, and run the devp2p tests.

This was run with reth logs set to:

RUST_LOG=net=trace,reth_eth_wire=trace 

Node logs

No response

Platform(s)

No response

What version/commit are you on?

No response

Code of Conduct

  • I agree to follow the Code of Conduct
@Rjected Rjected added C-bug An unexpected or incorrect behavior S-needs-triage This issue needs to be labelled labels Feb 22, 2023
@Rjected Rjected added A-networking Related to networking in general and removed S-needs-triage This issue needs to be labelled labels Feb 22, 2023
@mattsse
Copy link
Collaborator

mattsse commented Feb 22, 2023

The other issue is that because of this, the test cannot establish a connection for its other malicious handshakes, since TestMaliciousHandshake actually tests sending multiple malicious handshakes that should not succeed.

this is in the same test with same PeerId?
can you please link that here.

@Rjected
Copy link
Member Author

Rjected commented Feb 23, 2023

Update:

This happens because we ban the hive simulator container's IP after it sends the first malicious handshake. These containers have non-routable IPs however, so we do not necessarily need to IP ban them. To fix this, we should create a helper is_global method that determines whether or not the IP is globally routable, similar to the nightly IpAddr::is_global method. We should not ban non-routable peers.

Geth also implements this functionality, which is why it does not throttle hive test peers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general C-bug An unexpected or incorrect behavior
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants