The issue is that common_setup() wasn't called by the fuzz target,
leaving secp256k1_ctx as NULL.
UBSan error:
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" \
./fuzz-channel_id crash-1575b41ef09e62e4c09c165e6dc037a110b113f2
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1153355603
INFO: Loaded 1 modules (25915 inline 8-bit counters): 25915 [0x563bae7ac3a8, 0x563bae7b28e3),
INFO: Loaded 1 PC tables (25915 PCs): 25915 [0x563bae7b28e8,0x563bae817c98),
./fuzz-channel_id: Running 1 inputs 1 time(s) each.
Running: crash-1575b41ef09e62e4c09c165e6dc037a110b113f2
bitcoin/pubkey.c:22:33: runtime error: null pointer passed as argument 1, which is declared to never be null
external/libwally-core/src/secp256k1/include/secp256k1.h:373:3: note: nonnull attribute specified here
#0 0x563bae41e3db in pubkey_from_der bitcoin/pubkey.c:19:7
#1 0x563bae4205e0 in fromwire_pubkey bitcoin/pubkey.c:111:7
#2 0x563bae46437c in run tests/fuzz/fuzz-channel_id.c:42:3
#3 0x563bae2f6016 in LLVMFuzzerTestOneInput tests/fuzz/libfuzz.c:23:2
#4 0x563bae20a450 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long)
#5 0x563bae1f4c3f in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long)
#6 0x563bae1fa6e6 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long))
#7 0x563bae223052 in main (tests/fuzz/fuzz-channel_id+0x181052) (BuildId: f7f56e14ffc06df54ab732d79ea922e773de1f25)
#8 0x7fa7fa113082 in __libc_start_main
#9 0x563bae1efbdd in _start
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior bitcoin/pubkey.c:22:33 in
`struct lightningd` is not completely initialized, so we added a
"migration_context" which only had some of the fields. But we ended
up handing in `struct lightningd` anyway, so I don't think this
complexity is worthwhile.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
At the moment only lightingd needs it, and this avoids missing any
places where we do bip32 derivation.
This uses a hsm capability to mean we're backwards compatible with older
hsmds.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Protocol: we now always double-check bitcoin addresses are correct (no memory errors!) before issuing them.
It's needed as the db and wallet is being set up (db migrations), so
it's simpler this way to always use ld->bip32_base for the next patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Importantly, adds the version number at the *front* to help future
parsing.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Header from folded patch 'fix-hsm-check-pubkey.patch':
fixup! hsmd: capability addition: ability to check pubkeys.
We were handing 3 to hsmd (and Ken added that in 7b2c5617c1,
so I guess he's OK with that being the minimum supported version!).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We would sleep for 10msec (default) and try again, spamming the logs
every second. But we're waiting for revoke_and_ack, and that handler
already sets off the timer, so there's no need to spin at all!
Fixes: #6077
Changelog-Fixed: `channeld`: no longer spin and spam logs when waiting for revoke_and_ack.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This changes connectd to use `status_fail()` on TOR problems during statup
instead of `err()`. Using `err()` did not write to the logfile.
To find out TOR problems during startup, the user needed to stop the system
daemon and call `lightningd` manually in console to see the error.
`status_fail()` logs and exits, but also prints a whole stacktrace,
which is a bit too much imho on config errors. But currently there is
no `status_SOMETHING` method that logs, prints and exists on an error
without stacktrace.
Changelog-None
While the user trying to fetch an invoice by specifing the quantity we do
not work as expected.
Running the command
```
lightning-cli fetchinvoice -k offer='lno1qgsqvgnwgcg35z6ee2h3yczraddm72xrfua9uve2rlrm9deu7xyfzrcgqffqszsk2p6hycmgv9ek2grpyphxjcm9ypmkjer8v46pyzmhd9jxwet5wvhxxmmdzsqs593pq0ylsvakdua5h976f4g3eautgjt3udvtyga47eaw7339sjrhpwpwz' quantity=2
```
and we answer back with
```json
{
"code": -32602,
"message": "quantity parameter required"
}
```
This is caused because we forget to bind the `quanity` field from the
RPC into the `invrequest`.
Reported-by: @aaronbarnardsound
Link: https://github.com/ElementsProject/lightning/issues/6089
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Changelog-EXPERIMENTAL: fetchinvoice: fix: do not ignore the `quantity` field
into the invreq field.
When plugins receive a "shutdown" notification, then can call this
method which will shutdown `cln_plugin`.
Then they can await `plugin.join()` and do any remaining cleanup
there.
This helps avoid a pain-point where plugin authors need to handle
2 separate plugin shutdown mechanisms https://github.com/ElementsProject/lightning/issues/6040
It seems that bitcoind frequently dies on this test. I assume running
the multiple nodes under valgrind with the extra 214 blocks is too
memory-hungry?
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Removed: JSON-RPC: require the `"jsonrpc": "2.0"` property (requests without this deprecated in v0.10.2).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Removed: JSON-RPC: `checkmessage` now always returns an error when the pubkey is not specified and it is unknown in the network graph (deprecated v0.12.0)
This reintroduce lnprototest after 2 releases,
there was a lot of breaking around it and this
will patch them (most of them)!
However, there are some issue related to channel opening and closing
that need some additional love and are disabled for now, but I think it
is good to introduce lnprototest now again in the CI, to be able to
stress the fix for now and see if there are other problem around.
I will take care of it!
Changelog-None
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
It's still deprecated: we need the description since
1. This information is useful for any validation we want to do, such as
the HSM, or runes.
2. We want this information in listpays so we can tell what we actually paid.
3. In general, we should never sign commitments to things we don't have!
I expect to have this information about payments *whatever the frontend* is,
which is why we deprecated (and then removed) this unintended use. The spec
is pretty clear on this:
BOLT #11:
```
A reader:
...
- MUST check that the SHA2 256-bit hash in the `h` field exactly matches the hashed
description.
```
However, neither BTCPayServer nor lnbits updated despite the long deprecation
period, so revert 2afe7a1856.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1. Don't refer to obsolete send_invoice flag.
2. Don't refer to obsolete quantity_min field.
3. Don't refer to unsigned vs signed offers: they're all unsigned.
4. Add references to invoicerequest(7).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As reported on Discord, these are undocumented. And thus, um, hard to find!
Reported-by: Aaron Barnard
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Seems like LND is hanging up on receiving these messages, even though
they're odd :(
So, when a peer connects, check if it supplies or wants peer backup
(even if it doesn't support both, it shouldn't hang up, and I didn't
want to separate the two paths).
And when we go to send our own, updated backup, check features before
sending.
Fixes: #6065
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-EXPERIMENTAL: `experimental-peer-storage` caused LND to hang up on us, so only send to peers which support it.
e778ebb9af ("wallet: only log broken if we
have duplicate scids in channels.") downgraded the fatal() to a broken
log message, but the user reports it still won't start up.
Perhaps they're hitting the fatal() outside the loop? (And we're
not getting that output).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
ccan/io stores the context pointer for io_new_conn, but we were using
`daemon->listeners` which we reallocate, so it can use a stale pointer.
```
0x3e1700 call_error
ccan/ccan/tal/tal.c:93
0x3e1700 check_bounds
ccan/ccan/tal/tal.c:165
0x3e1700 to_tal_hdr
ccan/ccan/tal/tal.c:174
0x3e1211 to_tal_hdr_or_null
ccan/ccan/tal/tal.c:186
0x3e1211 tal_alloc_
ccan/ccan/tal/tal.c:426
0x3db8f4 io_new_conn_
ccan/ccan/io/io.c:91
0x3dd2e1 accept_conn
ccan/ccan/io/poll.c:277
0x3dd2e1 io_loop
ccan/ccan/io/poll.c:444
0x3419fa main
connectd/connectd.c:2081
```
Fixes: #6060
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This reverts us to the v22.11 behaviour, pending a revisit for the
next release.
Changelog-Changed: gossipd: revert zombification change, keep all gossip for now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Without this patch, we only ever loaded the "nodes" table once, then
didn't see updates.
How this ever got past CI is a mystery; perhaps valgrind was so slow that
the updated node_announcement hit the gossmap before we even asked sql
on l3 about the nodes table?
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Plugins: `sql` nodes table now gets refreshed when gossip changes.
Loading the gossip_store would not create a pending node announcement
when the node already had a zombie channel. This would cause the node
announcement to attempt to be loaded, but fail because it had no
broadcastable channels. Accepting a pending node announcement as when
normally loading from the channel corrects this.
`node_has_public_channels` taking into account zombie channels enables
this behavior.
Separately, node_announcements were still being flagged as zombies
in the gossip store despite that feature being removed.
Changelog-None
Without inheriting zombie status, gossipd would allow regular channel updates
into the store until the pruning cycle hits (and the channel is properly
flagged) which is 3.5 days. Applying zombie status when reading channel
updates from the store prevents this.
Changelog-None
