We were masquerading errors when parsing the request by reporting only
a bogus malformed `id` field in the response, when the real issue was
that we were unable to parse the request in the first place (which
caused the null-id error to be returned).
Fixes#4238
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since `tok->size` for a string is
0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.
Fixes#4258
In a couple of places we accept arrays of strings and don't validate
them. If we forward them, e.g., call a JSON-RPC method from the
plugin, we end up embedding the unverified string in the JSON-RPC
call without escaping, which then leads to invalid JSON being passed
on.
This at least partially causes #4238
We promised to deprecate the old "destination satoshi feerate utxos"
variant some time on mid-2020 so let's do just that. `txprepare`
doesn't parse it either anymore.
Changelog-Changed: pyln: `txprepare` no longer supports the deprecated `destination satoshi feerate utxos` call format.
While debugging a hanging channel with a user I noticed that they
called `close` on a channel, resulting in the channel showing
`CHANNELD_SHUTTING_DOWN`, but the billboard seemed to show the
information the wrong way around:
```json
{
"peers": [
{
"connected": true,
// ...
"channels": [
{
"state": "CHANNELD_SHUTTING_DOWN",
// ...
"status": [
"CHANNELD_SHUTTING_DOWN:Reconnected, and reestablished.",
"CHANNELD_SHUTTING_DOWN:Funding transaction locked. They need our announcement signatures. They've sent shutdown, waiting for ours"
],
// ...
}
]
}
]
}
```
Aside from the hung channel, the switch in direction of the status
seemed weird. Checking the billboard code seems to have the status
switched as well:
ff8830876d/channeld/channeld.c (L223-L226)
We set `shutdown_sent[LOCAL]` when we send the shutdown:
ff8830876d/channeld/channeld.c (L823-L839)
And we set `shutdown_sent[REMOTE]` when we receive the shutdown:
ff8830876d/channeld/channeld.c (L1730-L1781)
So I think the billboard code just needs to be switched around.
Changelog-Fixed: JSON-RPC: The status of the shutdown meesages being exchanged is now displayed correctly.
This is stolen from the bitcoind source, so we use their style
(no tabs, 4-wide spaces). Clean it up and add the emacs magic to
maintain it in future.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As reported by Wladimir J. van der Laan. Valgrind will complain
about padding and unset fields, so memset the structs.
```
==42653== Syscall param socketcall.connect(serv_addr..sa_len) points to uninitialised byte(s)
==42653== at 0x4C7D19A: _connect (in /lib/libc.so.7)
==42653== by 0x4EE1F35: ??? (in /lib/libthr.so.3)
==42653== by 0x249D57: get_local_sockname (netaddress.c:212)
==42653== by 0x249CDB: guess_address (netaddress.c:242)
==42653== by 0x2473D0: public_address (connectd.c:1003)
==42653== by 0x246CE4: setup_listeners (connectd.c:0)
==42653== by 0x246566: connect_init (connectd.c:1311)
==42653== by 0x270CEB: next_plan (io.c:59)
==42653== by 0x2713EE: io_ready (io.c:417)
==42653== by 0x2726B1: io_loop (poll.c:445)
==42653== by 0x24618A: main (connectd.c:1703)
==42653== Address 0x7fc000690 is on thread 1's stack
==42653== in frame #3, created by guess_address (netaddress.c:231)
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The commitment tx uses the same feerate as the HTLC txs (which we have
to grind to find), but we can't use it directly since the fee could be
increased by the presence of dust HTLCs. We can still use it to cap
the maximum though.
Time before: 1m6.984s
Time after: 0m15.794s
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: onchaind is much faster when unilaterally closing old channels.
We try signatures to see which HTLC (we can have many) is the right one;
we can trivially match htlcs against commitment tx outputs, but the CTLV
can vary, and that's inside the htlc tx itself.
By sorting them, it's easy to skip comparing duplicates:
Time before: 2m32.547s
Time after: 1m6.984s
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We assert() this in onchaind while grinding fees; better to free newtx.
Before this we hit 530MB, after a mere 2.5MB.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: onchaind uses much less memory on unilateral closes for old channels.
In particular, this made valgrind OOM, and chewed much CPU. I dumped the
input and output into a file to allow easy replay.
This will break as soon as we change onchaind's wire format, but it will
serve its purpose until then!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is vital for calculating merkle trees; I previously used
towire+fromwire to get this!
Requires generation change so we can magic the ARRAY_SIZE var (the C
pre-processor can't uppercase things).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Only way to be sure that plugins don't accidentally respond to onion_message
sent via reply path from another message (which would potentially leak our
identity!).
To quote BOLT #7 (Onion Messages) in the offers PR:
```markdown
The reader:
- MUST ignore any message which contains a `blinding` which it did not expect, or does not contain
a `blinding` when one is expected.
...
`blinding` is critical to the use of blinded paths: there are various
means by which a blinded path is passed to a node. The receipt of an
expected `blinding` indicates that blinded path has been used: it is
important that a node not accept unblinded messages when it is expecting
a blinded message, as this implies the sender is probing to detect if
the recipient is the terminus of the blinded path.
Similarly, since blinded paths don't expire, a node could try to use
a blinded path to send an unexpected message hoping for a response.
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Required to determine if this msg used expected reply path.
Also remove FIXME (om->enctlv is handled above).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When we support bolt12, this won't exist. We only need min_final_cltv_expiry,
routes and features, so put them into struct payment explicitly.
We move the default final ctlv out to the caller, too, which is clearer.
e.g. keysend was using this value, but it was hard to tell.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Logs may contain non-ASCII and non-UTF8 symbols, which crashes the
tailer. It's better to replace them with a glyph representing
undecodeable symbols instead, and handle the issue further up the
call-chain.
The latter is available only starting with Python 3.7, and it's the
only test that fails with <3.7. This allows us to keep on testing with
older versions.
We rely on it when logging, but didn't set it in the class, rather we
did in the sub-classes. This proved to be rather annoying to search
since it'd just fail silently and not log any output.
Found in tests/test_connection.py::test_restart_many_payments:
`lightningd: outstanding taken(): lightningd/peer_htlcs.c:532:towire_temporary_channel_failure(((void *)0), ((void *)0))`
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
```
channeld/channeld.c:237:2: error: ‘shutdown_status’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
```
Reported-by: az0re on IRC
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There's actually a (very unlikely) race here: we would previously have
crashed with an assertion in invoices_resolve.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We'll use it for figuring out whether or not to set a utxo witness
minimum, which comes much before we were setting this field.
Now we set the protocol as soon as we can reasonably deduce it.
Needed for v2 of channel opens, where the minimum weight is 110; a
'simple utxo' (sig + key) weighs in at 107, so we a need a way to
establish a floor for this case.
Changelog-Added: JSON-RPC: fundpsbt/utxopsbt have new param, `min_witness_utxo`, which sets a floor for the weight calculation of an added input
We register on it for offers, and without this nobody else can.
Changelog-Changed: plugins: more than one plugin can now register invoice_payment hook.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Avoids much cut & paste. Some tests don't need any of it, but most
want at least some of this infrastructure.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Christian Decker
Michael Schmoock
Rusty Russell
João Paulo
ZmnSCPxj jxPCSnmZ
niftynei