pytest: wean many tests off the assumption that listchannels shows private channels.

We will be changing this, or at least deprecating it, so get our
tests ready.

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

contrib/pyln-testing/pyln/testing/utils.py

@@ -956,9 +956,6 @@ class LightningNode(object):
 
         return '{}x{}x{}'.format(self.bitcoin.rpc.getblockcount(), txnum, res['outnum'])
 
-    def getactivechannels(self):
-        return [c for c in self.rpc.listchannels()['channels'] if c['active']]
-
     def db_query(self, query):
         return self.db.query(query)
 
@@ -1064,8 +1061,8 @@ class LightningNode(object):
                                  txnum, res['outnum'])
 
         if wait_for_active:
-            self.wait_channel_active(scid)
+            self.wait_local_channel_active(scid)
-            l2.wait_channel_active(scid)
+            l2.wait_local_channel_active(scid)
 
         return scid, res
 
@@ -1113,7 +1110,16 @@ class LightningNode(object):
             return None
         return channels[0]['channel_id']
 
+    def is_local_channel_active(self, scid):
+        """Is the local channel @scid usable?"""
+        channels = self.rpc.listpeerchannels()['channels']
+        return [c['state'] in ('CHANNELD_NORMAL', 'CHANNELD_AWAITING_SPLICE') for c in channels if c.get('short_channel_id') == scid] == [True]
+
+    def wait_local_channel_active(self, scid):
+        wait_for(lambda: self.is_local_channel_active(scid))
+
     def is_channel_active(self, chanid):
+        """Does gossip show this channel as enabled both ways?"""
         channels = self.rpc.listchannels(chanid)['channels']
         active = [(c['short_channel_id'], c['channel_flags']) for c in channels if c['active']]
         return (chanid, 0) in active and (chanid, 1) in active
@@ -1122,8 +1128,8 @@ class LightningNode(object):
         txid = only_one(self.rpc.listpeerchannels(peerid)['channels'])['scratch_txid']
         wait_for(lambda: txid in self.bitcoin.rpc.getrawmempool())
 
-    def wait_channel_active(self, chanid):
+    def wait_channel_active(self, scid):
-        wait_for(lambda: self.is_channel_active(chanid))
+        wait_for(lambda: self.is_channel_active(scid))
 
     # This waits until gossipd sees channel_update in both directions
     # (or for local channels, at least a local announcement)
@@ -1605,8 +1611,8 @@ class NodeFactory(object):
 
         # Wait for all channels to be active (locally)
         for i, n in enumerate(scids):
-            nodes[i].wait_channel_active(scids[i])
+            nodes[i].wait_local_channel_active(scids[i])
-            nodes[i + 1].wait_channel_active(scids[i])
+            nodes[i + 1].wait_local_channel_active(scids[i])
 
         if not wait_for_announce:
             return

tests/test_bookkeeper.py

@@ -366,7 +366,7 @@ def test_bookkeeping_missed_chans_leases(node_factory, bitcoind):
     bitcoind.generate_block(1, wait_for_mempool=[txid])
     wait_for(lambda: l1.channel_state(l2) == 'CHANNELD_NORMAL')
     scid = l1.get_channel_scid(l2)
-    l1.wait_channel_active(scid)
+    l1.wait_local_channel_active(scid)
     channel_id = first_channel_id(l1, l2)
 
     l1.pay(l2, invoice_msat)
@@ -431,7 +431,7 @@ def test_bookkeeping_missed_chans_pushed(node_factory, bitcoind):
     bitcoind.generate_block(1, wait_for_mempool=[txid])
     wait_for(lambda: l1.channel_state(l2) == 'CHANNELD_NORMAL')
     scid = l1.get_channel_scid(l2)
-    l1.wait_channel_active(scid)
+    l1.wait_local_channel_active(scid)
     channel_id = first_channel_id(l1, l2)
 
     # Send l2 funds via the channel
@@ -498,7 +498,7 @@ def test_bookkeeping_missed_chans_pay_after(node_factory, bitcoind):
     bitcoind.generate_block(1, wait_for_mempool=[txid])
     wait_for(lambda: l1.channel_state(l2) == 'CHANNELD_NORMAL')
     scid = l1.get_channel_scid(l2)
-    l1.wait_channel_active(scid)
+    l1.wait_local_channel_active(scid)
     channel_id = first_channel_id(l1, l2)
 
     # Now turn the bookkeeper on and restart
@@ -519,7 +519,8 @@ def test_bookkeeping_missed_chans_pay_after(node_factory, bitcoind):
         assert channel_id in accts
 
     # Send a payment, should be ok.
-    l1.wait_channel_active(scid)
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+    l1.wait_local_channel_active(scid)
     l1.pay(l2, invoice_msat)
     l1.daemon.wait_for_log(r'coin movement:.*\'invoice\'')
 

tests/test_cln_rs.py

@@ -3,6 +3,7 @@ from fixtures import *  # noqa: F401,F403
 from pathlib import Path
 from pyln import grpc as clnpb
 from pyln.testing.utils import env, TEST_NETWORK, wait_for, sync_blockheight, TIMEOUT
+from utils import first_scid
 import grpc
 import pytest
 import subprocess
@@ -306,6 +307,9 @@ def test_grpc_keysend_routehint(bitcoind, node_factory):
         ])
     ])
 
+    # FIXME: keysend needs (unannounced) channel in gossip_store
+    l1.wait_channel_active(first_scid(l1, l2))
+
     # And now we send a keysend with that routehint list
     call = clnpb.KeysendRequest(
         destination=bytes.fromhex(l3.info['id']),

tests/test_closing.py

@@ -38,9 +38,9 @@ def test_closing_simple(node_factory, bitcoind, chainparams):
     assert billboard == ['CHANNELD_NORMAL:Channel ready for use.']
 
     bitcoind.generate_block(5)
+    l1.wait_channel_active(chan)
+    l2.wait_channel_active(chan)
 
-    wait_for(lambda: len(l1.getactivechannels()) == 2)
-    wait_for(lambda: len(l2.getactivechannels()) == 2)
     billboard = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status']
     # This may either be from a local_update or an announce, so just
     # check for the substring
@@ -60,9 +60,9 @@ def test_closing_simple(node_factory, bitcoind, chainparams):
     l1.daemon.wait_for_log('sendrawtx exit 0')
     l2.daemon.wait_for_log('sendrawtx exit 0')
 
-    # Both nodes should have disabled the channel in their view
+    # Both nodes should have disabled the channel in gossip
-    wait_for(lambda: len(l1.getactivechannels()) == 0)
+    wait_for(lambda: not any([c['active'] for c in l1.rpc.listchannels()['channels']]))
-    wait_for(lambda: len(l2.getactivechannels()) == 0)
+    wait_for(lambda: not any([c['active'] for c in l2.rpc.listchannels()['channels']]))
 
     assert bitcoind.rpc.getmempoolinfo()['size'] == 1
 
@@ -304,8 +304,9 @@ def test_closing_specified_destination(node_factory, bitcoind, chainparams):
 
     l1.daemon.wait_for_logs([' to CLOSINGD_SIGEXCHANGE'] * 3)
 
-    # Both nodes should have disabled the channel in their view
+    # Both nodes should have disabled the channel in gossip
-    wait_for(lambda: len(l1.getactivechannels()) == 0)
+    wait_for(lambda: not any([c['active'] for c in l1.rpc.listchannels()['channels']]))
+    wait_for(lambda: not any([c['active'] for c in l2.rpc.listchannels()['channels']]))
 
     wait_for(lambda: bitcoind.rpc.getmempoolinfo()['size'] == 3)
 
@@ -504,12 +505,13 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams, anchors):
                                               opts])
 
     channel_id = first_channel_id(l1, l2)
+    scid = first_scid(l1, l2)
 
     # Now, this will get stuck due to l1 commit being disabled..
     t = executor.submit(l1.pay, l2, 100000000)
 
-    assert len(l1.getactivechannels()) == 2
+    assert l1.is_local_channel_active(scid)
-    assert len(l2.getactivechannels()) == 2
+    assert l2.is_local_channel_active(scid)
 
     # They should both have commitments blocked now.
     l1.daemon.wait_for_log('dev-disable-commit-after: disabling')
@@ -545,7 +547,7 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams, anchors):
     l2.daemon.wait_for_log(' to ONCHAIN')
 
     # FIXME: l1 should try to stumble along!
-    wait_for(lambda: len(l2.getactivechannels()) == 0)
+    wait_for(lambda: l2.is_local_channel_active(scid) is False)
 
     # l2 should spend all of the outputs (except to-us).
     # Could happen in any order, depending on commitment tx.
@@ -1593,13 +1595,14 @@ def test_penalty_rbf_normal(node_factory, bitcoind, executor, chainparams, ancho
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     l1.fundchannel(l2, 10**7)
     channel_id = first_channel_id(l1, l2)
+    scid = first_scid(l1, l2)
 
     # Trigger an HTLC being added.
     t = executor.submit(l1.pay, l2, 1000000 * 1000)
 
     # Make sure the channel is still alive.
-    assert len(l1.getactivechannels()) == 2
+    assert l1.is_local_channel_active(scid)
-    assert len(l2.getactivechannels()) == 2
+    assert l2.is_local_channel_active(scid)
 
     # Wait for the disconnection.
     l1.daemon.wait_for_log('dev-disable-commit-after: disabling')

tests/test_connection.py

@@ -2429,11 +2429,6 @@ def test_update_fee(node_factory, bitcoind):
     # Make l1 send out feechange.
     l1.set_feerates((14000, 11000, 7500, 3750))
 
-    # Now make sure an HTLC works.
-    # (First wait for route propagation.)
-    l1.wait_channel_active(chanid)
-    sync_blockheight(bitcoind, [l1, l2])
-
     # Make payments.
     l1.pay(l2, 200000000)
     # First payment causes fee update.

tests/test_gossip.py

@@ -449,8 +449,8 @@ def test_gossip_jsonrpc(node_factory):
     needle = "Received node_announcement for node"
     l1.daemon.wait_for_log(needle)
     l2.daemon.wait_for_log(needle)
-    wait_for(lambda: len(l1.getactivechannels()) == 2)
+    l1.wait_channel_active(only_one(channels1)['short_channel_id'])
-    wait_for(lambda: len(l2.getactivechannels()) == 2)
+    l2.wait_channel_active(only_one(channels1)['short_channel_id'])
 
     nodes = l1.rpc.listnodes()['nodes']
     assert set([n['nodeid'] for n in nodes]) == set([l1.info['id'], l2.info['id']])
@@ -1621,6 +1621,9 @@ def setup_gossip_store_test(node_factory, bitcoind):
     # Create another channel, which will stay private.
     scid12, _ = l1.fundchannel(l2, 10**6)
 
+    # FIXME: We assume that private announcements are in gossip_store!
+    l2.wait_channel_active(scid12)
+
     # Now insert channel_update for previous channel; now they're both past the
     # node announcements.
     l3.rpc.setchannel(l2.info['id'], feebase=20, feeppm=1000)
@@ -2031,8 +2034,8 @@ def test_tor_port_onions(node_factory):
 
 
 def test_routetool(node_factory):
-    """Test that route tool can see unpublished channels"""
+    """Test that route tool can see published channels"""
-    l1, l2 = node_factory.line_graph(2)
+    l1, l2 = node_factory.line_graph(2, wait_for_announce=True)
 
     subprocess.run(['devtools/route',
                     os.path.join(l1.daemon.lightning_dir,

tests/test_misc.py

@@ -320,9 +320,6 @@ def test_htlc_out_timeout(node_factory, bitcoind, executor):
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     chanid, _ = l1.fundchannel(l2, 10**6)
 
-    # Wait for route propagation.
-    l1.wait_channel_active(chanid)
-
     amt = 200000000
     inv = l2.rpc.invoice(amt, 'test_htlc_out_timeout', 'desc')['bolt11']
     assert only_one(l2.rpc.listinvoices('test_htlc_out_timeout')['invoices'])['status'] == 'unpaid'
@@ -392,7 +389,6 @@ def test_htlc_in_timeout(node_factory, bitcoind, executor):
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     chanid, _ = l1.fundchannel(l2, 10**6)
 
-    l1.wait_channel_active(chanid)
     sync_blockheight(bitcoind, [l1, l2])
 
     amt = 200000000
@@ -1235,12 +1231,6 @@ def test_blockchaintrack(node_factory, bitcoind):
     assert [o for o in l1.rpc.listfunds()['outputs'] if o['status'] != "unconfirmed"] == []
 
 
-def chan_active(node, scid, is_active):
-    chans = node.rpc.listchannels(scid)['channels']
-    print(chans)
-    return [c['active'] for c in chans] == [is_active, is_active]
-
-
 @pytest.mark.openchannel('v1')
 def test_funding_reorg_private(node_factory, bitcoind):
     """Change funding tx height after lockin, between node restart.
@@ -1268,8 +1258,8 @@ def test_funding_reorg_private(node_factory, bitcoind):
     wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['status']
              == ["{}_AWAITING_LOCKIN:They've confirmed channel ready, we haven't yet.".format(daemon)])
     bitcoind.generate_block(1)                      # height 107
-    l1.wait_channel_active('106x1x0')
+    l1.wait_local_channel_active('106x1x0')
-    l2.wait_channel_active('106x1x0')
+    l2.wait_local_channel_active('106x1x0')
     l1.stop()
 
     # Create a fork that changes short_channel_id from 106x1x0 to 108x1x0
@@ -1282,13 +1272,11 @@ def test_funding_reorg_private(node_factory, bitcoind):
                              r'Got depth change .->{} for .* REORG'.format(0)])
 
     # New one should replace old.
-    wait_for(lambda: chan_active(l2, '108x1x0', True))
+    wait_for(lambda: l2.is_local_channel_active('108x1x0'))
-    assert l2.rpc.listchannels('106x1x0')['channels'] == []
+    assert [c for c in l2.rpc.listpeerchannels()['channels'] if c['short_channel_id'] == '106x1x0'] == []
 
     l1.rpc.close(l2.info['id'])
     bitcoind.generate_block(1, True)
-    l1.daemon.wait_for_log(r'Deleting channel')
-    l2.daemon.wait_for_log(r'Deleting channel')
 
 
 @pytest.mark.openchannel('v1')
@@ -1307,8 +1295,8 @@ def test_funding_reorg_remote_lags(node_factory, bitcoind):
 
     l1.rpc.fundchannel(l2.info['id'], "all")
     bitcoind.generate_block(5)                      # heights 103 - 107
-    l1.wait_channel_active('103x1x0')
+    l1.wait_local_channel_active('103x1x0')
-    l2.wait_channel_active('103x1x0')
+    l2.wait_local_channel_active('103x1x0')
 
     # Make l2 temporary blind for blocks > 107
     def no_more_blocks(req):
@@ -1328,8 +1316,8 @@ def test_funding_reorg_remote_lags(node_factory, bitcoind):
     # Unblinding l2 brings it back in sync, restarts channeld and sends its announce sig
     l2.daemon.rpcproxy.mock_rpc('getblockhash', None)
 
-    wait_for(lambda: chan_active(l2, '104x1x0', True))
+    wait_for(lambda: l2.is_local_channel_active('104x1x0'))
-    assert l2.rpc.listchannels('103x1x0')['channels'] == []
+    assert [c for c in l2.rpc.listpeerchannels()['channels'] if c['short_channel_id'] == '103x1x0'] == []
 
     wait_for(lambda: only_one(l2.rpc.listpeerchannels()['channels'])['status'] == [
         'CHANNELD_NORMAL:Reconnected, and reestablished.',

tests/test_pay.py

@@ -430,7 +430,7 @@ def test_payment_success_persistence(node_factory, bitcoind, executor):
     assert len(payments) == 1 and payments[0]['status'] == 'complete'
     assert len(invoices) == 1 and invoices[0]['status'] == 'paid'
 
-    l1.wait_channel_active(chanid)
+    l1.wait_local_channel_active(chanid)
 
     # A duplicate should succeed immediately (nop) and return correct preimage.
     preimage = l1.dev_pay(
@@ -3961,13 +3961,13 @@ def test_mpp_waitblockheight_routehint_conflict(node_factory, bitcoind, executor
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     l1l2, _ = l1.fundchannel(l2, 10**7, announce_channel=True)
     l2.rpc.connect(l3.info['id'], 'localhost', l3.port)
-    l2.fundchannel(l3, 10**7, announce_channel=False)
+    l2l3, _ = l2.fundchannel(l3, 10**7, announce_channel=False)
 
     mine_funding_to_announce(bitcoind, [l1, l2, l3])
 
     # Wait for l3 to learn about l1->l2, otherwise it will think
     # l2 is a deadend and not add it to the routehint.
-    wait_for(lambda: len(l3.rpc.listchannels(l1l2)['channels']) >= 2)
+    l3.wait_channel_active(l1l2)
 
     # Now make the l1 payer stop receiving blocks.
     def no_more_blocks(req):
@@ -3979,7 +3979,11 @@ def test_mpp_waitblockheight_routehint_conflict(node_factory, bitcoind, executor
     bitcoind.generate_block(2)
     sync_blockheight(bitcoind, [l3])
 
+    # FIXME: routehint currently requires channels in gossip store
+    l3.wait_channel_active(l2l3)
+
     inv = l3.rpc.invoice(Millisatoshi(2 * 10000 * 1000), 'i', 'i', exposeprivatechannels=True)['bolt11']
+    assert 'routes' in l3.rpc.decodepay(inv)
 
     # Have l1 pay l3
     def pay(l1, inv):
@@ -4824,12 +4828,15 @@ gives a routehint straight to us causes an issue
 
     # Existence of l1 makes l3 use l2 for routehint (otherwise it sees deadend)
     l1, l2 = node_factory.line_graph(2, wait_for_announce=True)
+    scid12 = first_scid(l1, l2)
     l3 = node_factory.get_node()
     l3.rpc.connect(l2.info['id'], 'localhost', l2.port)
     scid23, _ = l2.fundchannel(l3, 1000000, announce_channel=False)
     # Make sure l3 sees l1->l2 channel.
-    wait_for(lambda: l3.rpc.listnodes(l1.info['id'])['nodes'] != [])
+    l3.wait_channel_active(scid12)
 
+    # FIXME: Routehint code currently relies on private gossip in store!
+    l3.wait_channel_active(scid23)
     inv = l3.rpc.invoice(10, "test", "test")['bolt11']
     decoded = l3.rpc.decodepay(inv)
     assert(only_one(only_one(decoded['routes']))['short_channel_id']
@@ -5303,7 +5310,7 @@ def test_pay_routehint_minhtlc(node_factory, bitcoind):
     l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True)
     l4 = node_factory.get_node()
 
-    l3.fundchannel(l4, announce_channel=False)
+    scid34, _ = l3.fundchannel(l4, announce_channel=False)
 
     # l2->l3 required htlc of at least 1sat
     scid = only_one(l2.rpc.setchannel(l3.info['id'], htlcmin=1000)['channels'])['short_channel_id']
@@ -5314,6 +5321,8 @@ def test_pay_routehint_minhtlc(node_factory, bitcoind):
     # And make sure l1 knows that l2->l3 has htlcmin 1000
     wait_for(lambda: l1.rpc.listchannels(scid)['channels'][0]['htlc_minimum_msat'] == Millisatoshi(1000))
 
+    # FIXME: Routehint code currently relies on private gossip in store!
+    l4.wait_channel_active(scid34)
     inv = l4.rpc.invoice(100000, "inv", "inv")
     assert only_one(l1.rpc.decodepay(inv['bolt11'])['routes'])
 

tests/test_plugin.py

@@ -2102,6 +2102,8 @@ def test_coin_movement_notices(node_factory, bitcoind, chainparams):
     # send a payment (originator)
     inv = l1.rpc.invoice(amount // 2, "second", "desc")
     payment_hash21 = inv['payment_hash']
+    # Make sure previous completely settled
+    wait_for(lambda: only_one(l2.rpc.listpeerchannels(l1.info['id'])['channels'])['htlcs'] == [])
     route = l2.rpc.getroute(l1.info['id'], amount // 2, 1)['route']
     l2.rpc.sendpay(route, payment_hash21, payment_secret=inv['payment_secret'])
     l2.rpc.waitsendpay(payment_hash21)