Rework ping logic, have channeld ping regularly. #4804
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustyrussell
force-pushed
the
guilt/tor-pings
branch
from
8b36215
to
212011d
Compare
vincenzopalazzo
requested changes
Sep 22, 2021
There was a problem hiding this comment.
LGTM 212011d
I have only a small comment, that I was not able to answer myself.
| @@ -254,7 +254,7 @@ def mock_getblock(r): | |||
|
|
|||
|
|
|||
| def test_ping(node_factory): | |||
| l1, l2 = node_factory.line_graph(2, fundchannel=False) | |||
| l1, l2 = node_factory.line_graph(2) | |||
|
|
|||
| def ping_tests(l1, l2): | |||
| # 0-byte pong gives just type + length field. | |||
There was a problem hiding this comment.
Suggested change
| # 0-byte pong gives just type + length field. | |
| # 0-byte pong gives just type + length field. | |
| l1.rpc.ping(l2.info['id']) |
Maybe we can test also if the node responds correctly to the ping command before test the failure?
rustyrussell
force-pushed
the
guilt/tor-pings
branch
from
212011d
to
1f47334
Compare
cdecker
force-pushed
the
guilt/tor-pings
branch
2 times, most recently
from
214a8e7
to
44d8459
Compare
|
Rebased on |
This is a noop for now, since gossipd handles them. But that will change in the next patch. Signed-off-by: Rusty Russell <[email protected]>
To minimize the diffs, we #if 0 the code. We'll reenable it once channeld is ready. We also temporarily disable the ping tests. Signed-off-by: Rusty Russell <[email protected]>
We're going to continuously ping, so this is redundant. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/tor-pings
branch
from
44d8459
to
d9f5a34
Compare
|
Rebased again, fixed accidental delete of member initialization. |
We would sleep until the next timer, even if that's long past when we would send gossip. Normally we use very short timers, so we didn't notice, but we will in the next patch, where we use continuous timers for pings. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/tor-pings
branch
from
d9f5a34
to
3e3e153
Compare
Send a ping every 15-45 seconds. If we try to send another one and we haven't got a reply, hang up. Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: Protocol: Send regular pings to detect dead connections (particularly for Tor).
It's probably not worth fixing for the other daemons. Changelog-Changed: JSON-RPC: `ping` now only works if we have a channel with the peer. Signed-off-by: Rusty Russell <[email protected]>
If it reconnects by itself, it will get a warning message: ``` lightningd-2: 2021-10-08T01:40:42.446Z DEBUG 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-channeld-chan#3: billboard: Sent reestablish, waiting for theirs lightningd-2: 2021-10-08T01:40:42.446Z DEBUG 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-channeld-chan#3: peer_in WIRE_ERROR lightningd-2: 2021-10-08T01:40:42.447Z DEBUG 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-channeld-chan#3: billboard perm: Received error channel 0a6220a3e904d17e72b5c3499928dc8a65720063c6395c999a129a0ff0b06afb: Forcibly closed by `close` command timeout lightningd-2: 2021-10-08T01:40:42.448Z INFO 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-chan#3: Peer transient failure in CHANNELD_NORMAL: channeld WARNING: error channel 0a6220a3e904d17e72b5c3499928dc8a65720063c6395c999a129a0ff0b06afb: Forcibly closed by `close` command timeout ``` And this will make CI complain. Signed-off-by: Rusty Russell <[email protected]>
Reconnect manually, don't wait for automatic reconnect. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/tor-pings
branch
from
3e3e153
to
e9551cf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This might help stability of Tor connections, but I made it universal since it's easier (in particular, it's hard to tell which incoming connections are Tor, since they usually come from localhost).