We will now simply reject old-style ones as invalid. Turns out the
only trace we could find is a channel between two nodes unconnected to
the rest of the network.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: We now require all channel_update messages include htlc_maximum_msat (as per latest BOLTs)
Many changes to gossmap (including the pending ones!) don't actually
concern readers, as long as they obey certain rules:
1. Ignore unknown messages.
2. Treat all 16 upper bits of length as flags, ignore unknown ones.
So now we split the version byte into MAJOR and MINOR, and you can
ignore MINOR changes.
We don't expose the internal version (for creating the map)
programmatically: you should really hardcode what major version you
understand!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is what we do in lightningd, which makes memleak much more forgiving:
you can hang temporaries off cmd without getting reports of leaks (also
when send_outreq called).
We remove all the notleak() calls in plugins which worked around this!
And avoid multiple notleak labels, since both send_outreq() and
command_still_pending() can be called multiple times.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Add memleak_ignore_children() so callers can do exclusions themselves.
Having two exclusions was always such a hack!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
They are surprisingly expensive!
Running `time ./plugins/renepay/test/run-not_mcf-gossmap gossip_store-sgl.rustcorp.com.au-2022-04-19 024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605 02ebb3b8a2316b3e876ea3f3d8124a3ab97f30b128f619608eb06b5251235dc2d9 10000000000 0.1`:
Before (-Og):
real 0m1.495s
Before (no opt):
real 0m2.552s
After (-Og):
real 0m0.579s
After (no opt):
real 0m1.061s
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This allows GDB to print values, but also allows us to use them in
'case' statements. This wasn't allowed before because they're not
constant terms.
This also made it clear there's a clash between two error codes,
so move one.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: JSON-RPC: Error code from bcli plugin changed from 400 to 500.
Apparently we had two private channel announcements (the !private assert
failed). While this shouldn't happen, don't crash because of it.
Fixes: #5578
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Plugins: topology plugin could crash when it sees duplicate private channel announcements.
The gossip_store version byte was unaccounted for in the initial traversal
of gossip_store_end. This lead to an offset and a bogus message length
field. As a result, an early portion of the gossip_store could have been
skipped, potentially leading to gossip propagation issues downstream.
Fixes#5572#5565
Changelog-fixed: proper gossip_store operation may resolve some previous gossip propagation issues
This alters the billboard, but that's a human-readable thing so not
noted in CHANGELOG.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: JSON-RPC: `listpeers` `status` now refers to "channel ready" rather than "funding locked" (BOLT language change for zeroconf channels)
Changelog-Added: JSON-RPC: `channel_opened` notification `channel_ready` flag.
Changelog-Deprecated: JSON-RPC: `channel_opened` notification `funding_locked` flag (use `channel_ready`: BOLTs namechange).
This contains the zeroconf stuff, with funding_locked renamed to
channel_ready. I change that everywhere, and try to fix up the
comments.
Also the `alias` field is called `short_channel_id`.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `funding_locked` is now called `channel_ready` as per latest BOLTs.
It was backwards, which made #5496 confusing.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: `listpeers` `status` "They've sent shutdown" and "We've sent shutdown" were backwards.
We need a record of the channel account before you start sending
payments through it. Normally we don't start allowing payments to be
sent until after the channel has locked in but zeroconf does away with
this assumption.
Instead we push out a "channel_proposed" event, which should only show
up for zeroconfs.
If we expect further events for an onchain output (because we can steal
it away from the 'external'/rightful owner), we mark them.
This prevents us from marking a channel as 'onchain-resolved' before
all events that we're interested in have actually hit the chain.
Case that this matters:
Peer publishes a (cheating) unilateral close and a timeout htlc (which
we can steal).
We then steal the timeout htlc.
W/o the stealable flag, we'd have marked the channel as resolved when
the peer published the timeout htlc, which is incorrect as we're still
waiting for the resolution of that timeout htlc (b/c we *can* steal it).
it's nice to know what node your channel was opened with. in theory we
could use listpeers to merge the data after the fact, except that
channels disappear after they've been closed for a bit. it's better to
just save the info.
we print it out in `listbalances`, as that's a great place account level
information
Anchor outputs are ignored by the clightning wallet, but we keep track
of them in the bookkeeper. This causes problems when we do the balance
checks on restart w/ the balance_snapshot -- it results in us printing
out a journal_entry to 'get rid of' the anchors that the clightning node
doesnt know about.
Instead, we mark some outputs as 'ignored' and exclude these from our
account balance sums when we're comparing to the clightning snapshot.
------------------------------- Valgrind errors --------------------------------
Valgrind error file: valgrind-errors.493330
==493330== Conditional jump or move depends on uninitialised value(s)
==493330== at 0x154051: opt_add_addr_withtype (options.c:275)
==493330== by 0x154406: opt_add_announce_addr (options.c:302)
==493330== by 0x2696E6: parse_one (parse.c:121)
==493330== by 0x25CFB5: opt_parse (opt.c:228)
==493330== by 0x155DB6: handle_opts (options.c:1413)
==493330== by 0x127317: main (lightningd.c:994)
==493330==
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:opt_add_addr_withtype
fun:opt_add_announce_addr
fun:parse_one
fun:opt_parse
fun:handle_opts
fun:main
}
--------------------------------------------------------------------------------
Leaving base_dir /tmp/ltests-iyf2dw3n intact, it still has test sub-directories with failure details: ['test_announce_dns_without_port_1']
====================================== short test summary info ======================================
ERROR tests/test_gossip.py::test_announce_dns_without_port - ValueError:
Most unexpected ones are still 1, but there are a few recognizable error codes
worth documenting.
Rename the HSM ones to put ERRCODE_ at the front, since we have non-HSM ones
too now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Returning an warning message when the pub key is not specified and there is no node in the graph.
We try to help people that use core lightning as a signer and nothings else.
Changelog-Deprecated: rpc: checkmessage return an error when the pubkey is not specified and it is unknown in the network graph.
First, connectd tells us the peer has connected, and we call the connected hook,
and if it says it's fine, we are actually connected and we fire off notifications.
Of course, we could be disconnected while in the connected hook, and that would
mean we tell people about a connection which is no longer current.
Make this clear with a tristate: if we're not marked disconnected by
the time the hooks finish, we're good. It also gives us a cleaner
"connect" command return when we connected but disconnected before
processing.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rather than a generic "add member", provide two routines: one which
doesn't quote, and one which does.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There are hardly any lightningd-specific JSON functions: all that's left
are the feerate ones, and there's already a comment that we should have
a lightningd/feerate.h.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We have them split over common/param.c, common/json.c,
common/json_helpers.c, common/json_tok.c and common/json_stream.c.
Change that to:
* common/json_parse (all the json_to_xxx routines)
* common/json_parse_simple (simplest the json parsing routines, for cli too)
* common/json_stream (all the json_add_xxx routines)
* common/json_param (all the param and param_xxx routines)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Since it's a deprecation, we simply ignore one, rather than properly
checking they match etc.
Fixes: #5386
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The gossip_store version was bumbed to 10 in PR #5239 and #5375 has
apparently not been rebased on top of that when it was merged, causing
the `run-gossmap_canned` test to fail when parsing the v9 gossip
store.
Changelog-None
This will be used to decouple internal use of gossip from what is
passed to gossip peers. Updates GOSSIP_STORE_VERION to 10.
Changelog-Changed: gossip_store updated to version 10.
Usually we won't see this, since private is deleted. But we could
have already read the private channel before that. Handle it properly.
(Tested by removing the gossip_store deletion code and making sure
this worked).
We have to fix up the test, which announces a channel twice!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Thanks to @zerofeerouting for another report.
"desc" here is the sanitized message, eg:
"ERROR error channel 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef: internal error"
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Prior to 0.11.0 we had cases where we would treat errors
as warnings: regretfully, this is still needed. This message
in particular has been widely reported, and it now causes
channel force closes.
Downgrade and log. I did insert some snarky log message earlier,
but hey, I'm sure CLN has done worse things to our peers!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Protocol: treat LND "internal error" as warnings, not force close events (as we did in v0.10).
@whitslack complained of large CPU usage by connectd at startup;
I ran perf record on connectd on my machine (which sees a little spike, only)
and I see the cost of reading and discarding the entries:
```
- 95.52% 5.24% lightning_conne lightning_connectd [.] gossip_store_next
- 90.28% gossip_store_next
+ 40.27% tal_alloc_arr_
+ 22.78% tal_free
+ 11.74% crc32c
+ 9.32% fromwire_peektype
+ 4.10% __libc_pread64 (inlined)
1.70% be32_to_cpu
```
Much of this is caused by the search for our own gossip: keeping this separately
would be even better, but this fix is minimal.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: connectd: reduce initial CPU load when connecting to peers.
This changes many fields: in non-deprecated mode, they're now raw integers.
This was always the intention, but the transition was never completed.
Suggested-By: @ShahanaFarooqui
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: JSON-RPC: "_msat" fields can be raw numbers, not "123msat" strings (please handle both!)
Changelog-Deprecated: JSON-RPC: "_msat" fields as "123msat" strings (will be only numbers)
We had json_add_amount_msat_only(), which was designed to be used to
print out msat fields, if we had sats.
However, we misused it, so split it into the three different cases:
1. json_add_amount_sat_msat: We are using it correctly, with a field called
xxx_msat.
2. json_add_amount_sats_deprecated: We were using it wrong, so deprecate
the old field and create a new one which does end in _msat.
3. json_add_sats: we were using it to hand sats as a JSON parameter to an
interface, where "XXXsat".
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Deprecated: Plugins: `rbf_channel` and `openchannel2` hooks `their_funding` (use `their_funding_msat`)
Changelog-Deprecated: Plugins: `openchannel2` hook `dust_limit_satoshis` (use `dust_limit_msat`)
Changelog-Deprecated: Plugins: `openchannel` hook `funding_satoshis` (use `funding_msat`)
Changelog-Deprecated: Plugins: `openchannel` hook `dust_limit_satoshis` (use `dust_limit_msat`)
Changelog-Deprecated: Plugins: `openchannel` hook `channel_reserve_satoshis` (use `channel_reserve_msat`)
Changelog-Deprecated: Plugins: `channel_opened` notification `amount` (use `funding_msat`)
Changelog-Deprecated: JSON-RPC: `listtransactions` `msat` (use `amount_msat`)
Changelog-Deprecated: Plugins: `htlc_accepted` `forward_amount` (use `forward_msat`)
Per BIP-0171, the signature map is of pubkey to "The signature as would
be pushed to the stack from a scriptSig or witness".
Fixes 5298
Changelog-Fixed: PSBT: Fix signature encoding to comply with BIP-0171.
Signed-off-by: Jon Griffiths <jon_p_griffiths@yahoo.com>
This likely lead to a number of false errors when attempting to
route. We deemed a channel to be unusable as soon as either direction
isn't usable. This is bad since it excludes not only zeroconf
channels (which have different scids for the two directions), but it
also excludes any channel that we haven't seen an update from
yet. This was likely introduced when attemting to exclude nodes that
haven't sent a disable, but their peer has, but this is not necessary
as the unresponsive node would be marked as isolated by all its peers,
so we don't need to artificially mark a channel direction as disabled
when really we can't even enter the node to traverse the channel in
that direction.
Changelog-Fixed: routing: Fixed an issue where we would exclude the entire channel if either direction was disabled, or we hadn't seen an update yet.
After this, we can exactly reproduce the vectors (in DEVELOPER mode).
1. Move payment_metadata position to match test vector.
2. Create flag to suppress `c` field production.
3. Some vectors put secret before payment_hash, hack that in.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We do this (send warnings) in almost all cases anyway, so mainly this
is a textual update, but there are some changes:
1. Send ERROR not WARNING if they send a malformed commitment secret.
2. Send WARNING not ERROR if they get the shutdown_scriptpubkey wrong (vs upfront)
3. Send WARNING not ERROR if they send a bad shutdown_scriptpubkey (e.g. p2pkh in future)
4. Rename some vars 'err' to 'warn' to make it clear we send a warning.
This means test_option_upfront_shutdown_script can be made reliable, too,
and it now warns and doesn't automatically close channel.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This doesn't have an effect now (except in experimental mode), but it
will when we support anchors. So we deprecate the use of those in the
close command too.
For experimental mode we have to avoid using p2pkh; adapt that test.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Deprecated: JSON-RPC: `shutdown` no longer allows p2pkh or p2sh addresses.
The signatures on the new examples are sometimes different from what we produce though?
They're valid, however.
And one example has an unneeded feature 5-bit; it's not *wrong*, but
it's not optimal.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Partial revert of 43a833e405
"lightningd: remove support for legacy onion format."; we restore the
ability to decode legacy onions for forwarding, but not to generate them.
(We don't accept them properly since making payment_secret compulsory
anyway, so no real change there!)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Removed: Protocol: ... but we still forward legacy HTLC onions for now.
I removed these prematurely: we *haven't* had a release since
introducing them!
This consists of reverting d15d629b8b
"plugins/fetchinvoice: remove obsolete string-based API." and
plugins/fetchinvoice: remove obsolete string-based
API. "onion_messages: remove obs2 support."
Some minor changes due to updated fromwire_tlv API since they
were removed, but not much.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-EXPERIMENTAL: REVERT: Removed backwards compat with onion messages from v0.10.1.
There's a race under CI, where a channel is deleted then we see the
channel_update in the gossip store. We assumed this wouldn't happen,
but it can!
```
[gw1] [ 95%] FAILED tests/test_connection.py::test_multichan
[gw1] [ 95%] ERROR tests/test_connection.py::test_multichan
...
> raise ValueError(str(errors))
E ValueError:
E Node errors:
E - lightningd-3: had BROKEN messages
E - lightningd-3: Node exited with return code 1
E Global errors:
...
lightningd-3: 2022-03-28T00:11:42.160Z DEBUG wallet: Owning output 0 100000sat (SEGWIT) txid 30616903feba1839a3834e2b3b6123759ce1fe0d76414ca77e2dbc17414772e0 CONFIRMED
lightningd-3: 2022-03-28T00:11:42.392Z DEBUG hsmd: Client: Received message 5 from client
lightningd-3: 2022-03-28T00:11:42.393Z DEBUG hsmd: new_client: 2
lightningd-3: 2022-03-28T00:11:42.398Z INFO plugin-topology: Killing plugin: exited during normal operation
lightningd-3: 2022-03-28T00:11:42.400Z **BROKEN** plugin-topology: Plugin marked as important, shutting down lightningd!
...
----------------------------- Captured stderr call -----------------------------
topology: update for channel 105x1x1 not found!
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Means that field is now optional in JSON output.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `delinvoice` has a new parameter `desconly` to remove description.
LNURL wants this so they can include images etc in descriptions.
Replaces: #4892
Changelog-Added: JSON-RPC: `invoice` has a new parameter `deschashonly` to put hash of description in bolt11.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It was tlv_fields_valid that wanted a non-const: now that's gone, we
can make this correctly const.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Callers were supposed to call "tlv_fields_valid" after fromwire_tlv,
but few did. Make this the default, and call the underlying function
directly where we want to be more flexible (one place).
This loses the ability to allow misordered fields, or to pass through
*any* even fields. We restore that for special cases in the next
patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Requiring the caller to allocate them is ugly, and differs from
other types.
This means we need a context arg if we don't have one already.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
No more "towire_offer", but "towire_tlv_offer".
This means we double-up on the unfortunately-named `tlv_payload` inside
the onion, but we should rename that in the spec when we remove
old payloads.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In particular, this changes the name of a field in invoice_request:
`payer_signature` becomes simply `signature`. So we allow both for
now, and send the old one unless deprecated_apis is disabled.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When we did fill them in, we filled them in wrong: the offset should be
the offset in the message, not the field number!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Also has to fix up tests.
Changelog-Fixed: cli doesn't required anymore to confirm the password if the `hsm_secret` is already encrypted.
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Things allocated by libwally all get the tal_name "wally_tal",
which cost me a few hours trying to find a leak.
In the case where we're making one of the allocations the parent
of the others (e.g. a wally_psbt), we can do better: supply a name
for the tal_wally_end().
So I add a new tal_wally_end_onto() which does the standard
tal_steal() trick, and also changes the (typechecked!) name.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>