As was pointed out by @robtex we have underspecified the format of the nested
`channel_update` in the onionreply: lnd and eclair inserted the raw
channel_update without the type prefix, while we went for the full wire format,
including the type prefix. While we agreed that with the type it is more
flexible, and consistent, we decided to adapt to the majority and at least be
compatibly broken.
This commit takes care of being able to interpret either format correctly. It's
not perfect since signatures can happen to start with 0x0102 (the channel_update
type) but that'll happen only once ever 65k failures.
The easiest way to do this is to play with the 'wallet_tx' semantics
and have 'amount' have meaning even when 'all_funds' is set.
Note that we change the string 'Cannot afford funding transaction' to
'Cannot afford transaction' as this code is also used for withdrawls.
Inspired-by: molz on #c-lightning
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The visible changes are:
1. tal_len() is renamed to tal_bytelen() for clarity.
2. tal allocations *always* know their length.
3. tal/str routines always set the length to strlen() + 1.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In several places we use low-level tal functions because we want the
label to be something other than the default. ccan/tal is adding
tal_*_label so replace them and shim it for now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
tal_count() is used where there's a type, even if it's char or u8, and
tal_bytelen() is going to replace tal_len() for clarity: it's only needed
where a pointer is void.
We shim tal_bytelen() for now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Valgrind error file: valgrind-errors.772802
==772802== Invalid read of size 1
==772802== at 0x4C32D04: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==772802== by 0x14479C: escape (json_escaped.c:41)
==772802== by 0x144B6C: json_escape (json_escaped.c:117)
==772802== by 0x118518: json_getnodes_reply (gossip_control.c:209)
==772802== by 0x139394: sd_msg_reply (subd.c:281)
==772802== by 0x139972: sd_msg_read (subd.c:418)
==772802== by 0x17ABB1: next_plan (io.c:59)
==772802== by 0x17B6A9: do_plan (io.c:387)
==772802== by 0x17B6E7: io_ready (io.c:397)
==772802== by 0x17D2C8: io_loop (poll.c:310)
==772802== by 0x121973: main (lightningd.c:450)
==772802== Address 0x6fe5168 is 0 bytes after a block of size 72 alloc'd
==772802== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==772802== by 0x18843E: allocate (tal.c:245)
==772802== by 0x18899D: tal_alloc_ (tal.c:421)
==772802== by 0x188B5E: tal_alloc_arr_ (tal.c:464)
==772802== by 0x119BAB: fromwire_gossip_getnodes_entry (gossip_msg.c:35)
==772802== by 0x15CCD6: fromwire_gossip_getnodes_reply (gen_gossip_wire.c:111)
==772802== by 0x118436: json_getnodes_reply (gossip_control.c:192)
==772802== by 0x139394: sd_msg_reply (subd.c:281)
==772802== by 0x139972: sd_msg_read (subd.c:418)
==772802== by 0x17ABB1: next_plan (io.c:59)
==772802== by 0x17B6A9: do_plan (io.c:387)
==772802== by 0x17B6E7: io_ready (io.c:397)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This seems like a premature optimization: it tried to cut down the number of
allocations by reusing the same `struct invoice_details` while iterating through
a number of results. But this sidesteps the checks by `valgrind` and we'd miss a
missing field that was set by the previous iteration.
Reported-by: @rustyrussell
Signed-off-by: Christian Decker <@cdecker>
`wallet_stmt2payment` always expects the same fields in the same order, so we
should make sure that we always fetch them in that order and all of them.
Several users have noticed that they cannot pay satoshis.place or similar places
that have tiny payment amounts if they are not directly connected. This is due
to the forwarding fee dominating the transferred amount.
This commit adds a new option, exempting tiny fees (up to 5 satoshis by default)
from having to pass the maxfeepercent flag. While we could have told users to
tweak maxfeepercent I think it is usefull to have a default exemption.
[Squashed --RR]
Developer errors result in command_fail being called
just like other errors. The bad_programmer() Test is now updated
and passing.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
They now just call command_fail() and cause param() to return false.
Temporarily disabled all the run-param.c tests that redirect
asserts so CI would still pass.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
The logs in various Travis failures show that it takes 20 seconds just for
closingd to read the init message. As a result, the close times out (default
is 30 seconds).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We use these for receiving arrays at init time, we should also use them
for fulfull/fail of HTLCs in normal operation. That we we benefit from all
those assertions.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There are three cases:
1. failcode is 0, scid is NULL, failreason is the onion to fwd.
2. failcode is non-zero, but UPDATE bit not set. scid is NULL, failreason NULL.
3. failcode has UPDATE bit set. scid is non-NULL, failreason is NULL.
Assert these on marshaling, and only send the parts we need so unmarshal is
always canonical.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The master tells us the short_channel_id of the outgoing channel, and
channeld is supposed to get the corresponding channel_update from gossipd.
Instead, it got the channel_update for the *local* channel and ignored
that one.
The master tells us the short_channel_id of the outgoing channel when
failing an HTLC, but channeld didn't store it anywhere. It also
didn't tell channeld the short_channel_id in the case where we're
reconnecting and it's feeding us an array of failed htlcs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We delay internally to reduce broadcastig route flap, but errors are
a special case: we want to send the latest, otherwise we might send an
old (non-disabled) update.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We used to just manually set ROUTING_FLAGS_DISABLED, but that means we
then suppressed the real channel_update because we thought it was a
duplicate!
So use a local flag: set it for the channel when the peer disconnects,
and clear it when channeld sends a local update.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This requires a tweak to generate-wire.py too, since it always called the
top-level routine 'print_message'.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Just log the failed ones, not every connection and successful commands.
Before (VALGRIND=0 -n10):
111 passed, 1 skipped in 175.78 seconds
After:
111 passed, 1 skipped in 173.92 seconds
111 passed, 1 skipped in 164.16 seconds
111 passed, 1 skipped in 171.30 seconds
111 passed, 1 skipped in 180.05 seconds
111 passed, 1 skipped in 180.04 seconds
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We were *supposed* to be waiting for the next commitment tx so we
made sure the one we broadcast was old, *but* the 'revoke_and_ack'
we were waiting for could be matched by the completion of the previous
'revoke_and_ack'.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Christian Decker
Rusty Russell
Simon Vrouwe
Mark Beckwith
Jordan Baczuk
William Casarin
Rene Pickhardt