This table doesn't have `id`, except as the implicit one in Sqlite3,
so we need to add it for postgres.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
peer_htlcs has become a bit of a dumping ground: move listforwards
etc to its own file.
Also move `struct channel_info` from peer_htlcs.h to channel.h where
it more logically belongs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Looking through logs I was surprise to see:
```
lightningd-1 2023-10-26T03:42:36.824Z INFO lightningd: Sending 200000000msat over 1 hops to deliver 200000000msat
```
On a re-payment where we simply returned from sendpay immediately! Move that log to later.
We used to have "unsaved" payments: now we don't we can use
our normal "iterator" pattern rather than returning arrays.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It used to be used for both `sendpay` and `waitsendpay` but now it's
only for the latter, so the name is confusing.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We didn't write to db immediately, but waited until it the actual HTLC got
added (or failed). That way we didn't have a separate transaction to
write the payment into the db, but the complexity is not worth it: it
makes the next refactors harder, since we can't use the normal
iterator patterns like we do with the rest of the db (as we have to add
the unstored ones).
We might as well also make sendpay return immediately: we used to return
once the HTLC had been confirmed sent, since we entered it in the db
at that point, but we can keep it simple now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Adding an index means:
1. Add the new subsystem, and new updated_index field to the db, and
create xxx_index_deleted/created/updated APIs.
2. Hook up these functions to the points they need to be called.
3. Add index, start and limit fields to the list command.
4. Add created_index and updated_index into the list command.
This does #1.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means refactoring out some of the generic anchor info, from the
per-commitment-tx info (we can have at least two, perhaps more with
splicing!).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's going to want to remember these, in case it encounters peers'
commitment tx and needs to boost it with CPFP on the anchor.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Tihis commit is implementing a 2-phase commit between
the signer the node and the peer.
The main reason for this is that everybody must agree on the lock,
otherwise one of them will want N signatures (on the splice candidates),
and another will produce only 1 signature.
check_outpoint is the "prepare" for the signer, and lock_outpoint is the
"commit". if check_outpoint returns true, lock_outpoint must not fail.
Link: https://github.com/ElementsProject/lightning/issues/6722
Suggested-by: @devrandom
Co-Developed-by: Ken Sedgwick <ken@bonsai.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Changelog-Added: JSON-RPC: `recover` command to force (unused) lightningd node to restart with `--recover` flag.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This makes `check` much more thorough, and useful.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `check` now does much more checking on every command (not just basic parameter types).
Put an assertion inside db.c, and run every command we do (in testing) through
a `check` variant.
I inserted a deliberate bug (made addpsbtoutput call wallet_get_newindex()
before returning when running `check`, and indeed, backtrace as expected.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We often want to do more parameter checks after param(), so allow a
new param_check(), with the proviso that the caller needs to also return
command_check_done() after other checks if command_check_only(cmd) is true.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We had a complaint that you can't CPFP a mutual close, which you
should be able to do.
Fixes: #6692
Changelog-Fixed: wallet: close change outputs show up immediately in `listfunds` so you can CPFP.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
datastoreusage returns the total_bytes that are stored under a given
{Key} or from root. {Key} is the entry point from which we begin to
traverse the datastore.
Changelog-Added: JSON-RPC: `datastoreusage`: returns the total bytes that are stored under a given key.
Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
```
Already in transaction from lightningd/plugin.c:727
```
There are two callers, and one didn't disable transactions, so do it in plugin_exclusive_loop.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now we've asserted that channeld would tell lightningd the same thing it
would do anyway, we can simply have channeld say "enable=True|False" and
lightningd fill in the other fields.
This means there's a pile of things channeld doesn't need to know any more!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
channeld used to talk directly to gossipd, so it made sense for it to
tell gossipd directly when it wanted it to make a new channel_update.
When that changed with v0.11, we simply directed the message via
lightningd.
But much of the information is actually told to channeld by lightningd!
So I applied this assertion and ran the test suite, before the next patch makes it redundant.
We got one assertion: test_setchannel_zero deliberately drives the
advertized htlc_max over the real htlc max in test_setchannel_zero for
testing.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We have to work quite hard to do this, since we don't want to call
finish if the broadcast has been freed in the meantime.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Previously, every broadcast was attached to a channel, but we can
make it explicit, so when the context is freed, the re-broadcast stops
(if rebroadcast is set).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If the context is freed, the callback isn't called. This doesn't matter
yet, since our callbacks tend to be such that the callback itself is
required to free things, but it's clearer this way and allows more
flexible usage in following patches.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We remove it from the pending_requests strmap before calling it,
so it doesn't get called again by destroy_plugin.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We should really unify the cases of a local request, vs a forwarded
request, but for now, don't steal the request onto the plugin, and
if we return from the plugin and the request is gone, don't get upset.
This uncovered a case where we weren't inside a transaction, in
test_hook_crash, so fix that.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now the internal code will generate a "PLUGIN_TERMINATED" response
when the plugin dies, we can handle it in plugin_hook.
But we can also simplify it by turning the snapshot of hooks into
a simple array: this means we are robust against any combination of plugins
exiting at any time.
Note: this reveals an issue with test_rpc_command_hook where we run
the request hook again (unexpectedly), so we disable that for the next
patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We had special code to fail a forwarded request, but not for an
internally-generated request. Instead, we should pretend the (dead)
plugin responded with a PLUGIN_TERMINATED error, and handle the
request through the normal paths.
This breaks the case where a plugin crashes (or stops itself) in a
hook, so we handle that next.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It was always a bit weird they weren't, and it seems a premature
optimization to make the callbacks to this themselves.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's per-plugin, so why is there a single map for all plugins? It
works because we always make unique ids, but it's weird.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When using DEBUG_SUBD with pytest:
```
lightningd: Unknown decode for --dev-debugger=<subprocess>
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
During the changeset calculation after the `openchannel2_sign`
hook.
So this commit patch the problem with the following change:
- Addressed an issue where `psbt_get_changeset` was modifying the original PSBT unnecessarily.
- This modification led to problems with a different hsmd, as referenced in [Issue #6672](https://github.com/ElementsProject/lightning/issues/6672).
- Noted a potential optimization where only a subpart of the PSBT
needs to be cloned, as the mutation is specific to inputs.
Link: https://github.com/ElementsProject/lightning/issues/6672
Reported-by: @devrandom
Suggested-by: Ken Sedgwick <ken@bonsai.com>
Co-Developed-by: Ken Sedgwick <ken@bonsai.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Changelog-Added: Plugins: plugins can now specify (unknown) even messages we should accept from peers.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We do it here, but it's not necessary, and we also deprive them of the
chance to do so (since we kill them).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Previously, we would forward the message to a subd, but now we have
the case where the subd is gone, but we're still connected. If the
peer anything but a reestablish in that state, we drop the connection.
Instead, an error should always make us fail the channel.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We generalize the current df-only "aborted" flag (and invert it) to a
"disconnected" flag in the peer status message.
We convert it back to the aborted flag for now inside subd.c, but that's
next.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Move the "no lease, return" to the top, to avoid testing twice. Also,
we won't spam now for most channels.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As a node matures and is no longer new, it can take some time
to determine which version of `cln` it's running.
To address this, we now display the version earlier. This ensures
that even in the event of a crash, we're aware of the running version.
(cln-meta-project-py3.11) ➜ lightning git:(macros/log-version) ✗ ./lightningd/lightningd
2023-10-12T19:21:00.899Z INFO lightningd: v23.08.1-209-gae94be4-modded
2023-10-12T19:21:00.994Z INFO lightningd: Creating configuration directory /home/vincent/.lightning/bitcoin
2023-10-12T19:21:01.235Z INFO lightningd: Creating database
2023-10-12T19:21:01.279Z UNUSUAL hsmd: HSM: created new hsm_secret file
Could not connect to bitcoind using bitcoin-cli. Is bitcoind running?
Make sure you have bitcoind running and that bitcoin-cli is able to connect to bitcoind.
You can verify that your Bitcoin Core installation is ready for use by running:
$ bitcoin-cli echo 'hello world'
2023-10-12T19:21:01.349Z **BROKEN** plugin-bcli: \nCould not connect to bitcoind using bitcoin-cli. Is bitcoind running?\n\nMake sure you have bitcoind running and that bitcoin-cli is able to connect to bitcoind.\n\nYou can verify that your Bitcoin Core installation is ready for use by running:\n\n $ bitcoin-cli echo 'hello world'\n
2023-10-12T19:21:01.349Z INFO plugin-bcli: Killing plugin: exited before we sent init
The Bitcoin backend died.
Link: https://github.com/ElementsProject/lightning/issues/6374
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
This makes it easier to use outside simple subds, and now lightningd can
simply dump to log rather than returning JSON.
JSON formatting was a lot of work, and we only did it for lightningd, not for
subdaemons. Easier to use the logs in all cases.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We didn't apply the inflight to the channel struct before asserting, so
we can break test_rbf_non_last_mined:
```
lightningd: lightningd/dual_open_control.c:981: dualopend_tell_depth: Assertion `bitcoin_txid_eq(&channel->funding.txid, txid)' failed.
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now we're not always using the same functions to watch during
dual-funding opening, we need to make sure we're watching the close
(in particular, df close before the opening is confirmed).
So, keep a pointer, and if it's not set in drop_to_chain, set it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We used the original channel funding output number. I'm not sure if this
was true in the previous code, or a regression I introduced, but it
caused occasonal failures in test_splice_gossip!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We use the *same* callback for the funding tx, as well as for inflight dual-funding txs, as well as inflight splice txs. This is deeply confusing!
Instead, use explicit cbs for splicing and df. Once they're locked in, use the normal callback.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We never do this, but we're about to (we always watch before
we broadcast a tx).
We use a `depth` member to avoid calling the callback multiple times
for the same event, but we initialize it to 0. This means if we
register a watch, and the first thing that happens is that it
reorganizes out, we *don't* make the callback.
Use an impossible value at initialization, instead.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The latter is used when we're put in the db, the former is the uncommitted state.
Currently dbid == 0 is used in addition to the state, which is unwieldy.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Experimental: JSON-RPC: added new dual-funding state `DUALOPEND_OPEN_COMMITTED`
We usually hand times by copy, not by pointer (and if we did, they should
be const!). I noticed this particularly for the state changed code, but
it goes down to to json_add_timeiso, so I fixed that too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We should use capability tests for states (can you add htlcs?) rather than vague
descriptions (are you closing?).
And as much as possible, use switch () statements to force us to think
about all the cases, especially when we add new states!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Currently it's half done in funding_depth_cb, and half in
channeld_tell_depth. It's very confusing as a result,
with splicing, dual-funding and zeroconf.
This does introduce a behaviour change: if a channel is NORMAL and
it gets reorganized, we force close (unless we were the one who funded
it, or it's zeroconf anyway). This is safer than continuing to use
the channel in this case!
Some tests are changed to zeroconf to make them work, but v2 doesn't
support zeroconf, so that's removed.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is a workaround, the real fix is to use a different
callback for inflight splice attempts, which comes later.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Not just if htlc addition is too slow, make this the default. dual-open's txabort
is excluded, however.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If you previously configured with `--enable-developer` we turn that into `--enable-debugbuild`.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Removed: build: `--enable-developer` arg to configure (and DEVELOPER variables): use `./configure --enable-debugbuild` and `developer` setting at runtime.
And require --developer to use them.
Also refuse redirection to deprecated APIs if deprecated APIs are disabled!
Changelog-Removed: `dev-sendcustommsg` (use `sendcustommsg`, which was added in v0.10.1)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Also requires us to expose memleak when !DEVELOPER, however we only
ever used the memleak tracking when the LIGHTNINGD_DEV_MEMLEAK
environment variable was set, so keep that.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Currently it just defaults to the DEVELOPER compile option, but we'll
move over to this.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Config: `--developer` enables developer options and changes default to be "disable deprecated APIs".
We check for list_empty, so it's always actually set.
```
lightningd/peer_control.c: In function ‘drop_to_chain’:
lightningd/peer_control.c:353:17: error: ‘tx’ may be used uninitialized [-Werror=maybe-uninitialized]
353 | resolve_close_command(ld, channel, cooperative, tx);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lightningd/peer_control.c:341:36: note: ‘tx’ was declared here
341 | struct bitcoin_tx *tx;
| ^~
cc1: all warnings being treated as errors
make: *** [Makefile:298: lightningd/peer_control.o] Error 1
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Added a test for splicing out that exposed some behavior and code glitches that are addressed in this commit.
Added test for splice gossip.
Also added documentation for how to do a splice out.
ChangeLog-Fixed: Added docs, testing, and some fixes related to splicing out, insufficent balance handling, and restarting during a splice.
json_add_timeabs only printed in milliseconds and json_add_time outputs a string which is weird
Changelog-Changed: JSON-RPC time fields now have full nanosecond precision (i.e. 9 decimals not 3): `listfowards` `received_time` `resolved_time` `listpays`/`listsendpays` `created_at`.
Explicitly allow all-zero in the onion_hash: we didn't do anything except log if it was unexpected anyway.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We were allowed to, but the spec removed that. So we handle warnings
differently from errors now.
This also means the LND "internal error" workaround is done in
lightningd (we still disconnect, but we don't want to close channel).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: we no longer disconnect every time we receive a warning message.
This seems to be a cut & paste bug (mine, AFAICT!) from the command code:
```
rune = rune_derive_start(cmd, master_rune,
tal_fmt(tmpctx, "%"PRIu64,
rune_counter ? *rune_counter : 0));
```
In that case, rune_counter was a pointer, which could be NULL.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It always is for runes we create, but in theory you can take our secret key
and make our own runes with your own tools.
(We correctly refuse runes without uniqueids if they're *not* ours
anyway: uniqueid is only used for our own runes).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
nodeid is only useful when we know the peer we're talking to (e.g. commando).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
No-schema-diff-check: We're simply making optional, not deprecating!
During tests we can see that the subdaemon can be restarted unnecessarily if we're slow enough; we don't need to do so if it's still running.
Reported-by: Matt Morehouse <mattmorehouse@gmail.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We determine whether they are allowed or not based on the hook return
value of `mindepth`. To do so we need to pass that value down to
`openingd` and verify that the `channel_type` and our permissions
match up.
1. announce-addr-discovered-port takes a port option.
2. accept-htlc-tlv-types was deprecated in favor of multiple accept-htlc-tlv-type.
3. Document clnrest.py options.
4. Don't list --version twice in lightningd --help (initial_config_opts calls
opt_register_version() already).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reported-by: @niftynei
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Plugins: we clean up properly if a plugin fails to start, and we don't kill all processes if it's from `plugin startdir`.
Don’t send the funding spend to onchaind if we detect it in inflights (aka. a splice). While we already prevented onchaind_funding_spent from being called directly, the call to wallet_channeltxs_add meant onchaind_funding_spent would be called *anyway* on restart. This is now fixed.
Additionally there was a potential for a race problem depending on the firing order of the channel depth and and funding spent events.
Instead of requiring these events fire in a specific order, we make a special “memory only” inflight object to prevent the race regardless of firing order.
Changelog-Fixed: Splice: bugfix for restart related race condition interacting with adversarial close detection.
Indeed, we can fall through this if it's not a valid enum value.
gcc-12 (Ubuntu 12.2.0-17ubuntu1) 12.2.0
```
In file included from plugins/commando.c:10:
ccan/ccan/tal/str/str.h: In function ‘rune_altern_to_english’:
ccan/ccan/tal/str/str.h:43:9: error: ‘cond_str’ may be used uninitialized [-Werror=maybe-uninitialized]
43 | tal_fmt_(ctx, TAL_LABEL(char, "[]"), __VA_ARGS__)
| ^~~~~~~~
plugins/commando.c:97:21: note: ‘cond_str’ was declared here
97 | const char *cond_str;
| ^~~~~~~~
cc1: all warnings being treated as errors
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In spec commit 498f104fd399488c77f449d05cb21c0b604636a2 (August 2021),
Bastien Teinturier removed the requirement that the mutual close fee be
less than or equal the final commitment tx.
We adopted that change in v0.10.2, but we made sure to never offer a fee
under the final commitment tx's fee, so we didn't break older nodes.
However, the closing tx can actually be larger than the final commitment tx!
The final commit tx has a 22-byte P2WKH output and a 34-byte P2WSH output;
the closing can have two 34-byte outputs, making it 4*8 = 32 Sipa heavier.
Previously this would only happen if both sides asked for P2WSH outputs,
but now it happens with P2TR, which we now do.
The result is that we create a tx which is below the finally commitment
tx fee, and may be below minrelayfee (as it was in regtest).
So it's time to remove that backwards-compatibility hack.
Changelog-Fixed: Protocol: We may propose mutual close transaction which has a slightly higher fee than the final commitment tx (depending on the outputs, e.g. two taproot outputs).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: #6545
Avoids a gratuitous "ctx" field, and the simplified declaration
is now understood by `make update-mocks`.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This was recommended by @t-bast: if the final spec commits to something
compatible, we can simply advertize and accept both features, but if it
does change in incompatible ways we won't cause problems for nodes
who implement the official spec.
(I split this, so first, we remove the OPT_SPLICE entirely, to make
sure we caught them all. --RR)
Suggested-by: @t-bast
Changelog-None
The nomenclature confusion mean that we were ANDING a capability
with a message number (29) which always returned non-zero. We really
do need a new capability which we can hand to channeld to make these
splice txs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I obviously like the word "capabilities" since I reused it to refer
to the HSM's overall features :(
Suggested-by: @ksedgwic
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Apparently MacOS doesn't always have fdatasync, so use fsync. Even more importantly
check whether it succeeds!
Fixes: #6516
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I noticed this while debugging an issue with ACINQ, that we got upset,
but didn't trigger a reconnect cycle.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Protocol: We now close connection with a peer if adding an HTLC times out (which may be a TCP connectivity issue).
In this case, the user's default was info, but they specifically asked for debug
from one plugin. Since there were no per-file filters, it set filtering to the
default level, info, and rejected it. Since it's been explicitly filtered in,
we need to pass it at this point.
Reported-by: @wtogami
Fixes: #6503
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This was strongly recommended by Russell O'Connor: the "ms" implies that
it's a BIP-32 master secret, and this is CLN specific.
If we changed the hrp to "cln" it would be better, but apparently that
means we no longer fit in a "standard billfold metal wallet" (and
our code assumes a 2-byte prefix anyway).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Thread the signed tx through so close's JSON return contains that,
rather than the unsigned channel->last_tx.
We have to split the "get cmd_id" from "resolve the close commands" though;
and of course, as before, we don't actually print the txids of multiple
transactions even though we may have multi in flight due to splice!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: JSON-RPC: `close` returns a `tx` field with witness data populated (i.e. signed).
Fixes: #6440
Update the lightningd <-> channeld interface with lots of new commands to needed to facilitate spicing.
Implement the channeld splicing protocol leveraging the interactivetx protocol.
Implement lightningd’s channel_control to support channeld in its splicing efforts.
Changelog-Added: Added the features to enable splicing & resizing of active channels.
Update gossip routiens and various other hecks on the channel state to consider AWAITING_SPLICE to be routable and treated similar to CHANNELD_NORMAL.
Small updates to psbt interface
Changelog-None
Firstly, I wanted the results easier to use:
1. Make them always lower case, even if the string was UPPER.
2. Decode the payload for them.
3. Don't give the user any fields they don't need, and make
the field sizes explicit.
Secondly, I wanted to avoid the pattern of "check in one place, assume
in another", in favour of "check on use".
So, I changed the code to lower the string if it needs to at the start,
and then changed the pull functions so we always use them to get data:
this way we should fail clearly and gracefully if we don't have enough data.
I made all the checks explicit, where we assign the fields.
I also addressed the FIXME: I think the array is *often* one shorter,
but not always, so I trim the last byte at the end if needed.
[ Aditya modified the tests to work ]
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Clean restart of daemon after a tx-abort is a nice way to work around
the 'persistent' disconnect that we t-bast noticed.
Changelog-Fixed: `dualopend`: Fix behavior for tx-aborts. No longer hangs, appropriately continues re-init of RBF requests without reconnction msg exchange.
This will at least *help* the case where these were not populated, causing us
to send errors without channel_updated appended.
It's not perfect: we can still send such errors if the gossip store is
corrupted, and we still send them for private channels, but it should
help.
(The much better fix is far more invasive, so slips to next release!)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This cause of cascading failure was pointed out by @t-bast: if fees spike and
you don't timeout an outgoing onchain HTLC, you should nonetheless fail the incoming htlc
because otherwise the incoming peer will close on you.
Of course, there's a risk of losing funds, but this only happens if you weren't going to get the HTLC spend in time anyway. And it would also catch any other reason that the downstream onchain goes wrong, containing the damage.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reported-by: @t-bast
Changelog-Fixed: Protocol: We will close incoming HTLCs early if the outgoing HTLC is stuck onchain long enough, to avoid cascating failure.
This is the simplest solution, not the best, but there's significant risk in try to remove the "we have a path" assumption in the code pay code.
Includes removing a `tal_steal` which was incorrect: the buffer has the same lifetime as the plugin, so if we steal it then things get messy when we free the struct payment.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Plugins: `pay` will now pay your own invoices if you try.
Previously, the "payment" and "invoice" paths were completely separate, but this now calls both. It bypasses htlc_sets (and thus, cannot do MPP), and bypasses the hook too: the former is tied closely to HTLCs, and the hook is also very htlc-centric.
Includes finishing unfinished sentence in sendpay man page, as a bonus.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Plugins: `sendpay` now allows self-payment of invoices, by specifying an empty route.
Clean these up: they were debug logs, but we want to pass this information
back for self-payments.
Also fixes "Attept" typo which altered tests!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
