Invalid recovery IDs cause
secp256k1_ecdsa_recoverable_signature_parse_compact to abort, which
crashes the entire node. We should return an error instead.
Detected by libFuzzer:
[libsecp256k1] illegal argument: recid >= 0 && recid <= 3
Remove the assertion so that an error is returned for invalid bech32.
An error is preferable to crashing the entire node if there's an extra
"lightning:" prefix:
$ lightning-cli pay "lightning:lightning:"
Node log:
pay: common/bolt11.c:718: bolt11_decode_nosig: Assertion `!has_lightning_prefix(str)' failed.
pay: FATAL SIGNAL 6
...
INFO plugin-pay: Killing plugin: exited during normal operation
**BROKEN** plugin-pay: Plugin marked as important, shutting down lightningd
If both databits and *data_len are 0, pull_uint return uninitialized
stack memory in *val.
Detected by valgrind and UBSan.
valgrind:
==173904== Use of uninitialised value of size 8
==173904== __sanitizer_cov_trace_cmp8
==173904== decode_c (bolt11.c:292)
==173904== bolt11_decode_nosig (bolt11.c:877)
UBSan:
common/bolt11.c:79:29: runtime error: shift exponent 64 is too large for 64-bit type 'uint64_t' (aka 'unsigned long')
Corpus input e6f7b9744a7d79b2aa4f7c477707bdd3483f40fa triggers the UBSan
report, but we didn't previously realize this because UBSan has been
disabled in the CI run. We rename the input to indicate its usefulness
as a permanent regression test.
Otherwise, if pull_all fails, we attempt to create a script from NULL,
causing a UBSan report:
bitcoin/script.c:29:28: runtime error: null pointer passed as argument 2, which is declared to never be null
Corpus input bf703c2c20c0818af70a8c4caad6e6fd8cfd1ac6 triggers the UBSan
report, but we didn't previously realize this because UBSan has been
disabled in the CI run. We rename the input to indicate its usefulness
as a permanent regression test.
During our development, we modified the way
we report backtraces.
On a minimal configuration in OpenBSD, it seems that we
no longer compile from commit a9f26b7d07 because
our conditional code is buggy.
With the following compiler
vultr# cc -v
OpenBSD clang version 13.0.0
Target: amd64-unknown-openbsd7.3
Thread model: posix
InstalledDir: /usr/bin
We have the following error
cc common/channel_id.c
cc common/daemon.c
common/daemon.c:218:2: error: implicit declaration of function 'add_steal_notifiers' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
add_steal_notifiers(NULL);
^
1 error generated.
gmake: *** [Makefile:298: common/daemon.o] Error 1
Reported-by: @grubles
Fixes a9f26b7d07
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
We show where the pointer was allocated, but it can be confusing if it
was later stolen onto another context. Save and report those too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 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>
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.
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".
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`.
Since we changed the default, it used to be required to set it. That was a while ago, though, so we can make it optional again.
Changelog-Changed: Protocol: `invoice` no longer explicitly encodes `c` if it's the default (18)
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.
Doesn't happen on x86, but struct gossmap_chan defines:
```
u32 private: 1;
u32 plus_scid_off: 31;
```
And complains when we initialize plus_scid_off and access it later:
```
VALGRIND=1 valgrind -q --error-exitcode=7 --track-origins=yes --leak-check=full --show-reachable=yes --errors-for-leak-kinds=all plugins/renepay/test/run-mcf > /dev/null
==186886== Conditional jump or move depends on uninitialised value(s)
==186886== at 0x10076388: chan_iter (gossmap.c:1098)
==186886== by 0x100797F3: gossmap_next_chan (gossmap.c:1112)
==186886== by 0x1008C5AF: main (run-mcf.c:309)
==186886== Uninitialised value was created by a heap allocation
==186886== at 0x40F0A44: malloc (vg_replace_malloc.c:431)
==186886== by 0x10072BAF: allocate (tal.c:256)
==186886== by 0x100737A7: tal_alloc_ (tal.c:463)
==186886== by 0x100738DF: tal_alloc_arr_ (tal.c:506)
==186886== by 0x10079507: load_gossip_store (gossmap.c:690)
==186886== by 0x10079667: gossmap_load (gossmap.c:978)
==186886== by 0x1008C4AF: main (run-mcf.c:295)
```
Reported-by: @grubles
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: #6557
As side-effect, getroute(0) is special too.
Reported-by: MiddleW4y in Discord
Fixes: #6577
Changelog-Fixed: `pay` will still use an invoice routehint if path to it doesn't take 1-msat payments.
This should provide the default help message and exit, but was
resulting in a segmentation fault from freeing pointers passed to
the default config.
Changelog-Fixed: lightning-cli properly returns help without argument
This is actually a valid complaint (though this is a sanity check for
things we make ourselves, still!).
```
In file included from common/test/run-blindedpath_onion.c:9:
common/test/../sphinx.c: In function ‘sphinx_add_hop_has_length’:
common/test/../sphinx.c:117:12: error: ‘prepended_len’ may be used uninitialized [-Werror=maybe-uninitialized]
117 | if (lenlen + prepended_len != tal_bytelen(payload))
| ^
common/test/../sphinx.c:109:27: note: ‘prepended_len’ was declared here
109 | bigsize_t lenlen, prepended_len;
| ^~~~~~~~~~~~~
cc1: all warnings being treated as errors
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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
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>
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>
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.
New daemon process means we don’t have to deal with gossip, so that gets removed along with error cleanup and a refactoring of how we calculating PDBT diffs.
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
