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

[Merged by Bors] - Packet filter cli option #2523

Closed
wants to merge 2 commits into from

Conversation

pawanjay176
Copy link
Member

@pawanjay176 pawanjay176 commented Aug 19, 2021

Issue Addressed

N/A

Proposed Changes

Adds a cli option to disable packet filter in lighthouse bootnode. This is useful in running local testnets as the bootnode bans requests from the same ip(localhost) if the packet filter is enabled.

@pawanjay176 pawanjay176 requested a review from AgeManning August 19, 2021 11:23
@paulhauner paulhauner added the ready-for-review The code is ready for review label Aug 20, 2021
@AgeManning
Copy link
Member

hmm.... I dont recall banning localhost in the packet filter. Are you sure disabling the packet filter resolves this? I think the packet filter just limits requests and will ban an ip if it spams requests, which is probably what you're doing on smaller testnets.

Also, on smaller testnets you should set the --target-peer paramter to the expected number of nodes to prevent continuous discovery requests which will lead to discovery spamming

@pawanjay176
Copy link
Member Author

It seems to be banning the last connecting localhost node from the logs

Aug 20 18:59:54.169 INFO Contact information                     multiaddrs: ["/ip4/127.0.0.1/udp/4242/p2p/16Uiu2HAm9youmPxCnW8WqdD9VNTqcTHfrd3PtDFsMEgsFZt7tkSj", "/ip4/127.0.0.1/tcp/4242/p2p/16Uiu2HAm9youmPxCnW8WqdD9VNTqcTHfrd3PtDFsMEgsFZt7tkSj"]
Aug 20 18:59:54.169 INFO Adding bootnode                         node_id: 0xaf62..e250, peer_id: 16Uiu2HAm9youmPxCnW8WqdD9VNTqcTHfrd3PtDFsMEgsFZt7tkSj, address: Some(127.0.0.1:4242)
Aug 20 18:59:54.169 INFO Discv5 Service started 
Aug 20 18:59:54.170 INFO Server metrics                          requests/s: 0.00, active_sessions: 0, connected_peers: 0
Aug 20 18:59:56.455 WARN Banning IP for excessive requests: 127.0.0.1 
Aug 20 19:00:04.170 INFO Server metrics                          requests/s: 2.00, active_sessions: 3, connected_peers: 3

This is on a local testnet with 4 beacon nodes all of them running with --target-peers 3.

@michaelsproul
Copy link
Member

^I've observed the same as Pawan when running local testnets

@AgeManning
Copy link
Member

Oh yeah. I think I added a limit per IP. So if all your nodes are under a single IP, its going to ban that IP.

Least the filters are working correctly.

I guess we should set this CLI flag for all the nodes on the testnet, not just the bootnodes, because the other nodes will also ban localhost from excessive traffic.

@pawanjay176
Copy link
Member Author

I guess we should set this CLI flag for all the nodes on the testnet, not just the bootnodes, because the other nodes will also ban localhost from excessive traffic.

I think this should only happen after > 10 localhost beacon nodes which is sufficient for CI atleast (doppleganger tests)

.filter_max_nodes_per_ip(Some(10))

Can add it in if required though :)

@AgeManning
Copy link
Member

That config says that we allow up to 10 people to talk to us per IP.

However in the packet filter, we set default rate limits. There is a limit of how many messages we can receive per IP. See here: https://github.com/sigp/discv5/blob/master/src/config.rs#L106

The default value is on average 9 per second. This means per IP we only allow 9 requests per second before we ban the IP. So even though we allow 10 peers, on a small network, they will hit us with lots of requests such that we ban the IP from excessive requests.

So I think other nodes will also hit this limit if everyone is under the same IP.

@pawanjay176
Copy link
Member Author

Ohh yeah that makes sense. Added the flag in beacon node as well in 2353618

Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Looks good to me

@AgeManning
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Aug 26, 2021
## Issue Addressed

N/A

## Proposed Changes

Adds a cli option to disable packet filter in `lighthouse bootnode`. This is useful in running local testnets as the bootnode bans requests from the same ip(localhost) if the packet filter is enabled.
@AgeManning AgeManning added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 26, 2021
@bors bors bot changed the title Packet filter cli option [Merged by Bors] - Packet filter cli option Aug 26, 2021
@bors bors bot closed this Aug 26, 2021
@pawanjay176 pawanjay176 deleted the packet-filter-cli branch August 26, 2021 12:45
pawanjay176 added a commit to pawanjay176/lighthouse that referenced this pull request Aug 27, 2021
## Issue Addressed

N/A

## Proposed Changes

Adds a cli option to disable packet filter in `lighthouse bootnode`. This is useful in running local testnets as the bootnode bans requests from the same ip(localhost) if the packet filter is enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants