We set the timeout on first HTLC, but didn't clear it if that HTLC failed.
It's saner to have a per-HTLC timeout (since that's what it is!) and
also our timer infra is specially coded to scale approximately infinitely so
trying to optimize this is vastly premature.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Protocol: We would sometimes gratuitously disconnect 30 seconds after an HTLC failed.
1. We don't need to check for NULL before tal_count(NULL).
2. Use of json_for_each_arr iterator is probably better.
3. Weird indent fixed.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Trying to put all the disconnect logic into the same path was a dumb
idea. If you asked to reconnect but passed in an 'unsaved' channel, we
would not call the 'reconnect' code.
Instead, we make a differentiation between "unsaved" channels
(ones that we haven't received commitment tx for) and handle the
disconnect for these separate from where we want to do a reconnect.
We were freeing the payload, which is then subsequently freed by the
plugin_hook caller. Whoops.
Now we pass through to the callback function and just clean up neatly.
------------------------------- Valgrind errors --------------------------------
Valgrind error file: valgrind-errors.406602
==406602== Invalid read of size 8
==406602== at 0x12AC93: openchannel2_hook_cb (dual_open_control.c:669)
==406602== by 0x12AF0A: openchannel2_hook_deserialize (dual_open_control.c:721)
==406602== by 0x16EF0E: plugin_hook_callback (plugin_hook.c:186)
==406602== by 0x169746: plugin_response_handle (plugin.c:514)
==406602== by 0x169959: plugin_read_json_one (plugin.c:620)
==406602== by 0x169B23: plugin_read_json (plugin.c:665)
==406602== by 0x1F4076: next_plan (io.c:59)
==406602== by 0x1F4C5B: do_plan (io.c:407)
==406602== by 0x1F4C9D: io_ready (io.c:417)
==406602== by 0x1F6F35: io_loop (poll.c:445)
==406602== by 0x13D48D: io_loop_with_timers (io_loop_with_timers.c:24)
==406602== by 0x143388: main (lightningd.c:1111)
==406602== Address 0x75e7418 is 56 bytes inside a block of size 3,520 free'd
==406602== at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==406602== by 0x204FB0: del_tree (tal.c:421)
==406602== by 0x20527E: tal_free (tal.c:486)
==406602== by 0x122D68: delete_channel (channel.c:124)
==406602== by 0x129291: channel_disconnect (dual_open_control.c:63)
==406602== by 0x129364: channel_close_conn (dual_open_control.c:82)
==406602== by 0x131CF6: peer_please_disconnect (connect_control.c:304)
==406602== by 0x131DEB: connectd_msg (connect_control.c:326)
==406602== by 0x172023: sd_msg_read (subd.c:509)
==406602== by 0x1F4076: next_plan (io.c:59)
==406602== by 0x1F4C5B: do_plan (io.c:407)
==406602== by 0x1F4C9D: io_ready (io.c:417)
==406602== Block was alloc'd at
==406602== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==406602== by 0x204A39: allocate (tal.c:250)
==406602== by 0x204FFA: tal_alloc_ (tal.c:428)
==406602== by 0x123165: new_unsaved_channel (channel.c:209)
==406602== by 0x130D34: peer_start_dualopend (dual_open_control.c:2985)
==406602== by 0x15BD2A: peer_connected_hook_final (peer_control.c:1105)
==406602== by 0x16F2E5: plugin_hook_call_ (plugin_hook.c:275)
==406602== by 0x15BF5C: plugin_hook_call_peer_connected (peer_control.c:1155)
==406602== by 0x15C16C: peer_connected (peer_control.c:1208)
==406602== by 0x131E3B: connectd_msg (connect_control.c:332)
==406602== by 0x172023: sd_msg_read (subd.c:509)
==406602== by 0x171842: read_fds (subd.c:310)
Fixes: #4494
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: config: New option `log-timestamps` allow disabling of timestamp prefix in logs.
Since plugins will start sending them soon, and they are likely to get
it wrong sometimes, be a bit more lenient, warn them in the logs
instead and then make sure it doesn't accidentally work anyway.
A plugin might subscribe to a notification topic that is only
registered by another plugin later, so push the check to that
consistency check phase where we do hook ordering as well.
We will eventually start emitting and dispatching custom notifications
from plugins just like we dispatch internal notifications. In order to
get reasonable error messages we need to make sure that the topics
plugins are asking for were correctly registered. When doing this we
don't really care about whether the plugin that registered the
notification is still alive or not (it might have died, but
subscribers should stay up and running), so we keep a list of all
topics attached to the `struct plugins` which gathers global plugin
information.
This avoids subdaemons complaining about malformed messages from us,
or doing the completely wrong thing, if they are really the wrong
version.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
You still shouldn't do this (you could get some transient failures),
but at least you have a decent chance if you reinstall over a running
daemon, instead of getting confusing internal errors if message
formats have changed.
Changelog-Added: lightningd: we now try to restart if subdaemons are upgraded underneath us.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: #4346
We generally hang things off our JSON response (this pattern predates
tmpctx!) but sometimes it gets reported as a memleak. I'd prefer not
to mark JSON responses as "notleak", since they can be allocated for
a while), so use tmpctx here.
```
E ValueError:
E Node errors:
E Global errors:
E - Node /tmp/ltests-spnausnb/test_htlc_out_timeout_1/lightning-1/ has memory leaks: [
E {
E "backtrace": [
E "ccan/ccan/tal/tal.c:442 (tal_alloc_)",
E "ccan/ccan/tal/tal.c:471 (tal_alloc_arr_)",
E "wallet/wallet.c:1775 (wallet_state_change_get)",
E "lightningd/peer_control.c:922 (json_add_channel)",
E "lightningd/peer_control.c:1424 (json_add_peer)",
E "lightningd/peer_control.c:1454 (json_listpeers)",
E "lightningd/jsonrpc.c:643 (command_exec)",
E "lightningd/jsonrpc.c:767 (rpc_command_hook_final)",
E "lightningd/plugin_hook.c:275 (plugin_hook_call_)",
E "lightningd/jsonrpc.c:855 (plugin_hook_call_rpc_command)",
E "lightningd/jsonrpc.c:942 (parse_request)",
E "lightningd/jsonrpc.c:1033 (read_json)",
E "ccan/ccan/io/io.c:59 (next_plan)",
E "ccan/ccan/io/io.c:435 (io_do_always)",
E "ccan/ccan/io/poll.c:300 (handle_always)",
E "ccan/ccan/io/poll.c:377 (io_loop)",
E "lightningd/io_loop_with_timers.c:24 (io_loop_with_timers)",
E "lightningd/lightningd.c:1097 (main)"
E ],
E "label": "wallet/wallet.c:1775:struct state_change_entry[]",
E "parents": [
E "common/json_stream.c:29:struct json_stream",
E "ccan/ccan/io/io.c:91:struct io_conn",
E "lightningd/lightningd.c:116:struct lightningd"
E ],
E "value": "0x55c6b02150b8"
E }
E ]
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The main change which affects us is that 2016 blocks to forget a channel
is a fixed number in the spec; we make this clear by renaming the
(developer-only) max_funding_unconfirmed to dev_max_funding_unconfirmed
and making it compile DEVELOPER only.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means remembering the connection direction. We also use the address to try
to reconnect, which we shouldn't bother with if they connect to us.
For peers from the database, we currently always save the addr: we shouldn't really
do this if they connected to us, since it's not useful for reconnecting (we don't
show the addr in JSON reply to listpeers unless we're connected, so it's only an
internal issue). This is left for future work.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This matters: if we connected, the address is probably usable for future connections.
But if they connected, the port is probably not (but the IP address may be).
Changelog-Added: JSON-RPC: `connect` returns "direction" ("in": they iniatated, or "out": we initiated)
Changelog-Added: plugins: `peer_connected` hook and `connect` notifications have "direction" field.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
You can now activate dual-funded channels using the
`--experimental-dual-fund` flag
Changelog-Changed: Config: `--experimental-dual-fund` runtime flag will enable dual-funded protocol on this node
As @fiatjaf points out we were notifying before we were actually set
on accepting, since the hook could also still reject. Switched them
around does and calling the notification only once it's been decided
is the correct thing to do.
Changelog-Fixed: plugin: The `invoice_payment` notification was being sent before the hook was called, which could still abort it.
Suggested-by: Fiatjaf <@fiatjaf>
Signed-off-by: Christian Decker <@cdecker>
There are perfectly valid reasons for us to not have a command on return
(something went boom while sending them our sigs and we've now gotten
their sigs during a reconnect and subsequently broadcast the tx)
