Skip to content

Commit

Permalink
multifundchannel: fix race where we restart fundchannel.
Browse files Browse the repository at this point in the history
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 = <pyln.testing.utils.NodeFactory object at 0x7f6c0c95c1c0>
bitcoind = <pyln.testing.utils.BitcoinD object at 0x7f6c0c92a880>

    @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 <[email protected]>
  • Loading branch information
rustyrussell authored and niftynei committed Jul 19, 2022
1 parent e59e12d commit aec307f
Showing 1 changed file with 14 additions and 5 deletions.
19 changes: 14 additions & 5 deletions plugins/spender/multifundchannel.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <common/addr.h>
#include <common/json_param.h>
#include <common/json_stream.h>
#include <common/memleak.h>
#include <common/psbt_open.h>
#include <common/pseudorand.h>
#include <common/type_to_string.h>
Expand Down Expand Up @@ -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);
}


Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 *
Expand Down Expand Up @@ -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[] = {
Expand Down

0 comments on commit aec307f

Please sign in to comment.