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

Add limit for incoming connections from peers without channels #2601

Merged
merged 27 commits into from
Mar 24, 2023

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Feb 17, 2023

Limit the number of incoming connections from nodes we don't have channels with.

  • when an incoming connection comes in, the switchboard checks if we have channels with that peer
  • if we have channels with an incoming peer, do not track it
  • if an incoming peer is on the sync-whitelist, do not track it
  • otherwise, track the connection
  • when we reach our configured limit of tracked peers without channels, disconnect the oldest tracked peer
  • when a tracked peer disconnects or creates a channel, we will stop tracking that peer

We do not track peers we initiate outgoing channel connections to

@thomash-acinq
Copy link
Member

We should think about how this interacts with onion messages. Anyone can connect directly to us to send us a message (to request an invoice for an offer for instance) and we want to keep this behavior. We already have a limit to the number of onion message we accept per second (https://github.com/ACINQ/eclair/blob/master/eclair-core/src/main/scala/fr/acinq/eclair/io/PeerConnection.scala#L182) but unless we are currently receiving a lot of messages, we want to continue accepting messages from strangers. Looking at your proposal, it seems that once we reach the maximum number of connections allowed we don't accept new connections (and thus don't accept onion messages), if it doesn't cause other problems I would prefer continuing accepting new connections but closing old idle ones to stay under the limit.

@remyers
Copy link
Contributor Author

remyers commented Feb 20, 2023

We should think about how this interacts with onion messages.

Good point; onion messages are going to be a new source of connections from nodes without channels. I didn't see anything in the Onion Message PR about whether or not a connection should be kept open indefinitely after sending an onion message. This could lead to hitting the limit proposed by this PR and cause an unintentional DoS for both opening channels and receiving onion messages. Without any limit it could also lead to an ever increasing number of connections.

As suggested, one approach is to track a timestamp for each connection without a channel and periodically close inactive connections. This would require adding a new data structures in Switchboard, or another dedicated class. I was trying to avoid that, but I'll take a look at what would be needed.

@remyers
Copy link
Contributor Author

remyers commented Feb 28, 2023

This PR implements similar functionality to LDK PR #1988 wrt limiting incoming connections.

 - added new child actor to Switchboard to track peers with inbound connections
 - terminate peer with oldest inbound connection if tracked peers are over limit
 - if the channel is killed while waiting to open, it will lead to sending PeerDisconnected if that is the last channel.
 - this ensures new connections will succeed while waiting for the PeerDisconnected event
 - will need to handle an extra unnecessary PeerDisconnected event after disconnecting an old node
 - clearer name and homage to similar LDK parameter
@remyers remyers force-pushed the incoming-connect-rate-limiter branch from 244a87a to 2cfe424 Compare March 2, 2023 10:23
@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2023

Codecov Report

Merging #2601 (a00f448) into master (a52a10a) will decrease coverage by 0.05%.
The diff coverage is 95.83%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #2601      +/-   ##
==========================================
- Coverage   85.69%   85.65%   -0.05%     
==========================================
  Files         211      212       +1     
  Lines       16919    16962      +43     
  Branches      728      723       -5     
==========================================
+ Hits        14499    14529      +30     
- Misses       2420     2433      +13     
Impacted Files Coverage Δ
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 87.89% <90.00%> (-0.08%) ⬇️
...r/acinq/eclair/io/IncomingConnectionsTracker.scala 94.73% <94.73%> (ø)
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 56.43% <100.00%> (ø)
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.68% <100.00%> (+0.04%) ⬆️
...src/main/scala/fr/acinq/eclair/io/Monitoring.scala 100.00% <100.00%> (ø)
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 86.90% <100.00%> (+0.09%) ⬆️
...rc/main/scala/fr/acinq/eclair/io/Switchboard.scala 85.71% <100.00%> (+1.40%) ⬆️
...cinq/eclair/remote/EclairInternalsSerializer.scala 97.77% <100.00%> (ø)

... and 12 files with indirect coverage changes

@remyers remyers marked this pull request as ready for review March 2, 2023 11:18
@remyers remyers requested a review from t-bast March 2, 2023 11:19
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Concept ACK! We'll probably need to beef up the release notes with more details about DoS issues at various level, I believe this should help node operators configure their eclair node to make sure this new feature doesn't create more problems that it solves.

 - also add test to confirm actor terminates when sent an unhandled message
 - prevent switchboard from sending an unhandled classic message if peer not found to disconnect
 - peer can receive `Disconnect` before `Init`
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM, I only have nits and we should be good to go 👍

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@remyers remyers merged commit 732eb31 into ACINQ:master Mar 24, 2023
@remyers remyers deleted the incoming-connect-rate-limiter branch March 24, 2023 13:05
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.

4 participants