From aec307f7ba760efcf1eea3d2ce87a9012188625a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 18 Jul 2022 21:42:28 +0930 Subject: [PATCH] multifundchannel: fix race where we restart fundchannel. Disconnecting a peer after openingd fails is not instantaneous: we abort the open, so openingd sends out a WIRE_ERROR which makes connectd close the connection. As a result this test fails often. The simplest fix is to wait for a second in multifundchannel before retrying, which is also robust against behaviour changes if we decide *not* to disconnect in future. Also make sure that addrhint ownership is correct, since this can lead to a use-after-free if we filter dests. ``` tests/test_connection.py::test_multifunding_best_effort FAILED [100%] ======================================================= FAILURES ======================================================== _____________________________________________ test_multifunding_best_effort _____________________________________________ node_factory = bitcoind = @pytest.mark.openchannel('v1') @pytest.mark.developer("disconnect=... needs DEVELOPER=1") def test_multifunding_best_effort(node_factory, bitcoind): ''' Check that best_effort flag works. ''' disconnects = ["-WIRE_INIT", "-WIRE_ACCEPT_CHANNEL", "-WIRE_FUNDING_SIGNED"] l1 = node_factory.get_node() l2 = node_factory.get_node() l3 = node_factory.get_node(disconnect=disconnects) l4 = node_factory.get_node() l1.fundwallet(2000000) destinations = [{"id": '{}@localhost:{}'.format(l2.info['id'], l2.port), "amount": 50000}, {"id": '{}@localhost:{}'.format(l3.info['id'], l3.port), "amount": 50000}, {"id": '{}@localhost:{}'.format(l4.info['id'], l4.port), "amount": 50000}] for i, d in enumerate(disconnects): # Should succeed due to best-effort flag. > l1.rpc.multifundchannel(destinations, minchannels=2) tests/test_connection.py:2070: ... > raise RpcError(method, payload, resp['error']) E pyln.client.lightning.RpcError: RPC call failed: method: multifundchannel, payload: {'destinations': [{'id': '022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59@localhost:41023', 'amount': 50000}, {'id': '035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d@localhost:41977', 'amount': 50000}, {'id': '0382ce59ebf18be7d84677c2e35f23294b9992ceca95491fcf8a56c6cb2d9de199@localhost:34943', 'amount': 50000}], 'minchannels': 2}, error: {'code': 305, 'message': 'Peer not connected at start', 'data': {'id': '0382ce59ebf18be7d84677c2e35f23294b9992ceca95491fcf8a56c6cb2d9de199', 'method': 'fundchannel_start'}} ``` Signed-off-by: Rusty Russell --- plugins/spender/multifundchannel.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/plugins/spender/multifundchannel.c b/plugins/spender/multifundchannel.c index 2c8a02ce5..4b9c1c6c3 100644 --- a/plugins/spender/multifundchannel.c +++ b/plugins/spender/multifundchannel.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -1702,10 +1703,10 @@ perform_multiconnect(struct multifundchannel_command *mfc) /* Initiate the multifundchannel execution. */ -static struct command_result * +static void perform_multifundchannel(struct multifundchannel_command *mfc) { - return perform_multiconnect(mfc); + perform_multiconnect(mfc); } @@ -1792,6 +1793,10 @@ post_cleanup_redo_multifundchannel(struct multifundchannel_redo *redo) */ /* Re-add to new destinations. */ tal_arr_expand(&new_destinations, *dest); + /* FIXME: If this were an array of pointers, + * we could make dest itself the parent of + * ->addrhint and not need this wart! */ + tal_steal(new_destinations, dest->addrhint); } } mfc->destinations = new_destinations; @@ -1825,8 +1830,11 @@ post_cleanup_redo_multifundchannel(struct multifundchannel_redo *redo) return mfc_finished(mfc, out); } - /* Okay, we still have destinations to try --- reinvoke. */ - return perform_multifundchannel(mfc); + /* Okay, we still have destinations to try: wait a second in case it + * takes that long to disconnect from peer, then retry. */ + notleak(plugin_timer(mfc->cmd->plugin, time_from_sec(1), + perform_multifundchannel, mfc)); + return command_still_pending(mfc->cmd); } struct command_result * @@ -2059,7 +2067,8 @@ json_multifundchannel(struct command *cmd, /* Stop memleak from complaining */ tal_free(minconf); - return perform_multifundchannel(mfc); + perform_multifundchannel(mfc); + return command_still_pending(mfc->cmd); } const struct plugin_command multifundchannel_commands[] = {