I'm not completely convinced that it's only ever set to a failcode
with the BADONION bit set, especially after the previous patches in
this series. Now that channeld can handle arbitrary failcodes passed
this way, simply rename it.
We add marshalling assertions that only one of failcode and failreason
is set, and we unmarshal an empty 'fail' to NULL (just the the
generated unmarshalling code does).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
None of these sanity checks should fail, but let's be thorough: we
were testing for htlc->fail but not failcode when fulfilling an HTLC.
The failing-htlc case had this correct already.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
'struct htlc' in channeld has a 'malformed' field, which is really only
used in the "retransmit updates on reconnect" case. That's quite confusing,
and I'm not entirely convinced that it can only be set to a failcode
with the BADONION bit set.
So generalize it, using the same logic we use in the master daemon:
failcode: a locally generated error, for channeld to turn into the appropriate
error message.
fail: a remotely generated onion error, for forwarding.
Either of these being non-zero/non-NULL means we've failed, and only one
should be set at any time.
We unify the "send htlc fail/fulfill update due to retransmit" and the
normal send update paths, by always calling send_fail_or_fulfill.
This unification revealed that we accidentally skipped the
onion-wrapping stage when we retransmit failed htlcs!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
==1224== Uninitialised byte(s) found during client check request
==1224== at 0x152CAD: memcheck_ (mem.h:247)
==1224== by 0x152D18: towire (towire.c:17)
==1224== by 0x152DA1: towire_u16 (towire.c:28)
==1224== by 0x142189: towire_failed_htlc (htlc_wire.c:29)
==1224== by 0x16343F: towire_channel_init (gen_channel_wire.c:596)
==1224== by 0x115C2C: peer_start_channeld (channel_control.c:249)
==1224== by 0x131701: peer_connected (peer_control.c:503)
==1224== by 0x117820: gossip_msg (gossip_control.c:182)
==1224== by 0x139D97: sd_msg_read (subd.c:500)
==1224== by 0x139676: read_fds (subd.c:327)
==1224== by 0x179D52: next_plan (io.c:59)
==1224== by 0x17A84F: do_plan (io.c:387)
==1224== Address 0x1ffefffabe is on thread 1's stack
==1224== in frame #2, created by towire_u16 (towire.c:26)
Followed by:
2018-06-18T21:53:04.129Z lightningd(1224): 03933884aaf1d6b108397e5efe5c86bcf2d8ca8d2f700eda99db9214fc2712b134 chan #1: Peer permanent failure in CHANNELD_NORMAL: lightning_channeld: received ERROR channel d0101486543e1a8b6871556a4fe1fba4ad4d83ce7f6f92919fd17bd1545d2fd5: UpdateFailMalformedHtlc message doesn't have BADONION bit set
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
typesafe_cb isn't suitable here, as it is simply a conditional cast,
and the result is passed through '...' and doesn't matter.
Reported-by: @wythe
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We weren't waiting for gossipd to actually process the
dev_set_max_scids_encode_size message, so under Travis it sometimes
split the reply before processing that.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Note that we mark both directions of the channel disabled immediately,
it's just the broadcast of the update which is delayed, just like the
ones generated when channeld tells us to.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We disable the channel every time the peer disconnects; if it reconnects
we get two updates.
The simplest solution: delay all updates by 15 seconds. Replace any
pending delayed update. If update is redundant after 15 seconds,
discard.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In this test we tell l3 to disconnect on sending WIRE_CHANNEL_ANNOUNCEMENT.
This is hit by gossipd (to disconnect from l2) but *also* channeld to
disconnect from l4. That's OK, because normally by this point l4 has
sent its real channel_update.
However, the next patch introduces a delay in sending channel_updates,
meaning l4 hasn't sent it yet. If l3 doesn't reconnect to l4, we
never get the channel_update and the test which expects l1 to eventually
see both sides of the channel fails.
So we manually reconnect then. Note that we remove the redundant
'dev-no-reconnect' option from l2: it's added automatically as it
doesn't set 'may_reconnect'.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This doesn't do anything for us now, since we actually tend to produce
DISABLE/ENABLE update pairs. But the infrastructure is useful for the
next patch.
We also add more details to the trace message in the core update code.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
json_listpeers returns an array of peers, and an array of nodes: the latter
is a subset of the former, and is used for printing alias/color information.
This changes it so there is a 1:1 correspondance between the peer information
and nodes, meaning no more O(n^2) search.
If there is no node_announce for a peer, we use a negative timestamp
(already used to indicate that the rest of the gossip_getnodes_entry
is not valid).
Other fixes:
1. Use get_node instead of iterating through the node map.
2. A node without addresses is perfectly valid: we have to use the timestamp
to see if the alias/color are set. Previously we wouldn't print that
if it didn't also advertize an address.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There doesn't seeem to be a need for this anymore (unless I'm missing something).
I added the sendpay_nulltok() unit test to confirm.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
@rustyrussell showed we don't need temporary objects for params.
This means params no longer need a tal context.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
@wythe points out we don't need to keep the around now param_is_set()
is removed. We can in fact go further and avoid marshalling them into
temporary objects at the caller altogether.
This means internally we have an array of struct param, rather than an
array of 'struct param *', which causes most of the noise in this
patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're using a macro anyway, so appending "" make it a compile-time check.
Complicates testing a bit, since we actually use generated names there.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is a bit more natural, IMHO. The only issue is that json_tok_tok is
special, so we end up with param_opt_tok() if you really want an optional
generic token.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is part of #1464 and incorporates Rusty's suggested updates from #1569.
See comment in param.h for description, here's the basics:
unsigned cltv;
const jsmntok_t *note;
u64 msatoshi;
struct param * mp;
if (!param_parse(cmd, buffer, tokens,
param_req("cltv", json_tok_number, &cltv),
param_opt("note", json_tok_tok, ¬e),
mp = param_opt("msatoshi", json_tok_u64, &msatoshi),
NULL))
return;
if (param_is_set(mp))
do_something()
There is a lot of developer mode code to make sure we don't make mistakes,
like trying to unmarshal into the same variable twice or adding a required param
after optional.
During testing, I found a bug (of sorts) in the current system. It allows you
to provide two named parameters with the same name without error; e.g.:
# cli/lightning-cli -k newaddr addresstype=p2sh-segwit addresstype=bech32
{
"address": "2N3r6fT65PhfhE1mcMS6TtcdaEurud6M7pA"
}
It just takes the first and ignores the second. The new system reports this as an
error for now. We can always change this later.
structeq() is too dangerous: if a structure has padding, it can fail
silently.
The new ccan/structeq instead provides a macro to define foo_eq(),
which does the right thing in case of padding (which none of our
structures currently have anyway).
Upgrade ccan, and use it everywhere. Except run-peer-wire.c, which
is only testing code and can use raw memcmp(): valgrind will tell us
if padding exists.
Interestingly, we still declared short_channel_id_eq, even though
we didn't define it any more!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We wrap it in 'struct pubkey' for typesafety and consistency, and the
next patch takes advantage of that when we move to pubkey_eq.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
`getinfo` has been providing the blockheight for a good while and doesn't
require the `DEVELOPER=1` flag during compilation, so it should be the preferred
method to retrieve the blockchain height.
Implements an EWMA for the fee estimation. Achieves 90% influence of the newer
fee after 5 minutes, and adjusts to the polling rate that is configured.
Gossipd will ignore the second one, but doing it in the front end
gives an explicit error message.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Python dict can't have duplicate entries, but some options can be specified
multiple times. The easiest way is to put a list in the dict.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Only --addr implies announce-if-public: --bind-addr does not.
It's also possible to have --bind-addr to an automatic Tor address:
you'd have to dig the onion address out of the logs or getinfo to use
it, but it's possible.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rusty Russell
William Casarin
Christian Decker
Mark Beckwith
Jan Sarenik
Sjors Provoost