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

Improve connectd to try non-tor connection first and filter duplicates #4731

Merged
merged 3 commits into from
Aug 23, 2021

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Aug 20, 2021

This does:

  • remove duplicate connection attempt if addr_hint was set and also added by gossip.
  • try non-tor connections first as they are usually faster and more reliable.
  • renames variable "use_proxy_always" to "always_use_proxy" to match config.

@m-schmoock
Copy link
Collaborator Author

@rustyrussell
I debugged connectd a bit and found out that my old TOR issues didn't apply to recent versions anymore and always_use_proxy seems to work as intended. Anyway I tweaked code a bit...

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Minor comments: would you like to also add code to send pings to Tor connections which we initiated, every rand(30) + 30 seconds? Seems it would help with connectivity...

lightningd/plugin.c Show resolved Hide resolved
connectd/connectd.c Show resolved Hide resolved
This does two things:
 - It moves non-tor addresses upfront so it prefers normal connection
   which are less laggy and more reliable.
 - It prevents connectd from trying the same wire_addr twice when the
   addr_hint was given and gossip also added the same address.

Changelog-Changed: connectd: try non-TOR connections first
Changelog-Fixed: connectd: do not try address hint twice
This renames all occurences of use_proxy_always to always_use_proxy
to keep it inline with config values. This was a bit confusing.

Only significant change is that the payload in the plugins init
requests also contained the old name. No plugin currently seems to make
use of this variable yet. The old name 'use_proxy_always' is added when
deprecated APIs is enabled.

Changelog-Deprecated: Plugins: Renames plugin init 'use_proxy_always' to 'always_use_proxy'
@m-schmoock
Copy link
Collaborator Author

@rustyrussell

I'm not too sure about the each 30secs ping on TOR connections just yet as there are some open questions:

  • What do we do if we don't get a response in time?
  • Should we kill the connection and retry?
    • If yes, after which timeout?
    • If no, what does the ping achieve? (Just hope that TOR does better internal magic?)
  • Is 30secs a good value for TOR?

Also Bolt#1 tells us using frequent lightning protocol pings can raise a problem:

	/* BOLT #1:
	 *
	 * A node receiving a `ping` message:
	 *  - SHOULD fail the channels if it has received significantly in
	 *    excess of one `ping` per 30 seconds.
	 */

When a connection gets laggy and say its TCP socket suddenly spits out 2minutes of 'old remote data' to the daemon (which happens on TCP over TOR), there may be implementations that then fail the channels as it would receive 4 pings at once and they can't know that they were actually distributed 'evenly' over the past 2 minutes.

@m-schmoock m-schmoock changed the title Improve connectd to filter duplicates and try non-tor connection first Improve connectd to try non-tor connection first and filter duplicates Aug 21, 2021
@rustyrussell
Copy link
Contributor

@rustyrussell

I'm not too sure about the each 30secs ping on TOR connections just yet as there are some open questions:

It's a separate PR, but I think it would be useful.

* What do we do if we don't get a response in time?

* Should we kill the connection and retry?

My advice: only send a ping if you made the connection (no point both sides doing it, might as well establish a convention).

  * If yes, after which timeout?
  * If no, what does the ping achieve? (Just hope that TOR does better internal magic?)

I think if you're trying to send the next ping and you haven't got the reply from the last, you close the connection.

* Is 30secs a good value for TOR?

Well, if we can't get an HTLC committed (add, commit) in 30 seconds we already kill the connection and fail the HTLC, and we won't send the commit unless we've heard from the peer in the last 30 seconds (in which case we ping).

So if your connection is slower than that, c-lightning isn't going to work. And while we could extend that, it's pretty unfriendly to the rest of the network.

Also Bolt#1 tells us using frequent lightning protocol pings can raise a problem:

	/* BOLT #1:
	 *
	 * A node receiving a `ping` message:
	 *  - SHOULD fail the channels if it has received significantly in
	 *    excess of one `ping` per 30 seconds.
	 */

When a connection gets laggy and say its TCP socket suddenly spits out 2minutes of 'old remote data' to the daemon (which happens on TCP over TOR), there may be implementations that then fail the channels as it would receive 4 pings at once and they can't know that they were actually distributed 'evenly' over the past 2 minutes.

No, because you are also not supposed to send one until you've got the previous reply.

Note that channeld already has some ping logic, and it's probably a good place to start with this for Tor.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack da0440e

@rustyrussell rustyrussell merged commit 24ea498 into ElementsProject:master Aug 23, 2021
@m-schmoock m-schmoock deleted the improve/connectd branch August 23, 2021 09:07
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