lightningd: don't explicitly tell connectd to disconnect, have it do it on sending error/warning.

Connectd already does this when we *receive* an error or warning, but
now do it on send.  This causes some slight behavior change: we don't
disconnect when we close a channel, for example (our behaviour here
has been inconsistent across versions, depending on the code).

When connectd is told to disconnect, it now does so immediately, and
doesn't wait for subds to drain etc.  That simplifies the manual
disconnect case, which now cleans up as it would from any other
disconnection when connectd says it's disconnected.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

tests/test_closing.py

@@ -550,7 +550,7 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams):
     bitcoind.generate_block(100)
 
     sync_blockheight(bitcoind, [l1, l2])
-    wait_for(lambda: len(l2.rpc.listpeers()['peers']) == 0)
+    wait_for(lambda: only_one(l2.rpc.listpeers()['peers'])['channels'] == [])
 
     # Do one last pass over the logs to extract the reactions l2 sent
     l2.daemon.logsearch_start = needle
@@ -679,7 +679,7 @@ def test_penalty_outhtlc(node_factory, bitcoind, executor, chainparams):
     bitcoind.generate_block(100)
 
     sync_blockheight(bitcoind, [l1, l2])
-    wait_for(lambda: len(l2.rpc.listpeers()['peers']) == 0)
+    wait_for(lambda: only_one(l2.rpc.listpeers()['peers'])['channels'] == [])
 
     # Do one last pass over the logs to extract the reactions l2 sent
     l2.daemon.logsearch_start = needle
@@ -3447,10 +3447,8 @@ def test_you_forgot_closed_channel(node_factory, executor):
     wait_for(lambda: only_one(only_one(l2.rpc.listpeers()['peers'])['channels'])['state'] == 'CLOSINGD_COMPLETE')
     assert only_one(only_one(l1.rpc.listpeers()['peers'])['channels'])['state'] == 'CLOSINGD_SIGEXCHANGE'
 
-    # l2 closes on us.
-    wait_for(lambda: only_one(l1.rpc.listpeers()['peers'])['connected'] is False)
-
-    # l1 reconnects, it should succeed.
+    # l1 won't send anything else until we reconnect, then it should succeed.
+    l1.rpc.disconnect(l2.info['id'], force=True)
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     fut.result(TIMEOUT)
 
@@ -3486,8 +3484,7 @@ def test_you_forgot_closed_channel_onchain(node_factory, bitcoind, executor):
     wait_for(lambda: only_one(only_one(l2.rpc.listpeers()['peers'])['channels'])['state'] == 'ONCHAIN')
 
     # l1 reconnects, it should succeed.
-    # l1 will disconnect once it sees block
-    wait_for(lambda: only_one(l1.rpc.listpeers()['peers'])['connected'] is False)
+    l1.rpc.disconnect(l2.info['id'], force=True)
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     fut.result(TIMEOUT)
 

tests/test_connection.py

@@ -956,8 +956,10 @@ def test_shutdown_awaiting_lockin(node_factory, bitcoind):
     l2.daemon.wait_for_log(' to ONCHAIN')
 
     bitcoind.generate_block(100)
-    wait_for(lambda: l1.rpc.listpeers()['peers'] == [])
-    wait_for(lambda: l2.rpc.listpeers()['peers'] == [])
+
+    # Won't disconnect!
+    wait_for(lambda: only_one(l1.rpc.listpeers()['peers'])['channels'] == [])
+    wait_for(lambda: only_one(l2.rpc.listpeers()['peers'])['channels'] == [])
 
 
 @pytest.mark.openchannel('v1')
@@ -1308,12 +1310,7 @@ def test_funding_external_wallet_corners(node_factory, bitcoind):
     assert l1.rpc.fundchannel_cancel(l2.info['id'])['cancelled']
     assert len(l1.rpc.listpeers()['peers']) == 0
 
-    # l2 still has the channel open/waiting
-    wait_for(lambda: only_one(only_one(l2.rpc.listpeers()['peers'])['channels'])['state']
-             == 'CHANNELD_AWAITING_LOCKIN')
-
     # on reconnect, channel should get destroyed
-    wait_for(lambda: l1.rpc.listpeers(l2.info['id'])['peers'] == [])
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     l1.daemon.wait_for_log('Unknown channel .* for WIRE_CHANNEL_REESTABLISH')
     wait_for(lambda: len(l1.rpc.listpeers()['peers']) == 0)
@@ -2535,13 +2532,10 @@ def test_multiple_channels(node_factory):
     l1 = node_factory.get_node()
     l2 = node_factory.get_node()
 
-    for i in range(3):
-        # FIXME: we shouldn't disconnect on close?
-        ret = l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
-        assert ret['id'] == l2.info['id']
+    ret = l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+    assert ret['id'] == l2.info['id']
 
-        l1.daemon.wait_for_log('Handed peer, entering loop')
-        l2.daemon.wait_for_log('Handed peer, entering loop')
+    for i in range(3):
         chan, _ = l1.fundchannel(l2, 10**6)
 
         l1.rpc.close(chan)
@@ -2551,7 +2545,6 @@ def test_multiple_channels(node_factory):
         l2.daemon.wait_for_log(
             r'State changed from CLOSINGD_SIGEXCHANGE to CLOSINGD_COMPLETE'
         )
-        wait_for(lambda: only_one(l1.rpc.listpeers(l2.info['id'])['peers'])['connected'] is False)
 
     channels = only_one(l1.rpc.listpeers()['peers'])['channels']
     assert len(channels) == 3
@@ -2581,7 +2574,7 @@ def test_forget_channel(node_factory):
 
     # Forcing should work
     l1.rpc.dev_forget_channel(l2.info['id'], True)
-    wait_for(lambda: l1.rpc.listpeers()['peers'] == [])
+    wait_for(lambda: only_one(l1.rpc.listpeers()['peers'])['channels'] == [])
 
     # And restarting should keep that peer forgotten
     l1.restart()
@@ -2637,13 +2630,12 @@ def test_peerinfo(node_factory, bitcoind):
     # Close the channel to forget the peer
     l1.rpc.close(chan)
 
-    wait_for(lambda: not only_one(l1.rpc.listpeers(l2.info['id'])['peers'])['connected'])
-    wait_for(lambda: not only_one(l2.rpc.listpeers(l1.info['id'])['peers'])['connected'])
-
     # Make sure close tx hits mempool before we mine blocks.
     bitcoind.generate_block(100, wait_for_mempool=1)
     l1.daemon.wait_for_log('onchaind complete, forgetting peer')
     l2.daemon.wait_for_log('onchaind complete, forgetting peer')
+    assert only_one(l1.rpc.listpeers(l2.info['id'])['peers'])['channels'] == []
+    assert only_one(l2.rpc.listpeers(l1.info['id'])['peers'])['channels'] == []
 
     # The only channel was closed, everybody should have forgotten the nodes
     assert l1.rpc.listnodes()['nodes'] == []
@@ -2728,8 +2720,8 @@ def test_fundee_forget_funding_tx_unconfirmed(node_factory, bitcoind):
     # (Note that we let the last number be anything (hence the {}\d)
     l2.daemon.wait_for_log(r'Forgetting channel: It has been {}\d blocks'.format(str(blocks)[:-1]))
 
-    # fundee will also forget and disconnect from peer.
-    wait_for(lambda: l2.rpc.listpeers(l1.info['id'])['peers'] == [])
+    # fundee will also forget, but not disconnect from peer.
+    wait_for(lambda: only_one(l2.rpc.listpeers(l1.info['id'])['peers'])['channels'] == [])
 
 
 @pytest.mark.developer("needs --dev-max-funding-unconfirmed-blocks")

tests/test_db.py

@@ -117,8 +117,8 @@ def test_max_channel_id(node_factory, bitcoind):
     l2.wait_for_channel_onchain(l1.info['id'])
 
     bitcoind.generate_block(101)
-    wait_for(lambda: l1.rpc.listpeers()['peers'] == [])
-    wait_for(lambda: l2.rpc.listpeers()['peers'] == [])
+    wait_for(lambda: only_one(l1.rpc.listpeers()['peers'])['channels'] == [])
+    wait_for(lambda: only_one(l2.rpc.listpeers()['peers'])['channels'] == [])
 
     # Stop l2, and restart
     l2.stop()

tests/test_plugin.py

@@ -740,10 +740,11 @@ def test_openchannel_hook_chaining(node_factory, bitcoind):
     # the third plugin must now not be called anymore
     assert not l2.daemon.is_in_log("reject on principle")
 
+    wait_for(lambda: l1.rpc.listpeers()['peers'] == [])
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     # 100000sat is good for hook_accepter, so it should fail 'on principle'
     # at third hook openchannel_reject.py
     with pytest.raises(RpcError, match=r'reject on principle'):
-        l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
         l1.rpc.fundchannel(l2.info['id'], 100000)
     assert l2.daemon.wait_for_log(hook_msg + "reject on principle")